Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2823303003:
base: remove -latomic in CrOS chroot (Closed)
|
Created:
3 years, 8 months ago by richard.townsend Modified:
3 years, 7 months ago CC:
chromium-reviews, cmtice, danakj+watch_chromium.org, vmpstr+watch_chromium.org, martijnc, lgarron Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionbase: remove -latomic in CrOS chroot
libbase is needed when using compiled_action targets (these are
helper programs compiled for use later in the build), but it doesn't
currently build for the host inside the CrOS sysroot because the
toolchain doesn't include the libatomic library. libbase does, however,
seem to compile without the -latomic flag.
TEST=Revert 5e671fffc45441c5dd3458871a8aa8f4a3f2e632, compile elm
BUG=710006, 710841
Review-Url: https://codereview.chromium.org/2823303003
Cr-Commit-Position: refs/heads/master@{#467721}
Committed: https://chromium.googlesource.com/chromium/src/+/2b156ccc00ebdb53389dc59917f60cf1d1dde6ac
Patch Set 1 #
Total comments: 4
Patch Set 2 : More robust check for CrOS environment #
Total comments: 1
Patch Set 3 : Fold if statement to outer level, improve inline documentation #Messages
Total messages: 45 (26 generated)
The CQ bit was checked by cavalcantii@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 ========== base: remove -latomic in CrOS chroot libbase is needed when using compiled_action targets (these are helper programs compiled for use later in the build), but it doesn't currently build for the host inside the CrOS sysroot because the toolchain doesn't include the libatomic library. libbase does, however, seem to compile without the -latomic flag. TEST=Revert 5e671fffc45441c5dd3458871a8aa8f4a3f2e632, compile elm BUG=710841 ========== to ========== base: remove -latomic in CrOS chroot libbase is needed when using compiled_action targets (these are helper programs compiled for use later in the build), but it doesn't currently build for the host inside the CrOS sysroot because the toolchain doesn't include the libatomic library. libbase does, however, seem to compile without the -latomic flag. TEST=Revert 5e671fffc45441c5dd3458871a8aa8f4a3f2e632, compile elm BUG=710006 ==========
richard.townsend@arm.com changed reviewers: + gurchetansingh@chromium.org, lgarron@chromium.org
This change is ready for review and addresses the root cause of http://crbug.com/710006 by removing -latomic from targets involving libbase built on CrOS. The advantage of doing it this way is that it's no longer necessary to modify the CrOS toolchain. It currently only affects binaries built on the host as part of the compilation process, so the behaviour is only exposed if efa139d3af68589d5b803a8a322f41d83e56af06 is reverted (or the CL in https://bugs.chromium.org/p/chromium/issues/detail?id=595493 is re-landed).
lgarron@chromium.org changed reviewers: - lgarron@chromium.org
Thanks! Could we also add 710841 to BUG=?
https://codereview.chromium.org/2823303003/diff/1/base/BUILD.gn File base/BUILD.gn (left): https://codereview.chromium.org/2823303003/diff/1/base/BUILD.gn#oldcode1131 base/BUILD.gn:1131: if (!use_sysroot && (is_android || (is_linux && !is_chromecast))) { You can simplify with an !is_chromeos check here.
Description was changed from ========== base: remove -latomic in CrOS chroot libbase is needed when using compiled_action targets (these are helper programs compiled for use later in the build), but it doesn't currently build for the host inside the CrOS sysroot because the toolchain doesn't include the libatomic library. libbase does, however, seem to compile without the -latomic flag. TEST=Revert 5e671fffc45441c5dd3458871a8aa8f4a3f2e632, compile elm BUG=710006 ========== to ========== base: remove -latomic in CrOS chroot libbase is needed when using compiled_action targets (these are helper programs compiled for use later in the build), but it doesn't currently build for the host inside the CrOS sysroot because the toolchain doesn't include the libatomic library. libbase does, however, seem to compile without the -latomic flag. TEST=Revert 5e671fffc45441c5dd3458871a8aa8f4a3f2e632, compile elm BUG=710006, 710841 ==========
https://codereview.chromium.org/2823303003/diff/1/base/BUILD.gn File base/BUILD.gn (left): https://codereview.chromium.org/2823303003/diff/1/base/BUILD.gn#oldcode1131 base/BUILD.gn:1131: if (!use_sysroot && (is_android || (is_linux && !is_chromecast))) { On 2017/04/20 21:43:13, gurchetansingh wrote: > You can simplify with an !is_chromeos check here. That's what I thought, but surprisingly it doesn't work. Adding a quick print(target_os, is_chromeos, is_linux, host_toolchain, current_toolchain) before the comment yields: chromeos true true //build/toolchain/cros:host //build/toolchain/cros:target chromeos false true //build/toolchain/cros:host //build/toolchain/cros:host chromeos false false //build/toolchain/cros:host //build/toolchain/nacl:newlib_pnacl_nonsfi chromeos false false //build/toolchain/cros:host //build/toolchain/nacl:irt_x64 chromeos false false //build/toolchain/cros:host //build/toolchain/nacl:newlib_pnacl So when building tools for the host with the compiled_action target, is_chromeos is false, which means the error doesn't go away.
On 2017/04/21 11:31:52, richard.townsend wrote: > https://codereview.chromium.org/2823303003/diff/1/base/BUILD.gn > File base/BUILD.gn (left): > > https://codereview.chromium.org/2823303003/diff/1/base/BUILD.gn#oldcode1131 > base/BUILD.gn:1131: if (!use_sysroot && (is_android || (is_linux && > !is_chromecast))) { > On 2017/04/20 21:43:13, gurchetansingh wrote: > > You can simplify with an !is_chromeos check here. > > That's what I thought, but surprisingly it doesn't work. Adding a quick > print(target_os, is_chromeos, is_linux, host_toolchain, current_toolchain) > before the comment yields: > > chromeos true true //build/toolchain/cros:host //build/toolchain/cros:target > chromeos false true //build/toolchain/cros:host //build/toolchain/cros:host > chromeos false false //build/toolchain/cros:host > //build/toolchain/nacl:newlib_pnacl_nonsfi > chromeos false false //build/toolchain/cros:host > //build/toolchain/nacl:irt_x64 > chromeos false false //build/toolchain/cros:host > //build/toolchain/nacl:newlib_pnacl > > So when building tools for the host with the compiled_action target, is_chromeos > is false, which means the error doesn't go away. lgtm
The CQ bit was checked by richard.townsend@arm.com
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
As all bots are green and the patch was lgtm ed, I'm adding it to the CQ.
The CQ bit was checked by cavalcantii@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
https://codereview.chromium.org/2823303003/diff/1/base/BUILD.gn File base/BUILD.gn (left): https://codereview.chromium.org/2823303003/diff/1/base/BUILD.gn#oldcode1131 base/BUILD.gn:1131: if (!use_sysroot && (is_android || (is_linux && !is_chromecast))) { On 2017/04/21 11:31:52, richard.townsend wrote: > On 2017/04/20 21:43:13, gurchetansingh wrote: > > You can simplify with an !is_chromeos check here. > > That's what I thought, but surprisingly it doesn't work. Adding a quick > print(target_os, is_chromeos, is_linux, host_toolchain, current_toolchain) > before the comment yields: > > chromeos true true //build/toolchain/cros:host //build/toolchain/cros:target > chromeos false true //build/toolchain/cros:host //build/toolchain/cros:host > chromeos false false //build/toolchain/cros:host > //build/toolchain/nacl:newlib_pnacl_nonsfi > chromeos false false //build/toolchain/cros:host > //build/toolchain/nacl:irt_x64 > chromeos false false //build/toolchain/cros:host > //build/toolchain/nacl:newlib_pnacl > > So when building tools for the host with the compiled_action target, is_chromeos > is false, which means the error doesn't go away. If I understand this correctly, basically the problem is that the cros build is "like" a sysroot build but isn't a sysroot build, in the sense that the chroot/ebuild also doesn't have a newer c++ library and so doesn't need -latomic. Is that right? Unfortunately, checking just for target_os != 'chromeos' won't work, because we also will want -latomic for the cases where we're building the "desktop chromeos" config, like on the https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%... builder. I think instead you should change this to look at the current_toolchain and compare it to '//build/toolschain/cros:target' or '//build/toolchain/cros:host' and add a comment similar to what I wrote in the first paragraph. I think that might make it a bit clearer what's going on. Checking against the toolchain is the "easiest" way of telling if we're in an ebuild or simplechrome build that we have at the moment.
I haven't yet updated the patch because it's looking less and less likely that the flag is needed under any circumstances. https://codereview.chromium.org/2823303003/diff/1/base/BUILD.gn File base/BUILD.gn (left): https://codereview.chromium.org/2823303003/diff/1/base/BUILD.gn#oldcode1131 base/BUILD.gn:1131: if (!use_sysroot && (is_android || (is_linux && !is_chromecast))) { On 2017/04/24 21:12:22, Dirk Pranke wrote: > On 2017/04/21 11:31:52, richard.townsend wrote: > > On 2017/04/20 21:43:13, gurchetansingh wrote: > > > You can simplify with an !is_chromeos check here. > > > > That's what I thought, but surprisingly it doesn't work. Adding a quick > > print(target_os, is_chromeos, is_linux, host_toolchain, current_toolchain) > > before the comment yields: > > > > chromeos true true //build/toolchain/cros:host //build/toolchain/cros:target > > chromeos false true //build/toolchain/cros:host //build/toolchain/cros:host > > chromeos false false //build/toolchain/cros:host > > //build/toolchain/nacl:newlib_pnacl_nonsfi > > chromeos false false //build/toolchain/cros:host > > //build/toolchain/nacl:irt_x64 > > chromeos false false //build/toolchain/cros:host > > //build/toolchain/nacl:newlib_pnacl > > > > So when building tools for the host with the compiled_action target, > is_chromeos > > is false, which means the error doesn't go away. > > If I understand this correctly, basically the problem is that the cros build is > "like" a sysroot build but isn't a sysroot build, in the sense that the > chroot/ebuild also doesn't have a newer c++ library and so doesn't need > -latomic. Is that right? > > Unfortunately, checking just for target_os != 'chromeos' won't work, because we > also will want -latomic for the cases where we're building the "desktop > chromeos" config, like on the > https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%... > builder. > > I think instead you should change this to look at the current_toolchain and > compare it to '//build/toolschain/cros:target' or '//build/toolchain/cros:host' > and add a comment similar to what I wrote in the first paragraph. I think that > might make it a bit clearer what's going on. > Checking against the toolchain is the "easiest" way of telling if we're in an > ebuild or simplechrome build that we have at the moment. > I did some further investigation on the native Linux/ChromeOS configuration on my Ubuntu 14.04 and 16.04 systems, just building the affected host tool (i.e. revert 5e671fffc45441c5dd3458871a8aa8f4a3f2e632, 'ninja -C out/ChromeOS net/http:generate_transport_security_state'), with these gn args: is_component_build = false is_debug = false target_os = "chromeos" Here's a summary of what I found. * Outside CrOS sysroot, target_os="chromeos", use_sysroot=true, prior to patch = no -latomic, compiles * Outside CrOS sysroot, target_os="chromeos", use_sysroot=true, with patch = no -latomic, compiles * Outside CrOS sysroot, target_os="chromeos", use_sysroot=false, prior to patch = -latomic, compiles * Outside CrOS sysroot, target_os="chromeos", use_sysroot=false, with patch = no -latomic, compiles * Inside CrOS sysroot, without patch, use_sysroot=false = -latomic, errors * Inside CrOS sysroot, without patch, use_sysroot=true = no -latomic, compiles * Inside CrOS sysroot, with patch , use_sysroot=false = no -latomic, compiles * Inside CrOS sysroot, with patch , use_sysroot=true = no -latomic, compiles The fact that you don't need '-latomic' to compile on the native system is surprising to me. Reading through the original commit at http://crrev.com/f3f51116a0689, it seems the author had problems compiling with GCC 5.x, but a quick gn arg is_clang=false on my Ubuntu 16.04 system (with GCC 5.4), again, compiles fine, with and without -latomic. My only remaining thought is that there must have been something wrong with the committer's environment: perhaps a custom-compiled GCC. So to summarize my summary, it shouldn't break this particular job, but the investigation throws further doubt on why this flag is needed.
The CQ bit was checked by cavalcantii@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.
Patchset #2 is ready for review and improves the check used to determine if Chromium's being built inside the cros_sdk environment by checking the toolchain used to compile host-only tools. Host-only tools are the only cases in CrOS where this behaviour occurs, because when building for the target board, use_sysroot is true. This change should also result in identical behaviour when compiling with target_os="chromeos" on regular desktop Linux builds.
richard.townsend@arm.com changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/2823303003/diff/20001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2823303003/diff/20001/base/BUILD.gn#newcode1137 base/BUILD.gn:1137: if (host_toolchain != "//build/toolchain/cros:host") { I'd fold the comment and the check into the outer if condition. Having an if contain a single additional if reads a little weird. Otherwise lgtm.
The CQ bit was checked by cavalcantii@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.
Patchset #3 is ready for review. It folds the condition into the outer if statement and clarifies why the check is necessary in the in-line documentation.
LGTM
still lgtm.
The CQ bit was checked by richard.townsend@arm.com
The patchset sent to the CQ was uploaded after l-g-t-m from cavalcantii@chromium.org, gurchetansingh@chromium.org Link to the patchset: https://codereview.chromium.org/2823303003/#ps40001 (title: "Fold if statement to outer level, improve inline documentation")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493313519896570,
"parent_rev": "ee08663ae2f6240b382249c849f5c42012143632", "commit_rev":
"2b156ccc00ebdb53389dc59917f60cf1d1dde6ac"}
Message was sent while issue was closed.
Description was changed from ========== base: remove -latomic in CrOS chroot libbase is needed when using compiled_action targets (these are helper programs compiled for use later in the build), but it doesn't currently build for the host inside the CrOS sysroot because the toolchain doesn't include the libatomic library. libbase does, however, seem to compile without the -latomic flag. TEST=Revert 5e671fffc45441c5dd3458871a8aa8f4a3f2e632, compile elm BUG=710006, 710841 ========== to ========== base: remove -latomic in CrOS chroot libbase is needed when using compiled_action targets (these are helper programs compiled for use later in the build), but it doesn't currently build for the host inside the CrOS sysroot because the toolchain doesn't include the libatomic library. libbase does, however, seem to compile without the -latomic flag. TEST=Revert 5e671fffc45441c5dd3458871a8aa8f4a3f2e632, compile elm BUG=710006, 710841 Review-Url: https://codereview.chromium.org/2823303003 Cr-Commit-Position: refs/heads/master@{#467721} Committed: https://chromium.googlesource.com/chromium/src/+/2b156ccc00ebdb53389dc59917f6... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2b156ccc00ebdb53389dc59917f6... |
