Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2161183003:
Build libmonochrome.so with secondary toolchain (Closed)
|
Created:
4 years, 5 months ago by michaelbai Modified:
4 years, 5 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBuild libmonochrome.so with secondary toolchain
This patch
- added Android secondary abi toolchain.
- built libmonochrome with secondary abi toolchain, the 64-bit
build starts to building 64-bit webview only libmonochrome.so
and 32-bit full libmonochrome.so.
BUG=605315
Committed: https://crrev.com/e56fa3c0528dc62a3d423ed2d0b83b5e039cb602
Cr-Commit-Position: refs/heads/master@{#406965}
Patch Set 1 #
Total comments: 7
Patch Set 2 : address comments #
Total comments: 7
Patch Set 3 : Remove the v8_foo toolchain & address comments #
Messages
Total messages: 30 (15 generated)
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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Build libmonochrome.so with secondary toolchain This patch - added Android secondary abi toolchain. - built libmonochrome with secondary abi toolchain, the 64-bit build starts to building 64-bit webview only libmonochrome.so and 32-bit full libmonochrome.so. BUG=605315 ========== to ========== Build libmonochrome.so with secondary toolchain This patch - added Android secondary abi toolchain. - built libmonochrome with secondary abi toolchain, the 64-bit build starts to building 64-bit webview only libmonochrome.so and 32-bit full libmonochrome.so. BUG=605315 ==========
michaelbai@chromium.org changed reviewers: + agrieve@chromium.org, dpranke@chromium.org
Andrew, maybe land this first, then see what's needed to enable the java template at runtime, otherwise, the fix might mix with this change.
lgtm, but I mostly defer to agrieve here.
Yep, I'm happy to wait for this before adding in that other assert change. https://codereview.chromium.org/2161183003/diff/1/build/config/android/config... File build/config/android/config.gni (right): https://codereview.chromium.org/2161183003/diff/1/build/config/android/config... build/config/android/config.gni:7: import("//build/config/android/secondary_abi.gni") Having this as a separate file doesn't really make sense to me if it's just going to be imported by the main config.gni. I think either: 1. Don't have it imported here, or 2. Inline into the logic below. https://codereview.chromium.org/2161183003/diff/1/build/config/android/config... build/config/android/config.gni:286: android_app_secondary_abi = "x86" If we're going to have a separate file for secondary_abi things, this value should probably be defined there. https://codereview.chromium.org/2161183003/diff/1/build/toolchain/android/BUI... File build/toolchain/android/BUILD.gn (right): https://codereview.chromium.org/2161183003/diff/1/build/toolchain/android/BUI... build/toolchain/android/BUILD.gn:122: android_gcc_toolchains_arm_helper("arm_v8_arm") { It's worth a comment about how these are different from the non-v8 ones. https://codereview.chromium.org/2161183003/diff/1/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2161183003/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:458: # //chrome/android/monochrome is only needed in two cases nit: comment says what the if statement says, but doesn't answer the question of why. It should say why it's needed in only these cases, and why it's worth putting behind an if()
PTAL https://codereview.chromium.org/2161183003/diff/1/build/config/android/config... File build/config/android/config.gni (right): https://codereview.chromium.org/2161183003/diff/1/build/config/android/config... build/config/android/config.gni:7: import("//build/config/android/secondary_abi.gni") On 2016/07/20 02:11:03, agrieve wrote: > Having this as a separate file doesn't really make sense to me if it's just > going to be imported by the main config.gni. I think either: > 1. Don't have it imported here, or > 2. Inline into the logic below. It was used in more than one place, moved into this file. https://codereview.chromium.org/2161183003/diff/1/build/toolchain/android/BUI... File build/toolchain/android/BUILD.gn (right): https://codereview.chromium.org/2161183003/diff/1/build/toolchain/android/BUI... build/toolchain/android/BUILD.gn:122: android_gcc_toolchains_arm_helper("arm_v8_arm") { On 2016/07/20 02:11:03, agrieve wrote: > It's worth a comment about how these are different from the non-v8 ones. Done. https://codereview.chromium.org/2161183003/diff/1/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2161183003/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:458: # //chrome/android/monochrome is only needed in two cases On 2016/07/20 02:11:03, agrieve wrote: > nit: comment says what the if statement says, but doesn't answer the question of > why. It should say why it's needed in only these cases, and why it's worth > putting behind an if() Refine the comment, hope it is clear now.
lgtm https://codereview.chromium.org/2161183003/diff/20001/build/config/android/co... File build/config/android/config.gni (right): https://codereview.chromium.org/2161183003/diff/20001/build/config/android/co... build/config/android/config.gni:312: android_secondary_abi_cpu = "arm" nit: can you add a comments saying why these are keyed off of target_cpu as opposed to android_app_secondary_abi, which is keyed off of current_cpu? https://codereview.chromium.org/2161183003/diff/20001/build/toolchain/android... File build/toolchain/android/BUILD.gn (right): https://codereview.chromium.org/2161183003/diff/20001/build/toolchain/android... build/toolchain/android/BUILD.gn:126: v8_toolchain_cpu = "arm" Just one more question the comment doesn't yet answer: Why is "arm" not already the default when current_cpu = arm? (should say what would happen if we didn't set this explicitly here)
https://codereview.chromium.org/2161183003/diff/20001/build/toolchain/android... File build/toolchain/android/BUILD.gn (right): https://codereview.chromium.org/2161183003/diff/20001/build/toolchain/android... build/toolchain/android/BUILD.gn:126: v8_toolchain_cpu = "arm" On 2016/07/21 16:57:46, agrieve wrote: > Just one more question the comment doesn't yet answer: Why is "arm" not already > the default when current_cpu = arm? (should say what would happen if we didn't > set this explicitly here) Right, this shouldn't be needed.
https://codereview.chromium.org/2161183003/diff/20001/build/toolchain/android... File build/toolchain/android/BUILD.gn (right): https://codereview.chromium.org/2161183003/diff/20001/build/toolchain/android... build/toolchain/android/BUILD.gn:126: v8_toolchain_cpu = "arm" On 2016/07/21 17:15:26, Dirk Pranke wrote: > On 2016/07/21 16:57:46, agrieve wrote: > > Just one more question the comment doesn't yet answer: Why is "arm" not > already > > the default when current_cpu = arm? (should say what would happen if we didn't > > set this explicitly here) > > Right, this shouldn't be needed. Dirk, could you clarify what shouldn't be needed?
https://codereview.chromium.org/2161183003/diff/20001/build/toolchain/android... File build/toolchain/android/BUILD.gn (right): https://codereview.chromium.org/2161183003/diff/20001/build/toolchain/android... build/toolchain/android/BUILD.gn:126: v8_toolchain_cpu = "arm" v8_toolchain_cpu will match toolchain_cpu by default, so you don't need to define these toolchains; they will have the same effect as the ones on lines 113-120.
PTAL https://codereview.chromium.org/2161183003/diff/20001/build/config/android/co... File build/config/android/config.gni (right): https://codereview.chromium.org/2161183003/diff/20001/build/config/android/co... build/config/android/config.gni:312: android_secondary_abi_cpu = "arm" On 2016/07/21 16:57:46, agrieve wrote: > nit: can you add a comments saying why these are keyed off of target_cpu as > opposed to android_app_secondary_abi, which is keyed off of current_cpu? I thought twice, it is actually better to move them here. https://codereview.chromium.org/2161183003/diff/20001/build/toolchain/android... File build/toolchain/android/BUILD.gn (right): https://codereview.chromium.org/2161183003/diff/20001/build/toolchain/android... build/toolchain/android/BUILD.gn:126: v8_toolchain_cpu = "arm" On 2016/07/21 18:31:56, Dirk Pranke wrote: > v8_toolchain_cpu will match toolchain_cpu by default, so you don't need to > define these toolchains; they will have the same effect as the ones on lines > 113-120. How could I miss that part! but I remember it didn't work at the first time I tried your v8 patch; the good news is this patch become very simple.
lgtm
michaelbai@chromium.org changed reviewers: + nyquist@chromium.org
nyquist@ need your owner approval for chrome/android
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...
chrome/android lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by michaelbai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2161183003/#ps40001 (title: "Remove the v8_foo toolchain & address comments")
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 libmonochrome.so with secondary toolchain This patch - added Android secondary abi toolchain. - built libmonochrome with secondary abi toolchain, the 64-bit build starts to building 64-bit webview only libmonochrome.so and 32-bit full libmonochrome.so. BUG=605315 ========== to ========== Build libmonochrome.so with secondary toolchain This patch - added Android secondary abi toolchain. - built libmonochrome with secondary abi toolchain, the 64-bit build starts to building 64-bit webview only libmonochrome.so and 32-bit full libmonochrome.so. BUG=605315 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Build libmonochrome.so with secondary toolchain This patch - added Android secondary abi toolchain. - built libmonochrome with secondary abi toolchain, the 64-bit build starts to building 64-bit webview only libmonochrome.so and 32-bit full libmonochrome.so. BUG=605315 ========== to ========== Build libmonochrome.so with secondary toolchain This patch - added Android secondary abi toolchain. - built libmonochrome with secondary abi toolchain, the 64-bit build starts to building 64-bit webview only libmonochrome.so and 32-bit full libmonochrome.so. BUG=605315 Committed: https://crrev.com/e56fa3c0528dc62a3d423ed2d0b83b5e039cb602 Cr-Commit-Position: refs/heads/master@{#406965} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e56fa3c0528dc62a3d423ed2d0b83b5e039cb602 Cr-Commit-Position: refs/heads/master@{#406965} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
