GN: Propagate top-level host_toolchain through to all toolchains

GN: Propagate top-level host_toolchain through to all toolchains

The logic to choose host_toolchain depends on variables like
is_clang that might be overridden in toolchain_args() as well as
set as a build argument by the user.  Only the top-level build
arguments should affect the host_toolchain value, not anything
set in some toolchain's toolchain_args() block.  To achieve this,
set host_toolchain once at top level and then make each toolchain
propagate that value down in its toolchain_args() block.

BUG=512869
R=dpranke@chromium.org

Committed: https://crrev.com/e393ea6912a747e8c340fc5a53f2be45028d70c1
Cr-Commit-Position: refs/heads/master@{#362758}

[Dirk Pranke, 2015-11-25 03:48:05]

okay, you beat me to the change. the toolchain changes are right, but I think
the BUILDCONFIG.gn changes are wrong (and you can see the failures on the
android and chromeos trybots).

https://codereview.chromium.org/1467413005/diff/20001/build/config/BUILDCONFI...
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/1467413005/diff/20001/build/config/BUILDCONFI...
build/config/BUILDCONFIG.gn:154: # block should have propagated a value down.
In the top level context (the first time through BUILDCONFIG.gn), current_cpu ==
target_cpu and current_os == target_os (see line 53).

I think the logic you implemented breaks on, e.g., android and ios.

[Roland McGrath, 2015-12-01 21:02:01]

PTAL

[Dirk Pranke, 2015-12-01 21:07:32]

lgtm

[Roland McGrath, 2015-12-01 21:18:36]

The CQ bit was checked by mcgrathr@chromium.org

[commit-bot: I haz the power, 2015-12-01 21:20:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1467413005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1467413005/40001

[Roland McGrath, 2015-12-01 21:53:40]

mcgrathr@chromium.org changed reviewers:
+ brettw@chromium.org

[Roland McGrath, 2015-12-01 21:53:40]

mcgrathr@chromium.org changed required reviewers:
+ brettw@chromium.org

[Roland McGrath, 2015-12-01 21:53:41]

Oops, failed to notice the per-file OWNERS there.
Brett, OWNERS review please.

[commit-bot: I haz the power, 2015-12-01 22:00:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-01 22:00:47]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[brettw, 2015-12-01 22:08:19]

https://codereview.chromium.org/1467413005/diff/40001/build/config/BUILDCONFI...
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/1467413005/diff/40001/build/config/BUILDCONFI...
build/config/BUILDCONFIG.gn:58: buildconfig_in_default_toolchain = current_cpu
== "" && current_os == ""
I'm not super fond of this hack and I'd prefer to avoid this type of thing. I
don't actually think it will be a big problem.

 I have some ideas of how toolchain setup can be improved. If forgetting the
host toolchain ends up being a problem in practice, we can implement that for a
precise way to tell this.

[Roland McGrath, 2015-12-01 22:12:18]

https://codereview.chromium.org/1467413005/diff/40001/build/config/BUILDCONFI...
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/1467413005/diff/40001/build/config/BUILDCONFI...
build/config/BUILDCONFIG.gn:58: buildconfig_in_default_toolchain = current_cpu
== "" && current_os == ""
On 2015/12/01 22:08:19, brettw wrote:
> I'm not super fond of this hack and I'd prefer to avoid this type of thing. I
> don't actually think it will be a big problem.
> 
>  I have some ideas of how toolchain setup can be improved. If forgetting the
> host toolchain ends up being a problem in practice, we can implement that for
a
> precise way to tell this.

My concern is that when it goes wrong the failure modes are likely to be quite
subtle.  I'd really like to have something to ensure that the host_toolchain
calculation is never carried out a second time inside a toolchain context.  If
there is nothing reliable we can do now but there will be something eventually,
then I'm fine with just replacing the assert with a TODO.  But I'm not at all
sanguine about just dropping the assert and hand-waving that it won't bite us in
the future.

[Dirk Pranke, 2015-12-01 22:34:37]

On 2015/12/01 22:12:18, Roland McGrath wrote:
>
https://codereview.chromium.org/1467413005/diff/40001/build/config/BUILDCONFI...
> File build/config/BUILDCONFIG.gn (right):
> 
>
https://codereview.chromium.org/1467413005/diff/40001/build/config/BUILDCONFI...
> build/config/BUILDCONFIG.gn:58: buildconfig_in_default_toolchain = current_cpu
> == "" && current_os == ""
> On 2015/12/01 22:08:19, brettw wrote:
> > I'm not super fond of this hack and I'd prefer to avoid this type of thing.
I
> > don't actually think it will be a big problem.
> > 
> >  I have some ideas of how toolchain setup can be improved. If forgetting the
> > host toolchain ends up being a problem in practice, we can implement that
for
> a
> > precise way to tell this.
> 
> My concern is that when it goes wrong the failure modes are likely to be quite
> subtle.  I'd really like to have something to ensure that the host_toolchain
> calculation is never carried out a second time inside a toolchain context.  If
> there is nothing reliable we can do now but there will be something
eventually,
> then I'm fine with just replacing the assert with a TODO.  But I'm not at all
> sanguine about just dropping the assert and hand-waving that it won't bite us
in
> the future.

Brett and I discussed this a bit and perhaps a better way to do this is to
rework how set_default_toolchain() works and have a more supported
mechanism to detect first-pass from subsequent passes. Let's add a TODO(dpranke)
for this. I'll file a bug for it as well when crbug comes back up.

[Roland McGrath, 2015-12-01 22:48:14]

On 2015/12/01 22:34:37, Dirk Pranke wrote:
> Brett and I discussed this a bit and perhaps a better way to do this is to
> rework how set_default_toolchain() works and have a more supported
> mechanism to detect first-pass from subsequent passes. Let's add a
TODO(dpranke)
> for this. I'll file a bug for it as well when crbug comes back up.

Done.  PTAL.

[Dirk Pranke, 2015-12-01 22:52:49]

lgtm. of course you still need brett's approval as well.

[brettw, 2015-12-01 22:59:29]

lgtm

[Roland McGrath, 2015-12-01 23:00:13]

The CQ bit was checked by mcgrathr@chromium.org

[commit-bot: I haz the power, 2015-12-01 23:00:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1467413005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1467413005/60001

[commit-bot: I haz the power, 2015-12-02 00:24:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-02 00:24:20]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Roland McGrath, 2015-12-02 18:18:12]

The CQ bit was checked by mcgrathr@chromium.org

[Roland McGrath, 2015-12-02 18:18:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1467413005/#ps80001
(title: "restore kludge for android case")

[commit-bot: I haz the power, 2015-12-02 18:18:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1467413005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1467413005/80001

[commit-bot: I haz the power, 2015-12-02 18:52:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-02 18:53:18]

Description was changed from

==========
GN: Propagate top-level host_toolchain through to all toolchains

The logic to choose host_toolchain depends on variables like
is_clang that might be overridden in toolchain_args() as well as
set as a build argument by the user.  Only the top-level build
arguments should affect the host_toolchain value, not anything
set in some toolchain's toolchain_args() block.  To achieve this,
set host_toolchain once at top level and then make each toolchain
propagate that value down in its toolchain_args() block.

BUG= 512869
R=dpranke@chromium.org
==========

to

==========
GN: Propagate top-level host_toolchain through to all toolchains

The logic to choose host_toolchain depends on variables like
is_clang that might be overridden in toolchain_args() as well as
set as a build argument by the user.  Only the top-level build
arguments should affect the host_toolchain value, not anything
set in some toolchain's toolchain_args() block.  To achieve this,
set host_toolchain once at top level and then make each toolchain
propagate that value down in its toolchain_args() block.

BUG= 512869
R=dpranke@chromium.org

Committed: https://crrev.com/e393ea6912a747e8c340fc5a53f2be45028d70c1
Cr-Commit-Position: refs/heads/master@{#362758}
==========

[commit-bot: I haz the power, 2015-12-02 18:53:19]

Patchset 5 (id:??) landed as
https://crrev.com/e393ea6912a747e8c340fc5a53f2be45028d70c1
Cr-Commit-Position: refs/heads/master@{#362758}