Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2265983002:
Use clang for snapshot_toolchain by default, except on ChromeOS. (Closed)
DescriptionUse clang for snapshot_toolchain by default, except on ChromeOS.
Non-CrOS builds should use clang for snapshot_toolchain, even on builds which use gcc for the target device. Change the default value for snapshot_toolchain to always use clang, except on ChromeOS gcc builds. In practice, every platform except ChromeOS always uses clang, so this should not affect any platform except non-CrOS gcc builds (like Cast)
BUG= internal b/30873074
Committed: https://crrev.com/5a8f92901badeeec5862931606974dc4a47d9d60
Cr-Commit-Position: refs/heads/master@{#38825}
Patch Set 1 #Patch Set 2 : Import order. #
Total comments: 3
Patch Set 3 : Change default. #
Total comments: 2
Messages
Total messages: 31 (15 generated)
slan@chromium.org changed reviewers: + dpranke@google.com, halliwell@chromium.org, jochen@chromium.org
Hey all, this is needed for the Chromecast build at TOT. Please take a look. +halliwell@ for FYI
The CQ bit was checked by slan@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: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/22274)
https://codereview.chromium.org/2265983002/diff/20001/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/2265983002/diff/20001/snapshot_toolchain.gni#... snapshot_toolchain.gni:28: import("//build/config/chromecast_build.gni") why is that required here?
https://codereview.chromium.org/2265983002/diff/20001/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/2265983002/diff/20001/snapshot_toolchain.gni#... snapshot_toolchain.gni:28: import("//build/config/chromecast_build.gni") On 2016/08/22 15:46:53, jochen wrote: > why is that required here? is_chromecast is declared in that file.
https://codereview.chromium.org/2265983002/diff/20001/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/2265983002/diff/20001/snapshot_toolchain.gni#... snapshot_toolchain.gni:28: import("//build/config/chromecast_build.gni") On 2016/08/22 at 15:54:25, slan wrote: > On 2016/08/22 15:46:53, jochen wrote: > > why is that required here? > > is_chromecast is declared in that file. but we also don't include special files to get the definition of is_android
On 2016/08/22 15:57:47, jochen wrote: > https://codereview.chromium.org/2265983002/diff/20001/snapshot_toolchain.gni > File snapshot_toolchain.gni (right): > > https://codereview.chromium.org/2265983002/diff/20001/snapshot_toolchain.gni#... > snapshot_toolchain.gni:28: import("//build/config/chromecast_build.gni") > On 2016/08/22 at 15:54:25, slan wrote: > > On 2016/08/22 15:46:53, jochen wrote: > > > why is that required here? > > > > is_chromecast is declared in that file. > > but we also don't include special files to get the definition of is_android is_android is declared in BUILDCONFIG.gn, which is implicitly in the scope of every BUILD.gn file. Though we discussed putting is_chromecast among the os and cpu flags, it is more of a feature than a platform. Admittedly, needing to import this file is a bit of a drag, but it seems more clear than giving this variable global visibility.
dunno, not convinced. Why would v8 have to know about chromecast? Dirk, mind chiming in?
On 2016/08/22 16:12:33, jochen wrote: > dunno, not convinced. Why would v8 have to know about chromecast? > > Dirk, mind chiming in? I agree with you - it's unsettling that a component like v8 should know anything about a feature like Cast. This applies to code all across Chrome: we had to decide between configuring *everything* with GN args (i.e. explicitly setting snapshot_toolchain in args.gn for every Cast build) and baking in knowledge of Cast into BUILD files. The main problem with configuring the build with GN args is that Cast builds will need dozens and dozens of these args to keep everything building correctly. However, setting the arg is still a viable option to solving our problem. Alternatively, is there a platform that requires that gcc is used for snapshot_toolchain? Does ChromeOS need this? If not, why not just use clang on every build?
On 2016/08/22 16:23:02, slan wrote: > On 2016/08/22 16:12:33, jochen wrote: > > dunno, not convinced. Why would v8 have to know about chromecast? > > > > Dirk, mind chiming in? > > I agree with you - it's unsettling that a component like v8 should know anything > about a feature like Cast. This applies to code all across Chrome: we had to > decide between configuring *everything* with GN args (i.e. explicitly setting > snapshot_toolchain in args.gn for every Cast build) and baking in knowledge of > Cast into BUILD files. The main problem with configuring the build with GN args > is that Cast builds will need dozens and dozens of these args to keep everything > building correctly. However, setting the arg is still a viable option to solving > our problem. > > Alternatively, is there a platform that requires that gcc is used for > snapshot_toolchain? Does ChromeOS need this? If not, why not just use clang on > every build? I would really like to avoid including the chromecast_build.gni file in snapshot_toolchain.gni. We don't want to put product-specific configuration into BUILDCONFIG.gn, so moving is_chromecast there is a non-starter. ChromeOS uses gcc intentionally across all of the toolchains for their real builds, but in that case they use their own toolchains and explicitly set a whole bunch of flags. In the "desktop ChromeOS" case, i.e., a normal chromium linux build w/ target_os="chromeos", it's probably not that important which compiler is used. So, I *think* we could probably just change this so that we used clang by default. We think in the longer-term, the way we want to do product-specific configuration is via GN args files that are checked-in. You can already see examples of this for blimp and headless chrome, in //build/args , and this is what we are doing for internal ios builds as well. The fact that we didn't do this for Cast is largely due to legacy compatibility w/ GYP, but if we were setting up Cast now we'd probably require that. I think we should see if we can get away w/ just changing the default, but failing that, we should evaluate moving cast (incrementally) to args files rather than changing snapshot_toolchain.gni. How does that sound?
Description was changed from ========== [Chromecast] Use clang for snapshot_toolchain on Cast builds. In an effort to use clang whenever possible, Chromecast builds should use clang for snapshot_toolchain, even when gcc is used for the target_toolchain. BUG= internal b/30873074 ========== to ========== Use clang for snapshot_toolchain by default, except on ChromeOS. Chromecast builds should use clang for snapshot_toolchain, even on builds which use gcc for the target device. Change the default value for snapshot_toolchain to always use clang, except on ChromeOS gcc builds. In practice, every platform except ChromeOS always uses clang, so this should not affect any platform except Chromecast. BUG= internal b/30873074 ==========
Description was changed from ========== Use clang for snapshot_toolchain by default, except on ChromeOS. Chromecast builds should use clang for snapshot_toolchain, even on builds which use gcc for the target device. Change the default value for snapshot_toolchain to always use clang, except on ChromeOS gcc builds. In practice, every platform except ChromeOS always uses clang, so this should not affect any platform except Chromecast. BUG= internal b/30873074 ========== to ========== Use clang for snapshot_toolchain by default, except on ChromeOS. Non-CrOS builds should use clang for snapshot_toolchain, even on builds which use gcc for the target device. Change the default value for snapshot_toolchain to always use clang, except on ChromeOS gcc builds. In practice, every platform except ChromeOS always uses clang, so this should not affect any platform except non-CrOS gcc builds (like Cast) BUG= internal b/30873074 ==========
On 2016/08/22 17:48:08, Dirk Pranke wrote: > On 2016/08/22 16:23:02, slan wrote: > > On 2016/08/22 16:12:33, jochen wrote: > > > dunno, not convinced. Why would v8 have to know about chromecast? > > > > > > Dirk, mind chiming in? > > > > I agree with you - it's unsettling that a component like v8 should know > anything > > about a feature like Cast. This applies to code all across Chrome: we had to > > decide between configuring *everything* with GN args (i.e. explicitly setting > > snapshot_toolchain in args.gn for every Cast build) and baking in knowledge of > > Cast into BUILD files. The main problem with configuring the build with GN > args > > is that Cast builds will need dozens and dozens of these args to keep > everything > > building correctly. However, setting the arg is still a viable option to > solving > > our problem. > > > > Alternatively, is there a platform that requires that gcc is used for > > snapshot_toolchain? Does ChromeOS need this? If not, why not just use clang on > > every build? > > I would really like to avoid including the chromecast_build.gni file in > snapshot_toolchain.gni. > > We don't want to put product-specific configuration into BUILDCONFIG.gn, so > moving is_chromecast there > is a non-starter. > > ChromeOS uses gcc intentionally across all of the toolchains for their real > builds, but in that case > they use their own toolchains and explicitly set a whole bunch of flags. In the > "desktop ChromeOS" > case, i.e., a normal chromium linux build w/ target_os="chromeos", it's probably > not that important > which compiler is used. So, I *think* we could probably just change this so that > we used clang by > default. SGTM - I've updated the default to always use clang, except on CrOS. No other defines v8 gcc toolchains, so this should not affect any builds except non-CrOS gcc builds (Cast). > > We think in the longer-term, the way we want to do product-specific > configuration is via > GN args files that are checked-in. You can already see examples of this for > blimp and headless > chrome, in //build/args , and this is what we are doing for internal ios builds > as well. The fact > that we didn't do this for Cast is largely due to legacy compatibility w/ GYP, > but if we were setting > up Cast now we'd probably require that. Yes, I like this a lot. Just haven't found time to implement :) > > I think we should see if we can get away w/ just changing the default, but > failing that, we should > evaluate moving cast (incrementally) to args files rather than changing > snapshot_toolchain.gni. > > How does that sound? Thanks for the discussion, hopefully the latest PS will work. PTAL!
The CQ bit was checked by slan@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.
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
lgtm. https://codereview.chromium.org/2265983002/diff/40001/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/2265983002/diff/40001/snapshot_toolchain.gni#... snapshot_toolchain.gni:97: "on $host_os $host_cpu") Was this indentation change intentional?
https://codereview.chromium.org/2265983002/diff/40001/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/2265983002/diff/40001/snapshot_toolchain.gni#... snapshot_toolchain.gni:97: "on $host_os $host_cpu") On 2016/08/22 23:08:45, Dirk Pranke wrote: > Was this indentation change intentional? That's what gn format wanted.
> On 2016/08/22 23:08:45, Dirk Pranke wrote: > > Was this indentation change intentional? > > That's what gn format wanted. Huh. I'm a bit surprised by that, but okay.
lgtm
The CQ bit was checked by slan@chromium.org
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 ========== Use clang for snapshot_toolchain by default, except on ChromeOS. Non-CrOS builds should use clang for snapshot_toolchain, even on builds which use gcc for the target device. Change the default value for snapshot_toolchain to always use clang, except on ChromeOS gcc builds. In practice, every platform except ChromeOS always uses clang, so this should not affect any platform except non-CrOS gcc builds (like Cast) BUG= internal b/30873074 ========== to ========== Use clang for snapshot_toolchain by default, except on ChromeOS. Non-CrOS builds should use clang for snapshot_toolchain, even on builds which use gcc for the target device. Change the default value for snapshot_toolchain to always use clang, except on ChromeOS gcc builds. In practice, every platform except ChromeOS always uses clang, so this should not affect any platform except non-CrOS gcc builds (like Cast) BUG= internal b/30873074 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Use clang for snapshot_toolchain by default, except on ChromeOS. Non-CrOS builds should use clang for snapshot_toolchain, even on builds which use gcc for the target device. Change the default value for snapshot_toolchain to always use clang, except on ChromeOS gcc builds. In practice, every platform except ChromeOS always uses clang, so this should not affect any platform except non-CrOS gcc builds (like Cast) BUG= internal b/30873074 ========== to ========== Use clang for snapshot_toolchain by default, except on ChromeOS. Non-CrOS builds should use clang for snapshot_toolchain, even on builds which use gcc for the target device. Change the default value for snapshot_toolchain to always use clang, except on ChromeOS gcc builds. In practice, every platform except ChromeOS always uses clang, so this should not affect any platform except non-CrOS gcc builds (like Cast) BUG= internal b/30873074 Committed: https://crrev.com/5a8f92901badeeec5862931606974dc4a47d9d60 Cr-Commit-Position: refs/heads/master@{#38825} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5a8f92901badeeec5862931606974dc4a47d9d60 Cr-Commit-Position: refs/heads/master@{#38825} |
