WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Patch
(12.01 KB, patch)
2021-04-21 11:46 PDT
,
Don Olmstead
no flags
Details
Formatted Diff
Diff
Patch
(15.13 KB, patch)
2021-04-21 12:05 PDT
,
Don Olmstead
no flags
Details
Formatted Diff
Diff
Patch
(15.01 KB, patch)
2021-04-21 12:17 PDT
,
Don Olmstead
no flags
Details
Formatted Diff
Diff
Patch
(15.03 KB, patch)
2021-04-21 12:27 PDT
,
Don Olmstead
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Don Olmstead
Comment 1
2021-04-21 11:09:51 PDT
Created
attachment 426715
[details]
Patch
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
Created
attachment 426721
[details]
Patch
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
Created
attachment 426725
[details]
Patch
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
Created
attachment 426729
[details]
Patch
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
<
rdar://problem/77048391
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug