http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/32021/steps/webkitpy-test/logs/stdio Suppressing most webkitpy logging while running unit tests. Skipping tests in the following modules or packages because they are really, really, slow: webkitpy.common.checkout.scm.scm_unittest (https://bugs.webkit.org/show_bug.cgi?id=31818; use --all to include) Skipping tests in the following modules or packages because they fail horribly on win32: webkitpy.common.checkout webkitpy.common.config webkitpy.tool (https://bugs.webkit.org/show_bug.cgi?id=54526; use --all to include) 11Checking autoinstalled packages ... Checking imports ... Finding the individual test methods ... Running the tests ... [111/1309] webkitpy.common.system.executive_unittest.ExecutiveTest.test_run_command_with_unicode erred: Traceback (most recent call last): File "E:\google-windows-4\chromium-win-release-tests\build\Tools\Scripts\webkitpy\common\system\executive_unittest.py", line 148, in test_run_command_with_unicode output = executive.run_command(command_line('echo', unicode_tor_input)) File "E:\google-windows-4\chromium-win-release-tests\build\Tools\Scripts\webkitpy\common\system\executive.py", line 419, in run_command close_fds=self._should_close_fds()) File "E:\google-windows-4\chromium-win-release-tests\build\Tools\Scripts\webkitpy\common\system\executive.py", line 489, in popen string_args = self._stringify_args(args) File "E:\google-windows-4\chromium-win-release-tests\build\Tools\Scripts\webkitpy\common\system\executive.py", line 476, in _stringify_args string_args = map(unicode, args) UnicodeDecodeError: 'ascii' codec can't decode byte 0xf8 in position 23: ordinal not in range(128) [904/1309] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_run_part passed Ran 1309 tests in 17.675s FAILED (failures=0, errors=1) program finished with exit code 1 elapsedTime=19.594000
I guess I'm not surprised by this. But it means we need to fix: http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/system/executive.py#L469 presumably to have a fallback character when the encoding fails. This is basically a real bug which we've had, but are now finally testing. :) This is just telling us that unicode-based contributor names (and any other unicode) currently cause webkit-patch to barf on windows.
I believe we just change that line to .encode(encoding, errors="replace") and it should do something sensible. The unittest will still fail, so we'd have to skip it for windows. I can't easily fix this w/o a windows machine. We can just skip the unittest. Dirk may have other thoughts.
Created attachment 180461 [details] Patch
(In reply to comment #2) > I believe we just change that line to .encode(encoding, errors="replace") and it should do something sensible. The unittest will still fail, so we'd have to skip it for windows. I can't easily fix this w/o a windows machine. We can just skip the unittest. Dirk may have other thoughts. No, I don't think we want to do that ... I think it was just a simple bug in your earlier patch where we were dual-encoding things. I've posted a fix and verified it works on both win and mac.
Comment on attachment 180461 [details] Patch This will cause the args argument of ScriptError to not be encoded. But maybe that's fine. It's possible the map(unicode, args) should be separated from the encode(), and I should not have grouped them both in _stringify. We can map(unicode) all we want, but we can only encode once, and need to do it right before we call out to subprocess.
(In reply to comment #5) > (From update of attachment 180461 [details]) > This will cause the args argument of ScriptError to not be encoded. But maybe that's fine. It's possible the map(unicode, args) should be separated from the encode(), and I should not have grouped them both in _stringify. We can map(unicode) all we want, but we can only encode once, and need to do it right before we call out to subprocess. Yeah. I would think either we should leave the args alone in ScriptError (thus capturing what was actually passed to run_command, rather than the encoded version), or stringify in the call to the ScriptError constructor. I'd probably vote for the former. See, and I told you to leave the stringifying alone :).
(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 180461 [details] [details]) > > This will cause the args argument of ScriptError to not be encoded. But maybe that's fine. It's possible the map(unicode, args) should be separated from the encode(), and I should not have grouped them both in _stringify. We can map(unicode) all we want, but we can only encode once, and need to do it right before we call out to subprocess. > > Yeah. I would think either we should leave the args alone in ScriptError (thus capturing what was actually passed to run_command, rather than the encoded version), or stringify in the call to the ScriptError constructor. I'd probably vote for the former. > > See, and I told you to leave the stringifying alone :). Yes. :) But now we understand what's going on! So I think the ideal fix IMO, is to get rid of _stringify_args, and instead manually run map(unicode, args) in the places we need to, and _encode_if_needed in the places we need to. Your current patch is also fine, and I'll r+ it. We can also land yours and I can look at "cleaning this up" more (aka breaking windows harder!) later. :)
The problem (as I see it), is that I attempted to get too cutesy in code, and ended up conflating two concepts. 1. The convenience of being able to pass non-string arguments (aka numbers) to run_command, popen, etc. and not have subprocess throw kittens at us. 2. The necessity of working around sucky windows unicode support by encoding to the local code-page before calling subprocess. Both of these are really things that I think subprocess should do for us... :) But I can see why they chose to make it simple/rigid like they did.
It's also really dangerous that I now know how to CC a former maintainer of subprocess on webkit bugs! Sorry Jeffery. Well, kinda sorry. :)
Dirk: It would be nice if we could come up with a way to test this on non-windows...
*** Bug 105989 has been marked as a duplicate of this bug. ***
Comment on attachment 180461 [details] Patch Red bots make noel sad.
Comment on attachment 180461 [details] Patch Clearing flags on attachment: 180461 Committed r139579: <http://trac.webkit.org/changeset/139579>
All reviewed patches have been landed. Closing bug.
(In reply to comment #12) > (From update of attachment 180461 [details]) > Red bots make noel sad. Some green over there would be nice for a change: http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29?numbuilds=200