Bug 68691 - new-run-webkit-tests is locale dependent
: new-run-webkit-tests is locale dependent
Status: RESOLVED FIXED
: WebKit
Tools / Tests
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 71199 72328
: 64491
  Show dependency treegraph
 
Reported: 2011-09-23 04:47 PST by
Modified: 2011-12-05 12:58 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-09-23 04:47:08 PST
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 From 2011-09-23 10:20:44 PST -------
We probably need to set the locale environment variable.
------- Comment #2 From 2011-09-23 17:14:46 PST -------
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 From 2011-10-29 12:52:54 PST -------
Created an attachment (id=112973) [details]
Patch
------- Comment #4 From 2011-10-29 12:53:14 PST -------
Hopefully this fixes your issue.
------- Comment #5 From 2011-10-29 13:10:59 PST -------
(From update of attachment 112973 [details])
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 From 2011-10-29 14:00:42 PST -------
Created an attachment (id=112975) [details]
Patch for landing
------- Comment #7 From 2011-10-29 15:07:06 PST -------
(From update of attachment 112975 [details])
Clearing flags on attachment: 112975

Committed r98819: <http://trac.webkit.org/changeset/98819>
------- Comment #8 From 2011-10-29 15:07:11 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #9 From 2011-10-29 17:38:29 PST -------
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 From 2011-10-29 19:54:52 PST -------
Fixing.
------- Comment #11 From 2011-10-29 20:06:21 PST -------
Committed r98823: <http://trac.webkit.org/changeset/98823>
------- Comment #12 From 2011-10-29 21:32:05 PST -------
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 From 2011-10-29 23:31:39 PST -------
Sorry.  Changes like this one are very dangerous. :(  Will fix shortly.
------- Comment #15 From 2011-10-30 00:56:45 PST -------
Created an attachment (id=112989) [details]
Patch for landing
------- Comment #16 From 2011-10-30 00:57:15 PST -------
Committed r98825: <http://trac.webkit.org/changeset/98825>
------- Comment #17 From 2011-10-30 00:57:49 PST -------
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 From 2011-10-30 10:54:47 PST -------
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 From 2011-10-30 13:01:37 PST -------
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 From 2011-10-30 13:02:25 PST -------
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 From 2011-10-30 13:26:12 PST -------
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 From 2011-10-30 15:23:33 PST -------
I will make CrWin copy the entire environment over.  Fixing now.
------- Comment #23 From 2011-10-30 15:42:46 PST -------
Committed r98830: <http://trac.webkit.org/changeset/98830>
------- Comment #24 From 2011-10-30 15:43:15 PST -------
Hopefully that will fix things.  Let me know if you see any further trouble.
------- Comment #25 From 2011-10-30 16:39:52 PST -------
(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 From 2011-10-30 16:47:25 PST -------
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 From 2011-10-30 16:49:03 PST -------
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 From 2011-10-30 18:43:30 PST -------
I'm going to rollout these changesets for now since we're losing 100% of layout test coverage due to this failure.
------- Comment #30 From 2011-10-30 18:50:20 PST -------
Reopen the bug since the changeset was rolled out in http://trac.webkit.org/changeset/98833.
------- Comment #31 From 2011-10-30 18:53:18 PST -------
(From update of attachment 112989 [details])
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 From 2011-10-30 20:52:36 PST -------
Thanks for your help, rniwa.
------- Comment #33 From 2011-11-09 13:53:40 PST -------
Created an attachment (id=114358) [details]
Set LC_ALL to C

The quickest fix for Unix environments would be to set "LC_ALL" to "C".
------- Comment #34 From 2011-11-09 13:56:19 PST -------
(From update of attachment 114358 [details])
Yes, I have a more complete fix which I"ll be posting shortly.
------- Comment #35 From 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 From 2011-11-09 16:11:50 PST -------
Seems like tests are expecting LC_ALL = "En_US"
------- Comment #37 From 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 From 2011-11-14 12:24:11 PST -------
Created an attachment (id=115004) [details]
Patch to land
------- Comment #39 From 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 From 2011-11-14 12:29:27 PST -------
(From update of attachment 115004 [details])
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 From 2011-11-14 12:29:59 PST -------
Created an attachment (id=115005) [details]
Patch for landing

Oops, my changelog was wrong
------- Comment #42 From 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 From 2011-11-14 12:35:50 PST -------
Created an attachment (id=115008) [details]
Patch

Hopefully, fixed style errors
------- Comment #44 From 2011-11-14 12:49:13 PST -------
Created an attachment (id=115013) [details]
Patch

Updated with review comments.
------- Comment #45 From 2011-11-14 13:00:27 PST -------
(From update of attachment 115013 [details])
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 From 2011-11-14 13:17:17 PST -------
Created an attachment (id=115018) [details]
Patch
------- Comment #47 From 2011-11-14 14:10:46 PST -------
(From update of attachment 115018 [details])
Fantastic!
------- Comment #48 From 2011-11-14 14:11:35 PST -------
I will land my more complicated patch again (using a separate bug) later this week.
------- Comment #49 From 2011-11-14 14:29:20 PST -------
(From update of attachment 115018 [details])
Clearing flags on attachment: 115018

Committed r100192: <http://trac.webkit.org/changeset/100192>
------- Comment #50 From 2011-11-14 14:29:28 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #52 From 2011-11-14 16:41:37 PST -------
That's so odd.  The change was designed to fix exactly those sorts of failures.
------- Comment #53 From 2011-11-14 16:45:10 PST -------
(From update of attachment 115018 [details])
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 From 2011-11-15 16:07:28 PST -------
(From update of attachment 115018 [details])
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 From 2011-11-17 13:34:02 PST -------
John's patch was rolled out in bug 72328.
------- Comment #56 From 2011-11-17 14:05:38 PST -------
Created an attachment (id=115673) [details]
updated to tip of tree
------- Comment #57 From 2011-11-17 14:13:50 PST -------
Committed r100674: <http://trac.webkit.org/changeset/100674>
------- Comment #58 From 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 From 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 From 2011-11-18 11:55:13 PST -------
Created an attachment (id=115842) [details]
Patch

Override LANG, LANGUAGE and LC_ALL to use en_US.UTF-8
------- Comment #61 From 2011-11-18 11:58:38 PST -------
(From update of attachment 115842 [details])
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 From 2011-11-18 12:23:10 PST -------
(From update of attachment 115842 [details])
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 From 2011-11-18 12:55:25 PST -------
Created an attachment (id=115850) [details]
Patch

See new patch with exception handling
------- Comment #64 From 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 From 2011-11-18 13:15:34 PST -------
Created an attachment (id=115856) [details]
Patch

Fix style
------- Comment #66 From 2011-11-21 12:56:40 PST -------
Created an attachment (id=116119) [details]
patch

review anybody?
------- Comment #67 From 2011-11-21 13:04:34 PST -------
(From update of attachment 116119 [details])
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 From 2011-11-22 14:33:16 PST -------
(From update of attachment 115018 [details])
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 From 2011-11-22 15:16:32 PST -------
Created an attachment (id=116280) [details]
Patch
------- Comment #70 From 2011-11-23 10:34:32 PST -------
(From update of attachment 116280 [details])
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 From 2011-11-23 11:12:13 PST -------
(From update of attachment 116280 [details])
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 From 2011-11-23 14:12:42 PST -------
Created an attachment (id=116425) [details]
Patch

Any luck this is a good patch?
------- Comment #73 From 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 From 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 From 2011-11-23 14:28:14 PST -------
Created an attachment (id=116429) [details]
patch

Ok, last try.
------- Comment #76 From 2011-11-28 12:48:45 PST -------
Committed r101274: <http://trac.webkit.org/changeset/101274>
------- Comment #77 From 2011-11-28 12:49:09 PST -------
(From update of attachment 116429 [details])
I modified your patch slightly and landed it
------- Comment #78 From 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 From 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.