Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2023223002:
[GN] Add script to compile assets catalog without using tools/gyp. (Closed)
Description[GN] Add script to compile assets catalog without using tools/gyp.
Implement the assets catalog compilation without using mac_tool.py
from tools/gyp in order to make //build independent from the rest
of Chromium checkout.
BUG=616813
Committed: https://crrev.com/17dd77eb2af78643ffeb32bbb2ee48c2d22106e7
Cr-Commit-Position: refs/heads/master@{#397469}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Use mac_sdk_name in "compile_xcassets" tool definition #
Messages
Total messages: 25 (11 generated)
Patchset #1 (id:1) has been deleted
sdefresne@chromium.org changed reviewers: + rsesek@chromium.org
Please take a look and send to CQ if LGTY.
Why not use //build/config/mac/xcrun.py directly? Also, no BUG=?
On 2016/06/02 14:34:34, Robert Sesek wrote: > Why not use //build/config/mac/xcrun.py directly? Because of this: # actool crashes if paths are relative, so use os.path.abspath to get absolute # path for input and outputs. > Also, no BUG=? Is 297668 good enough?
Description was changed from ========== [GN] Add script to compile assets catalog without using tools/gyp. Implement the assets catalog compilation without using mac_tool.py from tools/gyp in order to make //build independent from the rest of Chromium checkout. BUG=None ========== to ========== [GN] Add script to compile assets catalog without using tools/gyp. Implement the assets catalog compilation without using mac_tool.py from tools/gyp in order to make //build independent from the rest of Chromium checkout. BUG=297668 ==========
LGTM On 2016/06/02 14:42:04, sdefresne wrote: > On 2016/06/02 14:34:34, Robert Sesek wrote: > > Why not use //build/config/mac/xcrun.py directly? > > Because of this: > > # actool crashes if paths are relative, so use os.path.abspath to get absolute > # path for input and outputs. Ah, and you can't do rebase_path(X) give you absolute paths in a tool... > > Also, no BUG=? > > Is 297668 good enough? Sure.
On 2016/06/02 14:49:04, Robert Sesek wrote:
> LGTM
>
> On 2016/06/02 14:42:04, sdefresne wrote:
> > On 2016/06/02 14:34:34, Robert Sesek wrote:
> > > Why not use //build/config/mac/xcrun.py directly?
> >
> > Because of this:
> >
> > # actool crashes if paths are relative, so use os.path.abspath to get
> absolute
> > # path for input and outputs.
>
> Ah, and you can't do rebase_path(X) give you absolute paths in a tool...
No because the tool does not see the paths, it just convert {{sources}} to ${in}
which is then expanded by ninja.
> > > Also, no BUG=?
> >
> > Is 297668 good enough?
>
> Sure.
The CQ bit was checked by sdefresne@chromium.org
The CQ bit was unchecked by sdefresne@chromium.org
sdefresne@chromium.org changed reviewers: + dpranke@chromium.org
dpranke: need OWNERS approval :-)
lgtm w/ suggested, but optional, change. Of course, there's probably still lots of other uses of gyp-mac-tool yet to go, but I support the goal :). https://codereview.chromium.org/2023223002/diff/20001/build/toolchain/mac/com... File build/toolchain/mac/compile_xcassets.py (right): https://codereview.chromium.org/2023223002/diff/20001/build/toolchain/mac/com... build/toolchain/mac/compile_xcassets.py:15: choices=('macosx', 'iphoneos', 'iphonesimulator'), If the underlying arg uses 'mac' instead of 'macosx', and either does mac or does both of the other platforms, and we just make this a single --mac boolean arg or something else similar? I'd prefer not to use 'macosx' as an arg value if neither GN nor the underlying tool uses that string.
dpranke: PTAL (and send to CQ if still LGTY). https://codereview.chromium.org/2023223002/diff/20001/build/toolchain/mac/com... File build/toolchain/mac/compile_xcassets.py (right): https://codereview.chromium.org/2023223002/diff/20001/build/toolchain/mac/com... build/toolchain/mac/compile_xcassets.py:15: choices=('macosx', 'iphoneos', 'iphonesimulator'), On 2016/06/02 16:32:10, Dirk Pranke wrote: > If the underlying arg uses 'mac' instead of 'macosx', and either does mac or > does both of the other platforms, and we just make this a single --mac boolean > arg or something else similar? I'd prefer not to use 'macosx' as an arg value if > neither GN nor the underlying tool uses that string. The string is passed verbatim to the actool that only supports those three values (see line 36). I could add a check there and do the following: '--platform', args.platform if args.platform != 'mac' else 'macosx', but it looks uglier. The value is also available in GN in the mac_sdk_name variable (as it is used by the sdk_info.py script), so I've changed the tool definition to use the variable and remove the hardcoded string. Is it better?
the BUILD.gn change is a slight improvement, thanks! lgtm. https://codereview.chromium.org/2023223002/diff/20001/build/toolchain/mac/com... File build/toolchain/mac/compile_xcassets.py (right): https://codereview.chromium.org/2023223002/diff/20001/build/toolchain/mac/com... build/toolchain/mac/compile_xcassets.py:15: choices=('macosx', 'iphoneos', 'iphonesimulator'), On 2016/06/02 16:55:29, sdefresne wrote: > On 2016/06/02 16:32:10, Dirk Pranke wrote: > > If the underlying arg uses 'mac' instead of 'macosx', and either does mac or > > does both of the other platforms, and we just make this a single --mac boolean > > arg or something else similar? I'd prefer not to use 'macosx' as an arg value > if > > neither GN nor the underlying tool uses that string. > > The string is passed verbatim to the actool that only supports those three > values (see line 36). I could add a check there and do the following: > > '--platform', args.platform if args.platform != 'mac' else 'macosx', > > but it looks uglier. The value is also available in GN in the mac_sdk_name > variable (as it is used by the sdk_info.py script), so I've changed the tool > definition to use the variable and remove the hardcoded string. Is it better? Ah, I missed the --platform arg in the command line. So, yeah, I agree that there's no point in changing it.
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2023223002/#ps40001 (title: "Use mac_sdk_name in "compile_xcassets" tool definition")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023223002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023223002/40001
Description was changed from ========== [GN] Add script to compile assets catalog without using tools/gyp. Implement the assets catalog compilation without using mac_tool.py from tools/gyp in order to make //build independent from the rest of Chromium checkout. BUG=297668 ========== to ========== [GN] Add script to compile assets catalog without using tools/gyp. Implement the assets catalog compilation without using mac_tool.py from tools/gyp in order to make //build independent from the rest of Chromium checkout. BUG=616813 ==========
On 2016/06/02 14:49:04, Robert Sesek wrote: > > Is 297668 good enough? > > Sure. Switched to newly-filed https://crbug.com/616813
On 2016/06/02 18:06:00, Robert Sesek wrote: > On 2016/06/02 14:49:04, Robert Sesek wrote: > > > Is 297668 good enough? > > > > Sure. > > Switched to newly-filed https://crbug.com/616813 Thanks.
Message was sent while issue was closed.
Description was changed from ========== [GN] Add script to compile assets catalog without using tools/gyp. Implement the assets catalog compilation without using mac_tool.py from tools/gyp in order to make //build independent from the rest of Chromium checkout. BUG=616813 ========== to ========== [GN] Add script to compile assets catalog without using tools/gyp. Implement the assets catalog compilation without using mac_tool.py from tools/gyp in order to make //build independent from the rest of Chromium checkout. BUG=616813 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [GN] Add script to compile assets catalog without using tools/gyp. Implement the assets catalog compilation without using mac_tool.py from tools/gyp in order to make //build independent from the rest of Chromium checkout. BUG=616813 ========== to ========== [GN] Add script to compile assets catalog without using tools/gyp. Implement the assets catalog compilation without using mac_tool.py from tools/gyp in order to make //build independent from the rest of Chromium checkout. BUG=616813 Committed: https://crrev.com/17dd77eb2af78643ffeb32bbb2ee48c2d22106e7 Cr-Commit-Position: refs/heads/master@{#397469} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/17dd77eb2af78643ffeb32bbb2ee48c2d22106e7 Cr-Commit-Position: refs/heads/master@{#397469} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
