Allow a custom toolchain to be injected into BUILDCONFIG.gn

Allow a custom toolchain to be injected into BUILDCONFIG.gn

Similar to ChromeOS, Chromecast needs to be able to set an arbitrary
toolchain as the default. This solution allows the path to that
toolchain to be set as a build arg.

BUG=

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

[slan, 2015-09-25 22:09:05]

slan@chromium.org changed reviewers:
+ dpranke@chromium.org, halliwell@chromium.org

[slan, 2015-09-25 22:09:05]

Dirk,

We need a way to point to an internal toolchain. ChromeOS builds do this by
hardcoding to a generalized gcc_toolchain. We could probably do this too, or
even use //build/toolchain/cros:target, but that would require putting
is_chromecast in BUILDCONFIG.gn. This change is my attempt at avoiding that.

PTAL

[Dirk Pranke, 2015-09-28 19:16:33]

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

[Dirk Pranke, 2015-09-28 19:16:34]

This seems reasonable to me, but I'd like brettw to review this as well.

https://codereview.chromium.org/1373593003/diff/1/build/config/gcc/gcc_versio...
File build/config/gcc/gcc_version.gni (right):

https://codereview.chromium.org/1373593003/diff/1/build/config/gcc/gcc_versio...
build/config/gcc/gcc_version.gni:6: # This allows the gcc_version to be
overriden when using a custom toolchanin
typo: "toolchanin"

[slan, 2015-09-28 19:44:43]

https://codereview.chromium.org/1373593003/diff/1/build/config/gcc/gcc_versio...
File build/config/gcc/gcc_version.gni (right):

https://codereview.chromium.org/1373593003/diff/1/build/config/gcc/gcc_versio...
build/config/gcc/gcc_version.gni:6: # This allows the gcc_version to be
overriden when using a custom toolchanin
On 2015/09/28 19:16:33, Dirk Pranke wrote:
> typo: "toolchanin"

Done.

[brettw, 2015-09-28 19:58:28]

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

https://codereview.chromium.org/1373593003/diff/20001/build/config/BUILDCONFI...
build/config/BUILDCONFIG.gn:140: custom_toolchain = ""
We need to keep this file cleaner than this. Please restructure the cros code so
that everybody uses the same custom_toolchain variable.

https://codereview.chromium.org/1373593003/diff/20001/build/config/BUILDCONFI...
build/config/BUILDCONFIG.gn:566: set_default_toolchain(custom_toolchain)
let's restructure the code above to assign to a variable _default_toolchain and
then call set_default_toolchain once at the bottom, either with that value, or
the custom one. It seems like poor form to call a function and then call it
again with a different value.

[slan, 2015-09-28 20:50:40]

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

https://codereview.chromium.org/1373593003/diff/20001/build/config/BUILDCONFI...
build/config/BUILDCONFIG.gn:140: custom_toolchain = ""
On 2015/09/28 19:58:28, brettw wrote:
> We need to keep this file cleaner than this. Please restructure the cros code
so
> that everybody uses the same custom_toolchain variable.

There are two paths around this redundancy:

1) All ChromeOS builds use |custom_toolchain|, and |cros_use_custom_toolchain|
goes away.

The reason that I did not go straight to this solution is that it will break any
ChromeOS build relying on |cros_use_custom_toolchain|. Although this should have
minimal impact in Chrome, it will likely have a huge impact on the CrOS internal
build infrastructure. 

2) Allow chromecast builds to utilize //build/toolchain/cros:target as the
default toolchain. 

This is the path of least resistance, but it a) forces us to introduce
is_chromecast here, and b) is non-intuitive. I would view this as a temporary
solution, but I would prefer it to blocking this patch on  the issues presented
by #1

Which idea did you have in mind?

I will reach out to a couple of CrOS folks to figure out what needs to happen if
we go with #1.

https://codereview.chromium.org/1373593003/diff/20001/build/config/BUILDCONFI...
build/config/BUILDCONFIG.gn:566: set_default_toolchain(custom_toolchain)
On 2015/09/28 19:58:28, brettw wrote:
> let's restructure the code above to assign to a variable _default_toolchain
and
> then call set_default_toolchain once at the bottom, either with that value, or
> the custom one. It seems like poor form to call a function and then call it
> again with a different value.

Done.

[brettw, 2015-09-28 20:53:55]

I was thinking about #1. I don't know if the chromecast flag is used currently
in practice, and if it doesn't break anything on our waterfall, we should feel
free to change it.

