Bug 68691 - new-run-webkit-tests is locale dependent
: new-run-webkit-tests is locale dependent
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Tools / Tests
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Eric Seidel
:
Depends on: 71199 72328
Blocks: 64491
  Show dependency treegraph
 
Reported: 2011-09-23 04:47 PDT by Antti Koivisto
Modified: 2011-12-05 12:58 PST (History)
11 users (show)

See Also:


Attachments
Patch (8.12 KB, patch)
2011-10-29 12:52 PDT, Eric Seidel
no flags Details | Formatted Diff | Diff
Patch for landing (8.12 KB, patch)
2011-10-29 14:00 PDT, Eric Seidel
no flags Details | Formatted Diff | Diff
Patch for landing (21.70 KB, patch)
2011-10-30 00:56 PDT, Eric Seidel
no flags Details | Formatted Diff | Diff
Set LC_ALL to C (1.27 KB, patch)
2011-11-09 13:53 PST, vanuan
no flags Details | Formatted Diff | Diff
Patch to land (1.57 KB, patch)
2011-11-14 12:24 PST, vanuan
eric: review-
Details | Formatted Diff | Diff
Patch for landing (1.35 KB, patch)
2011-11-14 12:29 PST, vanuan
no flags Details | Formatted Diff | Diff
Patch (1.35 KB, patch)
2011-11-14 12:35 PST, vanuan
no flags Details | Formatted Diff | Diff
Patch (1.35 KB, patch)
2011-11-14 12:49 PST, vanuan
no flags Details | Formatted Diff | Diff
Patch (2.20 KB, patch)
2011-11-14 13:17 PST, vanuan
no flags Details | Formatted Diff | Diff
updated to tip of tree (22.67 KB, patch)
2011-11-17 14:05 PST, Eric Seidel
abarth: review+
Details | Formatted Diff | Diff
Patch (1.24 KB, patch)
2011-11-18 11:55 PST, vanuan
no flags Details | Formatted Diff | Diff
Patch (1.85 KB, patch)
2011-11-18 12:55 PST, vanuan
no flags Details | Formatted Diff | Diff
Patch (1.85 KB, patch)
2011-11-18 13:15 PST, vanuan
no flags Details | Formatted Diff | Diff
patch (1.86 KB, patch)
2011-11-21 12:56 PST, vanuan
no flags Details | Formatted Diff | Diff
Patch (4.54 KB, patch)
2011-11-22 15:16 PST, vanuan
eric: review-
Details | Formatted Diff | Diff
Patch (4.83 KB, patch)
2011-11-23 14:12 PST, vanuan
no flags Details | Formatted Diff | Diff
patch (4.67 KB, patch)
2011-11-23 14:28 PST, vanuan
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2011-09-23 04:47:08 PDT
Many tests are failing in my machine (Finnish locale) using run-webkit-tests with diffs like this (from compositing/iframes/become-composited-nested-iframes.html):

 (GraphicsLayer
-  (bounds 785.00 1500.00)
+  (bounds 785,00 1500,00)
   (children 1

They work fine with old-run-webkit-tests. The decimal separator in Finnish locale is comma, not period.
Comment 1 Adam Barth 2011-09-23 10:20:44 PDT
We probably need to set the locale environment variable.
Comment 2 Eric Seidel 2011-09-23 17:14:46 PDT
It appears the trick is that ORWT used a completely clean environment:
http://trac.webkit.org/browser/trunk/Tools/Scripts/old-run-webkit-tests#L1471

Where as NRWT does not:
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/base.py#L651
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py#L443

Very doable.

We have other general LOCALE problems in our python code, which also will need similar fixing, see bug 63452
Comment 3 Eric Seidel 2011-10-29 12:52:54 PDT
Created attachment 112973 [details]
Patch
Comment 4 Eric Seidel 2011-10-29 12:53:14 PDT
Hopefully this fixes your issue.
Comment 5 Adam Barth 2011-10-29 13:10:59 PDT
Comment on attachment 112973 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112973&action=review

> Tools/Scripts/webkitpy/layout_tests/port/efl.py:59
> +    # FIXME: I doublt EFL wants to override this method.

typo: doublt
Comment 6 Eric Seidel 2011-10-29 14:00:42 PDT
Created attachment 112975 [details]
Patch for landing
Comment 7 WebKit Review Bot 2011-10-29 15:07:06 PDT
Comment on attachment 112975 [details]
Patch for landing

Clearing flags on attachment: 112975

Committed r98819: <http://trac.webkit.org/changeset/98819>
Comment 8 WebKit Review Bot 2011-10-29 15:07:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Ryosuke Niwa 2011-10-29 17:38:29 PDT
It appears that this patch broke Qt bots:
http://build.webkit.org/builders/Qt%20Linux%20Release/builds/39108/steps/layout-test/logs/stdio
Traceback (most recent call last):
  File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 436, in <module>
    sys.exit(main())
  File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 431, in main
    return run(port, options, args)
  File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 105, in run
    result_summary = manager.set_up_run()
  File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 845, in set_up_run
    if not self._port.check_sys_deps(self.needs_servers()):
  File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/layout_tests/port/base.py", line 192, in check_sys_deps
    return self.check_httpd()
  File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/layout_tests/port/base.py", line 240, in check_httpd
    env = self.setup_environ_for_server(server_name)
  File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/layout_tests/port/qt.py", line 146, in setup_environ_for_server
    self._copy_value_from_environment(clean_env, 'QT_DRT_WEBVIEW_MODE')
AttributeError: 'QtPort' object has no attribute '_copy_value_from_environment'
Comment 10 Eric Seidel 2011-10-29 19:54:52 PDT
Fixing.
Comment 11 Eric Seidel 2011-10-29 20:06:21 PDT
Committed r98823: <http://trac.webkit.org/changeset/98823>
Comment 12 Ryosuke Niwa 2011-10-29 21:32:05 PDT
Hm... Chromium Windows appears to be broken as well:
Traceback (most recent call last):
  File "E:\google-windows-2\chromium-win-release-tests\build\Tools\Scripts\webkitpy\layout_tests\run_webkit_tests.py", line 436, in <module>
    sys.exit(main())
  File "E:\google-windows-2\chromium-win-release-tests\build\Tools\Scripts\webkitpy\layout_tests\run_webkit_tests.py", line 431, in main
    return run(port, options, args)
  File "E:\google-windows-2\chromium-win-release-tests\build\Tools\Scripts\webkitpy\layout_tests\run_webkit_tests.py", line 105, in run
    result_summary = manager.set_up_run()
  File "E:\google-windows-2\chromium-win-release-tests\build\Tools\Scripts\webkitpy\layout_tests\controllers\manager.py", line 845, in set_up_run
    if not self._port.check_sys_deps(self.needs_servers()):
  File "E:\google-windows-2\chromium-win-release-tests\build\Tools\Scripts\webkitpy\layout_tests\port\chromium.py", line 145, in check_sys_deps
    result = super(ChromiumPort, self).check_sys_deps(needs_http)
  File "E:\google-windows-2\chromium-win-release-tests\build\Tools\Scripts\webkitpy\layout_tests\port\base.py", line 192, in check_sys_deps
    return self.check_httpd()
  File "E:\google-windows-2\chromium-win-release-tests\build\Tools\Scripts\webkitpy\layout_tests\port\base.py", line 240, in check_httpd
    env = self.setup_environ_for_server(server_name)
  File "E:\google-windows-2\chromium-win-release-tests\build\Tools\Scripts\webkitpy\layout_tests\port\chromium_win.py", line 127, in setup_environ_for_server
    env["PATH"] = "%s;%s" % (self.path_from_chromium_base("third_party", "cygwin", "bin"), env["PATH"])
KeyError: 'PATH'
Comment 14 Eric Seidel 2011-10-29 23:31:39 PDT
Sorry.  Changes like this one are very dangerous. :(  Will fix shortly.
Comment 15 Eric Seidel 2011-10-30 00:56:45 PDT
Created attachment 112989 [details]
Patch for landing
Comment 16 Eric Seidel 2011-10-30 00:57:15 PDT
Committed r98825: <http://trac.webkit.org/changeset/98825>
Comment 17 Eric Seidel 2011-10-30 00:57:49 PDT
Turns out that we already had tests which would have caught this CrWin regression.  But those tests were only enabled on windows!  The fix was simple.  But it was a big change to actually fix those unittests to run everywhere. :(
Comment 18 Ryosuke Niwa 2011-10-30 10:54:47 PDT
It appears that cr-win is still broken :(

http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/20279/steps/layout-test/logs/stdio
Comment 19 Eric Seidel 2011-10-30 13:01:37 PDT
I suspect CrWin expects more environment to be transfered to lighttpd when called.

I can make Chromium ports copy the entire environment over instead?
Comment 20 Eric Seidel 2011-10-30 13:02:25 PDT
rniwa, dpranke: could you point me to the chromium master config for that bot?  It's possible it's explicitly setting some environment variable I'm supposed to be passing to lighttpd
Comment 21 Ryosuke Niwa 2011-10-30 13:26:12 PDT
http://trac.webkit.org/browser/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg

I don't see anything special for cr-win though.
Comment 22 Eric Seidel 2011-10-30 15:23:33 PDT
I will make CrWin copy the entire environment over.  Fixing now.
Comment 23 Eric Seidel 2011-10-30 15:42:46 PDT
Committed r98830: <http://trac.webkit.org/changeset/98830>
Comment 24 Eric Seidel 2011-10-30 15:43:15 PDT
Hopefully that will fix things.  Let me know if you see any further trouble.
Comment 25 Ryosuke Niwa 2011-10-30 16:39:52 PDT
(In reply to comment #24)
> Hopefully that will fix things.  Let me know if you see any further trouble.

It appears that bevc is conducting a scheduled maintenance on the Chromium bots so we'd have to wait until bots come back to see this fix worked or not.
Comment 26 Ryosuke Niwa 2011-10-30 16:47:25 PDT
Hm... it appears that you broke 3 cr-win tests on Mac bots:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/34309/steps/webkitpy-test/logs/stdio
Comment 27 Ryosuke Niwa 2011-10-30 16:49:03 PDT
At this point, I think we should roll these patches out. We've been piling unreviewed fixes on top of each other, and breaking different things at times.
Comment 29 Ryosuke Niwa 2011-10-30 18:43:30 PDT
I'm going to rollout these changesets for now since we're losing 100% of layout test coverage due to this failure.
Comment 30 Ryosuke Niwa 2011-10-30 18:50:20 PDT
Reopen the bug since the changeset was rolled out in http://trac.webkit.org/changeset/98833.
Comment 31 WebKit Review Bot 2011-10-30 18:53:18 PDT
Comment on attachment 112989 [details]
Patch for landing

Rejecting attachment 112989 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t_tests/port/base.py.rej
patching file Tools/Scripts/webkitpy/layout_tests/port/chromium.py
patching file Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py
patching file Tools/Scripts/webkitpy/layout_tests/port/chromium_win.py
patching file Tools/Scripts/webkitpy/layout_tests/port/chromium_win_unittest.py
patching file Tools/Scripts/webkitpy/layout_tests/port/google_chrome_unittest.py

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10245205
Comment 32 Eric Seidel 2011-10-30 20:52:36 PDT
Thanks for your help, rniwa.
Comment 33 vanuan 2011-11-09 13:53:40 PST
Created attachment 114358 [details]
Set LC_ALL to C

The quickest fix for Unix environments would be to set "LC_ALL" to "C".
Comment 34 Eric Seidel 2011-11-09 13:56:19 PST
Comment on attachment 114358 [details]
Set LC_ALL to C

Yes, I have a more complete fix which I"ll be posting shortly.
Comment 35 Eric Seidel 2011-11-09 14:09:08 PST
vanuan: I'm happy to chat about this, and other bugs more on #webkit if you're around.  I'm there as eseidel or eseidel2.
Comment 36 vanuan 2011-11-09 16:11:50 PST
Seems like tests are expecting LC_ALL = "En_US"
Comment 37 vanuan 2011-11-10 11:56:17 PST
To fix all locale dependent tests I have applied this patch:

===================================================================
--- base.py	(revision 99472)
+++ base.py	(working copy)
@@ -652,7 +652,14 @@
         Returns:
            Operating-system's environment.
         """
-        return os.environ.copy()
+        env = os.environ.copy()
+        """ This is a hack (tests shouldn't be locale dependent).
+            Works only in unix environments.
+        """
+        env['LANGUAGE']='en'
+        env['LC_MESSAGES']='en_US.utf8'
+        env['LANG']='en_US.UTF-8'
+        return env


Waiting for a more complete fix though.
Comment 38 vanuan 2011-11-14 12:24:11 PST
Created attachment 115004 [details]
Patch to land
Comment 39 WebKit Review Bot 2011-11-14 12:28:19 PST
Attachment 115004 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/Scripts/webkitpy/layout_tests/port/base.py:661:  missing whitespace around operator  [pep8/E225] [5]
Tools/Scripts/webkitpy/layout_tests/port/base.py:662:  missing whitespace around operator  [pep8/E225] [5]
Tools/Scripts/webkitpy/layout_tests/port/base.py:663:  missing whitespace around operator  [pep8/E225] [5]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 40 Eric Seidel 2011-11-14 12:29:27 PST
Comment on attachment 115004 [details]
Patch to land

View in context: https://bugs.webkit.org/attachment.cgi?id=115004&action=review

> Tools/ChangeLog:1
> +2011-11-14  Vanuan  <vanuan@gmail.com>

Normally we use our full names in ChangeLogs.

> Tools/ChangeLog:194
> +>>>>>>> .r100139

Conflict marker.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:660
> +        """ This is a hack (tests shouldn't be locale dependent).
> +            Works only in unix environments.
> +        """

I would have made this a comment instead of a doc-string.  I also would have used the keyword FIXME: at the beginning to it's easily searchable.

Something like this:
# FIXME: This is a hack.  Tests should not be locale dependent.  This only works in unix environments.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:663
> +        env['LANGUAGE']='en'
> +        env['LC_MESSAGES']='en_US.utf8'
> +        env['LANG']='en_US.UTF-8'

Also, this should go in setup_environ_for_server instead.
Comment 41 vanuan 2011-11-14 12:29:59 PST
Created attachment 115005 [details]
Patch for landing

Oops, my changelog was wrong
Comment 42 WebKit Review Bot 2011-11-14 12:31:24 PST
Attachment 115005 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/Scripts/webkitpy/layout_tests/port/base.py:661:  missing whitespace around operator  [pep8/E225] [5]
Tools/Scripts/webkitpy/layout_tests/port/base.py:662:  missing whitespace around operator  [pep8/E225] [5]
Tools/Scripts/webkitpy/layout_tests/port/base.py:663:  missing whitespace around operator  [pep8/E225] [5]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 43 vanuan 2011-11-14 12:35:50 PST
Created attachment 115008 [details]
Patch

Hopefully, fixed style errors
Comment 44 vanuan 2011-11-14 12:49:13 PST
Created attachment 115013 [details]
Patch

Updated with review comments.
Comment 45 Eric Seidel 2011-11-14 13:00:27 PST
Comment on attachment 115013 [details]
Patch

Ok, one last step.  You should unittest this.  It's very simple.  See http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py  Add a test_setup_environ_for_server(self) test which uses self.assertEquals(env['LANGUAGE'], 'en'), etc. to verify that these are set correctly, and then run test-webkitpy to make sure everything still works.

That's the final piece.  Otherwise looks great.  Thank you for your efforts.
Comment 46 vanuan 2011-11-14 13:17:17 PST
Created attachment 115018 [details]
Patch
Comment 47 Eric Seidel 2011-11-14 14:10:46 PST
Comment on attachment 115018 [details]
Patch

Fantastic!
Comment 48 Eric Seidel 2011-11-14 14:11:35 PST
I will land my more complicated patch again (using a separate bug) later this week.
Comment 49 WebKit Review Bot 2011-11-14 14:29:20 PST
Comment on attachment 115018 [details]
Patch

Clearing flags on attachment: 115018

Committed r100192: <http://trac.webkit.org/changeset/100192>
Comment 50 WebKit Review Bot 2011-11-14 14:29:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 52 Eric Seidel 2011-11-14 16:41:37 PST
That's so odd.  The change was designed to fix exactly those sorts of failures.
Comment 53 Eric Seidel 2011-11-14 16:45:10 PST
Comment on attachment 115018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115018&action=review

> Tools/Scripts/webkitpy/layout_tests/port/base.py:660
> +        env['LANGUAGE'] = 'en'

My theory is that maybe "en" is being interpreted as "en_gb" on mac somehow.
Comment 54 vanuan 2011-11-15 16:07:28 PST
Comment on attachment 115018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115018&action=review

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:660
>> +        env['LANGUAGE'] = 'en'
> 
> My theory is that maybe "en" is being interpreted as "en_gb" on mac somehow.

That's what I was afraid of. I didn't test on mac.
Well, decimal point in en_GB locale is also '.', so it's probably not the case.
More strangely, tests didn't fail on Lion Intel
Comment 55 Eric Seidel 2011-11-17 13:34:02 PST
John's patch was rolled out in bug 72328.
Comment 56 Eric Seidel 2011-11-17 14:05:38 PST
Created attachment 115673 [details]
updated to tip of tree
Comment 57 Eric Seidel 2011-11-17 14:13:50 PST
Committed r100674: <http://trac.webkit.org/changeset/100674>
Comment 58 vanuan 2011-11-18 07:28:18 PST
Seems like it is not finished yet.

I have a russian locale and I see the python exception:

File "/home/vanya/git-repos/opensource/webkit/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/svn.py", line 103, in value_from_svn_info
    raise ScriptError(script_args=svn_info_args, message='svn info did not contain a %s.' % field_name)
webkitpy.common.system.executive.ScriptError: svn info did not contain a Repository UUID.

It is because svn info when run under russian locale shows:

Путь: '.'
URL: http://svn.webkit.org/repository/webkit/trunk
Корень репозитория: http://svn.webkit.org/repository/webkit
UUID репозитория: 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Редакция: 100736
Вид узла: каталог
Задано: нормально
Автор последнего изменения: hausmann@webkit.org
Редакция последнего изменения: 100736
Дата последнего изменения: 2011-11-18 12:27:25 +0200 (Пт., 18 нояб. 2011)

So script can't parse russian equivalent of 'Repository UUID'.
Comment 59 vanuan 2011-11-18 11:31:41 PST
The bug is not fixed for me. I still have 500+ tests failing.
Actually, I don't see how the latest patch intends to fix this bug. I don't see it overrides LC_ALL and LANG anywhere.

It's not NRWT issue. Tests are failing even when I run ORWT under non-english locale.
Comment 60 vanuan 2011-11-18 11:55:13 PST
Created attachment 115842 [details]
Patch

Override LANG, LANGUAGE and LC_ALL to use en_US.UTF-8
Comment 61 Darin Adler 2011-11-18 11:58:38 PST
Comment on attachment 115842 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115842&action=review

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:414
> +    locale.setlocale(locale.LC_ALL, '')

Why is empty string the right value for this?
Comment 62 vanuan 2011-11-18 12:23:10 PST
Comment on attachment 115842 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115842&action=review

>> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:414
>> +    locale.setlocale(locale.LC_ALL, '')
> 
> Why is empty string the right value for this?

When LC_ALL is empty, locale is set to user setting specified in LANG or LANGUAGE variable.
http://docs.python.org/library/locale.html#locale.setlocale
Hm, I've realized that this might fail on windows.
Comment 63 vanuan 2011-11-18 12:55:25 PST
Created attachment 115850 [details]
Patch

See new patch with exception handling
Comment 64 WebKit Review Bot 2011-11-18 13:02:45 PST
Attachment 115850 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:410:  expected 2 blank lines, found 1  [pep8/E302] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 65 vanuan 2011-11-18 13:15:34 PST
Created attachment 115856 [details]
Patch

Fix style
Comment 66 vanuan 2011-11-21 12:56:40 PST
Created attachment 116119 [details]
patch

review anybody?
Comment 67 Eric Seidel 2011-11-21 13:04:34 PST
Comment on attachment 116119 [details]
patch

This seems like a rather large hammer.  This also won't help test-webkitpy (which will still fail for you, no?)

I would put this somewhere more central.  Possibly on Host() itself?
Comment 68 vanuan 2011-11-22 14:33:16 PST
Comment on attachment 115018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115018&action=review

> Tools/Scripts/webkitpy/layout_tests/port/base.py:661
> +        env['LC_MESSAGES'] = 'en_US.utf8'

Ok, I've figured it out. It should've been 'en_US.UTF-8'. 'en_US.utf8' is invalid locale under Snow Leopard, so the default value is set.
Comment 69 vanuan 2011-11-22 15:16:32 PST
Created attachment 116280 [details]
Patch
Comment 70 Eric Seidel 2011-11-23 10:34:32 PST
Comment on attachment 116280 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116280&action=review

> Tools/Scripts/webkitpy/common/system/executive.py:400
> +                    env=os.environ.copy(),
> +                    locale_string='en_US.UTF-8',

In python it's important to use None for these default values, as they're static.  The env that you are creating here is cretaed once, and shared between all calls to run_command as the default value.  Especially since you modify it, it will get confused and not do what you want.

> Tools/Scripts/webkitpy/common/system/executive.py:456
> +    @staticmethod
> +    def set_locale_environ(env, locale_string):
> +        # Override locale
> +        # FIXME works only in Unix environments.
> +        env['LANGUAGE'] = locale_string.split('_')[0]
> +        env['LANG'] = locale_string
> +        env['LC_MESSAGES'] = locale_string
> +        env['LC_ALL'] = ''
> +        return env
> +

We should just move this onto Host, like engage_awesome_svn_hacks is.  This is also a hack, and we can come up with something better over time, but doing this on Host is fine for now.

We shoulldn't be modifying global state from some random run_command call.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:693
> +        # Override locale
> +        clean_env = Executive.set_locale_environ(clean_env, 'en_US.UTF-8')

This function would do better on Environment, but I don't think I've landed that yet.
Comment 71 vanuan 2011-11-23 11:12:13 PST
Comment on attachment 116280 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116280&action=review

>> Tools/Scripts/webkitpy/common/system/executive.py:400
>> +                    locale_string='en_US.UTF-8',
> 
> In python it's important to use None for these default values, as they're static.  The env that you are creating here is cretaed once, and shared between all calls to run_command as the default value.  Especially since you modify it, it will get confused and not do what you want.

So, you recommend something like this:
if env is None:
    env=os.environ.copy()
?

Or I can't change parameters at all and should use copies everywhere?

> Tools/Scripts/webkitpy/common/system/executive.py:415
> +        self.set_locale_environ(env, locale_string)

Is it ok?

>> Tools/Scripts/webkitpy/common/system/executive.py:456
>> +
> 
> We should just move this onto Host, like engage_awesome_svn_hacks is.  This is also a hack, and we can come up with something better over time, but doing this on Host is fine for now.
> 
> We shoulldn't be modifying global state from some random run_command call.

So, should I just move this static method to Host and use Host.set_locale_environ()?

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:693

> 
> This function would do better on Environment, but I don't think I've landed that yet.

So? Is it fine for now?
Comment 72 vanuan 2011-11-23 14:12:42 PST
Created attachment 116425 [details]
Patch

Any luck this is a good patch?
Comment 73 WebKit Review Bot 2011-11-23 14:18:02 PST
Attachment 116425 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 42, in <module>
    from webkitpy.style.main import CheckWebKitStyle
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 27, in <module>
    import webkitpy.style.checker as checker
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 43, in <module>
    from checkers.test_expectations import TestExpectationsChecker
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 37, in <module>
    from webkitpy.common.host import Host
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/host.py", line 37, in <module>
    from webkitpy.common.net.buildbot.chromiumbuildbot import ChromiumBuildBot
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/net/buildbot/chromiumbuildbot.py", line 32, in <module>
    from webkitpy.layout_tests.port.builders import builder_path_from_name
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/__init__.py", line 34, in <module>
    from base import Port  # It's possible we don't need to export this virtual baseclass outside the module.
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/base.py", line 49, in <module>
    from webkitpy.common.host import Host
ImportError: cannot import name Host


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 74 vanuan 2011-11-23 14:21:52 PST
I've placed a function to Host as you suggested, but I've encountered circular dependencies when I'm trying to use that :(

I'm giving up...
Comment 75 vanuan 2011-11-23 14:28:14 PST
Created attachment 116429 [details]
patch

Ok, last try.
Comment 76 Eric Seidel 2011-11-28 12:48:45 PST
Committed r101274: <http://trac.webkit.org/changeset/101274>
Comment 77 Eric Seidel 2011-11-28 12:49:09 PST
Comment on attachment 116429 [details]
patch

I modified your patch slightly and landed it
Comment 78 vanuan 2011-12-03 15:22:09 PST
Thanks you! NRWT is now working for me.

However, test-webkitpy still fails.

Maybe you could also apply the second part of my patch? (That is override locale for executive)

Strangely enough, but it seems like somehow os.environ is not used when value_from_svn_info  calls Executive().run_command.

Completely clean env resolves this issue:

Index: Tools/Scripts/webkitpy/common/checkout/scm/svn.py
===================================================================
--- Tools/Scripts/webkitpy/common/checkout/scm/svn.py	(revision 101932)
+++ Tools/Scripts/webkitpy/common/checkout/scm/svn.py	(working copy)
@@ -97,7 +97,7 @@
     def value_from_svn_info(cls, path, field_name):
         svn_info_args = [cls.executable_name, 'info']
         # FIXME: This method should use a passed in executive or be made an instance method and use self._executive.
-        info_output = Executive().run_command(svn_info_args, cwd=path).rstrip()
+        info_output = Executive().run_command(svn_info_args, env={}, cwd=path).rstrip()
         match = re.search("^%s: (?P<value>.+)$" % field_name, info_output, re.MULTILINE)
         if not match:
             raise ScriptError(script_args=svn_info_args, message='svn info did not contain a %s.' % field_name)
Comment 79 Eric Seidel 2011-12-05 12:58:32 PST
We would just need to change test-webkitpy to instantiate a real Host() once, in order to have the environment hack engage.