Bug 68691

We probably need to set the locale environment variable.

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

Created attachment 112973 [details] Patch

Hopefully this fixes your issue.

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

Created attachment 112975 [details] Patch for landing

Comment on attachment 112975 [details] Patch for landing Clearing flags on attachment: 112975 Committed r98819: <http://trac.webkit.org/changeset/98819>

All reviewed patches have been landed. Closing bug.

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'

Fixing.

Committed r98823: <http://trac.webkit.org/changeset/98823>

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'

Sorry. Changes like this one are very dangerous. :( Will fix shortly.

Created attachment 112989 [details] Patch for landing

Committed r98825: <http://trac.webkit.org/changeset/98825>

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. :(

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

I suspect CrWin expects more environment to be transfered to lighttpd when called. I can make Chromium ports copy the entire environment over instead?

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

http://trac.webkit.org/browser/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg I don't see anything special for cr-win though.

I will make CrWin copy the entire environment over. Fixing now.

Committed r98830: <http://trac.webkit.org/changeset/98830>

Hopefully that will fix things. Let me know if you see any further trouble.

(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.

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

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.

Still broken: http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/20282/steps/layout-test/logs/stdio

I'm going to rollout these changesets for now since we're losing 100% of layout test coverage due to this failure.

Reopen the bug since the changeset was rolled out in http://trac.webkit.org/changeset/98833.

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

Thanks for your help, rniwa.

Created attachment 114358 [details] Set LC_ALL to C The quickest fix for Unix environments would be to set "LC_ALL" to "C".

Comment on attachment 114358 [details] Set LC_ALL to C Yes, I have a more complete fix which I"ll be posting shortly.

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.

Seems like tests are expecting LC_ALL = "En_US"

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.

Created attachment 115004 [details] Patch to land

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 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.

Created attachment 115005 [details] Patch for landing Oops, my changelog was wrong

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.

Created attachment 115008 [details] Patch Hopefully, fixed style errors

Created attachment 115013 [details] Patch Updated with review comments.

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.

Created attachment 115018 [details] Patch

Comment on attachment 115018 [details] Patch Fantastic!

I will land my more complicated patch again (using a separate bug) later this week.

Comment on attachment 115018 [details] Patch Clearing flags on attachment: 115018 Committed r100192: <http://trac.webkit.org/changeset/100192>

All reviewed patches have been landed. Closing bug.

This make lots of failures like this: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r100194%20(34692)/fast/repaint/moving-shadow-on-path-pretty-diff.html?format=txt

That's so odd. The change was designed to fix exactly those sorts of failures.

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 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

John's patch was rolled out in bug 72328.

Created attachment 115673 [details] updated to tip of tree

Committed r100674: <http://trac.webkit.org/changeset/100674>

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'.

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.

Created attachment 115842 [details] Patch Override LANG, LANGUAGE and LC_ALL to use en_US.UTF-8

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 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.

Created attachment 115850 [details] Patch See new patch with exception handling

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.

Created attachment 115856 [details] Patch Fix style

Created attachment 116119 [details] patch review anybody?

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 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.

Created attachment 116280 [details] Patch

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 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?

Created attachment 116425 [details] Patch Any luck this is a good patch?

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.

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...

Created attachment 116429 [details] patch Ok, last try.

Committed r101274: <http://trac.webkit.org/changeset/101274>

Comment on attachment 116429 [details] patch I modified your patch slightly and landed it

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)

We would just need to change test-webkitpy to instantiate a real Host() once, in order to have the environment hack engage.

Created attachment 405603 [details] Patch for landing (r101274)

Comment on attachment 405603 [details] Patch for landing (r101274) View in context: https://bugs.webkit.org/attachment.cgi?id=405603&action=review > Tools/Scripts/webkitpy/common/host.py:87 > + os.environ['LC_ALL'] = '' This causes a trouble for me. Filed: Bug 214983 – webkitpy: If LC_ALL is set to a empty string, svn doesn't use the password store