Bug 181254 – run-webkit-tests fails when there is a curly brace in Xcode build output

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

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)

[Alexey Proskuryakov]

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?

[Daniel Bates]

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.

[Alexey Proskuryakov]

> 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.

[Alexey Proskuryakov]

Comment on attachment 330409 [details]
proposed fix

Please feel encouraged to roll back if this breaks anything.

[WebKit Commit Bot]

Comment on attachment 330409 [details]
proposed fix

Clearing flags on attachment: 330409

Committed r226816: <https://trac.webkit.org/changeset/226816>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36453374>

[Ryan Haddad]

This broke LayoutTests on the bots:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/builds/2213

[Ryan Haddad]

Reverted r226816 for reason:

This change broke LayoutTests on the bots.

Committed r226837: <https://trac.webkit.org/changeset/226837>

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 331207 [details]
Patch

[Jonathan Bedard]

(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.

[Daniel Bates]

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))

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 331238 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #15)
> Created attachment 331238 [details]
> Patch

This will address Alexey's problem without the side-effects.

[Daniel Bates]

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.

[Jonathan Bedard]

Created attachment 331390 [details]
Patch

[Jonathan Bedard]

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.

[Daniel Bates]

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).

[Jonathan Bedard]

Created attachment 331528 [details]
Patch

[Jonathan Bedard]

(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.

[Daniel Bates]

Comment on attachment 331528 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 331528 [details]
Patch

Clearing flags on attachment: 331528

Committed r227222: <https://trac.webkit.org/changeset/227222>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.