Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2202873002:
Rework approach to allowing extra flags for CrOS builds. (Closed)
|
Created:
4 years, 4 months ago by Dirk Pranke Modified:
4 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@add_cros_nacl_bootstrap_args Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRework approach to allowing extra flags for CrOS builds.
The prior approach to letting the CrOS build system set extra
c/cpp/cxx/ldflags was to embed variables into the actual command
line strings for each tool, but this meant that the flags were not
exposed to particular targets and targets couldn't turn them off, and
this was problematic.
Instead, define an "extra_flags" config that can be shimmed in for
the CrOS toolchains, and otherwise be treated like any other config.
R=brettw@chromium.org, llozano@chromium.org
BUG=629593
Patch Set 1 #
Total comments: 6
Patch Set 2 : address review comments #Patch Set 3 : fix typos, type checking #
Messages
Total messages: 28 (7 generated)
Take a look and let me know what you think.
The upside to this approach is that we can solve the tcmalloc/AFDO problem by
adding this to the tcmalloc target:
if (is_chromeos) {
configs -= [ "//build/config/compiler:extra_flags" ]
}
The downside is that the extra flags are no longer guaranteed to be at the end
of the command line. I don't think there is any way using GN to ensure that
these flags are placed at the end, so if we really want that we may need to
change GN.
https://codereview.chromium.org/2202873002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2202873002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:1593: } Perhaps instead of hardcoding the toolchain and config names, this should be a build arg instead?
https://codereview.chromium.org/2202873002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2202873002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:1593: } Can this just be: if (target_os == "chromeos") ? https://codereview.chromium.org/2202873002/diff/1/build/toolchain/cros/BUILD.gn File build/toolchain/cros/BUILD.gn (right): https://codereview.chromium.org/2202873002/diff/1/build/toolchain/cros/BUILD.... build/toolchain/cros/BUILD.gn:118: config("extra_flags") { This should really be in //build/config/cros/BUILD.gn. It doesn't exist yet, but the toolchain dirs don't define configs or set flags, those go in build/config.
https://codereview.chromium.org/2202873002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2202873002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:1593: } On 2016/08/03 23:02:14, brettw (ping on IM after 24h) wrote: > Can this just be: > if (target_os == "chromeos") ? No, because that would break the "desktop chromeos" config, where we build using the chromium checked-in clang toolchain. Ideally some day we'd separate out "want chromeos ui and functionality" from "want to run on the chromeos operating system", in which case this distinction would be a lot clearer. https://codereview.chromium.org/2202873002/diff/1/build/toolchain/cros/BUILD.gn File build/toolchain/cros/BUILD.gn (right): https://codereview.chromium.org/2202873002/diff/1/build/toolchain/cros/BUILD.... build/toolchain/cros/BUILD.gn:118: config("extra_flags") { On 2016/08/03 23:02:14, brettw (ping on IM after 24h) wrote: > This should really be in //build/config/cros/BUILD.gn. It doesn't exist yet, but > the toolchain dirs don't define configs or set flags, those go in build/config. Good point. I'll move it.
https://codereview.chromium.org/2202873002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2202873002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:1593: } On 2016/08/03 23:09:07, Dirk Pranke wrote: > On 2016/08/03 23:02:14, brettw (ping on IM after 24h) wrote: > > Can this just be: > > if (target_os == "chromeos") ? > > No, because that would break the "desktop chromeos" config, > where we build using the chromium checked-in clang toolchain. Ah, then can you add an explicit comment explaining this? It's very subtle.
On 2016/08/08 22:10:56, brettw (ping on IM after 24h) wrote: > https://codereview.chromium.org/2202873002/diff/1/build/config/compiler/BUILD.gn > File build/config/compiler/BUILD.gn (right): > > https://codereview.chromium.org/2202873002/diff/1/build/config/compiler/BUILD... > build/config/compiler/BUILD.gn:1593: } > On 2016/08/03 23:09:07, Dirk Pranke wrote: > > On 2016/08/03 23:02:14, brettw (ping on IM after 24h) wrote: > > > Can this just be: > > > if (target_os == "chromeos") ? > > > > No, because that would break the "desktop chromeos" config, > > where we build using the chromium checked-in clang toolchain. > > Ah, then can you add an explicit comment explaining this? It's very subtle. Will do.
LGTM if you add the comment I suggested above.
@brettw, does this still look good to you?
On 2016/08/11 02:16:49, Dirk Pranke (slow) wrote:
> @brettw, does this still look good to you?
I applied the patch for this together with the patch for 2200543002
And I get the following error within the simple chrome workflow.
chromium_daisy_gn/src $ gn gen out_$SDK_BOARD/Release --args="$GN_ARGS"
ERROR at //build/toolchain/gcc_toolchain.gni:59:3: Duplicate definition.
toolchain(target_name) {
^-----------------------
The item
//build/toolchain/cros:target
was already defined.
See //build/toolchain/gcc_toolchain.gni:59:3: Previous definition:
toolchain(target_name) {
^-----------------------
GN_ARGS is:
$ echo $GN_ARGS
use_ozone = true custom_toolchain = "//build/toolchain/cros:target"
arm_float_abi = "hard" cros_v8_snapshot_extra_cppflags = "" ozone_platform_gbm =
true cros_v8_snapshot_cc =
"/usr/local/google2/home/llozano/proj/chromium_daisy_gn/src/third_party/llvm-build/Release+Asserts/bin/clang"
cros_target_cxx = "armv7a-cros-linux-gnueabi-clang++
-B/usr/local/google2/home/llozano/proj/chromium_daisy_gn/.cros_cache/chrome-sdk/tarballs/daisy+8694.0.0+target_toolchain/usr/x86_64-pc-linux-gnu/armv7a-cros-linux-gnueabi/binutils-bin/2.25.51-gold
-Wno-unknown-warning-option" cros_host_ar =
"/usr/local/google2/home/llozano/proj/chromium_daisy_gn/src/third_party/binutils/Linux_x64/Release/bin/ar"
use_v4lplugin = false cros_host_cxx =
"/usr/local/google2/home/llozano/proj/chromium_daisy_gn/src/third_party/llvm-build/Release+Asserts/bin/clang++"
cros_v8_snapshot_ar =
"/usr/local/google2/home/llozano/proj/chromium_daisy_gn/src/third_party/binutils/Linux_x64/Release/bin/ar"
use_cups = true use_system_harfbuzz = true arm_use_neon = true
cros_target_extra_cppflags = "" use_evdev_gestures = true use_goma = true
cros_host_extra_ldflags = "" cros_target_extra_cxxflags = "-pipe -march=armv7-a
-mtune=cortex-a15 -mfpu=neon -mfloat-abi=hard -D__google_stl_debug_vector=1 "
target_sysroot =
"/usr/local/google2/home/llozano/proj/chromium_daisy_gn/.cros_cache/chrome-sdk/tarballs/daisy+8694.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz"
clang_use_chrome_plugins = false symbol_level = 2 cros_host_extra_cppflags = ""
cros_host_is_clang = true cros_target_extra_cflags = "-pipe -march=armv7-a
-mtune=cortex-a15 -mfpu=neon -mfloat-abi=hard " cros_v8_snapshot_extra_cflags =
"" system_libdir = "lib" use_cras = true cros_host_cc =
"/usr/local/google2/home/llozano/proj/chromium_daisy_gn/src/third_party/llvm-build/Release+Asserts/bin/clang"
v8_snapshot_toolchain = "//build/toolchain/cros:v8_snapshot"
cros_v8_snapshot_cxx =
"/usr/local/google2/home/llozano/proj/chromium_daisy_gn/src/third_party/llvm-build/Release+Asserts/bin/clang++"
is_clang = true cros_v8_snapshot_is_clang = true use_v4l2_codec = true
cros_host_extra_cflags = "" host_toolchain = "//build/toolchain/cros:host"
use_xkbcommon = true icu_use_data_file = true cros_v8_snapshot_extra_ldflags =
"" enable_nacl = true linux_use_bundled_binutils = true cros_v8_snapshot_ld =
"/usr/local/google2/home/llozano/proj/chromium_daisy_gn/src/third_party/llvm-build/Release+Asserts/bin/clang++"
enable_remoting = true ozone_auto_platforms = false cros_target_ld =
"armv7a-cros-linux-gnueabi-clang++
-B/usr/local/google2/home/llozano/proj/chromium_daisy_gn/.cros_cache/chrome-sdk/tarballs/daisy+8694.0.0+target_toolchain/usr/x86_64-pc-linux-gnu/armv7a-cros-linux-gnueabi/binutils-bin/2.25.51-gold
-Wno-unknown-warning-option" target_os = "chromeos" use_system_minigbm = true
cros_host_extra_cxxflags = "" cros_target_ar = "armv7a-cros-linux-gnueabi-ar"
use_debug_fission = true cros_v8_snapshot_extra_cxxflags = "" target_cpu = "arm"
cros_target_extra_ldflags = "-Wl,-O1 -Wl,-O2 -Wl,--as-needed" is_asan = false
is_debug = false cros_host_ld =
"/usr/local/google2/home/llozano/proj/chromium_daisy_gn/src/third_party/llvm-build/Release+Asserts/bin/clang++"
ozone_platform = "gbm" goma_dir =
"/usr/local/google2/home/llozano/proj/chromium_daisy_gn/.cros_cache/common/goma+2"
cros_target_cc = "armv7a-cros-linux-gnueabi-clang
-B/usr/local/google2/home/llozano/proj/chromium_daisy_gn/.cros_cache/chrome-sdk/tarballs/daisy+8694.0.0+target_toolchain/usr/x86_64-pc-linux-gnu/armv7a-cros-linux-gnueabi/binutils-bin/2.25.51-gold
-Wno-unknown-warning-option"
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2202873002/#ps20001 (title: "address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> On 2016/08/11 02:16:49, Dirk Pranke (slow) wrote: > > @brettw, does this still look good to you? I'm going to assume that it's okay ... On 2016/08/11 23:11:07, llozano1 wrote: > I applied the patch for this together with the patch for 2200543002 > > And I get the following error within the simple chrome workflow. [ snip ] Strange. I wonder if this was a bad merge or something. Let's see what happens when we try to commit this.
Yes, this still LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
On 2016/08/12 22:39:27, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromeos_x86-generic_chromium_compile_only_ng on > master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) and, I guess that's a real error (or, at least, not user error on llozano's part). Hm.
On 2016/08/12 23:30:00, Dirk Pranke (slow) wrote: > On 2016/08/12 22:39:27, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromeos_x86-generic_chromium_compile_only_ng on > > master.tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) > > and, I guess that's a real error (or, at least, not user error on llozano's > part). Hm. it turns out that my fix for the tcmalloc problem is breaking someone using an older GCC compiler. So, it would be nice to have this fix soon..
On 2016/08/15 08:09:29, llozano wrote: > On 2016/08/12 23:30:00, Dirk Pranke (slow) wrote: > > and, I guess that's a real error (or, at least, not user error on llozano's > > part). Hm. > > it turns out that my fix for the tcmalloc problem is breaking someone using an > older GCC compiler. > So, it would be nice to have this fix soon.. Sorry for the delay, was OOO ... I think this latest patch should do the trick.
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2202873002/#ps40001 (title: "fix typos, type checking")
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
Okay, so, it turns out that this CL isn't going to play nicely with the way that the cros build system sets extra flags. More concretely, if we're setting cros_target_extra_cflags = "-pipe -pipe -march=x86-64 -msse3" then "-pipe -pipe -march=x86-64 -msse3" gets passed to gcc as a *single* argument. Which, obviously, isn't going to work. Instead, you would need to set cros_target_extra_cflags = [ "-pipe", "-pipe", "-march=x86-64", "-msse3" ] But, I suspect that this requires a non-trivial number of changes to how the ebuild is setting flags. This has caused me to step back for a moment, and re-ask: what do we *actually* need/want to change for things to work in the CrOS build? This CL doesn't actually fix the problem in bug 629593 by itself, since we need to change how debugallocation_shim.cc is compiled, which would *also* require us to move debugallocation_shim.cc into a new target and then subtract the //build/config/compiler:extra_flags config on that target, which may not actually be the right thing to do. So, how do we really want to fix that bug?
I'm going to abandon this CL. It doesn't work for CrOS builds as they exist today, it won't easily work for non-CrOS builds as it stands, and the underlying issue was addressed differently anyway. I will still try to do *something* that might work for packagers, but that's a different issue. To track that, see crbug.com/642016.
Description was changed from ========== Rework approach to allowing extra flags for CrOS builds. The prior approach to letting the CrOS build system set extra c/cpp/cxx/ldflags was to embed variables into the actual command line strings for each tool, but this meant that the flags were not exposed to particular targets and targets couldn't turn them off, and this was problematic. Instead, define an "extra_flags" config that can be shimmed in for the CrOS toolchains, and otherwise be treated like any other config. R=brettw@chromium.org, llozano@chromium.org BUG=629593 ========== to ========== Rework approach to allowing extra flags for CrOS builds. The prior approach to letting the CrOS build system set extra c/cpp/cxx/ldflags was to embed variables into the actual command line strings for each tool, but this meant that the flags were not exposed to particular targets and targets couldn't turn them off, and this was problematic. Instead, define an "extra_flags" config that can be shimmed in for the CrOS toolchains, and otherwise be treated like any other config. R=brettw@chromium.org, llozano@chromium.org BUG=629593 ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
