105380 2012-12-18 19:36:30 -0800 [chromium] webkitpy-test: executive.py stringify_args error on the release test bot 2013-01-13 19:18:23 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 noel.gordon dpranke abarth dpranke eric jyasskin ojan rniwa tony webkit.review.bot oldest_to_newest 794282 0 noel.gordon 2012-12-18 19:36:30 -0800 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 794284 1 eric 2012-12-18 19:41:41 -0800 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. 795859 2 eric 2012-12-20 15:20:54 -0800 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. 796064 3 180461 dpranke 2012-12-20 19:37:53 -0800 Created attachment 180461 Patch 796067 4 dpranke 2012-12-20 19:38:58 -0800 (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. 796070 5 180461 eric 2012-12-20 19:45:47 -0800 Comment on attachment 180461 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. 796076 6 dpranke 2012-12-20 19:53:04 -0800 (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 :). 796078 7 eric 2012-12-20 20:06:35 -0800 (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. :) 796082 8 eric 2012-12-20 20:10:35 -0800 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. 796083 9 eric 2012-12-20 20:11:18 -0800 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. :) 796092 10 eric 2012-12-20 20:21:53 -0800 Dirk: It would be nice if we could come up with a way to test this on non-windows... 799581 11 dpranke 2013-01-03 12:44:47 -0800 *** Bug 105989 has been marked as a duplicate of this bug. *** 806039 12 180461 eric 2013-01-13 18:21:13 -0800 Comment on attachment 180461 Patch Red bots make noel sad. 806040 13 180461 webkit.review.bot 2013-01-13 18:24:12 -0800 Comment on attachment 180461 Patch Clearing flags on attachment: 180461 Committed r139579: <http://trac.webkit.org/changeset/139579> 806041 14 webkit.review.bot 2013-01-13 18:24:16 -0800 All reviewed patches have been landed. Closing bug. 806049 15 noel.gordon 2013-01-13 19:18:23 -0800 (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 180461 2012-12-20 19:37:53 -0800 2013-01-13 18:24:12 -0800 Patch bug-105380-20121220193509.patch text/plain 1661 dpranke U3VidmVyc2lvbiBSZXZpc2lvbjogMTM4MzI3CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggNGE5N2NjYWI1ZWVkN2VmOGJmOTZjNWVlODU4ZTM0MDM3 Mjk2MmY0NS4uNTQ0OWRmYTQzNmM4Yjg4NGI1OGQ5MmRlZGQ3ZmI5YzYxNjljNmNjYyAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4 IEBACisyMDEyLTEyLTIwICBEaXJrIFByYW5rZSAgPGRwcmFua2VAY2hyb21pdW0ub3JnPgorCisg ICAgICAgIFtjaHJvbWl1bV0gd2Via2l0cHktdGVzdDogZXhlY3V0aXZlLnB5IHN0cmluZ2lmeV9h cmdzIGVycm9yIG9uIHRoZSByZWxlYXNlIHRlc3QgYm90CisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDUzODAKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXggYSByZWdyZXNzaW9uIGludHJvZHVjZWQgaW4g cjEzNzY5MiB3aGVyZSB3ZSB3ZXJlIGRvdWJsZS1lbmNvZGluZworICAgICAgICB0aGUgYXJndW1l bnRzIHRvIHBvcGVuKCk7IHRoaXMgd2FzIG9ubHkgYW4gaXNzdWUgb24gd2luZG93cywgd2hlcmUK KyAgICAgICAgd2Ugd291bGQgdHJ5IHRvIGVuY29kZSBzb21ldGhpbmcgdG8gbWJjcywgdGhlbiB0 cnkgdG8gZW5jb2RlIGl0CisgICAgICAgIHRvIHVuaWNvZGUgYXMgaWYgdGhlIGlucHV0IGFzIGFz Y2lpLgorCisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9jb21tb24vc3lzdGVtL2V4ZWN1dGl2 ZS5weToKKyAgICAgICAgKEV4ZWN1dGl2ZS5ydW5fY29tbWFuZCk6CisKIDIwMTItMTItMjAgIFJ5 dWFuIENob2kgIDxyeXVhbi5jaG9pQHNhbXN1bmcuY29tPgogCiAgICAgICAgIFtFRkxdIEJ1aWxk IGJyZWFrIHdpdGggbGF0ZXN0IEVGTCBsaWJyYXJpZXMuCmRpZmYgLS1naXQgYS9Ub29scy9TY3Jp cHRzL3dlYmtpdHB5L2NvbW1vbi9zeXN0ZW0vZXhlY3V0aXZlLnB5IGIvVG9vbHMvU2NyaXB0cy93 ZWJraXRweS9jb21tb24vc3lzdGVtL2V4ZWN1dGl2ZS5weQppbmRleCA0MGI4NzdlMTQ3MTFmMzQz ZmRkNGNmMzY5NzgzMzI5OWIyNTU2MzNjLi5mNWExYjViZTkwMTE2YTE0YzdlMjYxMmUwZTFmZmMx Y2Y2Zjg1MGE0IDEwMDY0NAotLS0gYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9zeXN0 ZW0vZXhlY3V0aXZlLnB5CisrKyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL3N5c3Rl bS9leGVjdXRpdmUucHkKQEAgLTQwNiw3ICs0MDYsNiBAQCBjbGFzcyBFeGVjdXRpdmUob2JqZWN0 KToKICAgICAgICAgIiIiUG9wZW4gd3JhcHBlciBmb3IgY29udmVuaWVuY2UgYW5kIHRvIHdvcmsg YXJvdW5kIHB5dGhvbiBidWdzLiIiIgogICAgICAgICBhc3NlcnQoaXNpbnN0YW5jZShhcmdzLCBs aXN0KSBvciBpc2luc3RhbmNlKGFyZ3MsIHR1cGxlKSkKICAgICAgICAgc3RhcnRfdGltZSA9IHRp bWUudGltZSgpCi0gICAgICAgIGFyZ3MgPSBzZWxmLl9zdHJpbmdpZnlfYXJncyhhcmdzKQogCiAg ICAgICAgIHN0ZGluLCBzdHJpbmdfdG9fY29tbXVuaWNhdGUgPSBzZWxmLl9jb21wdXRlX3N0ZGlu KGlucHV0KQogICAgICAgICBzdGRlcnIgPSBzZWxmLlNURE9VVCBpZiByZXR1cm5fc3RkZXJyIGVs c2UgTm9uZQo=