Bug 63452 - webkit-patch doesn't like UTF-8 characters in reviewers names
Summary: webkit-patch doesn't like UTF-8 characters in reviewers names
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Blocker
Assignee: Tom Zakrajsek
Depends on: 50843
  Show dependency treegraph
Reported: 2011-06-27 06:55 PDT by Csaba Osztrogonác
Modified: 2012-11-22 03:54 PST (History)
8 users (show)

See Also:

Patch (1.31 KB, patch)
2011-09-27 08:48 PDT, Tom Zakrajsek
no flags Details | Formatted Diff | Diff
Test case to show (405 bytes, text/x-python)
2011-09-27 11:49 PDT, Tom Zakrajsek
no flags Details
Patch (1.59 KB, patch)
2011-09-27 12:51 PDT, Tom Zakrajsek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2011-06-27 06:55:52 PDT

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 1

Logging in as webkit.review.bot@gmail.com...
Fetching: https://bugs.webkit.org/attachment.cgi?id=97789&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=61890&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 97789 from bug 61890.
Traceback (most recent call last):
  File "./Tools/Scripts/webkit-patch", line 66, in <module>
  File "./Tools/Scripts/webkit-patch", line 61, in main
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 308, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 117, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/commands/download.py", line 152, in execute
    self._process_patch(patch, options, args, tool)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/commands/download.py", line 171, in _process_patch
    self._main_sequence.run_and_handle_errors(tool, options, state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 70, in run_and_handle_errors
    self._run(tool, options, state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/applypatchwithlocalcommit.py", line 40, in run
    ApplyPatch.run(self, state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/applypatch.py", line 43, in run
    self._tool.checkout().apply_patch(state["patch"], force=self._options.non_interactive or self._options.force_patch)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/checkout/checkout.py", line 163, in apply_patch
    run_command(args, input=patch.contents())
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/system/executive.py", line 102, in run_command
    return Executive().run_command(*args, **kwargs)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/system/executive.py", line 382, in run_command
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/system/executive.py", line 438, in popen
    return subprocess.Popen(*args, **kwargs)
  File "/usr/lib/python2.6/subprocess.py", line 623, in __init__
    errread, errwrite)
  File "/usr/lib/python2.6/subprocess.py", line 1141, in _execute_child
    raise child_exception
TypeError: execv() arg 2 must contain only strings
Comment 1 Csaba Osztrogonác 2011-06-27 06:57:30 PDT
Kristóf, could you check this bug, please?
Comment 2 Eric Seidel (no email) 2011-06-27 10:15:59 PDT
I believe this is related to us setting LANG=ascii on the bots to work around a gcc issue.  See bug 50843.
Comment 3 Csaba Osztrogonác 2011-08-01 03:08:44 PDT
I set it to P1 blocker, because webkit-patch and CQ can't land patches reviewed by myself. :(
Comment 4 Csaba Osztrogonác 2011-08-01 03:09:34 PDT
Peter, could you check this bug with Kristof ASAP?
Comment 5 Eric Seidel (no email) 2011-08-08 09:29:11 PDT
Sigh.  I thought this had gone away.  I can take another look.  A test case (patch) would be most helpful.
Comment 6 Csaba Osztrogonác 2011-09-07 02:30:00 PDT
Guys wasn't able to reproduce it locally, but unfortunately it is still valid on the commit queue machine: https://bugs.webkit.org/show_bug.cgi?id=67646#c2

Eric or someone else who has access to this bot, could you check it, please?
It is so annoying that we can't land patches reviewed by me with the commit queue. :(
Comment 7 Eric Seidel (no email) 2011-09-07 11:38:06 PDT
It's on the (growing) list of bugs I'd like to fix. :(
Comment 8 Adam Barth 2011-09-07 11:43:24 PDT
@Eric: time to create a meta bug and burn through these?
Comment 9 Tom Zakrajsek 2011-09-27 08:48:31 PDT
Created attachment 108853 [details]
Comment 10 Tom Zakrajsek 2011-09-27 08:52:52 PDT
I'm not sure how to set up a regression test for this.  I can test that the mod (wrapping stdio with a codec) works in a standalone unit-test, but it's a lot more invasive to try to force unicode text through the output of webkit-patch itself.
Comment 11 Adam Barth 2011-09-27 09:53:48 PDT
Eric is probably your best reviewer here.  Generally, it's hard to test changes to the webkit-patch script self (as opposed to the webkitpy library).  I suspec this change wi'll have somewhat wide-ranging effects that will be difficult to predict, but we shouldn't let that stop us!
Comment 12 Tom Zakrajsek 2011-09-27 11:49:57 PDT
Created attachment 108876 [details]
Test case to show

Uploading an example script to demonstrate the problem/solution in a minimal case.  To see the issue, run the script and redirect/tee the output.
Comment 13 Eric Seidel (no email) 2011-09-27 12:21:46 PDT
Comment on attachment 108853 [details]

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

> Tools/Scripts/webkit-patch:41
> +sys.stdout = codecs.lookup('utf-8')[-1](sys.stdout)

Please add a (brief) comment explaing why we do this, and linking to this bug.  You should also consider linking from this bug or your ChangeLog to places on the web where they explain why this is the right way to fix this.   Just some sort of paper trail so that someone can understand later why this change was made.
Comment 14 Tom Zakrajsek 2011-09-27 12:51:44 PDT
Created attachment 108885 [details]
Comment 15 Tom Zakrajsek 2011-09-27 12:56:16 PDT
For more information, see "Python Cookbook 2nd Ed., recipe 1.22" and http://docs.python.org/library/codecs.html#module-codecs.
Comment 16 Eric Seidel (no email) 2011-09-27 14:01:48 PDT
Comment on attachment 108885 [details]

Comment 17 WebKit Review Bot 2011-09-27 15:03:27 PDT
Comment on attachment 108885 [details]

Clearing flags on attachment: 108885

Committed r96161: <http://trac.webkit.org/changeset/96161>
Comment 18 WebKit Review Bot 2011-09-27 15:03:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Csaba Osztrogonác 2011-09-28 12:55:49 PDT
Unfortunately CQ still hates me. :(
Comment 20 Csaba Osztrogonác 2011-09-28 13:29:26 PDT
(In reply to comment #19)
> Unfortunately CQ still hates me. :(

I missed to add the URL: https://bugs.webkit.org/show_bug.cgi?id=69004#c4
Comment 21 Eric Seidel (no email) 2011-11-07 12:36:10 PST
Another reproduction:
webkit-patch apply-attachment 113861 [details]

That doesn't repro on my local machine, but I suspect it will repro on our EWS instances.  I'll figure out how to SSH into one and try it.
Comment 22 Eric Seidel (no email) 2011-11-07 13:28:11 PST
The EC2 instances use some environment variables to work around fancy-quotes in GCC causing unicode decoding exceptions during other parts of processing.  Those environment variables are what's causing the problem here.  We may need to revisit which ones we set.

The following invocation of the command will reproduce this bug *on ec2* it still does not reproduce locally on my mac.

LANG=en_US.US-ASCII LC_ALL=C PYTHONIOENCODING=utf_8 ./Tools/Scripts/webkit-patch apply-attachment 113861 [details]
Comment 23 Csaba Osztrogonác 2012-11-22 03:54:31 PST
CQ works fine nowadays with patches reviewed by myself or Tor Arne :)