run-builtins-generator-tests does not correctly handle CRLFs from stderr
Created attachment 346736 [details] Patch
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
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".
(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.
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).
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'.
(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...
(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.
Created attachment 346776 [details] Patch
Comment on attachment 346776 [details] Patch Clearing flags on attachment: 346776 Committed r234711: <https://trac.webkit.org/changeset/234711>
All reviewed patches have been landed. Closing bug.
<rdar://problem/43065427>