[slan, 2015-09-28 21:21:42]

ChromeOS is not yet using this flag. Removed it to reduce redundancy.

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

https://codereview.chromium.org/1373593003/diff/20001/build/config/BUILDCONFI...
build/config/BUILDCONFIG.gn:140: custom_toolchain = ""
On 2015/09/28 20:50:40, slan wrote:
> On 2015/09/28 19:58:28, brettw wrote:
> > We need to keep this file cleaner than this. Please restructure the cros
code
> so
> > that everybody uses the same custom_toolchain variable.
> 
> There are two paths around this redundancy:
> 
> 1) All ChromeOS builds use |custom_toolchain|, and |cros_use_custom_toolchain|
> goes away.
> 
> The reason that I did not go straight to this solution is that it will break
any
> ChromeOS build relying on |cros_use_custom_toolchain|. Although this should
have
> minimal impact in Chrome, it will likely have a huge impact on the CrOS
internal
> build infrastructure. 
> 
> 2) Allow chromecast builds to utilize //build/toolchain/cros:target as the
> default toolchain. 
> 
> This is the path of least resistance, but it a) forces us to introduce
> is_chromecast here, and b) is non-intuitive. I would view this as a temporary
> solution, but I would prefer it to blocking this patch on  the issues
presented
> by #1
> 
> Which idea did you have in mind?
> 
> I will reach out to a couple of CrOS folks to figure out what needs to happen
if
> we go with #1.
> 

Removed.

[brettw, 2015-09-28 21:25:29]

lgtm

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

https://codereview.chromium.org/1373593003/diff/60001/build/config/BUILDCONFI...
build/config/BUILDCONFIG.gn:549: } else if (_default_toolchain != "") {
I think it's OK to remove this empty string check and also the initial
assignment to the empty string. GN will give you an error if you don't call
set_default_toolchain in this file. If somebody screws it up, giving the error
dereferencing an unknown variable might actually be more clear in this case.

[slan, 2015-09-28 21:41:17]

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

https://codereview.chromium.org/1373593003/diff/60001/build/config/BUILDCONFI...
build/config/BUILDCONFIG.gn:549: } else if (_default_toolchain != "") {
On 2015/09/28 21:25:29, brettw wrote:
> I think it's OK to remove this empty string check and also the initial
> assignment to the empty string. GN will give you an error if you don't call
> set_default_toolchain in this file. If somebody screws it up, giving the error
> dereferencing an unknown variable might actually be more clear in this case.

Done.

[slan, 2015-09-28 21:41:25]

The CQ bit was checked by slan@chromium.org

[slan, 2015-09-28 21:41:25]

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/1373593003/#ps80001
(title: "Remove empty-string check on _default_toolchain")

[commit-bot: I haz the power, 2015-09-28 21:43:32]

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

[commit-bot: I haz the power, 2015-09-28 21:59:32]

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

[commit-bot: I haz the power, 2015-09-28 21:59:33]

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_...)

[slan, 2015-09-28 23:47:29]

On 2015/09/28 21:59:33, commit-bot: I haz the power wrote:
> 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_...)

set_default_toolchain() is not called for nacl components on Linux builds. I
have reinserted the empty-string check to keep behavior consistent for these
builds.

[Dirk Pranke, 2015-09-29 00:01:10]

On 2015/09/28 23:47:29, slan wrote:
> On 2015/09/28 21:59:33, commit-bot: I haz the power wrote:
> > 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_...)
> 
> set_default_toolchain() is not called for nacl components on Linux builds. I
> have reinserted the empty-string check to keep behavior consistent for these
> builds.

I think the correct fix for this is probably to assert that default_toolchain
has already been set in the is_nacl block,
and then assert that either default_toolchain != "" or _default_toolchain != "",
and then call set_default_toolchain(_default_toolchain)
if the latter is true.

[slan, 2015-09-29 01:12:15]

On 2015/09/29 00:01:10, Dirk Pranke wrote:
> On 2015/09/28 23:47:29, slan wrote:
> > On 2015/09/28 21:59:33, commit-bot: I haz the power wrote:
> > > 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_...)
> > 
> > set_default_toolchain() is not called for nacl components on Linux builds. I
> > have reinserted the empty-string check to keep behavior consistent for these
> > builds.
> 
> I think the correct fix for this is probably to assert that default_toolchain
> has already been set in the is_nacl block,
> and then assert that either default_toolchain != "" or _default_toolchain !=
"",
> and then call set_default_toolchain(_default_toolchain)
> if the latter is true.

