Bug 224880 – [Python 3] Update gni-to-cmake.py

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Don Olmstead]

...

[Don Olmstead]

Created attachment 426715 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[James Darpinian]

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]

Created attachment 426721 [details]
Patch

[Sam Sneddon [:gsnedders]]

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]

Created attachment 426722 [details]
Patch

Making it more Pythonic

[James Darpinian]

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]

Created attachment 426725 [details]
Patch

[James Darpinian]

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]

Created attachment 426729 [details]
Patch

[Kenneth Russell]

Comment on attachment 426729 [details]
Patch

r+ mostly on jdarpinian's thorough review - also looks fine per quick scan.

[EWS]

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]

<rdar://problem/77048391>