Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2036263002:
💠Make Android toolchains work when current_toolchain != target_toolchain (Closed)
DescriptionMake Android toolchains work when current_toolchain != target_toolchain
"android_tool_prefix" within the toolchain definitions was always being
keyed off of target_cpu rather than the toolchain cpu.
BUG=616819
Committed: https://crrev.com/d13c10da70c170ddbdb044ef752f98a870915eee
Cr-Commit-Position: refs/heads/master@{#398463}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : aarch64->arm64 #
Total comments: 2
Patch Set 4 : "$var" -> var #Messages
Total messages: 27 (12 generated)
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Description was changed from ========== Make Android toolchains work when current_toolchain != target_toolchain "android_tool_prefix" within the toolchain definitions was always being keyed off of target_cpu rather than the toolchain cpu. BUG=616819 ========== to ========== Make Android toolchains work when current_toolchain != target_toolchain "android_tool_prefix" within the toolchain definitions was always being keyed off of target_cpu rather than the toolchain cpu. BUG=616819 ==========
agrieve@chromium.org changed reviewers: + dpranke@google.com
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2036263002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2036263002/20001
On 2016/06/03 15:48:59, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/2036263002/20001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/2036263002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
michaelbai@chromium.org changed reviewers: + michaelbai@chromium.org
https://codereview.chromium.org/2036263002/diff/20001/build/toolchain/android... File build/toolchain/android/BUILD.gn (right): https://codereview.chromium.org/2036263002/diff/20001/build/toolchain/android... build/toolchain/android/BUILD.gn:109: toolchain_cpu = "aarch64" toolchain_cpu is set to current_cpu in gcc_toolchain template, it will take effect if current toolchain is not default one. see https://cs.chromium.org/chromium/src/build/toolchain/gcc_toolchain.gni?rcl=0&... You may see current_cpu is "aarch64" if you set arm as default, and arm64 as secondary. Of course, this issue is not caused by your patch, I wonder if you could fix it in your patchr?
https://codereview.chromium.org/2036263002/diff/20001/build/toolchain/android... File build/toolchain/android/BUILD.gn (right): https://codereview.chromium.org/2036263002/diff/20001/build/toolchain/android... build/toolchain/android/BUILD.gn:109: toolchain_cpu = "aarch64" On 2016/06/03 17:52:35, michaelbai wrote: > toolchain_cpu is set to current_cpu in gcc_toolchain template, it will take > effect if current toolchain is not default one. see > > https://cs.chromium.org/chromium/src/build/toolchain/gcc_toolchain.gni?rcl=0&... > > You may see current_cpu is "aarch64" if you set arm as default, and arm64 as > secondary. > > Of course, this issue is not caused by your patch, I wonder if you could fix it > in your patchr? aah, I gotcha. Fixed!
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
This CL looks somewhat non-trivial, so I don't want to review it in a rush; I'll try to get to it later today.
On 2016/06/03 18:29:17, Dirk Pranke wrote: > This CL looks somewhat non-trivial, so I don't want to review it in a rush; I'll > try to get to it later today. Sounds good. Certainly if you see of a way to reduce duplication for android_tool_prefix (config.gni vs toolchain's build.gn), I couldn't figure out how. Since the toolchain's toolchain_args() don't take effect for the default toolchain, I couldn't see how to not just duplicate the logic.
Sorry, I still haven't had the time to look at this. I'll try hard to get to it tomorrow morning, though.
lgtm. It might be possible to reduce the duplication more, but I doubt it's worth it. https://codereview.chromium.org/2036263002/diff/40001/build/toolchain/android... File build/toolchain/android/BUILD.gn (right): https://codereview.chromium.org/2036263002/diff/40001/build/toolchain/android... build/toolchain/android/BUILD.gn:78: toolchain_root = "$x86_android_toolchain_root" Is there a reason to use a quoted interpolated variable here, rather than just: toolchain_root = x86_android_toolchain_root ?
https://codereview.chromium.org/2036263002/diff/40001/build/toolchain/android... File build/toolchain/android/BUILD.gn (right): https://codereview.chromium.org/2036263002/diff/40001/build/toolchain/android... build/toolchain/android/BUILD.gn:78: toolchain_root = "$x86_android_toolchain_root" On 2016/06/07 23:44:13, Dirk Pranke wrote: > Is there a reason to use a quoted interpolated variable here, rather than just: > > toolchain_root = x86_android_toolchain_root > > ? heh, copy & pasted from build/config/android/config.gni, where it does that as well. Fixed it up here.
The CQ bit was checked by agrieve@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/2036263002/#ps60001 (title: ""$var" -> var")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2036263002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2036263002/60001
Message was sent while issue was closed.
Description was changed from ========== Make Android toolchains work when current_toolchain != target_toolchain "android_tool_prefix" within the toolchain definitions was always being keyed off of target_cpu rather than the toolchain cpu. BUG=616819 ========== to ========== Make Android toolchains work when current_toolchain != target_toolchain "android_tool_prefix" within the toolchain definitions was always being keyed off of target_cpu rather than the toolchain cpu. BUG=616819 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Make Android toolchains work when current_toolchain != target_toolchain "android_tool_prefix" within the toolchain definitions was always being keyed off of target_cpu rather than the toolchain cpu. BUG=616819 ========== to ========== Make Android toolchains work when current_toolchain != target_toolchain "android_tool_prefix" within the toolchain definitions was always being keyed off of target_cpu rather than the toolchain cpu. BUG=616819 Committed: https://crrev.com/d13c10da70c170ddbdb044ef752f98a870915eee Cr-Commit-Position: refs/heads/master@{#398463} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d13c10da70c170ddbdb044ef752f98a870915eee Cr-Commit-Position: refs/heads/master@{#398463} |
