Bug 181254 - run-webkit-tests fails when there is a curly brace in Xcode build output
Summary: run-webkit-tests fails when there is a curly brace in Xcode build output
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-03 12:25 PST by Alexey Proskuryakov
Modified: 2018-01-19 11:16 PST (History)
9 users (show)

See Also:


Attachments
proposed fix (1.22 KB, patch)
2018-01-03 12:27 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
Patch (2.23 KB, patch)
2018-01-12 09:35 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (1.30 KB, patch)
2018-01-12 14:50 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (1.29 KB, patch)
2018-01-16 09:01 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (1.36 KB, patch)
2018-01-17 11:53 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2018-01-03 12:25:27 PST
I'm currently in a state where Xcode outputs something with a curly quote when building, and that breaks run-webkit-tests (which calls build-dumprendertree).

UnicodeEncodeError raised: 'ascii' codec can't encode character u'\u201c' in position 1514: ordinal not in range(128)
Traceback (most recent call last):
  File "/Users/ap/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 85, in main
    run_details = run(port, options, args, stderr)
  File "/Users/ap/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 445, in run
    run_details = manager.run(args)
  File "/Users/ap/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 246, in run
    if not self._set_up_run(tests_to_run):
  File "/Users/ap/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 164, in _set_up_run
    if not self._port.check_build(self.needs_servers(test_names)):
  File "/Users/ap/Safari/OpenSource/Tools/Scripts/webkitpy/port/base.py", line 230, in check_build
    if not self._root_was_set and self.get_option('build') and not self._build_driver():
  File "/Users/ap/Safari/OpenSource/Tools/Scripts/webkitpy/port/base.py", line 1438, in _build_driver
    self._run_script("build-dumprendertree", args=self._build_driver_flags(), env=env)
  File "/Users/ap/Safari/OpenSource/Tools/Scripts/webkitpy/port/base.py", line 1428, in _run_script
    _log.debug('Output of %s:\n%s' % (run_script_command, output))
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/logging/__init__.py", line 1153, in debug
    self._log(DEBUG, msg, args, **kwargs)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/logging/__init__.py", line 1284, in _log
    self.handle(record)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/logging/__init__.py", line 1294, in handle
    self.callHandlers(record)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/logging/__init__.py", line 1334, in callHandlers
    hdlr.handle(record)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/logging/__init__.py", line 757, in handle
    self.emit(record)
  File "/Users/ap/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/views/metered_stream.py", line 135, in emit
    self._meter.writeln(record.getMessage(), record.created, record.process)
  File "/Users/ap/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/views/metered_stream.py", line 111, in writeln
    self.write(self._ensure_newline(txt), now, pid)
  File "/Users/ap/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/views/metered_stream.py", line 108, in write
    self._stream.write(msg)
UnicodeEncodeError: 'ascii' codec can't encode character u'\u201c' in position 1514: ordinal not in range(128)
Comment 1 Alexey Proskuryakov 2018-01-03 12:27:12 PST
Created attachment 330409 [details]
proposed fix

I don't really know what I'm doing here, just patched it enough to work on my machine. Thoughts?
Comment 2 Daniel Bates 2018-01-11 10:33:59 PST
Comment on attachment 330409 [details]
proposed fix

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

> Tools/Scripts/webkitpy/layout_tests/views/metered_stream.py:108
> +        self._stream.write(msg.encode('utf-8'))

This makes the assumption that the output shell is using UTF-8 encoding. Is this the default shell encoding on macOS, Linux, Windows? Alternatively we could appropriately encode the output we get when running build-dumprendertree.
Comment 3 Alexey Proskuryakov 2018-01-11 10:39:38 PST
> Is this the default shell encoding on macOS, Linux, Windows?

I don't know how to answer that, or what the code was doing by default. I only know that this fixes my local build.
Comment 4 Alexey Proskuryakov 2018-01-11 15:50:48 PST
Comment on attachment 330409 [details]
proposed fix

Please feel encouraged to roll back if this breaks anything.
Comment 5 WebKit Commit Bot 2018-01-11 16:15:18 PST
Comment on attachment 330409 [details]
proposed fix

Clearing flags on attachment: 330409

Committed r226816: <https://trac.webkit.org/changeset/226816>
Comment 6 WebKit Commit Bot 2018-01-11 16:15:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2018-01-11 16:20:38 PST
<rdar://problem/36453374>
Comment 8 Ryan Haddad 2018-01-11 18:17:29 PST
This broke LayoutTests on the bots:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/builds/2213
Comment 9 Ryan Haddad 2018-01-11 18:19:30 PST
Reverted r226816 for reason:

This change broke LayoutTests on the bots.

Committed r226837: <https://trac.webkit.org/changeset/226837>
Comment 10 Jonathan Bedard 2018-01-12 08:33:03 PST
I'm not sure that this should be fixed in webkitpy.

