Add a way for CrOS toolchains to set custom flags for the nacl bootstrap.

Add a way for CrOS toolchains to set custom flags for the nacl bootstrap.

R=llozano@chromium.org, bradnelson@chromium.org
BUG=629593
Committed: https://crrev.com/2ffe82493c719eb280aa895608f16ae80898ad68
Cr-Commit-Position: refs/heads/master@{#411350}

[bradnelson, 2016-07-28 05:10:26]

The CQ bit was checked by bradnelson@chromium.org

[bradnelson, 2016-07-28 05:10:26]

lgtm

[commit-bot: I haz the power, 2016-07-28 05:10:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 05:13:43]

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

[commit-bot: I haz the power, 2016-07-28 05:13:44]

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

[Dirk Pranke, 2016-07-28 05:35:23]

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

[commit-bot: I haz the power, 2016-07-28 05:35:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dirk Pranke, 2016-07-28 05:36:22]

Okay, the first version of this patch was all kinds of wrong, gn-wise.

The second version should be closer to right; let's see what the CQ dryrun does,
but also wait to hear from llozano (or someone else) that the patch works before
trying to land this.

[commit-bot: I haz the power, 2016-07-28 06:35:29]

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

[commit-bot: I haz the power, 2016-07-28 06:35:29]

Dry run: This issue passed the CQ dry run.

[llozano1, 2016-07-28 17:12:24]

On 2016/07/28 05:36:22, Dirk Pranke wrote:
> Okay, the first version of this patch was all kinds of wrong, gn-wise.
> 
> The second version should be closer to right; let's see what the CQ dryrun
does,
> but also wait to hear from llozano (or someone else) that the patch works
before
> trying to land this.

trying it.

[llozano1, 2016-07-28 17:20:55]

llozano@google.com changed reviewers:
+ llozano@google.com

[llozano1, 2016-07-28 17:20:56]

https://codereview.chromium.org/2188633004/diff/20001/build/toolchain/cros/BU...
File build/toolchain/cros/BUILD.gn (right):

https://codereview.chromium.org/2188633004/diff/20001/build/toolchain/cros/BU...
build/toolchain/cros/BUILD.gn:51: gcc_toolchain("nacl_bootstrap") {
why the name change? does this mean this will not work when using the clang
compiler?

[Dirk Pranke, 2016-07-28 17:23:54]

https://codereview.chromium.org/2188633004/diff/20001/build/toolchain/cros/BU...
File build/toolchain/cros/BUILD.gn (right):

https://codereview.chromium.org/2188633004/diff/20001/build/toolchain/cros/BU...
build/toolchain/cros/BUILD.gn:51: gcc_toolchain("nacl_bootstrap") {
On 2016/07/28 17:20:56, llozano1 wrote:
> why the name change? does this mean this will not work when using the clang
> compiler?

No, it's fine. All I did was get rid of the template and expand the work the
template was doing, as there was less than 50% overlap between the two
toolchains with these changes.

The "clang_toolchain" template really just sets is_clang=true, and should go
away soon. This patch should not affect using clang or gcc.

[llozano1, 2016-08-01 08:09:10]

On 2016/07/28 17:23:54, Dirk Pranke wrote:
>
https://codereview.chromium.org/2188633004/diff/20001/build/toolchain/cros/BU...
> File build/toolchain/cros/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2188633004/diff/20001/build/toolchain/cros/BU...
> build/toolchain/cros/BUILD.gn:51: gcc_toolchain("nacl_bootstrap") {
> On 2016/07/28 17:20:56, llozano1 wrote:
> > why the name change? does this mean this will not work when using the clang
> > compiler?
> 
> No, it's fine. All I did was get rid of the template and expand the work the
> template was doing, as there was less than 50% overlap between the two
> toolchains with these changes.
> 
> The "clang_toolchain" template really just sets is_clang=true, and should go
> away soon. This patch should not affect using clang or gcc.

I think we still need this CL. 
Nacl bootstrap build is very particular. We should not be passing any extra
options to it.
(The title of this CL is a little misleading)

[Dirk Pranke, 2016-08-01 20:47:21]

On 2016/08/01 08:09:10, llozano1 wrote:
> On 2016/07/28 17:23:54, Dirk Pranke wrote:
> >
>
https://codereview.chromium.org/2188633004/diff/20001/build/toolchain/cros/BU...
> > File build/toolchain/cros/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2188633004/diff/20001/build/toolchain/cros/BU...
> > build/toolchain/cros/BUILD.gn:51: gcc_toolchain("nacl_bootstrap") {
> > On 2016/07/28 17:20:56, llozano1 wrote:
> > > why the name change? does this mean this will not work when using the
clang
> > > compiler?
> > 
> > No, it's fine. All I did was get rid of the template and expand the work the
> > template was doing, as there was less than 50% overlap between the two
> > toolchains with these changes.
> > 
> > The "clang_toolchain" template really just sets is_clang=true, and should go
> > away soon. This patch should not affect using clang or gcc.
> 
> I think we still need this CL. 
> Nacl bootstrap build is very particular. We should not be passing any extra
> options to it.
> (The title of this CL is a little misleading)

I think there's two different requirements here, and it's not clear which you
actually need.

1) The ability to add flags for just the one file

2) The ability to make sure that none of the flags applied to the other files
are applied to this file.

This CL gets you both features, at the cost of having to specify yet another set
of GN args.

At some point in the reasonably-near future I think we're going to rework how
all of these flags and
toolchains are defined to get rid of a lot of the redundancy, but I don't know
what that's going to look
like yet so we don't want to wait for it.

So, I think we should land this as-is and iterate down the road.

[Dirk Pranke, 2016-08-01 20:53:09]

https://codereview.chromium.org/2188633004/diff/20001/build/toolchain/cros/BU...
File build/toolchain/cros/BUILD.gn (right):

https://codereview.chromium.org/2188633004/diff/20001/build/toolchain/cros/BU...
build/toolchain/cros/BUILD.gn:51: gcc_toolchain("nacl_bootstrap") {
On 2016/07/28 17:23:53, Dirk Pranke wrote:
> On 2016/07/28 17:20:56, llozano1 wrote:
> > why the name change? does this mean this will not work when using the clang
> > compiler?
> 
> No, it's fine. All I did was get rid of the template and expand the work the
> template was doing, as there was less than 50% overlap between the two
> toolchains with these changes.
> 
> The "clang_toolchain" template really just sets is_clang=true, and should go
> away soon. This patch should not affect using clang or gcc.

Actually, the clang_toolchain() template does two things:

1) sets is_clang=true

2) sets up the toolchain to point to the *chromium-built* version of clang (in
src/third_party/llvm-build).

I want to double-check before I land this: You have your own clang binaries, and
don't want to use the chromium-built ones, right?

[llozano1, 2016-08-01 21:21:17]

On 2016/08/01 20:53:09, Dirk Pranke wrote:
>
https://codereview.chromium.org/2188633004/diff/20001/build/toolchain/cros/BU...
> File build/toolchain/cros/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2188633004/diff/20001/build/toolchain/cros/BU...
> build/toolchain/cros/BUILD.gn:51: gcc_toolchain("nacl_bootstrap") {
> On 2016/07/28 17:23:53, Dirk Pranke wrote:
> > On 2016/07/28 17:20:56, llozano1 wrote:
> > > why the name change? does this mean this will not work when using the
clang
> > > compiler?
> > 
> > No, it's fine. All I did was get rid of the template and expand the work the
> > template was doing, as there was less than 50% overlap between the two
> > toolchains with these changes.
> > 
> > The "clang_toolchain" template really just sets is_clang=true, and should go
> > away soon. This patch should not affect using clang or gcc.
> 
> Actually, the clang_toolchain() template does two things:
> 
> 1) sets is_clang=true
> 
> 2) sets up the toolchain to point to the *chromium-built* version of clang (in
> src/third_party/llvm-build).
> 
> I want to double-check before I land this: You have your own clang binaries,
and
> don't want to use the chromium-built ones, right?

it is a like this: In the ebuild environment, we always use our own clang
binaries for host and target.
in simple chrome workflow, we use the binaries that come with chromium for host
and the ones we have for target.

[Dirk Pranke, 2016-08-02 00:29:57]

I've updated this w/ my current thinking ... please take another look.

[llozano1, 2016-08-03 22:02:38]

lgtm

[Dirk Pranke, 2016-08-11 02:07:21]

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

[commit-bot: I haz the power, 2016-08-11 02:08:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 03:49:58]

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

[commit-bot: I haz the power, 2016-08-11 03:49:59]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2016-08-11 04:38:33]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2016-08-11 04:38:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org, llozano@google.com
Link to the patchset: https://codereview.chromium.org/2188633004/#ps60001
(title: "merge in changes to toolchain_args")

[commit-bot: I haz the power, 2016-08-11 04:38:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 04:40:11]

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

[commit-bot: I haz the power, 2016-08-11 04:40:12]

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

[Dirk Pranke, 2016-08-11 16:23:32]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2016-08-11 16:24:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 16:27:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-11 16:30:43]

Description was changed from

==========
Add a way for CrOS toolchains to set custom flags for the nacl bootstrap.

R=llozano@chromium.org, bradnelson@chromium.org
BUG=629593
==========

to

==========
Add a way for CrOS toolchains to set custom flags for the nacl bootstrap.

R=llozano@chromium.org, bradnelson@chromium.org
BUG=629593

Committed: https://crrev.com/2ffe82493c719eb280aa895608f16ae80898ad68
Cr-Commit-Position: refs/heads/master@{#411350}
==========

[commit-bot: I haz the power, 2016-08-11 16:30:44]

Patchset 4 (id:??) landed as
https://crrev.com/2ffe82493c719eb280aa895608f16ae80898ad68
Cr-Commit-Position: refs/heads/master@{#411350}