Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2331573003:
build full 64bit WebView (Closed)
|
Created:
4 years, 3 months ago by michaelbai Modified:
4 years, 3 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbuild full 64bit WebView
Enable to build full 64-bit WebView APK which has both 64-bit
and 32-bit binaries
BUG=645264
Committed: https://crrev.com/2d9977d86f8d6bb331ec472426fadb3e9f4c18e3
Cr-Commit-Position: refs/heads/master@{#419827}
Patch Set 1 #
Total comments: 2
Patch Set 2 : change to use webview_64bit_only #Patch Set 3 : using two variables #Patch Set 4 : build full 64bit WebView #
Total comments: 9
Patch Set 5 : address comment #
Total comments: 11
Patch Set 6 : add config.gni #Patch Set 7 : remove a unnecessary comment #
Total comments: 8
Patch Set 8 : back to one variable build_apk_secondary_abi #
Total comments: 2
Patch Set 9 : fix a error #
Messages
Total messages: 39 (13 generated)
Description was changed from ========== build full 64bit WebView Enable to build full 64-bit WebView APK which has both 64-bit and 32-bit binaries BUG=645264 ========== to ========== build full 64bit WebView Enable to build full 64-bit WebView APK which has both 64-bit and 32-bit binaries BUG=645264 ==========
michaelbai@chromium.org changed reviewers: + sgurun@chromium.org, torne@chromium.org
https://codereview.chromium.org/2331573003/diff/1/android_webview/system_webv... File android_webview/system_webview_apk_tmpl.gni (right): https://codereview.chromium.org/2331573003/diff/1/android_webview/system_webv... android_webview/system_webview_apk_tmpl.gni:29: if ((!defined(merge_64bit_webview) || !merge_64bit_webview) && Am I interepreting this correctly that setting merge_64bit_webview=true makes it build a WebView that's *only* 64-bit, suitable for later manual merging? If so that seems like a really unintuitive name for that variable - without any context I would have assumed that merge_64bit_webview=true is what I wanted to build a merged APK with both architectures. So unless I have this backwards, I'd suggest we rename this variable to something else. Something like "webview_64bit_only"? The merging doesn't seem to really be relevant - it's just about building a 64-bit only APK, no? Also: 1) What's the default behaviour going to be when you just build WebView without specifying any config? 2) How is this going to impact Monochrome? What's the default behaviour there, and how do we control it?
PTAL https://codereview.chromium.org/2331573003/diff/1/android_webview/system_webv... File android_webview/system_webview_apk_tmpl.gni (right): https://codereview.chromium.org/2331573003/diff/1/android_webview/system_webv... android_webview/system_webview_apk_tmpl.gni:29: if ((!defined(merge_64bit_webview) || !merge_64bit_webview) && On 2016/09/12 12:43:42, Torne wrote: > Am I interepreting this correctly that setting merge_64bit_webview=true makes it > build a WebView that's *only* 64-bit, suitable for later manual merging? > > If so that seems like a really unintuitive name for that variable - without any > context I would have assumed that merge_64bit_webview=true is what I wanted to > build a merged APK with both architectures. > > So unless I have this backwards, I'd suggest we rename this variable to > something else. Something like "webview_64bit_only"? The merging doesn't seem to > really be relevant - it's just about building a 64-bit only APK, no? > It seemed merge_64bit_webview is not a good name, sure, I changed it to webview_64bit_only. Yes, it should be webview_64bit_only, just find out my branch name for this patch is webview_64bit_only :). > Also: > 1) What's the default behaviour going to be when you just build WebView without > specifying any config? It will build full 64-bit WebView APK which has both 32-bit and 64-bit binaries. > 2) How is this going to impact Monochrome? What's the default behaviour there, > and how do we control it? Monochrome will use same variable webview_64bit_only, the default behavior is same as WebView, if webview_64bit_only is not true, Monochrome apk will have both 32-bit and 64-bit binaries. See downstream patch: https://chrome-internal-review.googlesource.com/285815 The places where it turns on are release bot and build bot to protect this configuration.
On 2016/09/12 19:41:13, michaelbai wrote: > Yes, it should be webview_64bit_only, just find out my branch name for this > patch is webview_64bit_only :). Doesn't there need to be a declare_args somewhere to actually make this a variable that can be changed in gn args? > > Also: > > 1) What's the default behaviour going to be when you just build WebView > without > > specifying any config? > > > It will build full 64-bit WebView APK which has both 32-bit and 64-bit binaries. OK, good, just make sure you send a PSA to the team to let them know this has changed, so they aren't surprised by the increased build time and can set the variable if they want a 64-bit only build for speed. > > 2) How is this going to impact Monochrome? What's the default behaviour there, > > and how do we control it? > > Monochrome will use same variable webview_64bit_only, the default behavior is > same as WebView, if webview_64bit_only is not true, Monochrome apk will have > both 32-bit and 64-bit binaries. Will both the binaries be monochrome, or will they just be webview, though? If webview_64bit_only is set, what will monochrome actually look like? If it's just the same 64-bit library we actually ship then that doesn't have the chrome code and it will just crash if launched as chrome, no? > See downstream patch: > https://chrome-internal-review.googlesource.com/285815 > > The places where it turns on are release bot and build bot to protect this > configuration.
On 2016/09/13 14:45:59, Torne wrote: > On 2016/09/12 19:41:13, michaelbai wrote: > > Yes, it should be webview_64bit_only, just find out my branch name for this > > patch is webview_64bit_only :). > > Doesn't there need to be a declare_args somewhere to actually make this a > variable that can be changed in gn args? > It is declared in downstream, I don't think anyone will enable it in upstream. > > > Also: > > > 1) What's the default behaviour going to be when you just build WebView > > without > > > specifying any config? > > > > > > It will build full 64-bit WebView APK which has both 32-bit and 64-bit > binaries. > > OK, good, just make sure you send a PSA to the team to let them know this has > changed, so they aren't surprised by the increased build time and can set the > variable if they want a 64-bit only build for speed. > Sure, will do. > > > 2) How is this going to impact Monochrome? What's the default behaviour > there, > > > and how do we control it? > > > > Monochrome will use same variable webview_64bit_only, the default behavior is > > same as WebView, if webview_64bit_only is not true, Monochrome apk will have > > both 32-bit and 64-bit binaries. > > Will both the binaries be monochrome, or will they just be webview, though? If > webview_64bit_only is set, what will monochrome actually look like? If it's just > the same 64-bit library we actually ship then that doesn't have the chrome code > and it will just crash if launched as chrome, no? > If webview_64bit_only is set, monochrome merely has 64-bit webview only native library, it will definitely crash, strictly speaking, standalone WebView will crash as well (webview crashed in 32-bit apps), crash is actually good here, because APK is not the final one we want to release. > > See downstream patch: > > https://chrome-internal-review.googlesource.com/285815 > > > > The places where it turns on are release bot and build bot to protect this > > configuration.
On 2016/09/13 18:19:06, michaelbai wrote: > On 2016/09/13 14:45:59, Torne wrote: > > On 2016/09/12 19:41:13, michaelbai wrote: > > > Yes, it should be webview_64bit_only, just find out my branch name for this > > > patch is webview_64bit_only :). > > > > Doesn't there need to be a declare_args somewhere to actually make this a > > variable that can be changed in gn args? > > > > > It is declared in downstream, I don't think anyone will enable it in upstream. It doesn't seem to make sense to have a variable that is declared downstream but controls the result of building an upstream target. Why not just declare it here? > > > > Also: > > > > 1) What's the default behaviour going to be when you just build WebView > > > without > > > > specifying any config? > > > > > > > > > It will build full 64-bit WebView APK which has both 32-bit and 64-bit > > binaries. > > > > OK, good, just make sure you send a PSA to the team to let them know this has > > changed, so they aren't surprised by the increased build time and can set the > > variable if they want a 64-bit only build for speed. > > > > Sure, will do. > > > > > 2) How is this going to impact Monochrome? What's the default behaviour > > there, > > > > and how do we control it? > > > > > > Monochrome will use same variable webview_64bit_only, the default behavior > is > > > same as WebView, if webview_64bit_only is not true, Monochrome apk will have > > > both 32-bit and 64-bit binaries. > > > > Will both the binaries be monochrome, or will they just be webview, though? If > > webview_64bit_only is set, what will monochrome actually look like? If it's > just > > the same 64-bit library we actually ship then that doesn't have the chrome > code > > and it will just crash if launched as chrome, no? > > > > If webview_64bit_only is set, monochrome merely has 64-bit webview only native > library, it will definitely crash, strictly speaking, standalone WebView > will crash as well (webview crashed in 32-bit apps), crash is actually > good here, because APK is not the final one we want to release. Webview works fine as 64-bit only if you don't run 32-bit apps. We want to have this flag so that we can build a 64-bit only version, because it's faster to build if that's all you need. I'm wondering what a sensible behaviour for monochrome is, here. Right now there's no way to build an actual 64-bit monochrome build, correct? That actually seems weird and undesirable, and is a regression from regular Chrome where it's perfectly possible to build a 64-bit version.. > > > > See downstream patch: > > > https://chrome-internal-review.googlesource.com/285815 > > > > > > The places where it turns on are release bot and build bot to protect this > > > configuration.
On 2016/09/14 15:21:57, Torne wrote: > On 2016/09/13 18:19:06, michaelbai wrote: > > On 2016/09/13 14:45:59, Torne wrote: > > > On 2016/09/12 19:41:13, michaelbai wrote: > > > > Yes, it should be webview_64bit_only, just find out my branch name for > this > > > > patch is webview_64bit_only :). > > > > > > Doesn't there need to be a declare_args somewhere to actually make this a > > > variable that can be changed in gn args? > > > > > > > > > It is declared in downstream, I don't think anyone will enable it in upstream. > > It doesn't seem to make sense to have a variable that is declared downstream but > controls the result of building an upstream target. Why not just declare it > here? > hmm, I think we should define this in upstream, because someone might want to build SystemWebView APK. > > > > > Also: > > > > > 1) What's the default behaviour going to be when you just build WebView > > > > without > > > > > specifying any config? > > > > > > > > > > > > It will build full 64-bit WebView APK which has both 32-bit and 64-bit > > > binaries. > > > > > > OK, good, just make sure you send a PSA to the team to let them know this > has > > > changed, so they aren't surprised by the increased build time and can set > the > > > variable if they want a 64-bit only build for speed. > > > > > > > Sure, will do. > > > > > > > 2) How is this going to impact Monochrome? What's the default behaviour > > > there, > > > > > and how do we control it? > > > > > > > > Monochrome will use same variable webview_64bit_only, the default behavior > > is > > > > same as WebView, if webview_64bit_only is not true, Monochrome apk will > have > > > > both 32-bit and 64-bit binaries. > > > > > > Will both the binaries be monochrome, or will they just be webview, though? > If > > > webview_64bit_only is set, what will monochrome actually look like? If it's > > just > > > the same 64-bit library we actually ship then that doesn't have the chrome > > code > > > and it will just crash if launched as chrome, no? > > > > > > > If webview_64bit_only is set, monochrome merely has 64-bit webview only native > > library, it will definitely crash, strictly speaking, standalone WebView > > will crash as well (webview crashed in 32-bit apps), crash is actually > > good here, because APK is not the final one we want to release. > > Webview works fine as 64-bit only if you don't run 32-bit apps. We want to have > this flag so that we can build a 64-bit only version, because it's faster to > build if that's all you need. I'm wondering what a sensible behaviour for > monochrome is, here. Right now there's no way to build an actual 64-bit > monochrome build, correct? That actually seems weird and undesirable, and is a > regression from regular Chrome where it's perfectly possible to build a 64-bit > version.. > Yes, there is no way to build Monochrome in which Chrome runs in 64-bit mode, I discussed this with Alex, it was not urgent thing to do, I will sync to him to see if he has plan to have new release bot to build this Monochrome. I will file a bug to track this. > > > > > > See downstream patch: > > > > https://chrome-internal-review.googlesource.com/285815 > > > > > > > > The places where it turns on are release bot and build bot to protect this > > > > configuration.
On 2016/09/14 15:56:39, michaelbai wrote: > On 2016/09/14 15:21:57, Torne wrote: > > On 2016/09/13 18:19:06, michaelbai wrote: > > > On 2016/09/13 14:45:59, Torne wrote: > > > > On 2016/09/12 19:41:13, michaelbai wrote: > > > > > Yes, it should be webview_64bit_only, just find out my branch name for > > this > > > > > patch is webview_64bit_only :). > > > > > > > > Doesn't there need to be a declare_args somewhere to actually make this a > > > > variable that can be changed in gn args? > > > > > > > > > > > > > It is declared in downstream, I don't think anyone will enable it in > upstream. > > > > It doesn't seem to make sense to have a variable that is declared downstream > but > > controls the result of building an upstream target. Why not just declare it > > here? > > > > hmm, I think we should define this in upstream, because someone might want to > build SystemWebView APK. > > > > > > > Also: > > > > > > 1) What's the default behaviour going to be when you just build > WebView > > > > > without > > > > > > specifying any config? > > > > > > > > > > > > > > > It will build full 64-bit WebView APK which has both 32-bit and 64-bit > > > > binaries. > > > > > > > > OK, good, just make sure you send a PSA to the team to let them know this > > has > > > > changed, so they aren't surprised by the increased build time and can set > > the > > > > variable if they want a 64-bit only build for speed. > > > > > > > > > > Sure, will do. > > > > > > > > > 2) How is this going to impact Monochrome? What's the default > behaviour > > > > there, > > > > > > and how do we control it? > > > > > > > > > > Monochrome will use same variable webview_64bit_only, the default > behavior > > > is > > > > > same as WebView, if webview_64bit_only is not true, Monochrome apk will > > have > > > > > both 32-bit and 64-bit binaries. > > > > > > > > Will both the binaries be monochrome, or will they just be webview, > though? > > If > > > > webview_64bit_only is set, what will monochrome actually look like? If > it's > > > just > > > > the same 64-bit library we actually ship then that doesn't have the chrome > > > code > > > > and it will just crash if launched as chrome, no? > > > > > > > > > > If webview_64bit_only is set, monochrome merely has 64-bit webview only > native > > > library, it will definitely crash, strictly speaking, standalone WebView > > > will crash as well (webview crashed in 32-bit apps), crash is actually > > > good here, because APK is not the final one we want to release. > > > > Webview works fine as 64-bit only if you don't run 32-bit apps. We want to > have > > this flag so that we can build a 64-bit only version, because it's faster to > > build if that's all you need. I'm wondering what a sensible behaviour for > > monochrome is, here. Right now there's no way to build an actual 64-bit > > monochrome build, correct? That actually seems weird and undesirable, and is a > > regression from regular Chrome where it's perfectly possible to build a 64-bit > > version.. > > > > Yes, there is no way to build Monochrome in which Chrome runs in 64-bit mode, I > discussed this with Alex, it was not urgent thing to do, I will sync to him to > see if he has plan to have new release bot to build this Monochrome. I will file > a bug to track this. > > > > > > > > > > See downstream patch: > > > > > https://chrome-internal-review.googlesource.com/285815 > > > > > > > > > > The places where it turns on are release bot and build bot to protect > this > > > > > configuration. Filed bug crbug.com/647386 to track build Monochrome that have Chrome run in 64-bit mode.
michaelbai@chromium.org changed reviewers: + amineer@chromium.org, jbudorick@chromium.org
I were missing the fact that 64-bit only WebView is still valuable, so I changed to patches to use 2 gn variable - build_64bit_only_webview, declared in upstream, if enabled, 64-bit standalone webview will only have 64-bit binaries. - merge_64bit_webview_and_monochrome, declared in downstream, if enabled, both webview and monochrome have only 64-bit binaries, it indicates build 64-bit only WebView, so build_64bit_only_webview is forced to be true. this variable is used by build bot and release bot, and only this variable should be set in them.
jbudorick, amineer, this patch is related to downstream https://chrome-internal-review.googlesource.com/#/c/285815/
PTAL
Thanks a lot; just a couple of minor comments below. I know it's not a priority to support running Monochrome in 64-bit and it doesn't have to be done in this CL, but it would be nice to do it even without a bot that builds that config. I'm imagining we would add a third build flag for that - and then it should be possible to set both "run_monochrome_64bit" *and* "build_32bit_webview_only", and end up with a Monochrome APK that doesn't contain a 32-bit library at all, but still works (except for in 32-bit apps that use webview). https://codereview.chromium.org/2331573003/diff/60001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2331573003/diff/60001/android_webview/BUILD.g... android_webview/BUILD.gn:426: shared_library("monochrome") { I know you aren't introducing this in this CL, but does this mean that there's always a "monochrome" target defined in android_webview, even in a normal build? It seems like this should only be defined when merge_64bit_webview_and_monochrome is set to avoid confusion, unless I'm missing something. https://codereview.chromium.org/2331573003/diff/60001/build/config/android/co... File build/config/android/config.gni (right): https://codereview.chromium.org/2331573003/diff/60001/build/config/android/co... build/config/android/config.gni:137: # Only build 64-bit binaries in 64-bit WebView APK. Maybe "Only build 64-bit binaries in 64-bit WebView APK. 32-bit apps will not be supported; for development only." to be a bit more explicit that this isn't the "right" thing to do for shipping? https://codereview.chromium.org/2331573003/diff/60001/build/config/android/co... build/config/android/config.gni:148: if (defined(merge_64bit_webview_and_monochrome) && I think you should also just declare this variable upstream as well. There's nothing secret about the ability to build the two parts of the APK separately and then merge them; anyone with a similar CI infrastructure could conceivably prefer to do it that way.
PTAL https://codereview.chromium.org/2331573003/diff/60001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2331573003/diff/60001/android_webview/BUILD.g... android_webview/BUILD.gn:426: shared_library("monochrome") { On 2016/09/16 14:04:18, Torne wrote: > I know you aren't introducing this in this CL, but does this mean that there's > always a "monochrome" target defined in android_webview, even in a normal build? > It seems like this should only be defined when > merge_64bit_webview_and_monochrome is set to avoid confusion, unless I'm missing > something. This target is also needed for build 64-bit Monochrome without merging, this is 64-bit shared library in it. https://codereview.chromium.org/2331573003/diff/60001/build/config/android/co... File build/config/android/config.gni (right): https://codereview.chromium.org/2331573003/diff/60001/build/config/android/co... build/config/android/config.gni:137: # Only build 64-bit binaries in 64-bit WebView APK. On 2016/09/16 14:04:18, Torne wrote: > Maybe "Only build 64-bit binaries in 64-bit WebView APK. 32-bit apps will not be > supported; for development only." to be a bit more explicit that this isn't the > "right" thing to do for shipping? Done. https://codereview.chromium.org/2331573003/diff/60001/build/config/android/co... build/config/android/config.gni:148: if (defined(merge_64bit_webview_and_monochrome) && On 2016/09/16 14:04:18, Torne wrote: > I think you should also just declare this variable upstream as well. There's > nothing secret about the ability to build the two parts of the APK separately > and then merge them; anyone with a similar CI infrastructure could conceivably > prefer to do it that way. It is not for hiding something, I think this should be internal only, here are some reasons - without internal code, no one could build Monochrome (We don't have public monochrome yet), - for the people want to merge 64-bit SystemWebView.apk, they can do it without setting this variable. - to have it in upstream might confuse people, as it is same as build_64bit_only_webview when people only have upstream code. they might wonder why I have to set this variable as I already set build_64bit_only_webview, what's the difference between them?. If people can NOT find it in gn args --list, they don't need to worry about it. this is beauty of GN variable scope. :) Do we want to tell people how to merge 64-bit SystemWebView.apk? If so, we should have document for it, maybe just add it in apk_merger script.
https://codereview.chromium.org/2331573003/diff/80001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2331573003/diff/80001/android_webview/BUILD.g... android_webview/BUILD.gn:160: (!defined(build_64bit_only_webview) || !build_64bit_only_webview)) { nit: this might be more readable as android_64bit_target_cpu && !(defined(build_64bit_only_webview) && build_64bit_only_webview)) https://codereview.chromium.org/2331573003/diff/80001/android_webview/BUILD.g... android_webview/BUILD.gn:403: group("webviewchromium_secondary_abi_lib") { Can system_webview_apk_tmpl use :libwebviewchromium($android_secondary_api_toolchain) directly? https://codereview.chromium.org/2331573003/diff/80001/android_webview/BUILD.g... android_webview/BUILD.gn:409: android_assets("v8_snapshot_secondary_abi_assets") { This appears to be unused. Are you planning to use it downstream...? https://codereview.chromium.org/2331573003/diff/80001/build/config/android/co... File build/config/android/config.gni (right): https://codereview.chromium.org/2331573003/diff/80001/build/config/android/co... build/config/android/config.gni:139: build_64bit_only_webview = false As this is webview-specific, it seems like it should be handled by a .gni in android_webview rather than in here. https://codereview.chromium.org/2331573003/diff/80001/build/config/android/co... build/config/android/config.gni:149: if (defined(merge_64bit_webview_and_monochrome) && I agree with torne -- this should be defined upstream.
https://codereview.chromium.org/2331573003/diff/60001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2331573003/diff/60001/android_webview/BUILD.g... android_webview/BUILD.gn:426: shared_library("monochrome") { On 2016/09/16 16:14:22, michaelbai wrote: > On 2016/09/16 14:04:18, Torne wrote: > > I know you aren't introducing this in this CL, but does this mean that there's > > always a "monochrome" target defined in android_webview, even in a normal > build? > > It seems like this should only be defined when > > merge_64bit_webview_and_monochrome is set to avoid confusion, unless I'm > missing > > something. > > This target is also needed for build 64-bit Monochrome without merging, this is > 64-bit shared library in it. Oops, yes. Sorry :) https://codereview.chromium.org/2331573003/diff/60001/build/config/android/co... File build/config/android/config.gni (right): https://codereview.chromium.org/2331573003/diff/60001/build/config/android/co... build/config/android/config.gni:148: if (defined(merge_64bit_webview_and_monochrome) && On 2016/09/16 16:14:22, michaelbai wrote: > On 2016/09/16 14:04:18, Torne wrote: > > I think you should also just declare this variable upstream as well. There's > > nothing secret about the ability to build the two parts of the APK separately > > and then merge them; anyone with a similar CI infrastructure could conceivably > > prefer to do it that way. > > It is not for hiding something, I think this should be internal only, here are > some reasons > - without internal code, no one could build Monochrome (We don't have public > monochrome yet), But we will at some point. > - for the people want to merge 64-bit SystemWebView.apk, they can do it without > setting this variable. > - to have it in upstream might confuse people, as it is same as > build_64bit_only_webview when people only have upstream code. they might wonder > why I have to set this variable as I already set build_64bit_only_webview, > what's the difference between them?. If people can NOT find it in gn args > --list, they don't need to worry about it. this is beauty of GN variable scope. > :) The rule of thumb should be that everything is public unless there is a specific reason why it needs to be internal (confidentiality, or a dependency on another internal thing). I don't think it matters whether it's actually useful right now or not. It *will* be useful when there's a public monochrome target, so why not just define it now instead of having to do a two-sided patch later to move it? > Do we want to tell people how to merge 64-bit SystemWebView.apk? If so, we > should have document for it, maybe just add it in apk_merger script. Yes, we should probably at least have the script contain an explanation of how to build the two "half" APKs and merge them together. I don't think it needs to be documented anywhere else in particular; most people will just use the self-contained combined build and that's a sensible default.
https://codereview.chromium.org/2331573003/diff/80001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2331573003/diff/80001/android_webview/BUILD.g... android_webview/BUILD.gn:409: android_assets("v8_snapshot_secondary_abi_assets") { On 2016/09/16 16:23:35, jbudorick wrote: > This appears to be unused. Are you planning to use it downstream...? It will be used by the downstream monochrome target (with a corresponding change that's yet to come); there isn't yet a public monochrome target, but we'll add one at some point, so defining it here is useful. https://codereview.chromium.org/2331573003/diff/80001/build/config/android/co... File build/config/android/config.gni (right): https://codereview.chromium.org/2331573003/diff/80001/build/config/android/co... build/config/android/config.gni:139: build_64bit_only_webview = false On 2016/09/16 16:23:35, jbudorick wrote: > As this is webview-specific, it seems like it should be handled by a .gni in > android_webview rather than in here. It'll also have an effect on the monochrome APK.
PTAL, main changes - Moved merge_64bit_webview_and_monochrome to upstream - Moved build_64bit_only_webview to android_webview/config.gni https://codereview.chromium.org/2331573003/diff/60001/build/config/android/co... File build/config/android/config.gni (right): https://codereview.chromium.org/2331573003/diff/60001/build/config/android/co... build/config/android/config.gni:148: if (defined(merge_64bit_webview_and_monochrome) && On 2016/09/16 16:35:33, Torne wrote: > On 2016/09/16 16:14:22, michaelbai wrote: > > On 2016/09/16 14:04:18, Torne wrote: > > > I think you should also just declare this variable upstream as well. There's > > > nothing secret about the ability to build the two parts of the APK > separately > > > and then merge them; anyone with a similar CI infrastructure could > conceivably > > > prefer to do it that way. > > > > It is not for hiding something, I think this should be internal only, here are > > some reasons > > - without internal code, no one could build Monochrome (We don't have public > > monochrome yet), > > But we will at some point. > > > - for the people want to merge 64-bit SystemWebView.apk, they can do it > without > > setting this variable. > > - to have it in upstream might confuse people, as it is same as > > build_64bit_only_webview when people only have upstream code. they might > wonder > > why I have to set this variable as I already set build_64bit_only_webview, > > what's the difference between them?. If people can NOT find it in gn args > > --list, they don't need to worry about it. this is beauty of GN variable > scope. > > :) > > The rule of thumb should be that everything is public unless there is a specific > reason why it needs to be internal (confidentiality, or a dependency on another > internal thing). I don't think it matters whether it's actually useful right now > or not. It *will* be useful when there's a public monochrome target, so why not > just define it now instead of having to do a two-sided patch later to move it? > > > Do we want to tell people how to merge 64-bit SystemWebView.apk? If so, we > > should have document for it, maybe just add it in apk_merger script. > > Yes, we should probably at least have the script contain an explanation of how > to build the two "half" APKs and merge them together. I don't think it needs to > be documented anywhere else in particular; most people will just use the > self-contained combined build and that's a sensible default. Fair enough, everything is in public https://codereview.chromium.org/2331573003/diff/80001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2331573003/diff/80001/android_webview/BUILD.g... android_webview/BUILD.gn:160: (!defined(build_64bit_only_webview) || !build_64bit_only_webview)) { On 2016/09/16 16:23:35, jbudorick wrote: > nit: this might be more readable as > > android_64bit_target_cpu && !(defined(build_64bit_only_webview) && > build_64bit_only_webview)) could be more simple android_64bit_target_cpu && ! build_64bit_only_webview https://codereview.chromium.org/2331573003/diff/80001/android_webview/BUILD.g... android_webview/BUILD.gn:403: group("webviewchromium_secondary_abi_lib") { On 2016/09/16 16:23:35, jbudorick wrote: > Can system_webview_apk_tmpl use > :libwebviewchromium($android_secondary_api_toolchain) directly? Thanks to spot it, I were copying this from Monochrome, this needn't be a separated target. https://codereview.chromium.org/2331573003/diff/80001/android_webview/BUILD.g... android_webview/BUILD.gn:409: android_assets("v8_snapshot_secondary_abi_assets") { On 2016/09/16 16:23:35, jbudorick wrote: > This appears to be unused. Are you planning to use it downstream...? It is used in line 161 https://codereview.chromium.org/2331573003/diff/80001/build/config/android/co... File build/config/android/config.gni (right): https://codereview.chromium.org/2331573003/diff/80001/build/config/android/co... build/config/android/config.gni:139: build_64bit_only_webview = false On 2016/09/16 16:37:57, Torne wrote: > On 2016/09/16 16:23:35, jbudorick wrote: > > As this is webview-specific, it seems like it should be handled by a .gni in > > android_webview rather than in here. > > It'll also have an effect on the monochrome APK. Since there are 2 separated variable, this one is webview specific, moved it to android_webview
michaelbai@chromium.org changed reviewers: + agrieve@chromium.org
https://codereview.chromium.org/2331573003/diff/120001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2331573003/diff/120001/android_webview/BUILD.... android_webview/BUILD.gn:159: if (android_64bit_target_cpu && !build_64bit_only_webview) { Doesn't this file need to include the new config.gni file to be able to check this variable? https://codereview.chromium.org/2331573003/diff/120001/build/config/android/c... File build/config/android/config.gni (right): https://codereview.chromium.org/2331573003/diff/120001/build/config/android/c... build/config/android/config.gni:139: merge_64bit_webview_and_monochrome = false The description of this implies that it does the exact opposite of what it actually does - it implies that setting this to true causes the APKs to be merged, and that if you don't set it you don't both architectures. The name itself also seems to suggest this, now that I'm thinking about it. I tried to think of a better way to word this, but in the process, I realised that I'm not sure why it's necessary to have two flags here in the first place. What we need, both for the "buildbot that will use apk_merger.py" case, and for the "developer who wants to skip the 32-bit build for speed" case, is a flag that says "don't bother doing the 32-bit part of this build, just do the 64-bit part". Why can't we just have one flag for this? The "fast build for development" case *will* want this to apply to monochrome as well, not just webview, as soon as we have the ability to also specify that chrome should be 64-bit, so I don't think that's a good reason to have two flags.
https://codereview.chromium.org/2331573003/diff/120001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2331573003/diff/120001/android_webview/BUILD.... android_webview/BUILD.gn:405: _secondary_abi_out_dir = it would be better to get this via: _secondary_abi_out_dir = get_label_info("//v8($android_secondary_abi_toolchain)", "root_out_dir") https://codereview.chromium.org/2331573003/diff/120001/android_webview/config... File android_webview/config.gni (right): https://codereview.chromium.org/2331573003/diff/120001/android_webview/config... android_webview/config.gni:10: build_64bit_only_webview = false It would be better to have: build_64bit_only_webview = merge_64bit_webview_and_monochrome and change the bellow if statement to an assert
PTAL https://codereview.chromium.org/2331573003/diff/120001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2331573003/diff/120001/android_webview/BUILD.... android_webview/BUILD.gn:159: if (android_64bit_target_cpu && !build_64bit_only_webview) { On 2016/09/19 13:36:28, Torne wrote: > Doesn't this file need to include the new config.gni file to be able to check > this variable? No, it doesn't, build_64bit_only_webview's definition comes from system_webview_apk_tmpl.gni which imports android_webview/config.gni. https://codereview.chromium.org/2331573003/diff/120001/android_webview/BUILD.... android_webview/BUILD.gn:405: _secondary_abi_out_dir = On 2016/09/19 14:29:18, agrieve wrote: > it would be better to get this via: > > > _secondary_abi_out_dir = > get_label_info("//v8($android_secondary_abi_toolchain)", "root_out_dir") Done. https://codereview.chromium.org/2331573003/diff/120001/android_webview/config... File android_webview/config.gni (right): https://codereview.chromium.org/2331573003/diff/120001/android_webview/config... android_webview/config.gni:10: build_64bit_only_webview = false On 2016/09/19 14:29:18, agrieve wrote: > It would be better to have: > build_64bit_only_webview = merge_64bit_webview_and_monochrome > > and change the bellow if statement to an assert Thanks, since we go one variable, I will do next time https://codereview.chromium.org/2331573003/diff/120001/build/config/android/c... File build/config/android/config.gni (right): https://codereview.chromium.org/2331573003/diff/120001/build/config/android/c... build/config/android/config.gni:139: merge_64bit_webview_and_monochrome = false On 2016/09/19 13:36:28, Torne wrote: > The description of this implies that it does the exact opposite of what it > actually does - it implies that setting this to true causes the APKs to be > merged, and that if you don't set it you don't both architectures. The name > itself also seems to suggest this, now that I'm thinking about it. > > I tried to think of a better way to word this, but in the process, I realised > that I'm not sure why it's necessary to have two flags here in the first place. > > What we need, both for the "buildbot that will use apk_merger.py" case, and for > the "developer who wants to skip the 32-bit build for speed" case, is a flag > that says "don't bother doing the 32-bit part of this build, just do the 64-bit > part". Why can't we just have one flag for this? > > The "fast build for development" case *will* want this to apply to monochrome as > well, not just webview, as soon as we have the ability to also specify that > chrome should be 64-bit, so I don't think that's a good reason to have two > flags. As Torne and I discussed offline, we came back to one variable, and give it good name - build_android_secondary_abi, now I think build_apk_secondary_abi is better than build_android_secondary_abi as secondary abi is meaningful in the context of 'APK', also APK is android specific, no one should use it for other platform.
lgtm https://codereview.chromium.org/2331573003/diff/140001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2331573003/diff/140001/android_webview/BUILD.... android_webview/BUILD.gn:404: get_label_info(android_secondary_abi_toolchain, "root_out_dir") I don't think this is actually right. You need to give get_label_info a target with android_secondary_abi_toolchain in parenthesis.
https://codereview.chromium.org/2331573003/diff/140001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2331573003/diff/140001/android_webview/BUILD.... android_webview/BUILD.gn:404: get_label_info(android_secondary_abi_toolchain, "root_out_dir") On 2016/09/19 23:54:13, agrieve wrote: > I don't think this is actually right. You need to give get_label_info a target > with android_secondary_abi_toolchain in parenthesis. Thanks for spotting this, it was merge error.
The CQ bit was checked by michaelbai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2331573003/#ps160001 (title: "fix a error")
The CQ bit was unchecked by michaelbai@chromium.org
The CQ bit was checked by michaelbai@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 michaelbai@chromium.org
LGTM, thanks for bearing with the suggestions Michael!
The CQ bit was checked by michaelbai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== build full 64bit WebView Enable to build full 64-bit WebView APK which has both 64-bit and 32-bit binaries BUG=645264 ========== to ========== build full 64bit WebView Enable to build full 64-bit WebView APK which has both 64-bit and 32-bit binaries BUG=645264 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== build full 64bit WebView Enable to build full 64-bit WebView APK which has both 64-bit and 32-bit binaries BUG=645264 ========== to ========== build full 64bit WebView Enable to build full 64-bit WebView APK which has both 64-bit and 32-bit binaries BUG=645264 Committed: https://crrev.com/2d9977d86f8d6bb331ec472426fadb3e9f4c18e3 Cr-Commit-Position: refs/heads/master@{#419827} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/2d9977d86f8d6bb331ec472426fadb3e9f4c18e3 Cr-Commit-Position: refs/heads/master@{#419827} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
