188392 2018-08-07 14:59:49 -0700 run-builtins-generator-tests does not correctly handle CRLFs from stderr 2018-08-08 14:55:18 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ross.kirsling ross.kirsling achristensen bburg commit-queue don.olmstead ews-watchlist fujii glenn saam webkit-bug-importer ysuzuki oldest_to_newest 1448598 0 ross.kirsling 2018-08-07 14:59:49 -0700 run-builtins-generator-tests does not correctly handle CRLFs from stderr 1448599 1 346736 ross.kirsling 2018-08-07 15:01:01 -0700 Created attachment 346736 Patch 1448600 2 ross.kirsling 2018-08-07 15:06:57 -0700 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 1448671 3 346736 fujii 2018-08-07 20:09:09 -0700 Comment on attachment 346736 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". 1448678 4 ross.kirsling 2018-08-07 20:58:54 -0700 (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. 1448685 5 fujii 2018-08-07 22:06:00 -0700 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). 1448686 6 fujii 2018-08-07 22:11:53 -0700 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'. 1448691 7 ross.kirsling 2018-08-07 22:39:05 -0700 (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... 1448692 8 fujii 2018-08-07 22:46:13 -0700 (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. 1448784 9 346776 ross.kirsling 2018-08-08 10:59:56 -0700 Created attachment 346776 Patch 1448857 10 346776 commit-queue 2018-08-08 14:54:57 -0700 Comment on attachment 346776 Patch Clearing flags on attachment: 346776 Committed r234711: <https://trac.webkit.org/changeset/234711> 1448858 11 commit-queue 2018-08-08 14:54:58 -0700 All reviewed patches have been landed. Closing bug. 1448859 12 webkit-bug-importer 2018-08-08 14:55:18 -0700 <rdar://problem/43065427> 346736 2018-08-07 15:01:01 -0700 2018-08-08 10:59:55 -0700 Patch bug-188392-20180807150100.patch text/plain 1681 ross.kirsling U3VidmVyc2lvbiBSZXZpc2lvbjogMjM0NjcwCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggY2IwZTVjYjFmYTA2N2U5NWQwNWZmM2U4ZDZlYjVlNTU1 ZmNjM2IyNC4uN2Q3OWZiNTMyNzAzZjc2YjIwZmQ2NGEwYzYxZThjNDk4MDM1ZDQ0ZSAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEz IEBACisyMDE4LTA4LTA3ICBSb3NzIEtpcnNsaW5nICA8cm9zcy5raXJzbGluZ0Bzb255LmNvbT4K KworICAgICAgICBydW4tYnVpbHRpbnMtZ2VuZXJhdG9yLXRlc3RzIGRvZXMgbm90IGNvcnJlY3Rs eSBoYW5kbGUgQ1JMRnMgZnJvbSBzdGRlcnIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTE4ODM5MgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9jb2RlZ2VuL21haW4ucHk6Cisg ICAgICAgIChCdWlsdGluc0dlbmVyYXRvclRlc3RzLndyaXRlX2Vycm9yX2ZpbGUpOgorCiAyMDE4 LTA4LTA3ICBMdWNhcyBGb3JzY2hsZXIgIDxsZm9yc2NobGVyQGFwcGxlLmNvbT4KIAogICAgICAg ICBJZiB0aGVyZSdzIGEgUmFkYXIgaW4gdGhlIENoYW5nZUxvZywgd2Via2l0LXBhdGNoIHVwbG9h ZC9jcmVhdGUtYnVnIHNob3VsZCBwdXQgdGhlIHJhZGFyIGluIHRoZSBidWcgYW5kIHNldCBJblJh ZGFyCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvZGVnZW4vbWFpbi5weSBi L1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29kZWdlbi9tYWluLnB5CmluZGV4IGQ2ZTdiNmI2Y2Mx YzE2ZTM0ZDExYjU5MzYxNGUwOTc2NzhhNmJmZTAuLjA2ZTIzMGEyNmM4YTU4NjFiZWNmMzRiNGVk OWFhNmNkMTlmNTk0NTUgMTAwNjQ0Ci0tLSBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29kZWdl bi9tYWluLnB5CisrKyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29kZWdlbi9tYWluLnB5CkBA IC0yNiw2ICsyNiw3IEBACiBpbXBvcnQgb3MKIGltcG9ydCBvcy5wYXRoCiBpbXBvcnQgc2h1dGls CitpbXBvcnQgc3lzCiBpbXBvcnQgdGVtcGZpbGUKIGZyb20gd2Via2l0cHkuY29tbW9uLmNoZWNr b3V0LnNjbS5kZXRlY3Rpb24gaW1wb3J0IGRldGVjdF9zY21fc3lzdGVtCiBmcm9tIHdlYmtpdHB5 LmNvbW1vbi5zeXN0ZW0uZXhlY3V0aXZlIGltcG9ydCBTY3JpcHRFcnJvcgpAQCAtNjUsNiArNjYs OSBAQCBjbGFzcyBCdWlsdGluc0dlbmVyYXRvclRlc3RzOgogICAgIGRlZiB3cml0ZV9lcnJvcl9m aWxlKHNlbGYsIGlucHV0X2ZpbGVwYXRoLCBvdXRwdXRfZGlyZWN0b3J5LCBlcnJvcl9vdXRwdXQp OgogICAgICAgICBvdXRwdXRfZmlsZXBhdGggPSBvcy5wYXRoLmpvaW4ob3V0cHV0X2RpcmVjdG9y eSwgb3MucGF0aC5iYXNlbmFtZShpbnB1dF9maWxlcGF0aCkgKyAnLWVycm9yJykKIAorICAgICAg ICBpZiBzeXMucGxhdGZvcm0gPT0gJ3dpbjMyJzoKKyAgICAgICAgICAgIGVycm9yX291dHB1dCA9 IGVycm9yX291dHB1dC5yZXBsYWNlKCdcclxuJywgJ1xuJykKKwogICAgICAgICB3aXRoIG9wZW4o b3V0cHV0X2ZpbGVwYXRoLCAidyIpIGFzIG91dHB1dF9maWxlOgogICAgICAgICAgICAgb3V0cHV0 X2ZpbGUud3JpdGUoZXJyb3Jfb3V0cHV0KQogCg== 346776 2018-08-08 10:59:56 -0700 2018-08-08 14:54:57 -0700 Patch bug-188392-20180808105955.patch text/plain 1980 ross.kirsling U3VidmVyc2lvbiBSZXZpc2lvbjogMjM0Njk4CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggNWJlYWUyYTMxYmQzN2YyY2ZkZGY1MThlYjg2ODRiYWUx YjMwNjEyOC4uZGRlNGMzNTI2ZGY2YWM3Yjk3NzgxNTcwNDNkZDFlMzZlOTlhOTMxZiAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3 IEBACisyMDE4LTA4LTA4ICBSb3NzIEtpcnNsaW5nICA8cm9zcy5raXJzbGluZ0Bzb255LmNvbT4K KworICAgICAgICBydW4tYnVpbHRpbnMtZ2VuZXJhdG9yLXRlc3RzIGRvZXMgbm90IGNvcnJlY3Rs eSBoYW5kbGUgQ1JMRnMgZnJvbSBzdGRlcnIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTE4ODM5MgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIEJhY2tncm91bmQ6CisgICAgICAgIGZpbGUud3JpdGUgY29udmVy dHMgTEYgdG8gb3MubGluZXNlcCwgc28gaWYgdGhlIGlucHV0IHN0cmluZyBjb250YWlucyBDUkxG LCB3ZSBlbmQgdXAgd3JpdGluZyBDUkNSTEYgdG8gdGhlIGZpbGUuCisKKyAgICAgICAgKiBTY3Jp cHRzL3dlYmtpdHB5L2NvbW1vbi9zeXN0ZW0vZXhlY3V0aXZlLnB5OgorICAgICAgICAoRXhlY3V0 aXZlLnJ1bl9jb21tYW5kKToKKyAgICAgICAgTm9ybWFsaXplIENSTEYgdG8gTEYgaW4gZGVjb2Rl ZCBzdGRvdXQvc3RkZXJyIGRhdGEsIHNvIHRoYXQgd2UgZG9uJ3QgcmV0dXJuIGNvbnN1bWVycyBh IHBsYXRmb3JtLXNwZWNpZmljIHN0cmluZy4KKwogMjAxOC0wOC0wOCAgV2Vuc29uIEhzaWVoICA8 d2Vuc29uX2hzaWVoQGFwcGxlLmNvbT4KIAogICAgICAgICBbaU9TXSBmYXN0L2V2ZW50cy9pb3Mv Y29udGVudGVkaXRhYmxlLWF1dG9jYXBpdGFsaXplLmh0bWwgaXMgYSBmbGFreSBmYWlsdXJlCmRp ZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9zeXN0ZW0vZXhlY3V0aXZl LnB5IGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vc3lzdGVtL2V4ZWN1dGl2ZS5weQpp bmRleCA4MmEwMWNhODIwNTY3NTkzNWEzNTMxOTUwYWJlZmQzMTgwZGFkNWU5Li40NGRiM2MxOTI0 YjI0ODczODU5NTNhZTM2OGRmNDA1OWY1YjJjNmRiIDEwMDY0NAotLS0gYS9Ub29scy9TY3JpcHRz L3dlYmtpdHB5L2NvbW1vbi9zeXN0ZW0vZXhlY3V0aXZlLnB5CisrKyBiL1Rvb2xzL1NjcmlwdHMv d2Via2l0cHkvY29tbW9uL3N5c3RlbS9leGVjdXRpdmUucHkKQEAgLTM4Niw5ICszODYsOSBAQCBj bGFzcyBFeGVjdXRpdmUoQWJzdHJhY3RFeGVjdXRpdmUpOgogICAgICAgICAgICAgICAgICAgICAg ICAgICAgICBjbG9zZV9mZHM9c2VsZi5fc2hvdWxkX2Nsb3NlX2ZkcygpKQogICAgICAgICBvdXRw dXQgPSBwcm9jZXNzLmNvbW11bmljYXRlKHN0cmluZ190b19jb21tdW5pY2F0ZSlbMF0KIAotICAg ICAgICAjIHJ1bl9jb21tYW5kIGF1dG9tYXRpY2FsbHkgZGVjb2RlcyB0byB1bmljb2RlKCkgdW5s ZXNzIGV4cGxpY2l0bHkgdG9sZCBub3QgdG8uCisgICAgICAgICMgcnVuX2NvbW1hbmQgYXV0b21h dGljYWxseSBkZWNvZGVzIHRvIHVuaWNvZGUoKSBhbmQgY29udmVydHMgQ1JMRiB0byBMRiB1bmxl c3MgZXhwbGljaXRseSB0b2xkIG5vdCB0by4KICAgICAgICAgaWYgZGVjb2RlX291dHB1dDoKLSAg ICAgICAgICAgIG91dHB1dCA9IG91dHB1dC5kZWNvZGUoc2VsZi5fY2hpbGRfcHJvY2Vzc19lbmNv ZGluZygpKQorICAgICAgICAgICAgb3V0cHV0ID0gb3V0cHV0LmRlY29kZShzZWxmLl9jaGlsZF9w cm9jZXNzX2VuY29kaW5nKCkpLnJlcGxhY2UoJ1xyXG4nLCAnXG4nKQogCiAgICAgICAgICMgd2Fp dCgpIGlzIG5vdCB0aHJlYWRzYWZlIGFuZCBjYW4gdGhyb3cgT1NFcnJvciBkdWUgdG86CiAgICAg ICAgICMgaHR0cDovL2J1Z3MucHl0aG9uLm9yZy9pc3N1ZTE3MzE3MTcK