Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2115583002:
GN Rule to build Cronet Dynamic Framework for iOS. (Closed)
DescriptionGN Rule to build Cronet Dynamic Framework for iOS.
For release builds set gn args enable_dsyms = true and enable_stripping = true to separate symbols from the framework binary.
Added components/cronet/tools/package_ios.py option --gn to use gn instead of gyp to build cronet_package target.
Committed: https://crrev.com/941bc7e523e5502a6d2211d0c0837504e0bfc5dd
Cr-Commit-Position: refs/heads/master@{#405768}
Patch Set 1 #Patch Set 2 : Generate dSYM #Patch Set 3 : Tweak Info.plist #Patch Set 4 : Format. #Patch Set 5 : Self Review. #
Total comments: 16
Patch Set 6 : Don't use @loader_path. #
Total comments: 4
Patch Set 7 : Follow Sylvain's suggestions. #Patch Set 8 : Use enable_dsyms and enable_stripping gn args instead of is_debug check. #Patch Set 9 : Add missing import. #Patch Set 10 : Updated package_ios.py script to use gn to build Cronet frameworks. #Patch Set 11 : Fix the case. #
Total comments: 5
Messages
Total messages: 31 (6 generated)
mef@chromium.org changed reviewers: + kapishnikov@chromium.org, rsesek@chromium.org, sdefresne@chromium.org
PTAL, cronet_framework target in this BUILD.gn appears to produce the Cronet.framework output matching the same in cronet.gypi. WDYT? https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:67: output_name = "$root_out_dir/Cronet.framework/Info.plist" Is there a way to avoid hardcoding output path? https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:102: ldflags += [ "-Wcrl,dsym," + rebase_path(root_out_dir) ] Is there a way to specify that this outputs Cronet.dSYM? https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:183: # sources += [ "$root_out_dir/Cronet.dSYM" ] Is there a way to specify that Cronet.dSYM is generated by cronet_framework target?
https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:67: output_name = "$root_out_dir/Cronet.framework/Info.plist" On 2016/06/30 21:29:06, mef wrote: > Is there a way to avoid hardcoding output path? It doesn't look like the ios_framework_bundle has an info_plist_target option like ios_app_bundle or mac_framework_bundle do. That's the proper way to handle this (via a bundle_data rule). https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:101: ldflags += [ "-Wcrl,strip,-x" ] Similarly, we handle stripping globally with enable_stripping=true and it gets picked up via this config: https://cs.chromium.org/chromium/src/build/config/mac/BUILD.gn?dr=C&sq=packag... https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:102: ldflags += [ "-Wcrl,dsym," + rebase_path(root_out_dir) ] On 2016/06/30 21:29:06, mef wrote: > Is there a way to specify that this outputs Cronet.dSYM? On Mac at least, we handle dSYMs and stripping at the build configuration level. One sets enable_dsyms = true and then the -Wcrl flag gets added by the toolchain (https://cs.chromium.org/chromium/src/build/toolchain/mac/BUILD.gn?type=cs&sq=...). https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:183: # sources += [ "$root_out_dir/Cronet.dSYM" ] On 2016/06/30 21:29:06, mef wrote: > Is there a way to specify that Cronet.dSYM is generated by cronet_framework > target? The toolchain lists it as an output if based on the above condition I highlighted.
https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:60: args = [ "--bundle_id=CRNT" ] Usually bundle id has a domain-like structure. I would suggest "org.chromium.net.Cronet".
https://codereview.chromium.org/2115583002/diff/100001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2115583002/diff/100001/components/cronet/ios/... components/cronet/ios/BUILD.gn:60: args = [ "--bundle_id=CRNT" ] The "Info.plist" file already contains the following, do you really need to change it: <key>CFBundleIdentifier</key> <string>org.chromium.net.Cronet</string> https://codereview.chromium.org/2115583002/diff/100001/components/cronet/ios/... components/cronet/ios/BUILD.gn:67: output_name = "$root_out_dir/Cronet.framework/Info.plist" I think this goes against how create_bundle/bundle_data have been designed. Moreover, I'm not sure you need to use this template as it looks like none of the substitutions performed by this template are needed. But if you do, I would instead use the following: tweak_info_plist("cronet_framework_plist") { info_plist = "Info.plist" } ios_info_plist("compile_info_plist") { executable_name = "Cronet" output_name = "$target_gen_dir/Info.plist" } bundle_data("cronet_info_plist") { sources = [ "$target_gen_dir/Info.plist" ] outputs = [ "{{bundle_root_dir}}/Info.plist" ] }
Thanks for your comments! https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:60: args = [ "--bundle_id=CRNT" ] On 2016/06/30 21:59:55, kapishnikov wrote: > Usually bundle id has a domain-like structure. I would suggest > "org.chromium.net.Cronet". Done. https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:67: output_name = "$root_out_dir/Cronet.framework/Info.plist" On 2016/06/30 21:43:14, Robert Sesek wrote: > On 2016/06/30 21:29:06, mef wrote: > > Is there a way to avoid hardcoding output path? > > It doesn't look like the ios_framework_bundle has an info_plist_target option > like ios_app_bundle or mac_framework_bundle do. That's the proper way to handle > this (via a bundle_data rule). I've followed Sylvain's suggestion, and it seemed to work WRT to avoiding to hard-code path to upper target. https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:101: ldflags += [ "-Wcrl,strip,-x" ] On 2016/06/30 21:43:15, Robert Sesek wrote: > Similarly, we handle stripping globally with enable_stripping=true and it gets > picked up via this config: > https://cs.chromium.org/chromium/src/build/config/mac/BUILD.gn?dr=C&sq=packag... If there is a better way via updating template, I'll be happy to do it. I've tried, but I've failed with that though. https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:102: ldflags += [ "-Wcrl,dsym," + rebase_path(root_out_dir) ] On 2016/06/30 21:43:15, Robert Sesek wrote: > On 2016/06/30 21:29:06, mef wrote: > > Is there a way to specify that this outputs Cronet.dSYM? > > On Mac at least, we handle dSYMs and stripping at the build configuration level. > One sets enable_dsyms = true and then the -Wcrl flag gets added by the toolchain > (https://cs.chromium.org/chromium/src/build/toolchain/mac/BUILD.gn?type=cs&sq=...). I've tried that, but it didn't work for ios_framework_bundle. I'd love to make it work, but I may need some help. https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:183: # sources += [ "$root_out_dir/Cronet.dSYM" ] On 2016/06/30 21:43:15, Robert Sesek wrote: > On 2016/06/30 21:29:06, mef wrote: > > Is there a way to specify that Cronet.dSYM is generated by cronet_framework > > target? > > The toolchain lists it as an output if based on the above condition I > highlighted. The toolchain just adds it to outputs: https://cs.chromium.org/chromium/src/build/toolchain/mac/BUILD.gn?rcl=0&l=236 but when I try to do the same inside of ios_framework_bundle("cronet_framework")" outputs += [ "$root_out_dir/Cronet.dSYM" ] I get the error: ERROR at //components/cronet/ios/BUILD.gn:112:5: Undefined variable for +=. It seems that |outputs| in template is not the same as |outputs| in target? https://codereview.chromium.org/2115583002/diff/100001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2115583002/diff/100001/components/cronet/ios/... components/cronet/ios/BUILD.gn:60: args = [ "--bundle_id=CRNT" ] On 2016/07/01 12:50:11, sdefresne wrote: > The "Info.plist" file already contains the following, do you really need to > change it: > > <key>CFBundleIdentifier</key> > <string>org.chromium.net.Cronet</string> Done. I've kept it because omitting args triggers an error, but it seems that empty args work fine. https://codereview.chromium.org/2115583002/diff/100001/components/cronet/ios/... components/cronet/ios/BUILD.gn:67: output_name = "$root_out_dir/Cronet.framework/Info.plist" On 2016/07/01 12:50:11, sdefresne wrote: > I think this goes against how create_bundle/bundle_data have been designed. > Moreover, I'm not sure you need to use this template as it looks like none of > the substitutions performed by this template are needed. tweak_info_plist also puts correct version and revision into tweaked plist, which is nice. > > But if you do, I would instead use the following: > > tweak_info_plist("cronet_framework_plist") { > info_plist = "Info.plist" > } > > ios_info_plist("compile_info_plist") { > executable_name = "Cronet" > output_name = "$target_gen_dir/Info.plist" > } > > bundle_data("cronet_info_plist") { > sources = [ "$target_gen_dir/Info.plist" ] > outputs = [ "{{bundle_root_dir}}/Info.plist" ] > } Thanks, that worked!
https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:102: ldflags += [ "-Wcrl,dsym," + rebase_path(root_out_dir) ] On 2016/07/01 14:10:08, mef wrote: > On 2016/06/30 21:43:15, Robert Sesek wrote: > > On 2016/06/30 21:29:06, mef wrote: > > > Is there a way to specify that this outputs Cronet.dSYM? > > > > On Mac at least, we handle dSYMs and stripping at the build configuration > level. > > One sets enable_dsyms = true and then the -Wcrl flag gets added by the > toolchain > > > (https://cs.chromium.org/chromium/src/build/toolchain/mac/BUILD.gn?type=cs&sq=...). > > I've tried that, but it didn't work for ios_framework_bundle. I'd love to make > it work, but I may need some help. Sorry if I wasn't being clear; these aren't arguments consumed by the template. They are build-level `gn args` arguments. Stripping and dSYM production applies to all targets in the build.
https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:102: ldflags += [ "-Wcrl,dsym," + rebase_path(root_out_dir) ] On 2016/07/01 15:37:43, Robert Sesek wrote: > On 2016/07/01 14:10:08, mef wrote: > > On 2016/06/30 21:43:15, Robert Sesek wrote: > > > On 2016/06/30 21:29:06, mef wrote: > > > > Is there a way to specify that this outputs Cronet.dSYM? > > > > > > On Mac at least, we handle dSYMs and stripping at the build configuration > > level. > > > One sets enable_dsyms = true and then the -Wcrl flag gets added by the > > toolchain > > > > > > (https://cs.chromium.org/chromium/src/build/toolchain/mac/BUILD.gn?type=cs&sq=...). > > > > I've tried that, but it didn't work for ios_framework_bundle. I'd love to make > > it work, but I may need some help. > > Sorry if I wasn't being clear; these aren't arguments consumed by the template. > They are build-level `gn args` arguments. Stripping and dSYM production applies > to all targets in the build. That's very cool, I didn't realize that. I've tried adding enable_dsyms to gn args, and it generated dsyms, however it didn't add Cronet.dSYM to the set of outputs, so line 183 below still fails with 'no target that generates this source'. I've also tried to specify 'enable_stripping = true' but that didn't seem to have any effect. Any ideas?
https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:102: ldflags += [ "-Wcrl,dsym," + rebase_path(root_out_dir) ] On 2016/07/01 16:02:35, mef wrote: > On 2016/07/01 15:37:43, Robert Sesek wrote: > > On 2016/07/01 14:10:08, mef wrote: > > > On 2016/06/30 21:43:15, Robert Sesek wrote: > > > > On 2016/06/30 21:29:06, mef wrote: > > > > > Is there a way to specify that this outputs Cronet.dSYM? > > > > > > > > On Mac at least, we handle dSYMs and stripping at the build configuration > > > level. > > > > One sets enable_dsyms = true and then the -Wcrl flag gets added by the > > > toolchain > > > > > > > > > > (https://cs.chromium.org/chromium/src/build/toolchain/mac/BUILD.gn?type=cs&sq=...). > > > > > > I've tried that, but it didn't work for ios_framework_bundle. I'd love to > make > > > it work, but I may need some help. > > > > Sorry if I wasn't being clear; these aren't arguments consumed by the > template. > > They are build-level `gn args` arguments. Stripping and dSYM production > applies > > to all targets in the build. > > That's very cool, I didn't realize that. I've tried adding enable_dsyms to gn > args, and it generated dsyms, however it didn't add Cronet.dSYM to the set of > outputs, so line 183 below still fails with 'no target that generates this > source'. > > I've also tried to specify 'enable_stripping = true' but that didn't seem to > have any effect. Any ideas? Hmm, I guess I worked around the dSYM output issue in the one instance where it came up: https://cs.chromium.org/chromium/src/chrome/BUILD.gn?q=enable_stripping&sq=pa... I don't know why they aren't being considered outputs. For the stripping, the BUILDCONFIG probably needs to be updated with an is_ios condition as well: https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?q=mac:strip_...
Description was changed from ========== GN Rule to build Cronet Dynamic Framework for iOS. BUG= ========== to ========== GN Rule to build Cronet Dynamic Framework for iOS. For release builds set enable_dsyms = true and enable_stripping = true to separate symbols from the framework binary. BUG= ==========
On 2016/07/01 16:50:28, Robert Sesek wrote: > https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... > File components/cronet/ios/BUILD.gn (right): > > https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... > components/cronet/ios/BUILD.gn:102: ldflags += [ "-Wcrl,dsym," + > rebase_path(root_out_dir) ] > On 2016/07/01 16:02:35, mef wrote: > > On 2016/07/01 15:37:43, Robert Sesek wrote: > > > On 2016/07/01 14:10:08, mef wrote: > > > > On 2016/06/30 21:43:15, Robert Sesek wrote: > > > > > On 2016/06/30 21:29:06, mef wrote: > > > > > > Is there a way to specify that this outputs Cronet.dSYM? > > > > > > > > > > On Mac at least, we handle dSYMs and stripping at the build > configuration > > > > level. > > > > > One sets enable_dsyms = true and then the -Wcrl flag gets added by the > > > > toolchain > > > > > > > > > > > > > > > (https://cs.chromium.org/chromium/src/build/toolchain/mac/BUILD.gn?type=cs&sq=...). > > > > > > > > I've tried that, but it didn't work for ios_framework_bundle. I'd love to > > make > > > > it work, but I may need some help. > > > > > > Sorry if I wasn't being clear; these aren't arguments consumed by the > > template. > > > They are build-level `gn args` arguments. Stripping and dSYM production > > applies > > > to all targets in the build. > > > > That's very cool, I didn't realize that. I've tried adding enable_dsyms to gn > > args, and it generated dsyms, however it didn't add Cronet.dSYM to the set of > > outputs, so line 183 below still fails with 'no target that generates this > > source'. > > > > I've also tried to specify 'enable_stripping = true' but that didn't seem to > > have any effect. Any ideas? > > Hmm, I guess I worked around the dSYM output issue in the one instance where it > came up: > https://cs.chromium.org/chromium/src/chrome/BUILD.gn?q=enable_stripping&sq=pa... > > I don't know why they aren't being considered outputs. I see. I guess I'll add archiving action as well unless I find a way to avoid that. > > For the stripping, the BUILDCONFIG probably needs to be updated with an is_ios > condition as well: > https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?q=mac:strip_... Cool! I've added strip_all config and it seems to do the right thing. One more question. In GYP setting target_subarch="both" produces fat binary with 32+64 lipo'd together. Is there matching option in GN?
On 2016/07/01 16:50:28, Robert Sesek wrote: > https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... > File components/cronet/ios/BUILD.gn (right): > > https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... > components/cronet/ios/BUILD.gn:102: ldflags += [ "-Wcrl,dsym," + > rebase_path(root_out_dir) ] > On 2016/07/01 16:02:35, mef wrote: > > On 2016/07/01 15:37:43, Robert Sesek wrote: > > > On 2016/07/01 14:10:08, mef wrote: > > > > On 2016/06/30 21:43:15, Robert Sesek wrote: > > > > > On 2016/06/30 21:29:06, mef wrote: > > > > > > Is there a way to specify that this outputs Cronet.dSYM? > > > > > > > > > > On Mac at least, we handle dSYMs and stripping at the build > configuration > > > > level. > > > > > One sets enable_dsyms = true and then the -Wcrl flag gets added by the > > > > toolchain > > > > > > > > > > > > > > > (https://cs.chromium.org/chromium/src/build/toolchain/mac/BUILD.gn?type=cs&sq=...). > > > > > > > > I've tried that, but it didn't work for ios_framework_bundle. I'd love to > > make > > > > it work, but I may need some help. > > > > > > Sorry if I wasn't being clear; these aren't arguments consumed by the > > template. > > > They are build-level `gn args` arguments. Stripping and dSYM production > > applies > > > to all targets in the build. > > > > That's very cool, I didn't realize that. I've tried adding enable_dsyms to gn > > args, and it generated dsyms, however it didn't add Cronet.dSYM to the set of > > outputs, so line 183 below still fails with 'no target that generates this > > source'. > > > > I've also tried to specify 'enable_stripping = true' but that didn't seem to > > have any effect. Any ideas? > > Hmm, I guess I worked around the dSYM output issue in the one instance where it > came up: > https://cs.chromium.org/chromium/src/chrome/BUILD.gn?q=enable_stripping&sq=pa... > > I don't know why they aren't being considered outputs. I see. I guess I'll add archiving action as well unless I find a way to avoid that. > > For the stripping, the BUILDCONFIG probably needs to be updated with an is_ios > condition as well: > https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?q=mac:strip_... Cool! I've added strip_all config and it seems to do the right thing. One more question. In GYP setting target_subarch="both" produces fat binary with 32+64 lipo'd together. Is there matching option in GN?
On 2016/07/01 19:51:20, mef wrote: > On 2016/07/01 16:50:28, Robert Sesek wrote: > > > https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... > > File components/cronet/ios/BUILD.gn (right): > > > > > https://codereview.chromium.org/2115583002/diff/80001/components/cronet/ios/B... > > components/cronet/ios/BUILD.gn:102: ldflags += [ "-Wcrl,dsym," + > > rebase_path(root_out_dir) ] > > On 2016/07/01 16:02:35, mef wrote: > > > On 2016/07/01 15:37:43, Robert Sesek wrote: > > > > On 2016/07/01 14:10:08, mef wrote: > > > > > On 2016/06/30 21:43:15, Robert Sesek wrote: > > > > > > On 2016/06/30 21:29:06, mef wrote: > > > > > > > Is there a way to specify that this outputs Cronet.dSYM? > > > > > > > > > > > > On Mac at least, we handle dSYMs and stripping at the build > > configuration > > > > > level. > > > > > > One sets enable_dsyms = true and then the -Wcrl flag gets added by the > > > > > toolchain > > > > > > > > > > > > > > > > > > > > > (https://cs.chromium.org/chromium/src/build/toolchain/mac/BUILD.gn?type=cs&sq=...). > > > > > > > > > > I've tried that, but it didn't work for ios_framework_bundle. I'd love > to > > > make > > > > > it work, but I may need some help. > > > > > > > > Sorry if I wasn't being clear; these aren't arguments consumed by the > > > template. > > > > They are build-level `gn args` arguments. Stripping and dSYM production > > > applies > > > > to all targets in the build. > > > > > > That's very cool, I didn't realize that. I've tried adding enable_dsyms to > gn > > > args, and it generated dsyms, however it didn't add Cronet.dSYM to the set > of > > > outputs, so line 183 below still fails with 'no target that generates this > > > source'. > > > > > > I've also tried to specify 'enable_stripping = true' but that didn't seem to > > > have any effect. Any ideas? > > > > Hmm, I guess I worked around the dSYM output issue in the one instance where > it > > came up: > > > https://cs.chromium.org/chromium/src/chrome/BUILD.gn?q=enable_stripping&sq=pa... > > > > I don't know why they aren't being considered outputs. > I see. I guess I'll add archiving action as well unless I find a way to avoid > that. > > > > For the stripping, the BUILDCONFIG probably needs to be updated with an is_ios > > condition as well: > > > https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?q=mac:strip_... > Cool! I've added strip_all config and it seems to do the right thing. > > One more question. In GYP setting target_subarch="both" produces fat binary with > 32+64 lipo'd together. > Is there matching option in GN? Sylvain is working on fat binaries for iOS but I think that's still a WIP.
PTAL. I've updated the package_ios.py script that we use to build all permutations of Cronet framework to build one target_cpu at a time and lipo/dsymutil/strip them explicitly. The resulting package matches the one generated using gyp.
LGTM. One comment. https://codereview.chromium.org/2115583002/diff/200001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2115583002/diff/200001/components/cronet/ios/... components/cronet/ios/BUILD.gn:110: configs += [ "//build/config/mac:strip_all" ] We should remove this line since we should not strip the symbols. The symbols should be preserved for the debug version. For the release version, we should strip them after dsym bundle is generated by dsymutil.
https://codereview.chromium.org/2115583002/diff/200001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2115583002/diff/200001/components/cronet/ios/... components/cronet/ios/BUILD.gn:110: configs += [ "//build/config/mac:strip_all" ] On 2016/07/14 21:37:37, kapishnikov wrote: > We should remove this line since we should not strip the symbols. The symbols > should be preserved for the debug version. For the release version, we should > strip them after dsym bundle is generated by dsymutil. The symbols are stripped after dsymutil is run. The way to preserve symbols in a debug build is to not have |enable_stripping=true| in your gn args.
https://codereview.chromium.org/2115583002/diff/200001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2115583002/diff/200001/components/cronet/ios/... components/cronet/ios/BUILD.gn:110: configs += [ "//build/config/mac:strip_all" ] On 2016/07/14 21:40:12, Robert Sesek wrote: > On 2016/07/14 21:37:37, kapishnikov wrote: > > We should remove this line since we should not strip the symbols. The symbols > > should be preserved for the debug version. For the release version, we should > > strip them after dsym bundle is generated by dsymutil. > > The symbols are stripped after dsymutil is run. The way to preserve symbols in a > debug build is to not have |enable_stripping=true| in your gn args. Does the target generate the dsym bundle? If yes, then we don't need to call dsymutil from package_ios.py. What flag determines whether dsym should be generated?
https://codereview.chromium.org/2115583002/diff/200001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2115583002/diff/200001/components/cronet/ios/... components/cronet/ios/BUILD.gn:110: configs += [ "//build/config/mac:strip_all" ] On 2016/07/14 21:45:50, kapishnikov wrote: > On 2016/07/14 21:40:12, Robert Sesek wrote: > > On 2016/07/14 21:37:37, kapishnikov wrote: > > > We should remove this line since we should not strip the symbols. The > symbols > > > should be preserved for the debug version. For the release version, we > should > > > strip them after dsym bundle is generated by dsymutil. > > > > The symbols are stripped after dsymutil is run. The way to preserve symbols in > a > > debug build is to not have |enable_stripping=true| in your gn args. > > Does the target generate the dsym bundle? If yes, then we don't need to call > dsymutil from package_ios.py. What flag determines whether dsym should be > generated? If the gn arg |enable_dsyms=true|, then all linked output will have a dSYM generated. This is done by //build/toolchain/mac/linker_driver.py, which is configured by //build/config/toolchain/mac/BUILD.gn. This is at least how we've set it up on Mac. I don't know if Sylvain has finished doing this on iOS, but it should be very similar.
https://codereview.chromium.org/2115583002/diff/200001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2115583002/diff/200001/components/cronet/ios/... components/cronet/ios/BUILD.gn:110: configs += [ "//build/config/mac:strip_all" ] On 2016/07/14 21:49:34, Robert Sesek wrote: > On 2016/07/14 21:45:50, kapishnikov wrote: > > On 2016/07/14 21:40:12, Robert Sesek wrote: > > > On 2016/07/14 21:37:37, kapishnikov wrote: > > > > We should remove this line since we should not strip the symbols. The > > symbols > > > > should be preserved for the debug version. For the release version, we > > should > > > > strip them after dsym bundle is generated by dsymutil. > > > > > > The symbols are stripped after dsymutil is run. The way to preserve symbols > in > > a > > > debug build is to not have |enable_stripping=true| in your gn args. > > > > Does the target generate the dsym bundle? If yes, then we don't need to call > > dsymutil from package_ios.py. What flag determines whether dsym should be > > generated? > > If the gn arg |enable_dsyms=true|, then all linked output will have a dSYM > generated. This is done by //build/toolchain/mac/linker_driver.py, which is > configured by //build/config/toolchain/mac/BUILD.gn. This is at least how we've > set it up on Mac. I don't know if Sylvain has finished doing this on iOS, but it > should be very similar. This works with gn arg enable_dsyms=true and enable_stripping=true, however currently gn doesn't support fat binaries (Sylvain is working on that), so symbol extraction and stripping is done explicitly in package_ios.py after lipo-ing fat library together.
Description was changed from ========== GN Rule to build Cronet Dynamic Framework for iOS. For release builds set enable_dsyms = true and enable_stripping = true to separate symbols from the framework binary. BUG= ========== to ========== GN Rule to build Cronet Dynamic Framework for iOS. For release builds set gn args enable_dsyms = true and enable_stripping = true to separate symbols from the framework binary. Added components/cronet/tools/package_ios.py option --gn to use gn instead of gyp to build cronet_package target. ==========
On 2016/07/15 13:50:28, mef wrote: > https://codereview.chromium.org/2115583002/diff/200001/components/cronet/ios/... > File components/cronet/ios/BUILD.gn (right): > > https://codereview.chromium.org/2115583002/diff/200001/components/cronet/ios/... > components/cronet/ios/BUILD.gn:110: configs += [ "//build/config/mac:strip_all" > ] > On 2016/07/14 21:49:34, Robert Sesek wrote: > > On 2016/07/14 21:45:50, kapishnikov wrote: > > > On 2016/07/14 21:40:12, Robert Sesek wrote: > > > > On 2016/07/14 21:37:37, kapishnikov wrote: > > > > > We should remove this line since we should not strip the symbols. The > > > symbols > > > > > should be preserved for the debug version. For the release version, we > > > should > > > > > strip them after dsym bundle is generated by dsymutil. > > > > > > > > The symbols are stripped after dsymutil is run. The way to preserve > symbols > > in > > > a > > > > debug build is to not have |enable_stripping=true| in your gn args. > > > > > > Does the target generate the dsym bundle? If yes, then we don't need to call > > > dsymutil from package_ios.py. What flag determines whether dsym should be > > > generated? > > > > If the gn arg |enable_dsyms=true|, then all linked output will have a dSYM > > generated. This is done by //build/toolchain/mac/linker_driver.py, which is > > configured by //build/config/toolchain/mac/BUILD.gn. This is at least how > we've > > set it up on Mac. I don't know if Sylvain has finished doing this on iOS, but > it > > should be very similar. > > This works with gn arg enable_dsyms=true and enable_stripping=true, however > currently gn doesn't support fat binaries (Sylvain is working on that), so > symbol extraction and stripping is done explicitly in package_ios.py after > lipo-ing fat library together. Acknowledged. I think the current implementation is the best way to work around the limitation with the fat binaries. After Robert's comments, I agree that we should keep "//build/config/mac:strip_all" config. Even though "package_ios.py" never applies the stripping through this config, it might be useful in some other scenarios. LGTM.
The CQ bit was checked by mef@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.
Description was changed from ========== GN Rule to build Cronet Dynamic Framework for iOS. For release builds set gn args enable_dsyms = true and enable_stripping = true to separate symbols from the framework binary. Added components/cronet/tools/package_ios.py option --gn to use gn instead of gyp to build cronet_package target. ========== to ========== GN Rule to build Cronet Dynamic Framework for iOS. For release builds set gn args enable_dsyms = true and enable_stripping = true to separate symbols from the framework binary. Added components/cronet/tools/package_ios.py option --gn to use gn instead of gyp to build cronet_package target. ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== GN Rule to build Cronet Dynamic Framework for iOS. For release builds set gn args enable_dsyms = true and enable_stripping = true to separate symbols from the framework binary. Added components/cronet/tools/package_ios.py option --gn to use gn instead of gyp to build cronet_package target. ========== to ========== GN Rule to build Cronet Dynamic Framework for iOS. For release builds set gn args enable_dsyms = true and enable_stripping = true to separate symbols from the framework binary. Added components/cronet/tools/package_ios.py option --gn to use gn instead of gyp to build cronet_package target. Committed: https://crrev.com/941bc7e523e5502a6d2211d0c0837504e0bfc5dd Cr-Commit-Position: refs/heads/master@{#405768} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/941bc7e523e5502a6d2211d0c0837504e0bfc5dd Cr-Commit-Position: refs/heads/master@{#405768}
Message was sent while issue was closed.
On 2016/07/15 16:20:37, commit-bot: I haz the power wrote: > Patchset 11 (id:??) landed as > https://crrev.com/941bc7e523e5502a6d2211d0c0837504e0bfc5dd > Cr-Commit-Position: refs/heads/master@{#405768} Support for fat binaries landed with https://codereview.chromium.org/2135323002/. To build fat binary, you would use "additional_target_cpus" when running "gn args". For example, to build for both 32-bit and 64-bit arm architectures, you would have the following in args.gn file: target_cpu = "arm64" additional_target_cpus = [ "arm" ] I have not tested the interaction with enable_dsyms=true. If this does not work as expected, then it is a bug and I'll address it before EOW.
Message was sent while issue was closed.
On 2016/07/18 12:51:12, sdefresne wrote: > On 2016/07/15 16:20:37, commit-bot: I haz the power wrote: > > Patchset 11 (id:??) landed as > > https://crrev.com/941bc7e523e5502a6d2211d0c0837504e0bfc5dd > > Cr-Commit-Position: refs/heads/master@{#405768} > > Support for fat binaries landed with > https://codereview.chromium.org/2135323002/. > > To build fat binary, you would use "additional_target_cpus" when running "gn > args". For example, to build for both 32-bit and 64-bit arm architectures, you > would have the following in args.gn file: > > target_cpu = "arm64" > additional_target_cpus = [ "arm" ] > > I have not tested the interaction with enable_dsyms=true. If this does not work > as expected, then it is a bug and I'll address it before EOW. Great, I'll try it out!
Message was sent while issue was closed.
On 2016/07/18 16:27:10, mef wrote: > On 2016/07/18 12:51:12, sdefresne wrote: > > On 2016/07/15 16:20:37, commit-bot: I haz the power wrote: > > > Patchset 11 (id:??) landed as > > > https://crrev.com/941bc7e523e5502a6d2211d0c0837504e0bfc5dd > > > Cr-Commit-Position: refs/heads/master@{#405768} > > > > Support for fat binaries landed with > > https://codereview.chromium.org/2135323002/. > > > > To build fat binary, you would use "additional_target_cpus" when running "gn > > args". For example, to build for both 32-bit and 64-bit arm architectures, you > > would have the following in args.gn file: > > > > target_cpu = "arm64" > > additional_target_cpus = [ "arm" ] > > > > I have not tested the interaction with enable_dsyms=true. If this does not > work > > as expected, then it is a bug and I'll address it before EOW. > > Great, I'll try it out! https://codereview.chromium.org/2160653002/ is there to fix the dSYM support for fat builds. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
