Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2941683002:
Add script to generate CronetChromeWebView.framework (Closed)
DescriptionAdd script to generate CronetChromeWebView.framework
By default, the script will build CronetChromeWebView.framework and copy the
results to ./out/CronetChromeWebView/. use_goma=true is also passed to gn unless
specified otherwise with the --no-goma option.
BUG=none
Review-Url: https://codereview.chromium.org/2941683002
Cr-Commit-Position: refs/heads/master@{#481957}
Committed: https://chromium.googlesource.com/chromium/src/+/f06b5d1964838e398b603c262435ead255c893d0
Patch Set 1 #
Total comments: 25
Patch Set 2 : Update build script. #Patch Set 3 : Add dep CL. #
Total comments: 12
Patch Set 4 : Respond to comments. #
Total comments: 2
Patch Set 5 : Rebase and update gn args. #Messages
Total messages: 32 (13 generated)
michaeldo@chromium.org changed reviewers: + eugenebut@chromium.org, ichikawa@chromium.org
There are a few formatting issues with the python script, but please feel free to take a look. It successfully produced output Frameworks for me. Note it builds all 4 combinations as cronet does, but I'm not sure we need all of them to be built.
Description was changed from ========== Add script to generate CronetChromeWebView.framework BUG= ========== to ========== Add script to generate CronetChromeWebView.framework Relative imports have to be used because we build multiple frameworks, with different names and the package_ios.py script forces a clean build. This exposes an existing issue with the framework style includes. The alternative would be to #define on which framework was being build and use different imports. (For example: <ChromeWebView/...> vs <CronetChromeWebView/..>) Either way, I'll pull that change out of this CL before it is landed. BUG= ==========
https://codereview.chromium.org/2941683002/diff/1/ios/web_view/public/ChromeW... File ios/web_view/public/ChromeWebView.h (left): https://codereview.chromium.org/2941683002/diff/1/ios/web_view/public/ChromeW... ios/web_view/public/ChromeWebView.h:8: #import <ChromeWebView/cwv_export.h> Just to clarify. This change and other import changes will not be a part of CL when you land it. Right? https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... File ios/web_view/tools/package_ios.py (right): https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:15: def run(command, extra_options=''): Please add comments for every non-trivial function https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:26: for (build_config, gn_extra_args) in [('Debug', 'is_debug=true use_xcode_clang=true'), This for loop has only 2 iterations and will unlikely have more. Would it be better to extract the code inside for-loop to a separate function and call that function twice? https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:28: for (target_device, target_cpu, additional_cpu) in [('os', 'arm', 'arm64'), ditto https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:39: #is_cronet_build=true? Remove this? https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:46: build_result = run('ninja -C %s ios/web_view:cronet_ios_web_view_package' % build_dir, Does it fit 80 symbols?
Oh and also, Sylvain may be a good person to review GN :)
https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... File ios/web_view/tools/package_ios.py (right): https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:7: package_ios.py - Build and Package Release and Debug fat libraries for iOS. Maybe it's better to say that this script is for CronetChromeWebView.framework? The current description sounds like for more general purpose, but it actually isn't. https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:26: for (build_config, gn_extra_args) in [('Debug', 'is_debug=true use_xcode_clang=true'), Why do you specify use_xcode_clang=true only for debug build? https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:27: ('Release', 'is_debug=false enable_stripping=true is_official_build=true')]: is_official_build should be false even for release. Confirmed with eugenebut@. Because this library is not a part of official Chrome release. https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:32: gn_args = 'target_os="ios" enable_websockets=false ' \ How about providing an option to use Goma? Otherwise it takes too long... https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:35: 'disable_brotli_filter=true enable_dsyms=true ' \ Just curious, do you know if enable_dsyms affects binary size? I have set this to false for release following your doc... https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:39: #is_cronet_build=true? On 2017/06/14 03:38:55, Eugene But wrote: > Remove this? Yeah I believe this should be set to false even for the combined library. Otherwise it strips out some functionality in //net etc. which are required for //ios/web_view. https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:63: # Copy the headers. Maybe you don't need to do this? We only expect headers to be in the framework of each target.
I've pulled out the relative header change into a dependent CL and refactored the script to be much clearer. Note that I made a few refactoring choices to minimize the use of the hardcoded "CronetChromeWebView" value in case we want to add support for building ChromeWebView in the future. https://codereview.chromium.org/2941683002/diff/1/ios/web_view/public/ChromeW... File ios/web_view/public/ChromeWebView.h (left): https://codereview.chromium.org/2941683002/diff/1/ios/web_view/public/ChromeW... ios/web_view/public/ChromeWebView.h:8: #import <ChromeWebView/cwv_export.h> On 2017/06/14 03:38:55, Eugene But wrote: > Just to clarify. This change and other import changes will not be a part of CL > when you land it. Right? Yes, moved to dependent CL. https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... File ios/web_view/tools/package_ios.py (right): https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:7: package_ios.py - Build and Package Release and Debug fat libraries for iOS. On 2017/06/14 06:31:58, Hiroshi Ichikawa wrote: > Maybe it's better to say that this script is for CronetChromeWebView.framework? > The current description sounds like for more general purpose, but it actually > isn't. Good point, updated. https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:15: def run(command, extra_options=''): On 2017/06/14 03:38:55, Eugene But wrote: > Please add comments for every non-trivial function Done. https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:26: for (build_config, gn_extra_args) in [('Debug', 'is_debug=true use_xcode_clang=true'), On 2017/06/14 03:38:55, Eugene But wrote: > This for loop has only 2 iterations and will unlikely have more. Would it be > better to extract the code inside for-loop to a separate function and call that > function twice? I agree, I reformatted this into method calls. https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:26: for (build_config, gn_extra_args) in [('Debug', 'is_debug=true use_xcode_clang=true'), On 2017/06/14 06:31:58, Hiroshi Ichikawa wrote: > Why do you specify use_xcode_clang=true only for debug build? I pulled the options from cronet package_ios.py. I don't know what this option is here. I've never set it locally. I'll check with cronet folks after I upload next patch. The default gn args that are setup for ios use this line, which may explain it: use_xcode_clang = is_official_build Even though we are not setting is_official_build, maybe we actually want to set use_xcode_clang=true for all configs. https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:27: ('Release', 'is_debug=false enable_stripping=true is_official_build=true')]: On 2017/06/14 06:31:58, Hiroshi Ichikawa wrote: > is_official_build should be false even for release. Confirmed with eugenebut@. > Because this library is not a part of official Chrome release. Done. https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:28: for (target_device, target_cpu, additional_cpu) in [('os', 'arm', 'arm64'), On 2017/06/14 03:38:55, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:32: gn_args = 'target_os="ios" enable_websockets=false ' \ On 2017/06/14 06:31:58, Hiroshi Ichikawa wrote: > How about providing an option to use Goma? Otherwise it takes too long... Great idea. Done. https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:35: 'disable_brotli_filter=true enable_dsyms=true ' \ On 2017/06/14 06:31:58, Hiroshi Ichikawa wrote: > Just curious, do you know if enable_dsyms affects binary size? I have set this > to false for release following your doc... Removing enable_dsyms or setting it to true or false all do the same thing. dsyms are always generated with this script. It's possible that another option forces the dsym to be created regardless of this specific setting. I will remove the option for now since it doesn't affect the build. That said, no it doesn't affect the size, but that seems to be only because this setting isn't doing anything. https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:39: #is_cronet_build=true? On 2017/06/14 06:31:58, Hiroshi Ichikawa wrote: > On 2017/06/14 03:38:55, Eugene But wrote: > > Remove this? > > Yeah I believe this should be set to false even for the combined library. > Otherwise it strips out some functionality in //net etc. which are required for > //ios/web_view. Yes, it should be false, but I left this here as a note to myself. Setting is_cronet_build sets CRONET_BUILD which is used here: https://cs.chromium.org/chromium/src/ios/net/cookies/cookie_store_ios.mm?rcl=... We need to make sure this won't cause issues or we need to define CRONET_BUILD ourselves. https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:46: build_result = run('ninja -C %s ios/web_view:cronet_ios_web_view_package' % build_dir, On 2017/06/14 03:38:55, Eugene But wrote: > Does it fit 80 symbols? Done. https://codereview.chromium.org/2941683002/diff/1/ios/web_view/tools/package_... ios/web_view/tools/package_ios.py:63: # Copy the headers. On 2017/06/14 06:31:58, Hiroshi Ichikawa wrote: > Maybe you don't need to do this? We only expect headers to be in the framework > of each target. Done. I agree, I used cronet script as a template, but I agree this isn't necessary.
Description was changed from ========== Add script to generate CronetChromeWebView.framework Relative imports have to be used because we build multiple frameworks, with different names and the package_ios.py script forces a clean build. This exposes an existing issue with the framework style includes. The alternative would be to #define on which framework was being build and use different imports. (For example: <ChromeWebView/...> vs <CronetChromeWebView/..>) Either way, I'll pull that change out of this CL before it is landed. BUG= ========== to ========== Add script to generate CronetChromeWebView.framework By default, the script will build CronetChromeWebView.framework and copy the results to ./out/CronetChromeWebView/. use_goma=true is also passed to gn unless specified otherwise with the --no-goma option. BUG=none ==========
michaeldo@chromium.org changed reviewers: + mef@chromium.org, sdefresne@chromium.org
sdefresne@ PTAL, especially at BUILD.gn changes mef@ PTAL at the gn args we are using in the python script. This is modeled after cronet build script. I know is_cronet_build must be false for our build to work, but do we need to define CRONET_BUILD? Thank you!
Python script lgtm https://codereview.chromium.org/2941683002/diff/40001/ios/web_view/tools/buil... File ios/web_view/tools/build.py (right): https://codereview.chromium.org/2941683002/diff/40001/ios/web_view/tools/buil... ios/web_view/tools/build.py:16: """Creates a defult output directory name for the given build_config and This link has an example of comments than conform to Python Style Guide: https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... 1. Line summary. 2. Longer description (optional) 3. Arguments description
https://codereview.chromium.org/2941683002/diff/40001/ios/web_view/tools/buil... File ios/web_view/tools/build.py (right): https://codereview.chromium.org/2941683002/diff/40001/ios/web_view/tools/buil... ios/web_view/tools/build.py:31: gn_extra_args = 'is_debug=true' Optional: This may be too confusing with extra_gn_options. May be more specific like like build_confg_options or build_config_args? https://codereview.chromium.org/2941683002/diff/40001/ios/web_view/tools/buil... ios/web_view/tools/build.py:36: gn_args = 'target_os="ios" enable_websockets=false ' \ It seems preferred to use parentheses than backslashes for multi-line expression: https://google.github.io/styleguide/pyguide.html#Line_length (assuming Chromium is following Google Python Style Guide) https://codereview.chromium.org/2941683002/diff/40001/ios/web_view/tools/buil... ios/web_view/tools/build.py:49: ninja_command = 'ninja -C %s ios/web_view:cronet_ios_web_view_package' % \ Ditto. https://codereview.chromium.org/2941683002/diff/40001/ios/web_view/tools/buil... ios/web_view/tools/build.py:124: print options Maybe prefix this output with "options: " or something, or maybe delete it if you just added this for debugging. https://codereview.chromium.org/2941683002/diff/40001/ios/web_view/tools/buil... ios/web_view/tools/build.py:136: gn_options = 'use_goma=true' Maybe cleaner to be one line?: gn_options = '' if options.no_goma else 'use_goma=true'
https://codereview.chromium.org/2941683002/diff/40001/ios/web_view/tools/buil... File ios/web_view/tools/build.py (right): https://codereview.chromium.org/2941683002/diff/40001/ios/web_view/tools/buil... ios/web_view/tools/build.py:16: """Creates a defult output directory name for the given build_config and On 2017/06/16 03:37:18, Eugene But wrote: > This link has an example of comments than conform to Python Style Guide: > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... > > 1. Line summary. > 2. Longer description (optional) > 3. Arguments description Thank you for the reference, updated function comments. https://codereview.chromium.org/2941683002/diff/40001/ios/web_view/tools/buil... ios/web_view/tools/build.py:31: gn_extra_args = 'is_debug=true' On 2017/06/16 07:41:59, Hiroshi Ichikawa wrote: > Optional: This may be too confusing with extra_gn_options. May be more specific > like like build_confg_options or build_config_args? I agree, I updated it to build_config_gn_args to be more explicit. https://codereview.chromium.org/2941683002/diff/40001/ios/web_view/tools/buil... ios/web_view/tools/build.py:36: gn_args = 'target_os="ios" enable_websockets=false ' \ On 2017/06/16 07:41:59, Hiroshi Ichikawa wrote: > It seems preferred to use parentheses than backslashes for multi-line > expression: > https://google.github.io/styleguide/pyguide.html#Line_length > (assuming Chromium is following Google Python Style Guide) Done. https://codereview.chromium.org/2941683002/diff/40001/ios/web_view/tools/buil... ios/web_view/tools/build.py:49: ninja_command = 'ninja -C %s ios/web_view:cronet_ios_web_view_package' % \ On 2017/06/16 07:41:59, Hiroshi Ichikawa wrote: > Ditto. Done. https://codereview.chromium.org/2941683002/diff/40001/ios/web_view/tools/buil... ios/web_view/tools/build.py:124: print options On 2017/06/16 07:41:59, Hiroshi Ichikawa wrote: > Maybe prefix this output with "options: " or something, or maybe delete it if > you just added this for debugging. I added a comment before the options are printed. I think it's useful for someone who never used the script before so they can see the defaults. https://codereview.chromium.org/2941683002/diff/40001/ios/web_view/tools/buil... ios/web_view/tools/build.py:136: gn_options = 'use_goma=true' On 2017/06/16 07:41:59, Hiroshi Ichikawa wrote: > Maybe cleaner to be one line?: > gn_options = '' if options.no_goma else 'use_goma=true' Done.
https://codereview.chromium.org/2941683002/diff/60001/ios/web_view/tools/buil... File ios/web_view/tools/build.py (right): https://codereview.chromium.org/2941683002/diff/60001/ios/web_view/tools/buil... ios/web_view/tools/build.py:51: 'is_component_build=false use_xcode_clang=false ' Is use_xcode_clang=false intentional? We've switched cronet to use_xcode_clang=true, and also is_official_build=true for release builds.
https://codereview.chromium.org/2941683002/diff/60001/ios/web_view/tools/buil... File ios/web_view/tools/build.py (right): https://codereview.chromium.org/2941683002/diff/60001/ios/web_view/tools/buil... ios/web_view/tools/build.py:51: 'is_component_build=false use_xcode_clang=false ' On 2017/06/16 19:29:00, mef wrote: > Is use_xcode_clang=false intentional? > We've switched cronet to use_xcode_clang=true, and also is_official_build=true > for release builds. In the default ios gn args setup, "use_xcode_clang = is_official_build", so I based setting use_xcode_clang to false on a prior comment in Patch 1 that we should set is_official_build to false even for release builds. I think it makes sense to set is_official_build to true, even though this isn't part of official Chrome release cycle. However, I'm not completely sure what that flag actually changes. eugenebut@ WDYT? mef@ what was the rational to switch to these for release builds? Is this at all unique to Cronet's maturity or a Chrome change?
On 2017/06/16 20:12:29, michaeldo wrote: > https://codereview.chromium.org/2941683002/diff/60001/ios/web_view/tools/buil... > File ios/web_view/tools/build.py (right): > > https://codereview.chromium.org/2941683002/diff/60001/ios/web_view/tools/buil... > ios/web_view/tools/build.py:51: 'is_component_build=false use_xcode_clang=false > ' > On 2017/06/16 19:29:00, mef wrote: > > Is use_xcode_clang=false intentional? > > We've switched cronet to use_xcode_clang=true, and also is_official_build=true > > for release builds. > > In the default ios gn args setup, "use_xcode_clang = is_official_build", so I > based setting use_xcode_clang to false on a prior comment in Patch 1 that we > should set is_official_build to false even for release builds. > > I think it makes sense to set is_official_build to true, even though this isn't > part of official Chrome release cycle. However, I'm not completely sure what > that flag actually changes. eugenebut@ WDYT? > > mef@ what was the rational to switch to these for release builds? Is this at all > unique to Cronet's maturity or a Chrome change? We've switched use_xcode_clang=true mainly for 2 reasons: 1. Binary size is a bit smaller. 2. The standard C++ library is different, and we are concerned that those differences could cause subtle runtime bugs. We've set is_official_build=true per conversation with sleevi@, who suggested it to enable additional optimizations.
On 2017/06/16 20:53:20, mef wrote: > On 2017/06/16 20:12:29, michaeldo wrote: > > > https://codereview.chromium.org/2941683002/diff/60001/ios/web_view/tools/buil... > > File ios/web_view/tools/build.py (right): > > > > > https://codereview.chromium.org/2941683002/diff/60001/ios/web_view/tools/buil... > > ios/web_view/tools/build.py:51: 'is_component_build=false > use_xcode_clang=false > > ' > > On 2017/06/16 19:29:00, mef wrote: > > > Is use_xcode_clang=false intentional? > > > We've switched cronet to use_xcode_clang=true, and also > is_official_build=true > > > for release builds. > > > > In the default ios gn args setup, "use_xcode_clang = is_official_build", so I > > based setting use_xcode_clang to false on a prior comment in Patch 1 that we > > should set is_official_build to false even for release builds. > > > > I think it makes sense to set is_official_build to true, even though this > isn't > > part of official Chrome release cycle. However, I'm not completely sure what > > that flag actually changes. eugenebut@ WDYT? > > > > mef@ what was the rational to switch to these for release builds? Is this at > all > > unique to Cronet's maturity or a Chrome change? > > We've switched use_xcode_clang=true mainly for 2 reasons: > > 1. Binary size is a bit smaller. > 2. The standard C++ library is different, and we are concerned that those > differences could cause subtle runtime bugs. > > We've set is_official_build=true per conversation with sleevi@, who suggested it > to enable additional optimizations. Ok, thank you for the details. I've updated the flags and turned off ios code signing to remove the embedded.mobilprovision file pre email with sdefresne@
Patchset #5 (id:80001) has been deleted
On 2017/06/19 17:01:44, michaeldo wrote: > On 2017/06/16 20:53:20, mef wrote: > > On 2017/06/16 20:12:29, michaeldo wrote: > > > > > > https://codereview.chromium.org/2941683002/diff/60001/ios/web_view/tools/buil... > > > File ios/web_view/tools/build.py (right): > > > > > > > > > https://codereview.chromium.org/2941683002/diff/60001/ios/web_view/tools/buil... > > > ios/web_view/tools/build.py:51: 'is_component_build=false > > use_xcode_clang=false > > > ' > > > On 2017/06/16 19:29:00, mef wrote: > > > > Is use_xcode_clang=false intentional? > > > > We've switched cronet to use_xcode_clang=true, and also > > is_official_build=true > > > > for release builds. > > > > > > In the default ios gn args setup, "use_xcode_clang = is_official_build", so > I > > > based setting use_xcode_clang to false on a prior comment in Patch 1 that we > > > should set is_official_build to false even for release builds. > > > > > > I think it makes sense to set is_official_build to true, even though this > > isn't > > > part of official Chrome release cycle. However, I'm not completely sure what > > > that flag actually changes. eugenebut@ WDYT? > > > > > > mef@ what was the rational to switch to these for release builds? Is this at > > all > > > unique to Cronet's maturity or a Chrome change? > > > > We've switched use_xcode_clang=true mainly for 2 reasons: > > > > 1. Binary size is a bit smaller. > > 2. The standard C++ library is different, and we are concerned that those > > differences could cause subtle runtime bugs. > > > > We've set is_official_build=true per conversation with sleevi@, who suggested > it > > to enable additional optimizations. > > Ok, thank you for the details. I've updated the flags and turned off ios code > signing to remove the embedded.mobilprovision file pre email with sdefresne@ lgtm, but I would like to understand the signing story, as I thought that dynamic framework must be signed on iOS 9+.
lgtm
lgtm
The CQ bit was checked by sdefresne@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: This issue passed the CQ dry run.
The CQ bit was checked by michaeldo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2941683002/#ps100001 (title: "Rebase and update gn args.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/20 21:43:43, mef wrote: > On 2017/06/19 17:01:44, michaeldo wrote: > > On 2017/06/16 20:53:20, mef wrote: > > > On 2017/06/16 20:12:29, michaeldo wrote: > > > > > > > > > > https://codereview.chromium.org/2941683002/diff/60001/ios/web_view/tools/buil... > > > > File ios/web_view/tools/build.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2941683002/diff/60001/ios/web_view/tools/buil... > > > > ios/web_view/tools/build.py:51: 'is_component_build=false > > > use_xcode_clang=false > > > > ' > > > > On 2017/06/16 19:29:00, mef wrote: > > > > > Is use_xcode_clang=false intentional? > > > > > We've switched cronet to use_xcode_clang=true, and also > > > is_official_build=true > > > > > for release builds. > > > > > > > > In the default ios gn args setup, "use_xcode_clang = is_official_build", > so > > I > > > > based setting use_xcode_clang to false on a prior comment in Patch 1 that > we > > > > should set is_official_build to false even for release builds. > > > > > > > > I think it makes sense to set is_official_build to true, even though this > > > isn't > > > > part of official Chrome release cycle. However, I'm not completely sure > what > > > > that flag actually changes. eugenebut@ WDYT? > > > > > > > > mef@ what was the rational to switch to these for release builds? Is this > at > > > all > > > > unique to Cronet's maturity or a Chrome change? > > > > > > We've switched use_xcode_clang=true mainly for 2 reasons: > > > > > > 1. Binary size is a bit smaller. > > > 2. The standard C++ library is different, and we are concerned that those > > > differences could cause subtle runtime bugs. > > > > > > We've set is_official_build=true per conversation with sleevi@, who > suggested > > it > > > to enable additional optimizations. > > > > Ok, thank you for the details. I've updated the flags and turned off ios code > > signing to remove the embedded.mobilprovision file pre email with sdefresne@ > > lgtm, but I would like to understand the signing story, as I thought that > dynamic framework must be signed on iOS 9+. This is still on my radar. The signing currently is definitely not what we want as it embeds the wrong provisioning when it signs. As far as I can tell, Xcode is the layer that enforces signing and maybe it isn't needed beyond when built with ninja. There wasn't an immediate clear answer to this from what I found, but I'll followup.
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498241193021100,
"parent_rev": "ca5afdce673a1578ad3970e9fc5c181a2646d81e", "commit_rev":
"f06b5d1964838e398b603c262435ead255c893d0"}
Message was sent while issue was closed.
Description was changed from ========== Add script to generate CronetChromeWebView.framework By default, the script will build CronetChromeWebView.framework and copy the results to ./out/CronetChromeWebView/. use_goma=true is also passed to gn unless specified otherwise with the --no-goma option. BUG=none ========== to ========== Add script to generate CronetChromeWebView.framework By default, the script will build CronetChromeWebView.framework and copy the results to ./out/CronetChromeWebView/. use_goma=true is also passed to gn unless specified otherwise with the --no-goma option. BUG=none Review-Url: https://codereview.chromium.org/2941683002 Cr-Commit-Position: refs/heads/master@{#481957} Committed: https://chromium.googlesource.com/chromium/src/+/f06b5d1964838e398b603c262435... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/f06b5d1964838e398b603c262435... |
