Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2085683004:
Update the nacl_bootstrap GN code to work in ChromeOS chroot builds. (Closed)
|
Created:
4 years, 6 months ago by Dirk Pranke Modified:
4 years, 5 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/src/native_client.git@master Target Ref:
refs/heads/master Project:
nacl Visibility:
Public. |
DescriptionUpdate the nacl_bootstrap GN code to work in ChromeOS chroot builds.
In ChromeOS chroot builds, they use their own compiler toolchain
and set a bunch of target flags, and the prior version of the
nacl_bootstrap_* toolchains weren't picking them up properly.
This CL switches to use the new //build/toolchain/cros:nacl_bootstrap
toolchain that mimics the //build/toolchain/cros:target toolchain and uses
all of the same settings except that it forces off use_debug_fission
and use_gold.
R=mcgrathr@chromium.org, llozano@chromium.org, bradnelson@chromium.org
BUG=621686
Committed: https://chromium.googlesource.com/native_client/src/native_client/+/0949e1bef9d6b25ee44eb69a54e0cc6f8a677375
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix variable declaration #Messages
Total messages: 17 (8 generated)
This patch depends on the //build changes in https://codereview.chromium.org/2083063003/ https://codereview.chromium.org/2085683004/diff/1/src/trusted/service_runtime... File src/trusted/service_runtime/linux/BUILD.gn (left): https://codereview.chromium.org/2085683004/diff/1/src/trusted/service_runtime... src/trusted/service_runtime/linux/BUILD.gn:53: if (target_os == "chromeos" && defined(cros_target_cxx) && This shouldn't have really worked before, since cros_target_cxx wasn't technically in scope. Now, we do pull in the right cros_target_cxx value, but that's always non-empty, so we can't check it to see if we need to use a custom value. Instead, we duplicate the logic in BUILDCONFIG.gn, and end up getting the same "g++" value, since that is the default either way.
Description was changed from ========== Update the nacl_boostrap GN code to work in ChromeOS chroot builds. In ChromeOS chroot builds, they use their own compiler toolchain and set a bunch of target flags, and the prior version of the nacl_bootstrap_* toolchains weren't picking them up properly. This CL switches to use the new //build/toolchain/cros:nacl_bootstrap toolchain that mimics the //build/toolchain/cros:target toolchain and uses all of the same settings except that it forces off use_debug_fission and use_gold. R=mcgrathr@chromium.org, llozano@chromium.org BUG=621686 ========== to ========== Update the nacl_boostrap GN code to work in ChromeOS chroot builds. In ChromeOS chroot builds, they use their own compiler toolchain and set a bunch of target flags, and the prior version of the nacl_bootstrap_* toolchains weren't picking them up properly. This CL switches to use the new //build/toolchain/cros:nacl_bootstrap toolchain that mimics the //build/toolchain/cros:target toolchain and uses all of the same settings except that it forces off use_debug_fission and use_gold. R=mcgrathr@chromium.org, llozano@chromium.org, bradnelson@chromium.org BUG=621686 ==========
dpranke@chromium.org changed reviewers: + bradnelson@chromium.org
lgtm
Description was changed from ========== Update the nacl_boostrap GN code to work in ChromeOS chroot builds. In ChromeOS chroot builds, they use their own compiler toolchain and set a bunch of target flags, and the prior version of the nacl_bootstrap_* toolchains weren't picking them up properly. This CL switches to use the new //build/toolchain/cros:nacl_bootstrap toolchain that mimics the //build/toolchain/cros:target toolchain and uses all of the same settings except that it forces off use_debug_fission and use_gold. R=mcgrathr@chromium.org, llozano@chromium.org, bradnelson@chromium.org BUG=621686 ========== to ========== Update the nacl_bootstrap GN code to work in ChromeOS chroot builds. In ChromeOS chroot builds, they use their own compiler toolchain and set a bunch of target flags, and the prior version of the nacl_bootstrap_* toolchains weren't picking them up properly. This CL switches to use the new //build/toolchain/cros:nacl_bootstrap toolchain that mimics the //build/toolchain/cros:target toolchain and uses all of the same settings except that it forces off use_debug_fission and use_gold. R=mcgrathr@chromium.org, llozano@chromium.org, bradnelson@chromium.org BUG=621686 ==========
https://codereview.chromium.org/2085683004/diff/1/src/trusted/service_runtime... File src/trusted/service_runtime/linux/BUILD.gn (right): https://codereview.chromium.org/2085683004/diff/1/src/trusted/service_runtime... src/trusted/service_runtime/linux/BUILD.gn:20: # which is the same way we check in //build/config/BUILDCONFIG.gn. If the same check is done in BUILDCONFIG.gn already, then why not have that set the variable and just use it here? Or else, put the magic into cros_toolchain.gni so here we can just import that and test variables it sets. It seems pretty nasty to have fiddly logic like this here rather than somewhere in build/.
On 2016/06/22 19:29:48, Roland McGrath wrote: > https://codereview.chromium.org/2085683004/diff/1/src/trusted/service_runtime... > File src/trusted/service_runtime/linux/BUILD.gn (right): > > https://codereview.chromium.org/2085683004/diff/1/src/trusted/service_runtime... > src/trusted/service_runtime/linux/BUILD.gn:20: # which is the same way we check > in //build/config/BUILDCONFIG.gn. > If the same check is done in BUILDCONFIG.gn already, then why not have that set > the variable and just use it here? > > Or else, put the magic into cros_toolchain.gni so here we can just import that > and test variables it sets. I don't think we can (or at least we don't want to) import files into BUILDCONFIG.gn, so I don't think we can easily share things. There is a TODO to clean up all of the cros toolchain logic so I think I'll get this cleaned up as part of that fairly soon. > It seems pretty nasty to have fiddly logic like this here rather than somewhere > in build/. Yup, agreed.
The CQ bit was checked by dpranke@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.
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bradnelson@chromium.org Link to the patchset: https://codereview.chromium.org/2085683004/#ps20001 (title: "fix variable declaration")
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 ========== Update the nacl_bootstrap GN code to work in ChromeOS chroot builds. In ChromeOS chroot builds, they use their own compiler toolchain and set a bunch of target flags, and the prior version of the nacl_bootstrap_* toolchains weren't picking them up properly. This CL switches to use the new //build/toolchain/cros:nacl_bootstrap toolchain that mimics the //build/toolchain/cros:target toolchain and uses all of the same settings except that it forces off use_debug_fission and use_gold. R=mcgrathr@chromium.org, llozano@chromium.org, bradnelson@chromium.org BUG=621686 ========== to ========== Update the nacl_bootstrap GN code to work in ChromeOS chroot builds. In ChromeOS chroot builds, they use their own compiler toolchain and set a bunch of target flags, and the prior version of the nacl_bootstrap_* toolchains weren't picking them up properly. This CL switches to use the new //build/toolchain/cros:nacl_bootstrap toolchain that mimics the //build/toolchain/cros:target toolchain and uses all of the same settings except that it forces off use_debug_fission and use_gold. R=mcgrathr@chromium.org, llozano@chromium.org, bradnelson@chromium.org BUG=621686 Committed: https://chromium.googlesource.com/native_client/src/native_client/+/0949e1bef... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/native_client/src/native_client/+/0949e1bef...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2188303002/ by dpranke@chromium.org. The reason for reverting is: It looks like this breaks CrOS ARM builds; see crbug.com/629593 .. |
