Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1430143002:
Install both 32 and 64 bit x86 sysroots when arm is the target_cpu (Closed)
DescriptionInstall both 32 and 64 bit x86 sysroots when arm is the target_cpu
This is required by Android since it builds both 64 and 32 bit v8
snapshots, which need to be built on the host by toolchains with the matching bit-width.
BUG=552040
Committed: https://crrev.com/79822596ae07da6b5b7c9a43d2c7bfffc3fa0d06
Cr-Commit-Position: refs/heads/master@{#359212}
Patch Set 1 #
Total comments: 2
Patch Set 2 : rejigger #Patch Set 3 : big rejigger #
Total comments: 1
Patch Set 4 : third time's a charm #
Total comments: 2
Patch Set 5 : update a comment #Messages
Total messages: 20 (6 generated)
Description was changed from ========== Install both 32 and 64 bit sysroots when x64 is the target This is required by Android since it builds both 64 and 32 bit v8 snapshots. BUG=552040 ========== to ========== Install both 32 and 64 bit sysroots when x64 is the target This is required by Android since it builds both 64 and 32 bit v8 snapshots. BUG=552040 ==========
agrieve@chromium.org changed reviewers: + dpranke@chromium.org
On 2015/11/05 20:27:16, agrieve wrote: > mailto:agrieve@chromium.org changed reviewers: > + mailto:dpranke@chromium.org ptal - didn't bother to restrict to OS=android since it's a fast download and is used only for release builds.
https://codereview.chromium.org/1430143002/diff/1/build/linux/sysroot_scripts... File build/linux/sysroot_scripts/install-sysroot.py (right): https://codereview.chromium.org/1430143002/diff/1/build/linux/sysroot_scripts... build/linux/sysroot_scripts/install-sysroot.py:136: if target_arch == 'amd64': I'm confused. I thought we established that you need both 32-bit and 64-bit x86 sysroots for *32-bit arm* target_arch builds? Shouldn't this be: if target_arch == 'arm': ret = _InstallSysroot('amd64') if ret: return ret ret = _InstallSysroot('i386') if ret: return ret return _InstallSysroot(target_arch) ?
https://codereview.chromium.org/1430143002/diff/1/build/linux/sysroot_scripts... File build/linux/sysroot_scripts/install-sysroot.py (right): https://codereview.chromium.org/1430143002/diff/1/build/linux/sysroot_scripts... build/linux/sysroot_scripts/install-sysroot.py:136: if target_arch == 'amd64': On 2015/11/05 22:49:38, Dirk Pranke wrote: > I'm confused. I thought we established that you need both 32-bit and 64-bit x86 > sysroots for *32-bit arm* target_arch builds? > > Shouldn't this be: > > if target_arch == 'arm': > ret = _InstallSysroot('amd64') > if ret: > return ret > ret = _InstallSysroot('i386') > if ret: > return ret > > return _InstallSysroot(target_arch) > > ? Ah, okay, the build instructions I was following didn't include setting target_cpu directly, and the script doesn't have a "if OS=android, then target_arch = arm", which is what GYP / GN do. So... you're right that is would have done the wrong thing if target_arch was explicitly set. I've changed it now to look for OS=android, and also consult chromium.gyp_env (which is where all of our build instructions assume you set OS=android). Verified you need only arm64 for 64 bit builds, and x86 for 32 bit builds. You don't actually need both.
https://codereview.chromium.org/1430143002/diff/2/build/linux/sysroot_scripts... File build/linux/sysroot_scripts/install-sysroot.py (right): https://codereview.chromium.org/1430143002/diff/2/build/linux/sysroot_scripts... build/linux/sysroot_scripts/install-sysroot.py:151: # Used for v8 snapshots. 1) don't you need to install *both* the 32-bit x86 sysroot and the arm sysroot? 2) didn't we agree that you need to install 3 sysroots if it's 32-bit arm, since the actual host toolchain is 64-bit x86 (i.e., you need default_toolchain, host_toolchain, snapshot_toolchain)?
On 2015/11/06 20:48:52, Dirk Pranke wrote: > https://codereview.chromium.org/1430143002/diff/2/build/linux/sysroot_scripts... > File build/linux/sysroot_scripts/install-sysroot.py (right): > > https://codereview.chromium.org/1430143002/diff/2/build/linux/sysroot_scripts... > build/linux/sysroot_scripts/install-sysroot.py:151: # Used for v8 snapshots. > 1) don't you need to install *both* the 32-bit x86 sysroot and the arm sysroot? Seems not. To build arm, you just need 32-bit. To build arm64, you just need 64 bit. > > 2) didn't we agree that you need to install 3 sysroots if it's 32-bit arm, since > the actual host > toolchain is 64-bit x86 (i.e., you need default_toolchain, host_toolchain, > snapshot_toolchain)? To compile for android, we always use a sysroot that comes with the ndk, so it's only host toolsets that we're concerned about.
On 2015/11/06 20:58:35, agrieve wrote: > On 2015/11/06 20:48:52, Dirk Pranke wrote: > > > https://codereview.chromium.org/1430143002/diff/2/build/linux/sysroot_scripts... > > File build/linux/sysroot_scripts/install-sysroot.py (right): > > > > > https://codereview.chromium.org/1430143002/diff/2/build/linux/sysroot_scripts... > > build/linux/sysroot_scripts/install-sysroot.py:151: # Used for v8 snapshots. > > 1) don't you need to install *both* the 32-bit x86 sysroot and the arm > sysroot? > Seems not. To build arm, you just need 32-bit. To build arm64, you just need 64 > bit. I think that means that you're just getting the non-sysroot host toolchain build, then, and I thought we had agreed to change that? > > 2) didn't we agree that you need to install 3 sysroots if it's 32-bit arm, > since > > the actual host > > toolchain is 64-bit x86 (i.e., you need default_toolchain, host_toolchain, > > snapshot_toolchain)? > To compile for android, we always use a sysroot that comes with the ndk, so it's > only host toolsets that we're concerned about. Ah, right. I got that confused w/ linux-arm where we still install the sysroot, I think. So, yeah, you have to install the 32-bit x86 toolchain for any android/arm32 build, and we should probably install the 64-bit x86 toolchain as well, at least for the official builds, though the risk of bugs there is probably quite low.
On 2015/11/06 21:29:27, Dirk Pranke wrote: > On 2015/11/06 20:58:35, agrieve wrote: > > On 2015/11/06 20:48:52, Dirk Pranke wrote: > > > > > > https://codereview.chromium.org/1430143002/diff/2/build/linux/sysroot_scripts... > > > File build/linux/sysroot_scripts/install-sysroot.py (right): > > > > > > > > > https://codereview.chromium.org/1430143002/diff/2/build/linux/sysroot_scripts... > > > build/linux/sysroot_scripts/install-sysroot.py:151: # Used for v8 snapshots. > > > 1) don't you need to install *both* the 32-bit x86 sysroot and the arm > > sysroot? > > Seems not. To build arm, you just need 32-bit. To build arm64, you just need > 64 > > bit. > > I think that means that you're just getting the non-sysroot host toolchain > build, then, > and I thought we had agreed to change that? Could be I didn't do a proper clean when I tried it. Just tried "ninja all" and it did indeed fail from not having a 64bit sysroot when target_cpu=arm Updated to install both for 32-bit targets. Third time's a charm? > > > > 2) didn't we agree that you need to install 3 sysroots if it's 32-bit arm, > > since > > > the actual host > > > toolchain is 64-bit x86 (i.e., you need default_toolchain, host_toolchain, > > > snapshot_toolchain)? > > To compile for android, we always use a sysroot that comes with the ndk, so > it's > > only host toolsets that we're concerned about. > > Ah, right. I got that confused w/ linux-arm where we still install the sysroot, > I think. > > So, yeah, you have to install the 32-bit x86 toolchain for any android/arm32 > build, > and we should probably install the 64-bit x86 toolchain as well, at least for > the official builds, though the risk of bugs there is probably quite low.
Description was changed from ========== Install both 32 and 64 bit sysroots when x64 is the target This is required by Android since it builds both 64 and 32 bit v8 snapshots. BUG=552040 ========== to ========== Install both 32 and 64 bit x86 sysroots when arm is the target_cpu This is required by Android since it builds both 64 and 32 bit v8 snapshots, which need to be built on the host by toolchains with the matching bit-width. BUG=552040 ==========
lgtm. https://codereview.chromium.org/1430143002/diff/50001/build/linux/sysroot_scr... File build/linux/sysroot_scripts/install-sysroot.py (right): https://codereview.chromium.org/1430143002/diff/50001/build/linux/sysroot_scr... build/linux/sysroot_scripts/install-sysroot.py:112: if not is_android and target_arch in ('arm', 'mips', 'i386'): maybe update the comment to "cross-compiling non-android builds"
https://codereview.chromium.org/1430143002/diff/50001/build/linux/sysroot_scr... File build/linux/sysroot_scripts/install-sysroot.py (right): https://codereview.chromium.org/1430143002/diff/50001/build/linux/sysroot_scr... build/linux/sysroot_scripts/install-sysroot.py:112: if not is_android and target_arch in ('arm', 'mips', 'i386'): On 2015/11/11 22:27:18, Dirk Pranke wrote: > maybe update the comment to "cross-compiling non-android builds" Done.
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/1430143002/#ps70001 (title: "update a comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1430143002/70001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1430143002/70001
Message was sent while issue was closed.
Committed patchset #5 (id:70001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/79822596ae07da6b5b7c9a43d2c7bfffc3fa0d06 Cr-Commit-Position: refs/heads/master@{#359212}
Message was sent while issue was closed.
sbc@chromium.org changed reviewers: + sbc@chromium.org
Message was sent while issue was closed.
I'm a little confused by this change. It seems there is an early out that means that sysroot are never installed when OS=android, which means they never get installed on the bot, right? Also, the only code I can find that uses these sysroots only does so when OS=linux (build/config/sysroot.gni build/common.gypi). So, correct me if I'm wrong, but it looks like that android builds never use the sysroot.
Message was sent while issue was closed.
On 2015/11/24 02:15:12, Sam Clegg wrote: > I'm a little confused by this change. > > It seems there is an early out that means that sysroot are never installed when > OS=android, which means they never get installed on the bot, right? > > Also, the only code I can find that uses these sysroots only does so when > OS=linux (build/config/sysroot.gni build/common.gypi). > > So, correct me if I'm wrong, but it looks like that android builds never use the > sysroot. That is correct, we don't want to use the sysroot for target builds when the target is android, because we pull in the equivalent of the sysroot from the android ndk. However, we do want to install the host 64-bit and 32-bit sysroots when building for an android target. I need to update the other CL; I lgtm'ed that incorrectly. |
