Bug 188392 – run-builtins-generator-tests does not correctly handle CRLFs from stderr

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

run-builtins-generator-tests does not correctly handle CRLFs from stderr

[Ross Kirsling]

Created attachment 346736 [details]
Patch

[Ross Kirsling]

Python's file.write converts '\n' to os.linesep, so if the input string contains "\r\n", we end up with "\r\r\n" written to the file:
https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Release%20%28Tests%29/builds/933/steps/builtins-generator-tests/logs/stdio

FWIW, DRT already does the same sort of replacement here:
https://github.com/WebKit/webkit/blob/master/Tools/Scripts/webkitpy/port/base.py#L514

[Fujii Hironori]

Comment on attachment 346736 [details]
Patch

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

> Tools/Scripts/webkitpy/codegen/main.py:72
>          with open(output_filepath, "w") as output_file:

This issues happens because run_command retrieves output as binary, but write_error_file opens a file as text mode.
I think "w" should be replaced with "wb".

[Ross Kirsling]

(In reply to Fujii Hironori from comment #3)
> Comment on attachment 346736 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346736&action=review
> 
> > Tools/Scripts/webkitpy/codegen/main.py:72
> >          with open(output_filepath, "w") as output_file:
> 
> This issues happens because run_command retrieves output as binary, but
> write_error_file opens a file as text mode.
> I think "w" should be replaced with "wb".

That will certainly also work, but I felt like it disguises the actual issue, since the input and output really are text (run_command gets data from subprocess.communicate but it ultimately returns a UTF-8 string), and our concern is simply with normalizing carriage returns.

[Fujii Hironori]

I think the script should generate files in Unix style because otherwise it can't be used for expectations for other platforms.

There are two potential issues:

1. run_command doesn't normalize '\r\n'
2. write_error_file converts '\n' to '\r\n'

Fortunately, Windows diff.exe normalizes '\r\n', we don't need to fix both, but only one to pass run-builtins-generator-tests.

If you don't like (2), I think you should take (1).

[Fujii Hironori]

https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/Scripts/tests/builtins/expected
This is the directory of expectations.

https://docs.python.org/3/library/functions.html#open
'open' of Python 3 can specify newline='\n'.

[Ross Kirsling]

(In reply to Fujii Hironori from comment #5)
> I think the script should generate files in Unix style because otherwise it
> can't be used for expectations for other platforms.
> 
> There are two potential issues:
> 
> 1. run_command doesn't normalize '\r\n'
> 2. write_error_file converts '\n' to '\r\n'
> 
> Fortunately, Windows diff.exe normalizes '\r\n', we don't need to fix both, but only one to pass run-builtins-generator-tests.
> 
> If you don't like (2), I think you should take (1).

One way or another, the "actual" file needs to contain CRLF on Windows, because the "expected" file will be automatically checked out by Git/SVN with CRLF on Windows.

Perhaps the best solution is to address (1) by putting the replace call here instead then:
https://github.com/WebKit/webkit/blob/master/Tools/Scripts/webkitpy/common/system/executive.py#L391


(In reply to Fujii Hironori from comment #6)
> https://docs.python.org/3/library/functions.html#open
> 'open' of Python 3 can specify newline='\n'.

It's true that opening with newline='\r\n' would also solve the problem, but we're still on Python 2...

[Fujii Hironori]

(In reply to Ross Kirsling from comment #7)
> One way or another, the "actual" file needs to contain CRLF on Windows,
> because the "expected" file will be automatically checked out by Git/SVN
> with CRLF on Windows.

Oh, that's right! I forgot that because I'm using Cygwin git.

> Perhaps the best solution is to address (1) by putting the replace call here
> instead then:
> https://github.com/WebKit/webkit/blob/master/Tools/Scripts/webkitpy/common/
> system/executive.py#L391

Agreed.

[Ross Kirsling]

Created attachment 346776 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 346776 [details]
Patch

Clearing flags on attachment: 346776

Committed r234711: <https://trac.webkit.org/changeset/234711>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43065427>