RESOLVED FIXED 224880
[Python 3] Update gni-to-cmake.py
https://bugs.webkit.org/show_bug.cgi?id=224880
Summary [Python 3] Update gni-to-cmake.py
Don Olmstead
Reported 2021-04-21 10:56:26 PDT
...
Attachments
Patch (11.48 KB, patch)
2021-04-21 11:09 PDT, Don Olmstead
ews-feeder: commit-queue-
Patch (12.01 KB, patch)
2021-04-21 11:46 PDT, Don Olmstead
no flags
Patch (15.13 KB, patch)
2021-04-21 12:05 PDT, Don Olmstead
no flags
Patch (15.01 KB, patch)
2021-04-21 12:17 PDT, Don Olmstead
no flags
Patch (15.03 KB, patch)
2021-04-21 12:27 PDT, Don Olmstead
no flags
Don Olmstead
Comment 1 2021-04-21 11:09:51 PDT
EWS Watchlist
Comment 2 2021-04-21 11:10:52 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
James Darpinian
Comment 3 2021-04-21 11:27:51 PDT
Comment on attachment 426715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426715&action=review Thanks for fixing for Python 3! > Source/ThirdParty/ANGLE/gni-to-cmake.py:20 > +parser.add_argument('--prepend', default='', help='the path to prepend to each file name') You'll need to update Tools/Scripts/update-angle which calls this script to use the new long form --prepend argument. > Source/ThirdParty/ANGLE/gni-to-cmake.py:68 > +out.write('# ./gni-to-cmake.py {} {}'.format(args.gni, args.cmake)) I would prefer to keep this the way it was, with quoting/escaping, including all arguments instead of a subset, and without hardcoding the script name. Does it need tweaking for Python 3 or Windows? I guess the argument escaping isn't strictly correct for Windows cmd but I think that's OK.
Don Olmstead
Comment 4 2021-04-21 11:46:37 PDT
Sam Sneddon [:gsnedders]
Comment 5 2021-04-21 11:49:38 PDT
Comment on attachment 426721 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426721&action=review > Source/ThirdParty/ANGLE/gni-to-cmake.py:24 > +file = open(args.gni, 'rb').read().decode('utf-8') file = open(args.gni, 'r', encoding="utf-8", newline="").read() would be better (or omit the newline if you're happy for them to be converted) Better still would be using a context manager given Python 3 will warn when it GCs the unclosed file object. > Source/ThirdParty/ANGLE/gni-to-cmake.py:65 > +out = open(args.cmake, 'w') needs an explicit encoding= also another file that needs closed
Don Olmstead
Comment 6 2021-04-21 12:05:51 PDT
Created attachment 426722 [details] Patch Making it more Pythonic
James Darpinian
Comment 7 2021-04-21 12:11:07 PDT
Comment on attachment 426722 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426722&action=review > Source/ThirdParty/ANGLE/gni-to-cmake.py:68 > + cmake.write('# "./gni-to-cmake.py" "{}" "{}"'.format(args.gni, args.cmake)) The name of the script is still hardcoded. Can you change it back to printing everything in sys.argv including argv[0], instead of hardcoding the script name and a fixed list of arguments?
Don Olmstead
Comment 8 2021-04-21 12:17:35 PDT
James Darpinian
Comment 9 2021-04-21 12:21:50 PDT
Comment on attachment 426725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426725&action=review Thanks, looks good! I'm not a reviewer so someone else will have to r+. > Source/ThirdParty/ANGLE/D3D.cmake:2 > +# "./gni-to-cmake.py" "src/libANGLE/renderer/d3d/BUILD.gn" "D3D.cmake" --prepend "D3D.cmake" Rerun the script to fix this line.
Don Olmstead
Comment 10 2021-04-21 12:27:37 PDT
Kenneth Russell
Comment 11 2021-04-21 12:33:18 PDT
Comment on attachment 426729 [details] Patch r+ mostly on jdarpinian's thorough review - also looks fine per quick scan.
EWS
Comment 12 2021-04-21 14:13:49 PDT
Committed r276390 (236865@main): <https://commits.webkit.org/236865@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426729 [details].
Radar WebKit Bug Importer
Comment 13 2021-04-22 18:00:56 PDT
Note You need to log in before you can comment on or make changes to this bug.