Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2807283002:
[Cronet] Build static libcronet.a for iOS with complete dependencies. (Closed)
Description[Cronet] Build static libcronet.a for iOS with complete dependencies.
- Hide dependencies inside of the library to avoid symbol conflicts.
- Add cronet_consumer_static target that builds consumer app with static library.
BUG=721922
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2807283002
Cr-Original-Commit-Position: refs/heads/master@{#475158}
Committed: https://chromium.googlesource.com/chromium/src/+/3412c3c60b69798430b3a3cc9d2834f5d44e6042
Review-Url: https://codereview.chromium.org/2807283002
Cr-Commit-Position: refs/heads/master@{#476986}
Committed: https://chromium.googlesource.com/chromium/src/+/f035c8c0053e7bcec27f968bf7fbbfc062369e0f
Patch Set 1 #
Total comments: 3
Patch Set 2 : Change package_ios.py to build per architecture and skip x86. #
Total comments: 3
Patch Set 3 : Sync #Patch Set 4 : Remove cronet_sample, make cronet_consumer_static buildable with lib_cronet. #
Total comments: 13
Patch Set 5 : Working fat libcronet.a #Patch Set 6 : Cleanup and address some comments. #Patch Set 7 : Sync #
Total comments: 2
Patch Set 8 : Workaround i386 debug failure by using input library without dependencies. #
Total comments: 2
Patch Set 9 : Define and use public configs for include_dirs and libs. #Patch Set 10 : Create cronet_consumer_template and use it for static and regular consumer. #
Total comments: 13
Patch Set 11 : Clean up and add static framework. #
Total comments: 3
Patch Set 12 : Add rudimentary ios_static_framework template. #
Total comments: 7
Patch Set 13 : Use libtool to combine i386 Cronet library and its dependencies. #
Total comments: 15
Patch Set 14 : More generic ios_static_framework template. #Patch Set 15 : Back to x64. #Patch Set 16 : Make cronet_consumer_static work without cronet_framework+link. #Patch Set 17 : Use dummy action to move from CronetStatic.framework to Static/Cronet.framework #Patch Set 18 : Sync and format. #Patch Set 19 : Define bundle_resources_dir, etc to fix build on regular ios bots. #Patch Set 20 : Only build cronet_consumer_static on Cronet builders (is_cronet_build). #Patch Set 21 : Disable cronet_consumer_static target for fat builds. #
Total comments: 4
Messages
Total messages: 84 (42 generated)
Description was changed from ========== [Cronet] Prototype static libcronet.a for iOS with complete dependencies. Prototype Cronet static library and Cronet Sample App for iOS. BUG= ========== to ========== [Cronet] Prototype static libcronet.a for iOS with complete dependencies. Prototype Cronet static library and Cronet Sample App for iOS. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
The CQ bit was checked by mef@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Cronet] Prototype static libcronet.a for iOS with complete dependencies. Prototype Cronet static library and Cronet Sample App for iOS. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [Cronet] Prototype static libcronet.a for iOS with complete dependencies. Prototype Cronet static library and Cronet Sample App for iOS. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
mef@chromium.org changed reviewers: + ichikawa@chromium.org
Hi Hiroshi, this is my prototype CL, and I have 2 unresolved questions / issues. Do you have any suggestions? thanks, -m https://codereview.chromium.org/2807283002/diff/1/components/cronet/tools/hid... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/1/components/cronet/tools/hid... components/cronet/tools/hide_symbols.py:61: '-arch', GN_CPU_TO_LD_ARCH[options.current_cpu], This gets an error in debug simulator build: ld: scattered reloc r_address too large for architecture i386 https://codereview.chromium.org/2807283002/diff/20001/components/cronet/tools... File components/cronet/tools/package_ios.py (left): https://codereview.chromium.org/2807283002/diff/20001/components/cronet/tools... components/cronet/tools/package_ios.py:104: 'target_cpu="%s" additional_target_cpus = ["%s"] %s' % \ Using |additional_target_cpus| fails for hide_symbols script. Official Cronet builders currently use |additional_target_cpus| to build fat framework. Any ideas on how to fix this without creating a new set of builders?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2807283002/diff/1/components/cronet/tools/hid... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/1/components/cronet/tools/hid... components/cronet/tools/hide_symbols.py:61: '-arch', GN_CPU_TO_LD_ARCH[options.current_cpu], On 2017/04/11 21:05:09, mef wrote: > This gets an error in debug simulator build: > > ld: scattered reloc r_address too large for architecture i386 Does it happen only on trybot, or does it happen on local build too? For me, it worked fine with local build, and the issue happened only on trybot. If it's also the case for you, one possible workaround is to stop integrating hide_symbols.py into the BUILD.gn file, and run the script manually locally to build the static library. It's not a great solution, though. Another possible solution is to upgrade the version of XCode toolchain in trybot. I'm doubting that the issue happens only with an old version of XCode toolchain, though not verified. https://codereview.chromium.org/2807283002/diff/20001/components/cronet/tools... File components/cronet/tools/package_ios.py (left): https://codereview.chromium.org/2807283002/diff/20001/components/cronet/tools... components/cronet/tools/package_ios.py:104: 'target_cpu="%s" additional_target_cpus = ["%s"] %s' % \ On 2017/04/11 21:05:09, mef wrote: > Using |additional_target_cpus| fails for hide_symbols script. > Official Cronet builders currently use |additional_target_cpus| to build fat > framework. > Any ideas on how to fix this without creating a new set of builders? So far I build the library in multiple architectures and hit lipo command manually to build a fat binary. You may do the same in this script, or write a new GN rule to do something similar to ios_framework_bundle rule does: https://cs.chromium.org/chromium/src/build/config/ios/rules.gni?l=990&rcl=b6d...
Thanks for your comments! https://codereview.chromium.org/2807283002/diff/1/components/cronet/tools/hid... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/1/components/cronet/tools/hid... components/cronet/tools/hide_symbols.py:61: '-arch', GN_CPU_TO_LD_ARCH[options.current_cpu], On 2017/04/12 04:35:25, Hiroshi Ichikawa wrote: > On 2017/04/11 21:05:09, mef wrote: > > This gets an error in debug simulator build: > > > > ld: scattered reloc r_address too large for architecture i386 > > Does it happen only on trybot, or does it happen on local build too? It happens locally, but only for one configuration - x86 Debug. Possibly because of extra debug symbols that make object file too big? > > For me, it worked fine with local build, and the issue happened only on trybot. > > If it's also the case for you, one possible workaround is to stop integrating > hide_symbols.py into the BUILD.gn file, and run the script manually locally to > build the static library. It's not a great solution, though. I'm thinking about reducing symbol level and/or splitting combined .o file into two or more. Not that great either. > > Another possible solution is to upgrade the version of XCode toolchain in > trybot. I'm doubting that the issue happens only with an old version of XCode > toolchain, though not verified. I'm using XCode 8.2.1, which (IIUIC) is current. https://codereview.chromium.org/2807283002/diff/20001/components/cronet/tools... File components/cronet/tools/package_ios.py (left): https://codereview.chromium.org/2807283002/diff/20001/components/cronet/tools... components/cronet/tools/package_ios.py:104: 'target_cpu="%s" additional_target_cpus = ["%s"] %s' % \ On 2017/04/12 04:35:25, Hiroshi Ichikawa wrote: > On 2017/04/11 21:05:09, mef wrote: > > Using |additional_target_cpus| fails for hide_symbols script. > > Official Cronet builders currently use |additional_target_cpus| to build fat > > framework. > > Any ideas on how to fix this without creating a new set of builders? > > So far I build the library in multiple architectures and hit lipo command > manually to build a fat binary. > > You may do the same in this script, or write a new GN rule to do something > similar to ios_framework_bundle rule does: > https://cs.chromium.org/chromium/src/build/config/ios/rules.gni?l=990&rcl=b6d... Thanks for the pointer! I'll give it a try if this experiment pans out.
mef@chromium.org changed reviewers: + kapishnikov@chromium.org, lilyhoughton@chromium.org
Description was changed from ========== [Cronet] Prototype static libcronet.a for iOS with complete dependencies. Prototype Cronet static library and Cronet Sample App for iOS. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [Cronet] Build static libcronet.a for iOS with complete dependencies. Prototype Cronet static library and Cronet Sample App for iOS. BUG=721922 ==========
On 2017/04/12 15:57:50, mef wrote: > Thanks for your comments! > > https://codereview.chromium.org/2807283002/diff/1/components/cronet/tools/hid... > File components/cronet/tools/hide_symbols.py (right): > > https://codereview.chromium.org/2807283002/diff/1/components/cronet/tools/hid... > components/cronet/tools/hide_symbols.py:61: '-arch', > GN_CPU_TO_LD_ARCH[options.current_cpu], > On 2017/04/12 04:35:25, Hiroshi Ichikawa wrote: > > On 2017/04/11 21:05:09, mef wrote: > > > This gets an error in debug simulator build: > > > > > > ld: scattered reloc r_address too large for architecture i386 > > > > Does it happen only on trybot, or does it happen on local build too? > It happens locally, but only for one configuration - x86 Debug. > Possibly because of extra debug symbols that make object file too big? > > > > For me, it worked fine with local build, and the issue happened only on > trybot. > > > > If it's also the case for you, one possible workaround is to stop integrating > > hide_symbols.py into the BUILD.gn file, and run the script manually locally to > > build the static library. It's not a great solution, though. > I'm thinking about reducing symbol level and/or splitting combined .o file into > two or more. > Not that great either. > > > > Another possible solution is to upgrade the version of XCode toolchain in > > trybot. I'm doubting that the issue happens only with an old version of XCode > > toolchain, though not verified. > I'm using XCode 8.2.1, which (IIUIC) is current. > > https://codereview.chromium.org/2807283002/diff/20001/components/cronet/tools... > File components/cronet/tools/package_ios.py (left): > > https://codereview.chromium.org/2807283002/diff/20001/components/cronet/tools... > components/cronet/tools/package_ios.py:104: 'target_cpu="%s" > additional_target_cpus = ["%s"] %s' % \ > On 2017/04/12 04:35:25, Hiroshi Ichikawa wrote: > > On 2017/04/11 21:05:09, mef wrote: > > > Using |additional_target_cpus| fails for hide_symbols script. > > > Official Cronet builders currently use |additional_target_cpus| to build fat > > > framework. > > > Any ideas on how to fix this without creating a new set of builders? > > > > So far I build the library in multiple architectures and hit lipo command > > manually to build a fat binary. > > > > You may do the same in this script, or write a new GN rule to do something > > similar to ios_framework_bundle rule does: > > > https://cs.chromium.org/chromium/src/build/config/ios/rules.gni?l=990&rcl=b6d... > > Thanks for the pointer! I'll give it a try if this experiment pans out. Hey mef, can you provide the command you used to build each individual architecture? In particular, I'm trying to build static cronet for arm64.
https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/B... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:187: static_library("deps_complete") { Should we add "cronet_" prefix in front of the name? Even though the target is private, I think it can be invoked from the command line. https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:190: deps = _deps Why don't we add "cronet_static" as the dependency? https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:205: action("lib_cronet") { Is it possible to build a static iOS framework and expose it to the public instead of the stand-alone static library? https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:307: ":generate_license", Should the sample be part of the package? At least, we can use this target to compile it. https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_consumer/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_consumer/BUILD.gn:34: ios_app_bundle("cronet_consumer_static") { It looks that "cronet_consumer" & "cronet_consumer_static" are almost identical. Should we create a template and pass either "//components/cronet/ios:cronet_framework+link" or "//components/cronet/ios:lib_cronet" depending on the cronet library we want to use. https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_consumer/cronet_consumer_app_delegate.mm (right): https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_consumer/cronet_consumer_app_delegate.mm:7: #import "components/cronet/ios/Cronet.h" Ideally, we should not make this change but depend on the headers from the static/dynamic framework. https://codereview.chromium.org/2807283002/diff/60001/components/cronet/tools... File components/cronet/tools/package_ios.py (right): https://codereview.chromium.org/2807283002/diff/60001/components/cronet/tools... components/cronet/tools/package_ios.py:103: for target_cpu in target_cpus: The output has changed to contain thin libraries instead of the 'fat' ones, i.e. there will be a separate library for 'arm', a separate library for 'arm64' instead of the one 'arm+arm64' library. I think the embedders would prefer a fat one, otherwise it will be necessary to run 'lipo' manually.
On 2017/05/17 07:24:54, jzw1 wrote: > On 2017/04/12 15:57:50, mef wrote: > > Thanks for your comments! > > > > > https://codereview.chromium.org/2807283002/diff/1/components/cronet/tools/hid... > > File components/cronet/tools/hide_symbols.py (right): > > > > > https://codereview.chromium.org/2807283002/diff/1/components/cronet/tools/hid... > > components/cronet/tools/hide_symbols.py:61: '-arch', > > GN_CPU_TO_LD_ARCH[options.current_cpu], > > On 2017/04/12 04:35:25, Hiroshi Ichikawa wrote: > > > On 2017/04/11 21:05:09, mef wrote: > > > > This gets an error in debug simulator build: > > > > > > > > ld: scattered reloc r_address too large for architecture i386 > > > > > > Does it happen only on trybot, or does it happen on local build too? > > It happens locally, but only for one configuration - x86 Debug. > > Possibly because of extra debug symbols that make object file too big? > > > > > > For me, it worked fine with local build, and the issue happened only on > > trybot. > > > > > > If it's also the case for you, one possible workaround is to stop > integrating > > > hide_symbols.py into the BUILD.gn file, and run the script manually locally > to > > > build the static library. It's not a great solution, though. > > I'm thinking about reducing symbol level and/or splitting combined .o file > into > > two or more. > > Not that great either. > > > > > > Another possible solution is to upgrade the version of XCode toolchain in > > > trybot. I'm doubting that the issue happens only with an old version of > XCode > > > toolchain, though not verified. > > I'm using XCode 8.2.1, which (IIUIC) is current. > > > > > https://codereview.chromium.org/2807283002/diff/20001/components/cronet/tools... > > File components/cronet/tools/package_ios.py (left): > > > > > https://codereview.chromium.org/2807283002/diff/20001/components/cronet/tools... > > components/cronet/tools/package_ios.py:104: 'target_cpu="%s" > > additional_target_cpus = ["%s"] %s' % \ > > On 2017/04/12 04:35:25, Hiroshi Ichikawa wrote: > > > On 2017/04/11 21:05:09, mef wrote: > > > > Using |additional_target_cpus| fails for hide_symbols script. > > > > Official Cronet builders currently use |additional_target_cpus| to build > fat > > > > framework. > > > > Any ideas on how to fix this without creating a new set of builders? > > > > > > So far I build the library in multiple architectures and hit lipo command > > > manually to build a fat binary. > > > > > > You may do the same in this script, or write a new GN rule to do something > > > similar to ios_framework_bundle rule does: > > > > > > https://cs.chromium.org/chromium/src/build/config/ios/rules.gni?l=990&rcl=b6d... > > > > Thanks for the pointer! I'll give it a try if this experiment pans out. > > Hey mef, can you provide the command you used to build each individual > architecture? In particular, I'm trying to build static cronet for arm64. I've used the command to build for all supported architectures: components/cronet/tools/package_ios.py -g out/Cronet In order to build just ARM64 release version you'd need to run: gn gen out/Release-iphoneos --args='is_cronet_build=true is_component_build=false use_xcode_clang=true target_cpu="arm64" target_os="ios" enable_websockets=false disable_file_support=true disable_ftp_support=true use_platform_icu_alternatives=true is_component_build=false ignore_elf32_limitations=true use_partition_alloc=false is_debug=false ' --ide=xcode followed by ninja -C out/Release-iphoneos lib_cronet
Description was changed from ========== [Cronet] Build static libcronet.a for iOS with complete dependencies. Prototype Cronet static library and Cronet Sample App for iOS. BUG=721922 ========== to ========== [Cronet] Build static libcronet.a for iOS with complete dependencies. Prototype Cronet static library and Cronet Sample App for iOS. BUG=721922 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
mef@chromium.org changed reviewers: + rsesek@chromium.org
PTAL, we've got this to work for fat arm32+arm64 builds, so this should be able to run on cronet buildbots. Robert, could you take a look at BUILD.gn file changes to see if they make sense. https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/B... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:187: static_library("deps_complete") { On 2017/05/17 16:02:49, kapishnikov wrote: > Should we add "cronet_" prefix in front of the name? Even though the target is > private, I think it can be invoked from the command line. Done. https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/B... components/cronet/ios/BUILD.gn:205: action("lib_cronet") { On 2017/05/17 16:02:49, kapishnikov wrote: > Is it possible to build a static iOS framework and expose it to the public > instead of the stand-alone static library? Yes, we should be able to do that. Next patch maybe? https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_consumer/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_consumer/BUILD.gn:34: ios_app_bundle("cronet_consumer_static") { On 2017/05/17 16:02:49, kapishnikov wrote: > It looks that "cronet_consumer" & "cronet_consumer_static" are almost identical. > Should we create a template and pass either > "//components/cronet/ios:cronet_framework+link" or > "//components/cronet/ios:lib_cronet" depending on the cronet library we want to > use. Good idea. I'll try that in the next patch. https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_consumer/cronet_consumer_app_delegate.mm (right): https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_consumer/cronet_consumer_app_delegate.mm:7: #import "components/cronet/ios/Cronet.h" On 2017/05/17 16:02:49, kapishnikov wrote: > Ideally, we should not make this change but depend on the headers from the > static/dynamic framework. Yes, ideally we'll make a static framework, so consumer #include would not change. https://codereview.chromium.org/2807283002/diff/60001/components/cronet/tools... File components/cronet/tools/package_ios.py (right): https://codereview.chromium.org/2807283002/diff/60001/components/cronet/tools... components/cronet/tools/package_ios.py:103: for target_cpu in target_cpus: On 2017/05/17 16:02:49, kapishnikov wrote: > The output has changed to contain thin libraries instead of the 'fat' ones, i.e. > there will be a separate library for 'arm', a separate library for 'arm64' > instead of the one 'arm+arm64' library. I think the embedders would prefer a fat > one, otherwise it will be necessary to run 'lipo' manually. With latest changes in BUILD.gn we should be able to avoid splitting builds per target_cpu.
Description was changed from ========== [Cronet] Build static libcronet.a for iOS with complete dependencies. Prototype Cronet static library and Cronet Sample App for iOS. BUG=721922 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [Cronet] Build static libcronet.a for iOS with complete dependencies. - Hide dependencies inside of the library to avoid symbol conflicts. - Add cronet_consumer_static target that builds consumer app with static library. BUG=721922 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
https://codereview.chromium.org/2807283002/diff/120001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/120001/components/cronet/ios/... components/cronet/ios/BUILD.gn:145: static_library("cronet_static") { Why not use a public_deps on :cronet_sources ? https://codereview.chromium.org/2807283002/diff/140001/components/cronet/ios/... File components/cronet/ios/cronet_consumer/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_consumer/BUILD.gn:27: include_dirs = [ "//components/grpc_support/include" ] If all cronet consumers will need this include_dirs set, consider creating a public_config for this instead.
https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_consumer/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_consumer/BUILD.gn:34: ios_app_bundle("cronet_consumer_static") { On 2017/05/19 18:07:59, mef wrote: > On 2017/05/17 16:02:49, kapishnikov wrote: > > It looks that "cronet_consumer" & "cronet_consumer_static" are almost > identical. > > Should we create a template and pass either > > "//components/cronet/ios:cronet_framework+link" or > > "//components/cronet/ios:lib_cronet" depending on the cronet library we want > to > > use. > > Good idea. I'll try that in the next patch. Done. https://codereview.chromium.org/2807283002/diff/120001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/120001/components/cronet/ios/... components/cronet/ios/BUILD.gn:145: static_library("cronet_static") { On 2017/05/19 20:01:41, Robert Sesek (OOO til May-31) wrote: > Why not use a public_deps on :cronet_sources ? Hrm, I'm not sure what that means. I'll reach out to you after you return. https://codereview.chromium.org/2807283002/diff/140001/components/cronet/ios/... File components/cronet/ios/cronet_consumer/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_consumer/BUILD.gn:27: include_dirs = [ "//components/grpc_support/include" ] On 2017/05/19 20:01:41, Robert Sesek (OOO til May-31) wrote: > If all cronet consumers will need this include_dirs set, consider creating a > public_config for this instead. Ideally those headers will come from Cronet[Static].framework, so there will be no need for explicit include.
https://codereview.chromium.org/2807283002/diff/180001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/180001/components/cronet/ios/... components/cronet/ios/BUILD.gn:108: include_dirs = [ "//components/grpc_support/include" ] Is it still needed with the public_configs below? https://codereview.chromium.org/2807283002/diff/180001/components/cronet/ios/... components/cronet/ios/BUILD.gn:170: include_dirs = [ "//components/grpc_support/include" ] Replace it with "cronet_include_config"? https://codereview.chromium.org/2807283002/diff/180001/components/cronet/ios/... components/cronet/ios/BUILD.gn:193: action("cronet_hide") { Do we intend to keep this target public? If yes, we should give it a more meaningful name. https://codereview.chromium.org/2807283002/diff/180001/components/cronet/ios/... components/cronet/ios/BUILD.gn:218: ":cronet_include_config", Should we add ":cronet_include_config" to ":cronet_static", so it come from there? https://codereview.chromium.org/2807283002/diff/180001/components/cronet/tool... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/180001/components/cronet/tool... components/cronet/tools/hide_symbols.py:83: '-arch', 'i386', options.input_libs, Should "options.deps_lib" be also included?
Thanks, PTAL! https://codereview.chromium.org/2807283002/diff/180001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/180001/components/cronet/ios/... components/cronet/ios/BUILD.gn:108: include_dirs = [ "//components/grpc_support/include" ] On 2017/05/22 17:15:57, kapishnikov wrote: > Is it still needed with the public_configs below? Done. https://codereview.chromium.org/2807283002/diff/180001/components/cronet/ios/... components/cronet/ios/BUILD.gn:170: include_dirs = [ "//components/grpc_support/include" ] On 2017/05/22 17:15:57, kapishnikov wrote: > Replace it with "cronet_include_config"? Done. https://codereview.chromium.org/2807283002/diff/180001/components/cronet/ios/... components/cronet/ios/BUILD.gn:193: action("cronet_hide") { On 2017/05/22 17:15:57, kapishnikov wrote: > Do we intend to keep this target public? If yes, we should give it a more > meaningful name. It is currently used by cronet_consumer_static. If we can make static framework, then we could hide this. I've renamed it for now. https://codereview.chromium.org/2807283002/diff/180001/components/cronet/ios/... components/cronet/ios/BUILD.gn:218: ":cronet_include_config", On 2017/05/22 17:15:56, kapishnikov wrote: > Should we add ":cronet_include_config" to ":cronet_static", so it come from > there? Done. https://codereview.chromium.org/2807283002/diff/180001/components/cronet/tool... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/180001/components/cronet/tool... components/cronet/tools/hide_symbols.py:83: '-arch', 'i386', options.input_libs, On 2017/05/22 17:15:57, kapishnikov wrote: > Should "options.deps_lib" be also included? Maybe. I'm getting cryptic error though: fatal error: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/lipo: obj/components/cronet/ios/libcronet_static.a and obj/components/cronet/ios/libcronet_deps_complete.a have the same architectures (i386) and can't be in the same fat output file
https://codereview.chromium.org/2807283002/diff/180001/components/cronet/tool... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/180001/components/cronet/tool... components/cronet/tools/hide_symbols.py:83: '-arch', 'i386', options.input_libs, On 2017/05/22 20:53:53, mef wrote: > On 2017/05/22 17:15:57, kapishnikov wrote: > > Should "options.deps_lib" be also included? > > Maybe. I'm getting cryptic error though: > fatal error: > /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/lipo: > obj/components/cronet/ios/libcronet_static.a and > obj/components/cronet/ios/libcronet_deps_complete.a have the same architectures > (i386) and can't be in the same fat output file Lipo is a command to combine libraries in mulitple architectures into a single fat binary. And it reports the error probably because it doesn't have a feature to combine multiple libraries in the same architecture, which is the role of "ld" command. If the goal here is to combine input_libs and deps_lib without hiding the symbols, you can probably use "ld" command in the similar way as above, but without "-r" parameter, and specify output_lib instead of output_obj as an output: 59 command = [ 60 'xcrun', 'ld', 61 '-arch', GN_CPU_TO_LD_ARCH[options.current_cpu], 63 ] 64 for input_lib in options.input_libs.split(','): 65 # By default, ld only pulls .o files out of a static library if needed to 66 # resolve some symbol reference. We apply -force_load option to input_lib 67 # (but not to deps_lib) to force pulling all .o files. 68 command += ['-force_load', input_lib] 69 command += [ 70 options.deps_lib, 71 '-o', options.output_lib 72 ]
https://codereview.chromium.org/2807283002/diff/200001/components/cronet/tool... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/200001/components/cronet/tool... components/cronet/tools/hide_symbols.py:1: #!/usr/bin/env python It would be great if you put this script at more general place like //tools because //ios/web_view will also use this. I can move it later, though.
On 2017/05/22 23:46:39, Hiroshi Ichikawa wrote: > https://codereview.chromium.org/2807283002/diff/200001/components/cronet/tool... > File components/cronet/tools/hide_symbols.py (right): > > https://codereview.chromium.org/2807283002/diff/200001/components/cronet/tool... > components/cronet/tools/hide_symbols.py:1: #!/usr/bin/env python > It would be great if you put this script at more general place like //tools > because //ios/web_view will also use this. I can move it later, though. Hey Misha, I patched your diff and tried to build "chrome" itself, but ran into a couple errors like: ERROR at //components/cronet/ios/cronet_consumer/cronet_consumer_view_controller.m:7:10: Can't include this header from here. #import "components/cronet/ios/Cronet.h" ^----------------------------- The target: //components/cronet/ios/cronet_consumer:cronet_consumer_static_arch_executable_sources is including a file from the target: //components/cronet/ios:cronet_static It's usually best to depend directly on the destination target. In some cases, the destination target is considered a subcomponent of an intermediate target. In this case, the intermediate target should depend publicly on the destination to forward the ability to include headers. Dependency chain (there may also be others): //components/cronet/ios/cronet_consumer:cronet_consumer_static_arch_executable_sources --> //components/cronet/ios:cronet_static_complete --[private]--> //components/cronet/ios:cronet_static Am I doing something wrong?
On 2017/05/23 07:05:19, jzw1 wrote: > On 2017/05/22 23:46:39, Hiroshi Ichikawa wrote: > > > https://codereview.chromium.org/2807283002/diff/200001/components/cronet/tool... > > File components/cronet/tools/hide_symbols.py (right): > > > > > https://codereview.chromium.org/2807283002/diff/200001/components/cronet/tool... > > components/cronet/tools/hide_symbols.py:1: #!/usr/bin/env python > > It would be great if you put this script at more general place like //tools > > because //ios/web_view will also use this. I can move it later, though. > > Hey Misha, > > I patched your diff and tried to build "chrome" itself, but ran into a couple > errors like: > > ERROR at > //components/cronet/ios/cronet_consumer/cronet_consumer_view_controller.m:7:10: > Can't include this header from here. > #import "components/cronet/ios/Cronet.h" > ^----------------------------- > The target: > > //components/cronet/ios/cronet_consumer:cronet_consumer_static_arch_executable_sources > is including a file from the target: > //components/cronet/ios:cronet_static > It's usually best to depend directly on the destination target. > In some cases, the destination target is considered a subcomponent > of an intermediate target. In this case, the intermediate target > should depend publicly on the destination to forward the ability > to include headers. > Dependency chain (there may also be others): > > //components/cronet/ios/cronet_consumer:cronet_consumer_static_arch_executable_sources > --> > //components/cronet/ios:cronet_static_complete --[private]--> > //components/cronet/ios:cronet_static > > Am I doing something wrong? No, it was actual problem with includes. Try now.
Thanks for your comments! I think I've addressed them all, so PTAL. https://codereview.chromium.org/2807283002/diff/180001/components/cronet/tool... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/180001/components/cronet/tool... components/cronet/tools/hide_symbols.py:83: '-arch', 'i386', options.input_libs, On 2017/05/22 23:42:36, Hiroshi Ichikawa wrote: > On 2017/05/22 20:53:53, mef wrote: > > On 2017/05/22 17:15:57, kapishnikov wrote: > > > Should "options.deps_lib" be also included? > > > > Maybe. I'm getting cryptic error though: > > fatal error: > > > /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/lipo: > > obj/components/cronet/ios/libcronet_static.a and > > obj/components/cronet/ios/libcronet_deps_complete.a have the same > architectures > > (i386) and can't be in the same fat output file > > Lipo is a command to combine libraries in mulitple architectures into a single > fat binary. And it reports the error probably because it doesn't have a feature > to combine multiple libraries in the same architecture, which is the role of > "ld" command. > > If the goal here is to combine input_libs and deps_lib without hiding the > symbols, you can probably use "ld" command in the similar way as above, but > without "-r" parameter, and specify output_lib instead of output_obj as an > output: > > 59 command = [ > 60 'xcrun', 'ld', > 61 '-arch', GN_CPU_TO_LD_ARCH[options.current_cpu], > 63 ] > 64 for input_lib in options.input_libs.split(','): > 65 # By default, ld only pulls .o files out of a static library if needed > to > 66 # resolve some symbol reference. We apply -force_load option to > input_lib > 67 # (but not to deps_lib) to force pulling all .o files. > 68 command += ['-force_load', input_lib] > 69 command += [ > 70 options.deps_lib, > 71 '-o', options.output_lib > 72 ] Great point! I've used libtool to merge libcronet_static.a and libcronet_deps_complete.a and it appears to produce the right output. The size of combined library is ~780MB, which could cause problems further down the line, so we may want to strip some of the symbols. https://codereview.chromium.org/2807283002/diff/200001/components/cronet/tool... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/200001/components/cronet/tool... components/cronet/tools/hide_symbols.py:1: #!/usr/bin/env python On 2017/05/22 23:46:39, Hiroshi Ichikawa wrote: > It would be great if you put this script at more general place like //tools > because //ios/web_view will also use this. I can move it later, though. I agree that it should move to more general location, maybe alongside with |ios_static_framework| template. I'd prefer later, so I could land this CL this week, and make it more generic afterwards.
https://codereview.chromium.org/2807283002/diff/220001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/220001/components/cronet/ios/... components/cronet/ios/BUILD.gn:234: "{{bundle_root_dir}}/Cronet", Can we somehow parameterize the binary name? Target :cronet_static_framework creates CronetStatic.framework which will contain binary with "Cronet" file name. This won't work since the name of the framework and the binary should be the same. We rename it in package_ios.py but it would be great if the valid framework can be produced by gn. https://codereview.chromium.org/2807283002/diff/220001/components/cronet/ios/... components/cronet/ios/BUILD.gn:252: # get_label_info("$_target_name($default_toolchain)", "root_out_dir") Should remove it before submitting if it is not needed. https://codereview.chromium.org/2807283002/diff/220001/components/cronet/ios/... components/cronet/ios/BUILD.gn:281: # bundle_root_dir = "$root_out_dir/CronetStatic.framework" Remove it if it is not needed. https://codereview.chromium.org/2807283002/diff/220001/components/cronet/ios/... File components/cronet/ios/cronet_consumer/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/220001/components/cronet/ios/... components/cronet/ios/cronet_consumer/BUILD.gn:48: "//components/cronet/ios:cronet_framework+link", Why do we need "cronet_framework+link" here? https://codereview.chromium.org/2807283002/diff/240001/components/cronet/ios/... File components/cronet/ios/cronet_consumer/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/240001/components/cronet/ios/... components/cronet/ios/cronet_consumer/BUILD.gn:48: # TODO(mef): This dep is used to make #import <Cronet/Cronet.h> work. I think we should fix it. Linking two frameworks doesn't seem right. https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... File components/cronet/tools/cr_cronet.py (right): https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... components/cronet/tools/cr_cronet.py:90: gn_args += ' target_cpu="x86" ' Is "x64" a better default choice? https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... components/cronet/tools/hide_symbols.py:93: 'xcrun', 'ar', '-r', Should this command be run if it is 'x86'? https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... File components/cronet/tools/package_ios.py (right): https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... components/cronet/tools/package_ios.py:120: os.path.join(out_dir, target_dir, 'Cronet.framework')) Since we added 'Static' subdirectory for the static framework, should we add "Embedded" or "Dynamic" for the dynamic framework?
https://codereview.chromium.org/2807283002/diff/180001/components/cronet/tool... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/180001/components/cronet/tool... components/cronet/tools/hide_symbols.py:83: '-arch', 'i386', options.input_libs, On 2017/05/23 20:56:36, mef wrote: > On 2017/05/22 23:42:36, Hiroshi Ichikawa wrote: > > On 2017/05/22 20:53:53, mef wrote: > > > On 2017/05/22 17:15:57, kapishnikov wrote: > > > > Should "options.deps_lib" be also included? > > > > > > Maybe. I'm getting cryptic error though: > > > fatal error: > > > > > > /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/lipo: > > > obj/components/cronet/ios/libcronet_static.a and > > > obj/components/cronet/ios/libcronet_deps_complete.a have the same > > architectures > > > (i386) and can't be in the same fat output file > > > > Lipo is a command to combine libraries in mulitple architectures into a single > > fat binary. And it reports the error probably because it doesn't have a > feature > > to combine multiple libraries in the same architecture, which is the role of > > "ld" command. > > > > If the goal here is to combine input_libs and deps_lib without hiding the > > symbols, you can probably use "ld" command in the similar way as above, but > > without "-r" parameter, and specify output_lib instead of output_obj as an > > output: > > > > 59 command = [ > > 60 'xcrun', 'ld', > > 61 '-arch', GN_CPU_TO_LD_ARCH[options.current_cpu], > > 63 ] > > 64 for input_lib in options.input_libs.split(','): > > 65 # By default, ld only pulls .o files out of a static library if > needed > > to > > 66 # resolve some symbol reference. We apply -force_load option to > > input_lib > > 67 # (but not to deps_lib) to force pulling all .o files. > > 68 command += ['-force_load', input_lib] > > 69 command += [ > > 70 options.deps_lib, > > 71 '-o', options.output_lib > > 72 ] > > Great point! I've used libtool to merge libcronet_static.a and > libcronet_deps_complete.a and it appears to produce the right output. > The size of combined library is ~780MB, which could cause problems further down > the line, so we may want to strip some of the symbols. > In the ld command above, it automatically strips unused object files in deps_lib (but not in input_libs), by applying -force_load option to input_libs but not to deps_lib. Not really sure what happens with your command line using libtools, but maybe it doesn't strip any object files? Anyway I'm fine leaving it as is for now because it may not matter. But you may want to leave TODO for it. https://codereview.chromium.org/2807283002/diff/200001/components/cronet/tool... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/200001/components/cronet/tool... components/cronet/tools/hide_symbols.py:1: #!/usr/bin/env python On 2017/05/23 20:56:36, mef wrote: > On 2017/05/22 23:46:39, Hiroshi Ichikawa wrote: > > It would be great if you put this script at more general place like //tools > > because //ios/web_view will also use this. I can move it later, though. > > I agree that it should move to more general location, maybe alongside with > |ios_static_framework| template. > I'd prefer later, so I could land this CL this week, and make it more generic > afterwards. Sounds good, thanks! https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... components/cronet/tools/hide_symbols.py:93: 'xcrun', 'ar', '-r', On 2017/05/23 21:37:46, kapishnikov wrote: > Should this command be run if it is 'x86'? It should work. ar works independent of the architecture AFAIK.
jzw@chromium.org changed reviewers: + jzw@chromium.org
https://codereview.chromium.org/2807283002/diff/240001/components/cronet/ios/... File components/cronet/ios/cronet_consumer/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/240001/components/cronet/ios/... components/cronet/ios/cronet_consumer/BUILD.gn:48: # TODO(mef): This dep is used to make #import <Cronet/Cronet.h> work. On 2017/05/23 21:37:46, kapishnikov wrote: > I think we should fix it. Linking two frameworks doesn't seem right. Might be related, but when I build this on device I get: dyld: Library not loaded: @rpath/Cronet.framework/Cronet Referenced from: /var/containers/Bundle/Application/0DED222D-1A82-48A0-A205-FBD6E460F6BF/cronet_consumer_static.app/cronet_consumer_static Reason: image not found
Thanks for your comments! PTAL. https://codereview.chromium.org/2807283002/diff/220001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/220001/components/cronet/ios/... components/cronet/ios/BUILD.gn:234: "{{bundle_root_dir}}/Cronet", On 2017/05/23 21:37:46, kapishnikov wrote: > Can we somehow parameterize the binary name? Target :cronet_static_framework > creates CronetStatic.framework which will contain binary with "Cronet" file > name. This won't work since the name of the framework and the binary should be > the same. We rename it in package_ios.py but it would be great if the valid > framework can be produced by gn. Done. https://codereview.chromium.org/2807283002/diff/220001/components/cronet/ios/... components/cronet/ios/BUILD.gn:252: # get_label_info("$_target_name($default_toolchain)", "root_out_dir") On 2017/05/23 21:37:46, kapishnikov wrote: > Should remove it before submitting if it is not needed. Done. https://codereview.chromium.org/2807283002/diff/220001/components/cronet/ios/... components/cronet/ios/BUILD.gn:281: # bundle_root_dir = "$root_out_dir/CronetStatic.framework" On 2017/05/23 21:37:46, kapishnikov wrote: > Remove it if it is not needed. Done. https://codereview.chromium.org/2807283002/diff/240001/components/cronet/ios/... File components/cronet/ios/cronet_consumer/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/240001/components/cronet/ios/... components/cronet/ios/cronet_consumer/BUILD.gn:48: # TODO(mef): This dep is used to make #import <Cronet/Cronet.h> work. On 2017/05/23 21:37:46, kapishnikov wrote: > I think we should fix it. Linking two frameworks doesn't seem right. I agree that it is not great, and will be happy to fix it if there are good ideas how. I *think* it needs some public config magic combined with header maps. I'm open for suggestions. https://codereview.chromium.org/2807283002/diff/240001/components/cronet/ios/... components/cronet/ios/cronet_consumer/BUILD.gn:48: # TODO(mef): This dep is used to make #import <Cronet/Cronet.h> work. On 2017/05/24 06:15:26, jzw1 wrote: > On 2017/05/23 21:37:46, kapishnikov wrote: > > I think we should fix it. Linking two frameworks doesn't seem right. > Might be related, but when I build this on device I get: > dyld: Library not loaded: @rpath/Cronet.framework/Cronet > Referenced from: > /var/containers/Bundle/Application/0DED222D-1A82-48A0-A205-FBD6E460F6BF/cronet_consumer_static.app/cronet_consumer_static > Reason: image not found Yes, I think it is a result of linking cronet_framework but not including it into the bundle. I still don't know how to make it work though. https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... File components/cronet/tools/cr_cronet.py (right): https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... components/cronet/tools/cr_cronet.py:90: gn_args += ' target_cpu="x86" ' On 2017/05/23 21:37:46, kapishnikov wrote: > Is "x64" a better default choice? Done. Spurious edit from my experiments. https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... components/cronet/tools/hide_symbols.py:93: 'xcrun', 'ar', '-r', On 2017/05/23 21:37:46, kapishnikov wrote: > Should this command be run if it is 'x86'? No, the libtool above creates the |output_lib| and returns out of main. https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... File components/cronet/tools/package_ios.py (right): https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... components/cronet/tools/package_ios.py:120: os.path.join(out_dir, target_dir, 'Cronet.framework')) On 2017/05/23 21:37:46, kapishnikov wrote: > Since we added 'Static' subdirectory for the static framework, should we add > "Embedded" or "Dynamic" for the dynamic framework? Done.
https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... File components/cronet/tools/cr_cronet.py (right): https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... components/cronet/tools/cr_cronet.py:90: gn_args += ' target_cpu="x86" ' On 2017/05/24 21:36:59, mef wrote: > On 2017/05/23 21:37:46, kapishnikov wrote: > > Is "x64" a better default choice? > > Done. Spurious edit from my experiments. Looks like this is still not fixed with Patch Set 14? https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... components/cronet/tools/hide_symbols.py:93: 'xcrun', 'ar', '-r', On 2017/05/24 21:36:59, mef wrote: > On 2017/05/23 21:37:46, kapishnikov wrote: > > Should this command be run if it is 'x86'? > > No, the libtool above creates the |output_lib| and returns out of main. Oh I see I now understand what the original comment mean :)
Thanks! https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... File components/cronet/tools/cr_cronet.py (right): https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... components/cronet/tools/cr_cronet.py:90: gn_args += ' target_cpu="x86" ' On 2017/05/25 02:28:32, Hiroshi Ichikawa wrote: > On 2017/05/24 21:36:59, mef wrote: > > On 2017/05/23 21:37:46, kapishnikov wrote: > > > Is "x64" a better default choice? > > > > Done. Spurious edit from my experiments. > > Looks like this is still not fixed with Patch Set 14? Done. Switching between two computers lost some changes. https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... File components/cronet/tools/hide_symbols.py (right): https://codereview.chromium.org/2807283002/diff/240001/components/cronet/tool... components/cronet/tools/hide_symbols.py:93: 'xcrun', 'ar', '-r', On 2017/05/25 02:28:32, Hiroshi Ichikawa wrote: > On 2017/05/24 21:36:59, mef wrote: > > On 2017/05/23 21:37:46, kapishnikov wrote: > > > Should this command be run if it is 'x86'? > > > > No, the libtool above creates the |output_lib| and returns out of main. > > Oh I see I now understand what the original comment mean :) I wonder whether this should be extracted into separate function to make it clearer.
PTAL.
lgtm
lgtm
The CQ bit was checked by mef@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kapishnikov@chromium.org, ichikawa@chromium.org Link to the patchset: https://codereview.chromium.org/2807283002/#ps340001 (title: "Sync and format.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by mef@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by mef@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by mef@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by mef@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kapishnikov@chromium.org, ichikawa@chromium.org Link to the patchset: https://codereview.chromium.org/2807283002/#ps380001 (title: "Only build cronet_consumer_static on Cronet builders (is_cronet_build).")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 380001, "attempt_start_ts": 1495834011859530,
"parent_rev": "95c9cb5e460bdf1a02b7e7f886e982b1df33065e", "commit_rev":
"3412c3c60b69798430b3a3cc9d2834f5d44e6042"}
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Build static libcronet.a for iOS with complete dependencies. - Hide dependencies inside of the library to avoid symbol conflicts. - Add cronet_consumer_static target that builds consumer app with static library. BUG=721922 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [Cronet] Build static libcronet.a for iOS with complete dependencies. - Hide dependencies inside of the library to avoid symbol conflicts. - Add cronet_consumer_static target that builds consumer app with static library. BUG=721922 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2807283002 Cr-Commit-Position: refs/heads/master@{#475158} Committed: https://chromium.googlesource.com/chromium/src/+/3412c3c60b69798430b3a3cc9d28... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001) as https://chromium.googlesource.com/chromium/src/+/3412c3c60b69798430b3a3cc9d28...
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:380001) has been created in https://codereview.chromium.org/2907023002/ by mef@chromium.org. The reason for reverting is: Broke official Cronet builders..
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Build static libcronet.a for iOS with complete dependencies. - Hide dependencies inside of the library to avoid symbol conflicts. - Add cronet_consumer_static target that builds consumer app with static library. BUG=721922 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2807283002 Cr-Commit-Position: refs/heads/master@{#475158} Committed: https://chromium.googlesource.com/chromium/src/+/3412c3c60b69798430b3a3cc9d28... ========== to ========== [Cronet] Build static libcronet.a for iOS with complete dependencies. - Hide dependencies inside of the library to avoid symbol conflicts. - Add cronet_consumer_static target that builds consumer app with static library. BUG=721922 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2807283002 Cr-Commit-Position: refs/heads/master@{#475158} Committed: https://chromium.googlesource.com/chromium/src/+/3412c3c60b69798430b3a3cc9d28... ==========
The CQ bit was checked by mef@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mef@chromium.org changed reviewers: + sdefresne@chromium.org
IIUIC the problem is that in builds with additional_target_cpus the
cronet_consumer_static target is built for 2 different CPUs, but both reference
the same cronet_static_framework target, which depends on
lipo_binary("ibcronet") to build fat static framework.
It looks like other iOS-specific templates like ios_app_bundle have some special
handling for different toolchains, but I don't quite understand how to properly
simulate it.
Disabling cronet_consumer_static target in case of additional_target_cpus makes
bots happy, but I'm sure there is a better way.
Robert & Sylvain, do you have any suggestions?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/01 21:44:25, mef wrote:
> IIUIC the problem is that in builds with additional_target_cpus the
> cronet_consumer_static target is built for 2 different CPUs, but both
reference
> the same cronet_static_framework target, which depends on
> lipo_binary("ibcronet") to build fat static framework.
I guess they should instead depends on the architecture dependent static
library. That is do not depend on the fat static library, but just on the
individual static libraries. You can find their name by looking at how the
target is defined (looking at the code or at the template code) or by using "gn
desc out/foo //path/to/target". You'll probably find a target of type
static_library that has name foo and foo(toolchain). You'll then just add a
dependency on that target without the toolchain.
> It looks like other iOS-specific templates like ios_app_bundle have some
special
> handling for different toolchains, but I don't quite understand how to
properly
> simulate it.
When you build with multiple target cpu, each target is defined multiple time
for each toolchain. When a dependency on target foo is defined, unless a
specific toolchain is specified (the name of the toolchain is in parenthesis
behind the target name), then the dependency is for the foo target in the same
toolchain.
> Disabling cronet_consumer_static target in case of additional_target_cpus
makes
> bots happy, but I'm sure there is a better way.
>
> Robert & Sylvain, do you have any suggestions?
On 2017/06/02 08:21:34, sdefresne wrote:
> On 2017/06/01 21:44:25, mef wrote:
> > IIUIC the problem is that in builds with additional_target_cpus the
> > cronet_consumer_static target is built for 2 different CPUs, but both
> reference
> > the same cronet_static_framework target, which depends on
> > lipo_binary("ibcronet") to build fat static framework.
>
> I guess they should instead depends on the architecture dependent static
> library. That is do not depend on the fat static library, but just on the
> individual static libraries. You can find their name by looking at how the
> target is defined (looking at the code or at the template code) or by using
"gn
> desc out/foo //path/to/target". You'll probably find a target of type
> static_library that has name foo and foo(toolchain). You'll then just add a
> dependency on that target without the toolchain.
>
> > It looks like other iOS-specific templates like ios_app_bundle have some
> special
> > handling for different toolchains, but I don't quite understand how to
> properly
> > simulate it.
>
> When you build with multiple target cpu, each target is defined multiple time
> for each toolchain. When a dependency on target foo is defined, unless a
> specific toolchain is specified (the name of the toolchain is in parenthesis
> behind the target name), then the dependency is for the foo target in the same
> toolchain.
>
> > Disabling cronet_consumer_static target in case of additional_target_cpus
> makes
> > bots happy, but I'm sure there is a better way.
> >
> > Robert & Sylvain, do you have any suggestions?
So, I applied the patch locally, build components/cronet/ios:cronet_static_copy
and got a static framework:
$ ninja -C out/Debug-iphonesimulator components/cronet/ios:cronet_static_copy
$ find out/Debug-iphonesimulator/Static -type f
out/Debug-iphonesimulator/Static/Cronet.framework/Cronet
out/Debug-iphonesimulator/Static/Cronet.framework/Headers/bidirectional_stream_c.h
out/Debug-iphonesimulator/Static/Cronet.framework/Headers/Cronet.h
out/Debug-iphonesimulator/Static/Cronet.framework/Headers/cronet_c_for_grpc.h
out/Debug-iphonesimulator/Static/Cronet.framework/icudtl.dat
$ file out/Debug-iphonesimulator/Static/Cronet.framework/Cronet
out/Debug-iphonesimulator/Static/Cronet.framework/Cronet: Mach-O universal
binary with 2 architectures: [i386: current ar archive] [x86_64]
out/Debug-iphonesimulator/Static/Cronet.framework/Cronet (for architecture
i386): current ar archive
out/Debug-iphonesimulator/Static/Cronet.framework/Cronet (for architecture
x86_64): current ar archive random library
So it looks like this build a fat static library without problem (there are some
warnings due to
https://groups.google.com/a/chromium.org/d/msg/gn-dev/bbOBxxheLgc/NTkliYHAFAAJ)
but apparently they can be ignored. Looks like everything works as expected and
you just need to fix the official builder.
On 2017/06/02 13:53:38, sdefresne wrote:
> On 2017/06/02 08:21:34, sdefresne wrote:
> > On 2017/06/01 21:44:25, mef wrote:
> > > IIUIC the problem is that in builds with additional_target_cpus the
> > > cronet_consumer_static target is built for 2 different CPUs, but both
> > reference
> > > the same cronet_static_framework target, which depends on
> > > lipo_binary("ibcronet") to build fat static framework.
> >
> > I guess they should instead depends on the architecture dependent static
> > library. That is do not depend on the fat static library, but just on the
> > individual static libraries. You can find their name by looking at how the
> > target is defined (looking at the code or at the template code) or by using
> "gn
> > desc out/foo //path/to/target". You'll probably find a target of type
> > static_library that has name foo and foo(toolchain). You'll then just add a
> > dependency on that target without the toolchain.
> >
> > > It looks like other iOS-specific templates like ios_app_bundle have some
> > special
> > > handling for different toolchains, but I don't quite understand how to
> > properly
> > > simulate it.
> >
> > When you build with multiple target cpu, each target is defined multiple
time
> > for each toolchain. When a dependency on target foo is defined, unless a
> > specific toolchain is specified (the name of the toolchain is in parenthesis
> > behind the target name), then the dependency is for the foo target in the
same
> > toolchain.
> >
> > > Disabling cronet_consumer_static target in case of additional_target_cpus
> > makes
> > > bots happy, but I'm sure there is a better way.
> > >
> > > Robert & Sylvain, do you have any suggestions?
>
> So, I applied the patch locally, build
components/cronet/ios:cronet_static_copy
> and got a static framework:
>
> $ ninja -C out/Debug-iphonesimulator components/cronet/ios:cronet_static_copy
> $ find out/Debug-iphonesimulator/Static -type f
> out/Debug-iphonesimulator/Static/Cronet.framework/Cronet
>
out/Debug-iphonesimulator/Static/Cronet.framework/Headers/bidirectional_stream_c.h
> out/Debug-iphonesimulator/Static/Cronet.framework/Headers/Cronet.h
> out/Debug-iphonesimulator/Static/Cronet.framework/Headers/cronet_c_for_grpc.h
> out/Debug-iphonesimulator/Static/Cronet.framework/icudtl.dat
> $ file out/Debug-iphonesimulator/Static/Cronet.framework/Cronet
> out/Debug-iphonesimulator/Static/Cronet.framework/Cronet: Mach-O universal
> binary with 2 architectures: [i386: current ar archive] [x86_64]
> out/Debug-iphonesimulator/Static/Cronet.framework/Cronet (for architecture
> i386): current ar archive
> out/Debug-iphonesimulator/Static/Cronet.framework/Cronet (for architecture
> x86_64): current ar archive random library
>
> So it looks like this build a fat static library without problem (there are
some
> warnings due to
>
https://groups.google.com/a/chromium.org/d/msg/gn-dev/bbOBxxheLgc/NTkliYHAFAAJ)
> but apparently they can be ignored. Looks like everything works as expected
and
> you just need to fix the official builder.
BTW, this also works for building fat library on device:
$ file out/Debug-iphoneos/Static/Cronet.framework/Cronet
out/Debug-iphoneos/Static/Cronet.framework/Cronet: Mach-O universal binary with
2 architectures: [arm_v7: current ar archive random library] [arm64]
out/Debug-iphoneos/Static/Cronet.framework/Cronet (for architecture
armv7): current ar archive random library
out/Debug-iphoneos/Static/Cronet.framework/Cronet (for architecture
arm64): current ar archive random library
On 2017/06/02 08:21:34, sdefresne wrote:
> On 2017/06/01 21:44:25, mef wrote:
> > IIUIC the problem is that in builds with additional_target_cpus the
> > cronet_consumer_static target is built for 2 different CPUs, but both
> reference
> > the same cronet_static_framework target, which depends on
> > lipo_binary("ibcronet") to build fat static framework.
>
> I guess they should instead depends on the architecture dependent static
> library. That is do not depend on the fat static library, but just on the
> individual static libraries. You can find their name by looking at how the
> target is defined (looking at the code or at the template code) or by using
"gn
> desc out/foo //path/to/target". You'll probably find a target of type
> static_library that has name foo and foo(toolchain). You'll then just add a
> dependency on that target without the toolchain.
Right, we do have a separate target |cronet_static_complete| with thin library,
but it means that |cronet_consumer_static| doesn't use real static
Cronet.framework.
Looking at ios_framework_bundle template and the fact that |cronet_consumer|
does depend on fat dynamic framework, we hoped that we could get similar result
for static framework.
This seems to require some magic with conditional current_toolchain !=
default_toolchain processing, and that's where your GN expertise is the key. :)
>
> > It looks like other iOS-specific templates like ios_app_bundle have some
> special
> > handling for different toolchains, but I don't quite understand how to
> properly
> > simulate it.
>
> When you build with multiple target cpu, each target is defined multiple time
> for each toolchain. When a dependency on target foo is defined, unless a
> specific toolchain is specified (the name of the toolchain is in parenthesis
> behind the target name), then the dependency is for the foo target in the same
> toolchain.
Is it possible to have |cronet_static_framework| target build FAT framework when
called for default toolchain, and just depend on that result for all other
toolchains?
>
> > Disabling cronet_consumer_static target in case of additional_target_cpus
> makes
> > bots happy, but I'm sure there is a better way.
> >
> > Robert & Sylvain, do you have any suggestions?
On 2017/06/02 13:53:38, sdefresne wrote:
> On 2017/06/02 08:21:34, sdefresne wrote:
> > On 2017/06/01 21:44:25, mef wrote:
> > > IIUIC the problem is that in builds with additional_target_cpus the
> > > cronet_consumer_static target is built for 2 different CPUs, but both
> > reference
> > > the same cronet_static_framework target, which depends on
> > > lipo_binary("ibcronet") to build fat static framework.
> >
> > I guess they should instead depends on the architecture dependent static
> > library. That is do not depend on the fat static library, but just on the
> > individual static libraries. You can find their name by looking at how the
> > target is defined (looking at the code or at the template code) or by using
> "gn
> > desc out/foo //path/to/target". You'll probably find a target of type
> > static_library that has name foo and foo(toolchain). You'll then just add a
> > dependency on that target without the toolchain.
> >
> > > It looks like other iOS-specific templates like ios_app_bundle have some
> > special
> > > handling for different toolchains, but I don't quite understand how to
> > properly
> > > simulate it.
> >
> > When you build with multiple target cpu, each target is defined multiple
time
> > for each toolchain. When a dependency on target foo is defined, unless a
> > specific toolchain is specified (the name of the toolchain is in parenthesis
> > behind the target name), then the dependency is for the foo target in the
same
> > toolchain.
> >
> > > Disabling cronet_consumer_static target in case of additional_target_cpus
> > makes
> > > bots happy, but I'm sure there is a better way.
> > >
> > > Robert & Sylvain, do you have any suggestions?
>
> So, I applied the patch locally, build
components/cronet/ios:cronet_static_copy
> and got a static framework:
>
> $ ninja -C out/Debug-iphonesimulator components/cronet/ios:cronet_static_copy
> $ find out/Debug-iphonesimulator/Static -type f
> out/Debug-iphonesimulator/Static/Cronet.framework/Cronet
>
out/Debug-iphonesimulator/Static/Cronet.framework/Headers/bidirectional_stream_c.h
> out/Debug-iphonesimulator/Static/Cronet.framework/Headers/Cronet.h
> out/Debug-iphonesimulator/Static/Cronet.framework/Headers/cronet_c_for_grpc.h
> out/Debug-iphonesimulator/Static/Cronet.framework/icudtl.dat
> $ file out/Debug-iphonesimulator/Static/Cronet.framework/Cronet
> out/Debug-iphonesimulator/Static/Cronet.framework/Cronet: Mach-O universal
> binary with 2 architectures: [i386: current ar archive] [x86_64]
> out/Debug-iphonesimulator/Static/Cronet.framework/Cronet (for architecture
> i386): current ar archive
> out/Debug-iphonesimulator/Static/Cronet.framework/Cronet (for architecture
> x86_64): current ar archive random library
>
> So it looks like this build a fat static library without problem (there are
some
> warnings due to
>
https://groups.google.com/a/chromium.org/d/msg/gn-dev/bbOBxxheLgc/NTkliYHAFAAJ)
> but apparently they can be ignored. Looks like everything works as expected
and
> you just need to fix the official builder.
Yes, current CL passes trybots, and should work on Cronet builders, so I'll be
happy to land it, and create a separate CL for further improvements.
On 2017/06/02 13:57:35, sdefresne wrote:
> On 2017/06/02 13:53:38, sdefresne wrote:
> > On 2017/06/02 08:21:34, sdefresne wrote:
> > > On 2017/06/01 21:44:25, mef wrote:
> > > > IIUIC the problem is that in builds with additional_target_cpus the
> > > > cronet_consumer_static target is built for 2 different CPUs, but both
> > > reference
> > > > the same cronet_static_framework target, which depends on
> > > > lipo_binary("ibcronet") to build fat static framework.
> > >
> > > I guess they should instead depends on the architecture dependent static
> > > library. That is do not depend on the fat static library, but just on the
> > > individual static libraries. You can find their name by looking at how the
> > > target is defined (looking at the code or at the template code) or by
using
> > "gn
> > > desc out/foo //path/to/target". You'll probably find a target of type
> > > static_library that has name foo and foo(toolchain). You'll then just add
a
> > > dependency on that target without the toolchain.
> > >
> > > > It looks like other iOS-specific templates like ios_app_bundle have some
> > > special
> > > > handling for different toolchains, but I don't quite understand how to
> > > properly
> > > > simulate it.
> > >
> > > When you build with multiple target cpu, each target is defined multiple
> time
> > > for each toolchain. When a dependency on target foo is defined, unless a
> > > specific toolchain is specified (the name of the toolchain is in
parenthesis
> > > behind the target name), then the dependency is for the foo target in the
> same
> > > toolchain.
> > >
> > > > Disabling cronet_consumer_static target in case of
additional_target_cpus
> > > makes
> > > > bots happy, but I'm sure there is a better way.
> > > >
> > > > Robert & Sylvain, do you have any suggestions?
> >
> > So, I applied the patch locally, build
> components/cronet/ios:cronet_static_copy
> > and got a static framework:
> >
> > $ ninja -C out/Debug-iphonesimulator
components/cronet/ios:cronet_static_copy
> > $ find out/Debug-iphonesimulator/Static -type f
> > out/Debug-iphonesimulator/Static/Cronet.framework/Cronet
> >
>
out/Debug-iphonesimulator/Static/Cronet.framework/Headers/bidirectional_stream_c.h
> > out/Debug-iphonesimulator/Static/Cronet.framework/Headers/Cronet.h
> >
out/Debug-iphonesimulator/Static/Cronet.framework/Headers/cronet_c_for_grpc.h
> > out/Debug-iphonesimulator/Static/Cronet.framework/icudtl.dat
> > $ file out/Debug-iphonesimulator/Static/Cronet.framework/Cronet
> > out/Debug-iphonesimulator/Static/Cronet.framework/Cronet: Mach-O universal
> > binary with 2 architectures: [i386: current ar archive] [x86_64]
> > out/Debug-iphonesimulator/Static/Cronet.framework/Cronet (for architecture
> > i386): current ar archive
> > out/Debug-iphonesimulator/Static/Cronet.framework/Cronet (for architecture
> > x86_64): current ar archive random library
> >
> > So it looks like this build a fat static library without problem (there are
> some
> > warnings due to
> >
>
https://groups.google.com/a/chromium.org/d/msg/gn-dev/bbOBxxheLgc/NTkliYHAFAAJ)
> > but apparently they can be ignored. Looks like everything works as expected
> and
> > you just need to fix the official builder.
>
> BTW, this also works for building fat library on device:
> $ file out/Debug-iphoneos/Static/Cronet.framework/Cronet
> out/Debug-iphoneos/Static/Cronet.framework/Cronet: Mach-O universal binary
with
> 2 architectures: [arm_v7: current ar archive random library] [arm64]
> out/Debug-iphoneos/Static/Cronet.framework/Cronet (for architecture
> armv7): current ar archive random library
> out/Debug-iphoneos/Static/Cronet.framework/Cronet (for architecture
> arm64): current ar archive random library
Yes, it properly builds fat dynamic and static frameworks, but it doesn't build
fat cronet_consumer_static, which is nice to have, but not super critical.
Hi Sylvain and Robert, these are 3 main open questions where we could use your gn advice. thanks, -m https://codereview.chromium.org/2807283002/diff/400001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/400001/components/cronet/ios/... components/cronet/ios/BUILD.gn:198: rebase_path("$target_out_dir/libcronet_deps_complete.a", root_build_dir), The x86 debug size of libcronet_deps_complete.a is over 700mb and it causes problems with ld, so we use ar to build cronet_static_complete.a without hiding symbols for this configuration. This causes problems later on, when static Cronet.framework becomes too big to get into internal source repository. Is there a good way to strip some of the debug info prior to ld? https://codereview.chromium.org/2807283002/diff/400001/components/cronet/ios/... components/cronet/ios/BUILD.gn:269: script = "//components/cronet/tools/dummy.py" This dummy action is used to put output of |cronet_static_framework| target into "$root_out_dir/Static/Cronet.framework" instead of $root_out_dir/CronetStatic.framework". Without it GN has complained that 'Cronet.framework' is being generated by 2 targets - cronet_framework and cronet_static_framework. Is there a better solution? https://codereview.chromium.org/2807283002/diff/400001/components/cronet/ios/... components/cronet/ios/BUILD.gn:284: static_library_target = ":libcronet" Depending on lipo_binary target to produce fat static library results in errors with additional_target_cpus where lipo tries to merge 2 libraries with the same architecture. We need to do something similar to ios_framework_bundle to properly handle additional cpus. https://codereview.chromium.org/2807283002/diff/400001/components/cronet/ios/... File components/cronet/ios/cronet_consumer/BUILD.gn (right): https://codereview.chromium.org/2807283002/diff/400001/components/cronet/ios/... components/cronet/ios/cronet_consumer/BUILD.gn:55: cflags = [ What's a better way to add $root_out_dir/Static to framework search path, so it would find 'Cronet.framework' ?
The CQ bit was checked by mef@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kapishnikov@chromium.org, ichikawa@chromium.org Link to the patchset: https://codereview.chromium.org/2807283002/#ps400001 (title: "Disable cronet_consumer_static target for fat builds.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 mef@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 400001, "attempt_start_ts": 1496670906079690,
"parent_rev": "6444a99819d7fe77ee5c8f895e1e47b95698c716", "commit_rev":
"f035c8c0053e7bcec27f968bf7fbbfc062369e0f"}
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Build static libcronet.a for iOS with complete dependencies. - Hide dependencies inside of the library to avoid symbol conflicts. - Add cronet_consumer_static target that builds consumer app with static library. BUG=721922 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2807283002 Cr-Commit-Position: refs/heads/master@{#475158} Committed: https://chromium.googlesource.com/chromium/src/+/3412c3c60b69798430b3a3cc9d28... ========== to ========== [Cronet] Build static libcronet.a for iOS with complete dependencies. - Hide dependencies inside of the library to avoid symbol conflicts. - Add cronet_consumer_static target that builds consumer app with static library. BUG=721922 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2807283002 Cr-Original-Commit-Position: refs/heads/master@{#475158} Committed: https://chromium.googlesource.com/chromium/src/+/3412c3c60b69798430b3a3cc9d28... Review-Url: https://codereview.chromium.org/2807283002 Cr-Commit-Position: refs/heads/master@{#476986} Committed: https://chromium.googlesource.com/chromium/src/+/f035c8c0053e7bcec27f968bf7fb... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/f035c8c0053e7bcec27f968bf7fb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