I've seen a similar issue where LEFT and RIGHT quotations coming from xcodebuild have the same problem.  Perhaps we should place a filter around calls to xcodebuild in perl.
Comment 11 Jonathan Bedard 2018-01-12 09:35:46 PST
Created attachment 331207 [details]
Patch
Comment 12 Jonathan Bedard 2018-01-12 09:37:37 PST
(In reply to Jonathan Bedard from comment #11)
> Created attachment 331207 [details]
> Patch

Talking with Alexey, it seems that we are not willing to filter in build-webkit. I think we need a larger discussion of which string encoding we would like to standardize in webkitpy, but for the time being, we need to prevent exceptions.
Comment 13 Daniel Bates 2018-01-12 11:47:55 PST
Comment on attachment 331207 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/views/metered_stream.py:109
> +        self._stream.write(msg.encode(self._encoding, 'replace'))

Did you confirm that this actually fixes the issue? To be more specific, were you able to reproduce the original bug? And did you verify that this actually fixes the bug? From briefly looking at the backtrace in <https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/builds/2213/steps/layout-test/logs/stdio> I get the impression that the bug was that we were UTF-8 encoding twice (e.g. u"\u2013".encode('utf-8').encode('utf-8')). Assuming this is the bug then I would expect the fix to be that we UTF-8 encode |output| before perform string interpolation and passing the result to _log.debug() as seen in the original backtrace:

 File "/Users/ap/Safari/OpenSource/Tools/Scripts/webkitpy/port/base.py", line 1428, in _run_script
    _log.debug('Output of %s:\n%s' % (run_script_command, output))
Comment 14 Jonathan Bedard 2018-01-12 14:08:23 PST
Comment on attachment 331207 [details]
Patch

I had not yet verified that this fixed the new bug.

I managed to reproduce the new bug locally, the proposed patch does not address it.
Comment 15 Jonathan Bedard 2018-01-12 14:50:44 PST
Created attachment 331238 [details]
Patch
Comment 16 Jonathan Bedard 2018-01-12 14:53:42 PST
(In reply to Jonathan Bedard from comment #15)
> Created attachment 331238 [details]
> Patch

This will address Alexey's problem without the side-effects.
Comment 17 Daniel Bates 2018-01-12 15:27:24 PST
Comment on attachment 331238 [details]
Patch

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

> Tools/Scripts/webkitpy/port/base.py:1429
> +        if decode_output:
> +            output = output.decode('utf-8')

This does not make sense because |output| represents the decoded output when decode_output is True. Please start reading from the last sentence of the first paragraph in comment #13 again.
Comment 18 Jonathan Bedard 2018-01-16 09:01:36 PST
Created attachment 331390 [details]
Patch
Comment 19 Jonathan Bedard 2018-01-16 09:35:36 PST
Comment on attachment 331390 [details]
Patch

I looked closer at how I was attempting to reproduce the failure that Alexey saw, and realized that I have yet to actually reproduce it. Dan's comments were accurate, and this change essentially applies Alexey's previous patch to only decoded script runs.
Comment 20 Daniel Bates 2018-01-17 11:40:46 PST
Comment on attachment 331390 [details]
Patch

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

> Tools/Scripts/webkitpy/port/base.py:1430
> +        if decode_output:
> +            output = output.encode('utf-8')
>          _log.debug('Output of %s:\n%s' % (run_script_command, output))

We should only encode |output| for the purpose of calling _log.debug(). That is, we should not return the encoded data because the caller expects a string of characters (not a string of bytes).
Comment 21 Jonathan Bedard 2018-01-17 11:53:08 PST
Created attachment 331528 [details]
Patch
Comment 22 Jonathan Bedard 2018-01-17 11:54:44 PST
(In reply to Daniel Bates from comment #20)
> Comment on attachment 331390 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331390&action=review
> 
> > Tools/Scripts/webkitpy/port/base.py:1430
> > +        if decode_output:
> > +            output = output.encode('utf-8')
> >          _log.debug('Output of %s:\n%s' % (run_script_command, output))
> 
> We should only encode |output| for the purpose of calling _log.debug(). That
> is, we should not return the encoded data because the caller expects a
> string of characters (not a string of bytes).

The output is unused by the callers, but this is a fair point. Especially since this bug only occurs with an explicit request to decode the output.
Comment 23 Daniel Bates 2018-01-19 10:34:06 PST
Comment on attachment 331528 [details]
Patch

r=me
Comment 24 WebKit Commit Bot 2018-01-19 11:16:03 PST
Comment on attachment 331528 [details]
Patch

Clearing flags on attachment: 331528

Committed r227222: <https://trac.webkit.org/changeset/227222>
Comment 25 WebKit Commit Bot 2018-01-19 11:16:04 PST
All reviewed patches have been landed.  Closing bug.