I tried this before submitting my latest patch. Oddly, the following assertion
fails inside the nacl block, independently of my patch:

  assert(defined(default_toolchain))

I need to better understand the nacl build. What is the mechanism that is
supposed to shout at you if you have not defined the default_toolchain?

[slan, 2015-09-29 16:03:22]

The CQ bit was checked by slan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-29 16:03:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373593003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373593003/100001

[commit-bot: I haz the power, 2015-09-29 16:35:54]

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

[commit-bot: I haz the power, 2015-09-29 16:35:55]

Dry run: This issue passed the CQ dry run.

[slan, 2015-10-01 18:26:58]

On 2015/09/29 01:12:15, slan wrote:
> On 2015/09/29 00:01:10, Dirk Pranke wrote:
> > On 2015/09/28 23:47:29, slan wrote:
> > > On 2015/09/28 21:59:33, commit-bot: I haz the power wrote:
> > > > 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_...)
> > > 
> > > set_default_toolchain() is not called for nacl components on Linux builds.
I
> > > have reinserted the empty-string check to keep behavior consistent for
these
> > > builds.
> > 
> > I think the correct fix for this is probably to assert that
default_toolchain
> > has already been set in the is_nacl block,
> > and then assert that either default_toolchain != "" or _default_toolchain !=
> "",
> > and then call set_default_toolchain(_default_toolchain)
> > if the latter is true.
> 
> I tried this before submitting my latest patch. Oddly, the following assertion
> fails inside the nacl block, independently of my patch:
> 
>   assert(defined(default_toolchain))
> 
> I need to better understand the nacl build. What is the mechanism that is
> supposed to shout at you if you have not defined the default_toolchain?

Ping for feedback here. The default toolchain is not set on a nacl build, which
is respected by the latest patch. Are we clear to land this? Or does this
warrant more investigation?

[Dirk Pranke, 2015-10-01 20:04:05]

On 2015/10/01 18:26:58, slan wrote:
> On 2015/09/29 01:12:15, slan wrote:
> > I tried this before submitting my latest patch. Oddly, the following
assertion
> > fails inside the nacl block, independently of my patch:
> > 
> >   assert(defined(default_toolchain))
> > 
> > I need to better understand the nacl build. What is the mechanism that is
> > supposed to shout at you if you have not defined the default_toolchain?
> 
> Ping for feedback here. The default toolchain is not set on a nacl build,
which
> is respected by the latest patch. Are we clear to land this? Or does this
> warrant more investigation?

Sorry, I dropped this on the floor. 

Brett, should default_toolchain be set when BUILDCONFIG.gn is being re-processed
for secondary toolchains, or is the value not available?

[brettw, 2015-10-01 20:28:57]

On 2015/10/01 18:26:58, slan wrote:
> Ping for feedback here. The default toolchain is not set on a nacl build,
which
> is respected by the latest patch. Are we clear to land this? Or does this
> warrant more investigation?

This is built-in. The first invocation of BUILDCONFIG.gn must call
set_default_toolchain or GN will error out (since it doesn't know what to do).

I couldn't say whether the variable default_toolchain will be set in secondary
invocations, it doesn't surprise me that it's not, and it doesn't seem
necessary.

[brettw, 2015-10-01 20:29:37]

lgtm

[Dirk Pranke, 2015-10-01 20:29:53]

ok, lgtm then as well.

[slan, 2015-10-01 20:32:25]

The CQ bit was checked by slan@chromium.org

[commit-bot: I haz the power, 2015-10-01 20:33:37]

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

[commit-bot: I haz the power, 2015-10-01 20:43:04]

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

[commit-bot: I haz the power, 2015-10-01 20:43:05]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[slan, 2015-10-01 21:47:15]

On 2015/10/01 20:43:05, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

Seems win trybots are flaking massively:
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...

Will circle back in a bit to retry.

[slan, 2015-10-01 23:01:14]

The CQ bit was checked by slan@chromium.org

[commit-bot: I haz the power, 2015-10-01 23:01:33]

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

[commit-bot: I haz the power, 2015-10-01 23:13:17]

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

[commit-bot: I haz the power, 2015-10-01 23:13:18]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[slan, 2015-10-02 14:31:02]

The CQ bit was checked by slan@chromium.org

[commit-bot: I haz the power, 2015-10-02 14:32:23]

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

[commit-bot: I haz the power, 2015-10-02 15:05:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-02 15:06:40]

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