Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1983613002:
Rework the way ChromiumOS toolchains will work in GN. (Closed)
|
Created:
4 years, 7 months ago by Dirk Pranke Modified:
4 years, 7 months ago CC:
chromium-reviews, Nico, scottmg Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRework the way ChromiumOS toolchains will work in GN.
This CL reworks the way ChromiumOS toolchains work in GN, in such a way
that they might actually have all the flags they need for the
ChromiumOS ebuild files to be able to set all of the flags it needs
(though there are still some missing GN build_args).
Specifically, the ebuild will now need to set the following in args.gn:
host_toolchain = "//build/toolchain/cros:host"
v8_snapshot_toolchain = "//build/toolchain/cros:v8_snapshot"
in order to support boards other than the amd64-generic build. The
ebuild should actually set these variables all the time; it just
happens that the amd64-generic build will work at the moment without
the variables, but that will not be guaranteed to remain true in the future.
This CL also adds the following optional build args that do pretty
much what you'd expect them to do:
cros_target_ld, cros_target_extra_cflags, cros_target_extra_cppflags,
cros_target_extra_cxx_flags, cros_target_extra_ldflags,
cros_host_ar, cros_host_cc, cros_host_cxx, cros_host_ld,
cros_host_is_clang, cros_host_extra_cflags, cros_host_extra_cppflags,
cros_host_extra_cxx_flags, cros_host_extra_ldflags,
cros_v8_snapshot_ar, cros_v8_snapshot_cc, cros_v8_snapshot_cxx,
cros_v8_snapshot_ld, cros_v8_snapshot_extra_cflags,
cros_v8_snapshot_extra_cppflags, cros_v8_snapshot_extra_cxx_flags,
cros_v8_snapshot_extra_ldflags
This CL should be backwards-compatible with the existing linux desktop
ChromiumOS builds and the amd64-generic simplechrome/ebuild (i.e., it
can be landed and reverted w/o requiring any other changes to be made).
It is a big hammer intended to un-block the ChromiumOS GN migration
while we continue thinking about how to best support ChromiumOS.
R=stevenjb@chromium.org, brettw@chromium.org
BUG=608596, 595653
Committed: https://crrev.com/8ad2f49335feaddddbd3f318fc6f4d13eb52760b
Cr-Commit-Position: refs/heads/master@{#394534}
Patch Set 1 #
Total comments: 11
Patch Set 2 : update w/ review comments #Patch Set 3 : remove unneeded TODO #
Total comments: 2
Messages
Total messages: 24 (8 generated)
I need to think about how to land both https://codereview.chromium.org/1979883002/ and this CL so that nothing breaks, but apart from that, I think this is more-or-less what we want.
On 2016/05/15 01:37:29, Dirk Pranke wrote: > I need to think about how to land both > https://codereview.chromium.org/1979883002/ and this CL so that nothing breaks, > but apart from that, I think this is more-or-less what we want. Okay, having thought about it more, I'm fairly sure that if we land this CL before we land the v8 CL, things will not regress.
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/patch-status/1983613002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1983613002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
stevenjb@chromium.org changed reviewers: + hashimoto@chromium.org, ihf@chromium.org, llozano@chromium.org
I'm really not the right person to review this. I don't see any obvious problems here. I will try to test this today. +others to also take a look since Brett is OOO.
I tested this, including with https://codereview.chromium.org/1979883002/, and everything seems to work great! LGTM
I am sorry, I looked at it, but I don't know enough.
https://codereview.chromium.org/1983613002/diff/1/build/toolchain/cros/BUILD.gn File build/toolchain/cros/BUILD.gn (right): https://codereview.chromium.org/1983613002/diff/1/build/toolchain/cros/BUILD.... build/toolchain/cros/BUILD.gn:84: # build/toolchain/cros:target in BUILDCONFIG.gn How are we going to use this toolchain after removing the references to cros:target and cros:clang_target from BUILDCONFIG.gn (IIUC it's going to use linux:target and linux:clang_target even when target_os=="chromeos")? By specifying custom_toolchain? https://codereview.chromium.org/1983613002/diff/1/build/toolchain/cros/BUILD.... build/toolchain/cros/BUILD.gn:108: ar = cros_host_ar nit: Please make the order consistent ("cc cxx ar ld" or "ar cc cxx ld") among all three toolchains. https://codereview.chromium.org/1983613002/diff/1/build/toolchain/gcc_toolcha... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/1983613002/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:39: # - extra_cppflags nit: It'd be nice to make it clear the difference between this and extra_cxxflags. People may think cpp stands for c-plus-plus. https://codereview.chromium.org/1983613002/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:42: # Extra flags to be appended when compiling C or C++ files nit: Not when compiling C? https://codereview.chromium.org/1983613002/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:465: use_sysroot = invoker.use_sysroot Sorry, I'm not familiar enough with GN to understand this code. Does this mean the global use_sysroot (i.e. the one in args.gn) is hidden by this local use_sysroot when using this toolchain?
https://codereview.chromium.org/1983613002/diff/1/build/toolchain/cros/BUILD.gn File build/toolchain/cros/BUILD.gn (right): https://codereview.chromium.org/1983613002/diff/1/build/toolchain/cros/BUILD.... build/toolchain/cros/BUILD.gn:108: ar = cros_host_ar On 2016/05/17 06:26:31, hashimoto wrote: > nit: Please make the order consistent ("cc cxx ar ld" or "ar cc cxx ld") among > all three toolchains. Yup, that's unintentionally different. Will fix. https://codereview.chromium.org/1983613002/diff/1/build/toolchain/gcc_toolcha... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/1983613002/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:39: # - extra_cppflags On 2016/05/17 06:26:31, hashimoto wrote: > nit: It'd be nice to make it clear the difference between this and > extra_cxxflags. People may think cpp stands for c-plus-plus. Will add comments. https://codereview.chromium.org/1983613002/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:42: # Extra flags to be appended when compiling C or C++ files On 2016/05/17 06:26:31, hashimoto wrote: > nit: Not when compiling C? Whoops. Will fix. https://codereview.chromium.org/1983613002/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:465: use_sysroot = invoker.use_sysroot On 2016/05/17 06:26:31, hashimoto wrote: > Sorry, I'm not familiar enough with GN to understand this code. > Does this mean the global use_sysroot (i.e. the one in args.gn) is hidden by > this local use_sysroot when using this toolchain? Yes, exactly. See https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen... (`gn help toolchain` and `gn help toolchain_args`). Basically these variables will override the ones that are set globally in args.gn (or have the default values) *for targets using that toolchain*.
lgtm https://codereview.chromium.org/1983613002/diff/1/build/toolchain/cros/BUILD.gn File build/toolchain/cros/BUILD.gn (right): https://codereview.chromium.org/1983613002/diff/1/build/toolchain/cros/BUILD.... build/toolchain/cros/BUILD.gn:84: # build/toolchain/cros:target in BUILDCONFIG.gn On 2016/05/17 06:26:31, hashimoto wrote: > How are we going to use this toolchain after removing the references to > cros:target and cros:clang_target from BUILDCONFIG.gn (IIUC it's going to use > linux:target and linux:clang_target even when target_os=="chromeos")? > By specifying custom_toolchain? ping?
https://codereview.chromium.org/1983613002/diff/1/build/toolchain/cros/BUILD.gn File build/toolchain/cros/BUILD.gn (right): https://codereview.chromium.org/1983613002/diff/1/build/toolchain/cros/BUILD.... build/toolchain/cros/BUILD.gn:84: # build/toolchain/cros:target in BUILDCONFIG.gn On 2016/05/18 03:24:01, hashimoto wrote: > On 2016/05/17 06:26:31, hashimoto wrote: > > How are we going to use this toolchain after removing the references to > > cros:target and cros:clang_target from BUILDCONFIG.gn (IIUC it's going to use > > linux:target and linux:clang_target even when target_os=="chromeos")? > > By specifying custom_toolchain? > > ping? That's weird, I thought I wrote an answer for this. This TODO is wrong and should be deleted. We're keeping the cros:target toolchain, and we have more sensible defaults now.
Description was changed from ========== Rework the way ChromiumOS toolchains will work in GN. This CL reworks the way ChromiumOS toolchains work in GN, in such a way that they might actually have all the flags they need for the ChromiumOS ebuild files to be able to set all of the flags it needs (though there are still some missing GN build_args). Specifically, the ebuild will now need to set the following in args.gn: host_toolchain = "//build/toolchain/cros:host" v8_snapshot_toolchain = "//build/toolchain/cros:v8_snapshot" in order to support boards other than the amd64-generic build. The ebuild should actually set these variables all the time; it just happens that the amd64-generic build will work at the moment without the variables, but that will not be guaranteed to remain true in the future. This CL also adds the following optional build args that do pretty much what you'd expect them to do: cros_target_ld, cros_target_extra_cflags, cros_target_extra_cppflags, cros_target_extra_cxx_flags, cros_target_extra_ldflags, cros_host_ar, cros_host_cc, cros_host_cxx, cros_host_ld, cros_host_is_clang, cros_host_extra_cflags, cros_host_extra_cppflags, cros_host_extra_cxx_flags, cros_host_extra_ldflags, cros_v8_snapshot_ld, cros_v8_snapshot_extra_cflags, cros_v8_snapshot_extra_cppflags, cros_v8_snapshot_extra_cxx_flags, cros_v8_snapshot_extra_ldflags This CL should be backwards-compatible with the existing linux desktop ChromiumOS builds and the amd64-generic simplechrome/ebuild (i.e., it can be landed and reverted w/o requiring any other changes to be made). It is a big hammer intended to un-block the ChromiumOS GN migration while we continue thinking about how to best support ChromiumOS. R=stevenjb@chromium.org, brettw@chromium.org BUG=608596, 595653 ========== to ========== Rework the way ChromiumOS toolchains will work in GN. This CL reworks the way ChromiumOS toolchains work in GN, in such a way that they might actually have all the flags they need for the ChromiumOS ebuild files to be able to set all of the flags it needs (though there are still some missing GN build_args). Specifically, the ebuild will now need to set the following in args.gn: host_toolchain = "//build/toolchain/cros:host" v8_snapshot_toolchain = "//build/toolchain/cros:v8_snapshot" in order to support boards other than the amd64-generic build. The ebuild should actually set these variables all the time; it just happens that the amd64-generic build will work at the moment without the variables, but that will not be guaranteed to remain true in the future. This CL also adds the following optional build args that do pretty much what you'd expect them to do: cros_target_ld, cros_target_extra_cflags, cros_target_extra_cppflags, cros_target_extra_cxx_flags, cros_target_extra_ldflags, cros_host_ar, cros_host_cc, cros_host_cxx, cros_host_ld, cros_host_is_clang, cros_host_extra_cflags, cros_host_extra_cppflags, cros_host_extra_cxx_flags, cros_host_extra_ldflags, cros_v8_snapshot_ar, cros_v8_snapshot_cc, cros_v8_snapshot_cxx, cros_v8_snapshot_ld, cros_v8_snapshot_extra_cflags, cros_v8_snapshot_extra_cppflags, cros_v8_snapshot_extra_cxx_flags, cros_v8_snapshot_extra_ldflags This CL should be backwards-compatible with the existing linux desktop ChromiumOS builds and the amd64-generic simplechrome/ebuild (i.e., it can be landed and reverted w/o requiring any other changes to be made). It is a big hammer intended to un-block the ChromiumOS GN migration while we continue thinking about how to best support ChromiumOS. R=stevenjb@chromium.org, brettw@chromium.org BUG=608596, 595653 ==========
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/1983613002/#ps40001 (title: "remove unneeded TODO")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1983613002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1983613002/40001
Message was sent while issue was closed.
Description was changed from ========== Rework the way ChromiumOS toolchains will work in GN. This CL reworks the way ChromiumOS toolchains work in GN, in such a way that they might actually have all the flags they need for the ChromiumOS ebuild files to be able to set all of the flags it needs (though there are still some missing GN build_args). Specifically, the ebuild will now need to set the following in args.gn: host_toolchain = "//build/toolchain/cros:host" v8_snapshot_toolchain = "//build/toolchain/cros:v8_snapshot" in order to support boards other than the amd64-generic build. The ebuild should actually set these variables all the time; it just happens that the amd64-generic build will work at the moment without the variables, but that will not be guaranteed to remain true in the future. This CL also adds the following optional build args that do pretty much what you'd expect them to do: cros_target_ld, cros_target_extra_cflags, cros_target_extra_cppflags, cros_target_extra_cxx_flags, cros_target_extra_ldflags, cros_host_ar, cros_host_cc, cros_host_cxx, cros_host_ld, cros_host_is_clang, cros_host_extra_cflags, cros_host_extra_cppflags, cros_host_extra_cxx_flags, cros_host_extra_ldflags, cros_v8_snapshot_ar, cros_v8_snapshot_cc, cros_v8_snapshot_cxx, cros_v8_snapshot_ld, cros_v8_snapshot_extra_cflags, cros_v8_snapshot_extra_cppflags, cros_v8_snapshot_extra_cxx_flags, cros_v8_snapshot_extra_ldflags This CL should be backwards-compatible with the existing linux desktop ChromiumOS builds and the amd64-generic simplechrome/ebuild (i.e., it can be landed and reverted w/o requiring any other changes to be made). It is a big hammer intended to un-block the ChromiumOS GN migration while we continue thinking about how to best support ChromiumOS. R=stevenjb@chromium.org, brettw@chromium.org BUG=608596, 595653 ========== to ========== Rework the way ChromiumOS toolchains will work in GN. This CL reworks the way ChromiumOS toolchains work in GN, in such a way that they might actually have all the flags they need for the ChromiumOS ebuild files to be able to set all of the flags it needs (though there are still some missing GN build_args). Specifically, the ebuild will now need to set the following in args.gn: host_toolchain = "//build/toolchain/cros:host" v8_snapshot_toolchain = "//build/toolchain/cros:v8_snapshot" in order to support boards other than the amd64-generic build. The ebuild should actually set these variables all the time; it just happens that the amd64-generic build will work at the moment without the variables, but that will not be guaranteed to remain true in the future. This CL also adds the following optional build args that do pretty much what you'd expect them to do: cros_target_ld, cros_target_extra_cflags, cros_target_extra_cppflags, cros_target_extra_cxx_flags, cros_target_extra_ldflags, cros_host_ar, cros_host_cc, cros_host_cxx, cros_host_ld, cros_host_is_clang, cros_host_extra_cflags, cros_host_extra_cppflags, cros_host_extra_cxx_flags, cros_host_extra_ldflags, cros_v8_snapshot_ar, cros_v8_snapshot_cc, cros_v8_snapshot_cxx, cros_v8_snapshot_ld, cros_v8_snapshot_extra_cflags, cros_v8_snapshot_extra_cppflags, cros_v8_snapshot_extra_cxx_flags, cros_v8_snapshot_extra_ldflags This CL should be backwards-compatible with the existing linux desktop ChromiumOS builds and the amd64-generic simplechrome/ebuild (i.e., it can be landed and reverted w/o requiring any other changes to be made). It is a big hammer intended to un-block the ChromiumOS GN migration while we continue thinking about how to best support ChromiumOS. R=stevenjb@chromium.org, brettw@chromium.org BUG=608596, 595653 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Rework the way ChromiumOS toolchains will work in GN. This CL reworks the way ChromiumOS toolchains work in GN, in such a way that they might actually have all the flags they need for the ChromiumOS ebuild files to be able to set all of the flags it needs (though there are still some missing GN build_args). Specifically, the ebuild will now need to set the following in args.gn: host_toolchain = "//build/toolchain/cros:host" v8_snapshot_toolchain = "//build/toolchain/cros:v8_snapshot" in order to support boards other than the amd64-generic build. The ebuild should actually set these variables all the time; it just happens that the amd64-generic build will work at the moment without the variables, but that will not be guaranteed to remain true in the future. This CL also adds the following optional build args that do pretty much what you'd expect them to do: cros_target_ld, cros_target_extra_cflags, cros_target_extra_cppflags, cros_target_extra_cxx_flags, cros_target_extra_ldflags, cros_host_ar, cros_host_cc, cros_host_cxx, cros_host_ld, cros_host_is_clang, cros_host_extra_cflags, cros_host_extra_cppflags, cros_host_extra_cxx_flags, cros_host_extra_ldflags, cros_v8_snapshot_ar, cros_v8_snapshot_cc, cros_v8_snapshot_cxx, cros_v8_snapshot_ld, cros_v8_snapshot_extra_cflags, cros_v8_snapshot_extra_cppflags, cros_v8_snapshot_extra_cxx_flags, cros_v8_snapshot_extra_ldflags This CL should be backwards-compatible with the existing linux desktop ChromiumOS builds and the amd64-generic simplechrome/ebuild (i.e., it can be landed and reverted w/o requiring any other changes to be made). It is a big hammer intended to un-block the ChromiumOS GN migration while we continue thinking about how to best support ChromiumOS. R=stevenjb@chromium.org, brettw@chromium.org BUG=608596, 595653 ========== to ========== Rework the way ChromiumOS toolchains will work in GN. This CL reworks the way ChromiumOS toolchains work in GN, in such a way that they might actually have all the flags they need for the ChromiumOS ebuild files to be able to set all of the flags it needs (though there are still some missing GN build_args). Specifically, the ebuild will now need to set the following in args.gn: host_toolchain = "//build/toolchain/cros:host" v8_snapshot_toolchain = "//build/toolchain/cros:v8_snapshot" in order to support boards other than the amd64-generic build. The ebuild should actually set these variables all the time; it just happens that the amd64-generic build will work at the moment without the variables, but that will not be guaranteed to remain true in the future. This CL also adds the following optional build args that do pretty much what you'd expect them to do: cros_target_ld, cros_target_extra_cflags, cros_target_extra_cppflags, cros_target_extra_cxx_flags, cros_target_extra_ldflags, cros_host_ar, cros_host_cc, cros_host_cxx, cros_host_ld, cros_host_is_clang, cros_host_extra_cflags, cros_host_extra_cppflags, cros_host_extra_cxx_flags, cros_host_extra_ldflags, cros_v8_snapshot_ar, cros_v8_snapshot_cc, cros_v8_snapshot_cxx, cros_v8_snapshot_ld, cros_v8_snapshot_extra_cflags, cros_v8_snapshot_extra_cppflags, cros_v8_snapshot_extra_cxx_flags, cros_v8_snapshot_extra_ldflags This CL should be backwards-compatible with the existing linux desktop ChromiumOS builds and the amd64-generic simplechrome/ebuild (i.e., it can be landed and reverted w/o requiring any other changes to be made). It is a big hammer intended to un-block the ChromiumOS GN migration while we continue thinking about how to best support ChromiumOS. R=stevenjb@chromium.org, brettw@chromium.org BUG=608596, 595653 Committed: https://crrev.com/8ad2f49335feaddddbd3f318fc6f4d13eb52760b Cr-Commit-Position: refs/heads/master@{#394534} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8ad2f49335feaddddbd3f318fc6f4d13eb52760b Cr-Commit-Position: refs/heads/master@{#394534}
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/1983613002/diff/40001/build/toolchain/cros/BU... File build/toolchain/cros/BUILD.gn (right): https://codereview.chromium.org/1983613002/diff/40001/build/toolchain/cros/BU... build/toolchain/cros/BUILD.gn:57: cros_host_extra_cppflags = "" It would be nice if this had a similar comment as the one you added to the toolchain.gni file about cpp-vs-cxx.
Message was sent while issue was closed.
https://codereview.chromium.org/1983613002/diff/40001/build/toolchain/cros/BU... File build/toolchain/cros/BUILD.gn (right): https://codereview.chromium.org/1983613002/diff/40001/build/toolchain/cros/BU... build/toolchain/cros/BUILD.gn:57: cros_host_extra_cppflags = "" On 2016/05/23 20:55:04, brettw wrote: > It would be nice if this had a similar comment as the one you added to the > toolchain.gni file about cpp-vs-cxx. yeah ... I waffled over how many comments I wanted to put in, and where, but it makes sensse that we should have at least one comment on that topic specifically. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
