Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2123253004:
Add support for multi-architecture application bundle on iOS. (Closed)
DescriptionAdd support for multi-architecture application bundle on iOS.
Add a new variable "multi_arch_target_cpus" that can be defined by the
user via "gn args" to build for more than one architecture. It should
be set to a list of two or more architectures supported by gn build.
Add support for multi-architectures application to the "ios_app_bundle"
template. The template expands to a simple "executable" target if the
toolchain is not the default toolchain. For the default toolchain the
real executable is generated by assembling all single architecture
binaries using "lipo" tool.
BUG=603180
Committed: https://crrev.com/dfc9cd81d9edc11b7cd0b0f7a5c26aff06422b9d
Cr-Commit-Position: refs/heads/master@{#405110}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add comments to explain how fat-build works. #
Total comments: 4
Patch Set 3 : Use target_out_dir instead of target_gen_dir. #Patch Set 4 : "regular build" -> "thin build" #
Total comments: 4
Patch Set 5 : Use additional_target_cpus variable instead of multi_arch_target_cpus. #
Total comments: 3
Patch Set 6 : Add comment on how to use additional_target_cpus variable #Messages
Total messages: 25 (8 generated)
sdefresne@chromium.org changed reviewers: + brettw@chromium.org, rsesek@chromium.org
Robert, can you review //build/config/ios/rules.gni this as expert of macOS/iOS build system? Brett, can you review //build/config/BUILDCONFIG.gn for the changes there? Please note that I think dSym generation may be broken and ios_framework_bundle has other issues when building fat binaries. This is know but I'd like to address them in followup CLs as I first want to have feedback on how I'm enabling fat binary build.
I'm having a little trouble following how this works. Could you add some inline documentation explaining the various steps? If I'm following this correctly, the executable target will get built for each defined toolchain. And you're making the first such toolchain special and attaching the lipo step to it. https://codereview.chromium.org/2123253004/diff/1/build/config/ios/rules.gni File build/config/ios/rules.gni (right): https://codereview.chromium.org/2123253004/diff/1/build/config/ios/rules.gni#... build/config/ios/rules.gni:257: if (_is_fat_build) { How many times is lipo going to get run? I'd think you'd only do it as the final output for _is_fat_build_main_target..
Yes, when multi_architecture_target_cpus is defined, a second list is defined with all the tool gains required for the fat build and the first one is picked as the main one (I had to choose one to make it simpler). Then the ios_app_bundle expands to just an executable for toolchains != default_toolchain. For the default_toolchain, the template has a new intermediate target that runs lipo once (per ios_app_bundle targets, so once per test target mostly) on all the executables from that target. There is some complications to deal with codesigning (as we sign the bundle and not the executable). If codesigning is enabled then we let the codesigning script copy the fat binary in the bundle otherwise we define an intermediate bundle_data target (this was pre-existing regular, or can I say slim, builds). I could simplify this a bit if I always call lipo (currently it is only called if we are doing a fat build). But I'm not sure it is worth it (and I don't know whether lipo supports to be called with just one binary). The reason for having different expansion of the template for default_toolchain and other toolchains is to avoid defining targets that are not used (they may break when trying to build "all"). This also make the template slightly simpler (I started by doing having the same sub targets even for secondary toolchains but since they could not do the lipo call, this ended up really hard to understand). I'll try to add some comments to explain what I'm doing here (probably also documenting the codesigning bits). Le jeudi 7 juillet 2016, <rsesek@chromium.org> a écrit : I'm having a little trouble following how this works. Could you add some inline documentation explaining the various steps? If I'm following this correctly, the executable target will get built for each defined toolchain. And you're making the first such toolchain special and attaching the lipo step to it. https://codereview.chromium.org/2123253004/diff/1/build/config/ios/rules.gni File build/config/ios/rules.gni (right): https://codereview.chromium.org/2123253004/diff/1/build/config/ios/rules.gni#... build/config/ios/rules.gni:257: if (_is_fat_build) { How many times is lipo going to get run? I'd think you'd only do it as the final output for _is_fat_build_main_target.. https://codereview.chromium.org/2123253004/
Patchset #2 (id:20001) has been deleted
Added some comments, please take another look.
LGTM % some comment nits. Thanks for the explanations. https://codereview.chromium.org/2123253004/diff/40001/build/config/ios/rules.gni File build/config/ios/rules.gni (right): https://codereview.chromium.org/2123253004/diff/40001/build/config/ios/rules.... build/config/ios/rules.gni:124: # For a regular build (i.e. when multi_arch_target_cpus is an empty list), Maybe use "thin" instead of "regular" as that's the term for the underlying MachO. https://codereview.chromium.org/2123253004/diff/40001/build/config/ios/rules.... build/config/ios/rules.gni:184: # This is either a regular build or the default toolchain of a fat-build. regular -> thin
Brett: ping ;-)
https://codereview.chromium.org/2123253004/diff/40001/build/config/ios/rules.gni File build/config/ios/rules.gni (right): https://codereview.chromium.org/2123253004/diff/40001/build/config/ios/rules.... build/config/ios/rules.gni:124: # For a regular build (i.e. when multi_arch_target_cpus is an empty list), On 2016/07/08 18:17:54, Robert Sesek wrote: > Maybe use "thin" instead of "regular" as that's the term for the underlying > MachO. Done. https://codereview.chromium.org/2123253004/diff/40001/build/config/ios/rules.... build/config/ios/rules.gni:184: # This is either a regular build or the default toolchain of a fat-build. On 2016/07/08 18:17:54, Robert Sesek wrote: > regular -> thin Done.
https://codereview.chromium.org/2123253004/diff/80001/build/config/BUILDCONFI... File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/2123253004/diff/80001/build/config/BUILDCONFI... build/config/BUILDCONFIG.gn:10: multi_arch_target_cpus = [] [read the next comment first] I'm trying *really* hard to keep stuff out of this file. This case I'm more sympathetic to than many because this does seem like global build configuration, but the only reason it's needed here (if you take my advice below to move the toolchain list) is for the target_cpu variable. At first I was thinking of making this variable in rules.gni, and then asserting that multi_arch_target_cpus[0] == target_cpu. But that's annoying. What do you think about making "additional_target_cpus" and then using target_cpu + these to define the multi_arch_toolchains. This means you don't have the annoying logic for making sure only one is used, target_cpu is always well-defined to be the default main thing you set, and we can remove this code from this file :) https://codereview.chromium.org/2123253004/diff/80001/build/config/BUILDCONFI... build/config/BUILDCONFIG.gn:245: multi_arch_toolchains = [] Can this code by put into build/config/ios/rules.gni or perhaps a new .gni file for this purpose?> I want to keep all non-essential logic out of BUILDCONFIG.
Brett: Looks nicer that way, thank you for suggesting not touching build/config/BUILDCONFIG.gn. I guess I don't need your OWNERS approval now ;-) Can you still take another look and send to CQ if it LGTY? https://codereview.chromium.org/2123253004/diff/80001/build/config/BUILDCONFI... File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/2123253004/diff/80001/build/config/BUILDCONFI... build/config/BUILDCONFIG.gn:10: multi_arch_target_cpus = [] On 2016/07/11 23:57:17, brettw (ping after 24h) wrote: > [read the next comment first] > > I'm trying *really* hard to keep stuff out of this file. This case I'm more > sympathetic to than many because this does seem like global build configuration, > but the only reason it's needed here (if you take my advice below to move the > toolchain list) is for the target_cpu variable. > > At first I was thinking of making this variable in rules.gni, and then asserting > that multi_arch_target_cpus[0] == target_cpu. But that's annoying. > > What do you think about making "additional_target_cpus" and then using > target_cpu + these to define the multi_arch_toolchains. This means you don't > have the annoying logic for making sure only one is used, target_cpu is always > well-defined to be the default main thing you set, and we can remove this code > from this file :) Done. https://codereview.chromium.org/2123253004/diff/80001/build/config/BUILDCONFI... build/config/BUILDCONFIG.gn:245: multi_arch_toolchains = [] On 2016/07/11 23:57:17, brettw (ping after 24h) wrote: > Can this code by put into build/config/ios/rules.gni or perhaps a new .gni file > for this purpose?> I want to keep all non-essential logic out of BUILDCONFIG. This will duplicate the "//build/toolchain/mac:ios_clang_$_cpu" string, but probably minor regarding modifying this file. Done.
Patchset #5 (id:100001) has been deleted
LGTM, I didn't really look at rules.gni in too much detail. https://codereview.chromium.org/2123253004/diff/120001/build/config/ios/ios_s... File build/config/ios/ios_sdk.gni (right): https://codereview.chromium.org/2123253004/diff/120001/build/config/ios/ios_s... build/config/ios/ios_sdk.gni:27: # If non-empty, this list must contain valid cpu architecture, and the final Here it might be good to give an example of setting this in the comment, since I expect that most people have never set a list as a GN arg before.
The CQ bit was checked by sdefresne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2123253004/#ps140001 (title: "Add comment on how to use additional_target_cpus variable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2123253004/diff/120001/build/config/ios/ios_s... File build/config/ios/ios_sdk.gni (right): https://codereview.chromium.org/2123253004/diff/120001/build/config/ios/ios_s... build/config/ios/ios_sdk.gni:27: # If non-empty, this list must contain valid cpu architecture, and the final On 2016/07/12 20:24:32, brettw (ping after 24h) wrote: > Here it might be good to give an example of setting this in the comment, since I > expect that most people have never set a list as a GN arg before. Done. https://codereview.chromium.org/2123253004/diff/120001/build/config/ios/ios_s... build/config/ios/ios_sdk.gni:27: # If non-empty, this list must contain valid cpu architecture, and the final On 2016/07/12 20:24:32, brettw (ping after 24h) wrote: > Here it might be good to give an example of setting this in the comment, since I > expect that most people have never set a list as a GN arg before. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add support for multi-architecture application bundle on iOS. Add a new variable "multi_arch_target_cpus" that can be defined by the user via "gn args" to build for more than one architecture. It should be set to a list of two or more architectures supported by gn build. Add support for multi-architectures application to the "ios_app_bundle" template. The template expands to a simple "executable" target if the toolchain is not the default toolchain. For the default toolchain the real executable is generated by assembling all single architecture binaries using "lipo" tool. BUG=603180 ========== to ========== Add support for multi-architecture application bundle on iOS. Add a new variable "multi_arch_target_cpus" that can be defined by the user via "gn args" to build for more than one architecture. It should be set to a list of two or more architectures supported by gn build. Add support for multi-architectures application to the "ios_app_bundle" template. The template expands to a simple "executable" target if the toolchain is not the default toolchain. For the default toolchain the real executable is generated by assembling all single architecture binaries using "lipo" tool. BUG=603180 Committed: https://crrev.com/dfc9cd81d9edc11b7cd0b0f7a5c26aff06422b9d Cr-Commit-Position: refs/heads/master@{#405110} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/dfc9cd81d9edc11b7cd0b0f7a5c26aff06422b9d Cr-Commit-Position: refs/heads/master@{#405110} |
