Summary: | webkit-patch doesn't like UTF-8 characters in reviewers names | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||||
Component: | Tools / Tests | Assignee: | Tom Zakrajsek <tomz> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Blocker | CC: | abarth, abecsi, eric, galpeter, ossy, tomz, vestbo, webkit.review.bot | ||||||||
Priority: | P1 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 50843 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Csaba Osztrogonác
2011-06-27 06:55:52 PDT
Kristóf, could you check this bug, please? I believe this is related to us setting LANG=ascii on the bots to work around a gcc issue. See bug 50843. I set it to P1 blocker, because webkit-patch and CQ can't land patches reviewed by myself. :( Peter, could you check this bug with Kristof ASAP? Sigh. I thought this had gone away. I can take another look. A test case (patch) would be most helpful. 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. :( It's on the (growing) list of bugs I'd like to fix. :( @Eric: time to create a meta bug and burn through these? Created attachment 108853 [details]
Patch
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. 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! 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 on attachment 108853 [details] Patch 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. Created attachment 108885 [details]
Patch
For more information, see "Python Cookbook 2nd Ed., recipe 1.22" and http://docs.python.org/library/codecs.html#module-codecs. Comment on attachment 108885 [details]
Patch
Thanks.
Comment on attachment 108885 [details] Patch Clearing flags on attachment: 108885 Committed r96161: <http://trac.webkit.org/changeset/96161> All reviewed patches have been landed. Closing bug. Unfortunately CQ still hates me. :( (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 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.
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]
CQ works fine nowadays with patches reviewed by myself or Tor Arne :) |