add nacl_toolchain to //build and flip to using it in the GN build.

Add nacl_toolchain to //build and flip to using it in the GN Build.

With this change, in the Chromium repo NaCl will now be built solely using Chromium's versions of the GN build files, rather than a mixture of chromium and NaCl files.

This will allow us to delete the NaCl versions in a subsequent NaCl-side CL and switch to pulling the chromium versions in via a DEPS entry, and thus "unfork" the build.

R=brettw@chromium.org, ncbray@chromium.org, mcgrathr@chromium.org
BUG=433528
Committed: https://crrev.com/fa6ffe2fb9f488ddb859555f5abc29de72407a62
Cr-Commit-Position: refs/heads/master@{#346452}

[Dirk Pranke, 2015-08-28 02:29:10]

dpranke@chromium.org changed reviewers:
+ brettw@chromium.org, mcgrathr@chromium.org, ncbray@chromium.org

[Dirk Pranke, 2015-08-28 02:29:41]

This patch depends on the gcc_toolchain changes in
https://codereview.chromium.org/1322523008/ landing first.

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/BUILD.gn
File build/config/nacl/BUILD.gn (right):

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/BUILD...
build/config/nacl/BUILD.gn:1: # Copyright (c) 2014 The Native Client Authors.
All rights reserved.
This file is simply copied over from //native_client/build/config/nacl/BUILD.gn

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/nacl_...
File build/config/nacl/nacl_defines.gni (right):

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/nacl_...
build/config/nacl/nacl_defines.gni:1: # Copyright (c) 2014 The Chromium Authors.
All rights reserved.
This file is simply copied over from
//native_client/build/config/nacl/nacl_defines.gni .

A comment from mseaborn yesterday indicated that much of this is no longer
needed, but I would prefer to clean that up in a subsequent patch.

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

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl/BU...
build/toolchain/nacl/BUILD.gn:5: import("//build/config/sysroot.gni")
The old version of this file was way out of date and not being used.

The new version of this file is mostly cloned over from
//native_client/build/toolchain/nacl/BUILD.gn, but is updated in a couple of
places to match the new gcc_toolchain parameter names.

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
File build/toolchain/nacl_toolchain.gni (right):

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
build/toolchain/nacl_toolchain.gni:1: # Copyright (c) 2014 The Native Client
Authors. All rights reserved.
This file is copied over from
//native_client/build/toolchain/nacl_toolchain.gni, but updated w/ the parameter
names for the latest gcc_toolchain.gni.

[brettw, 2015-08-28 18:25:52]

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/BUILD.gn
File build/config/nacl/BUILD.gn (right):

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/BUILD...
build/config/nacl/BUILD.gn:10: include_dirs = [ "//." ]
This should come from //build/config/compiler:default_include_dirs which should
be applied by default to all targets. So you can remove it.

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/nacl_...
File build/config/nacl/nacl_defines.gni (right):

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/nacl_...
build/config/nacl/nacl_defines.gni:5: nacl_defines = [
I don't see why this is in a .gni file and not just applied inline in the
nacl_defines config. Unless you can see an obvious use, we should just move this
there.

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/nacl_...
build/config/nacl/nacl_defines.gni:12: "_LARGEFILE64_SOURCE=1",
Looks like this is already defined in build/config/compiler:compiler for all
posix builds. I think you can remove this line.

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/nacl_...
build/config/nacl/nacl_defines.gni:14: "__STDC_FORMAT_MACROS=1",
__STDC_FORMAT_MACROS is set automatically by chromium_code. I think the relevant
nacl targets should be compiled with this config, so you can remove it.

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

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl/BU...
build/toolchain/nacl/BUILD.gn:9: nacl_toolchain_dir =
rebase_path("//native_client/toolchain", root_build_dir)
Is this ever set outside of the build today? I can't imagine it is. If I'm
correct, can you remove the declare_args?

If we need this, the current definition needs documentation and also changing.
The current definition is very weird, you have to specify a path relative to
your build directory to the toolchain. Instead, we should probably take an input
as a regular path and then rebase it to be relative to the build directory when
needed.

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
File build/toolchain/nacl_toolchain.gni (right):

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
build/toolchain/nacl_toolchain.gni:9: # The template defines both a "raw" and
"cooked" toolchain.  These toolchains
I don't understand this comment, I see only one toolchain in this template
definition.

[brettw, 2015-08-28 19:08:38]

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/nacl_...
File build/config/nacl/nacl_defines.gni (right):

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/nacl_...
build/config/nacl/nacl_defines.gni:1: # Copyright (c) 2014 The Chromium Authors.
All rights reserved.
On 2015/08/28 02:29:41, Dirk Pranke wrote:
> This file is simply copied over from
> //native_client/build/config/nacl/nacl_defines.gni .
> 
> A comment from mseaborn yesterday indicated that much of this is no longer
> needed, but I would prefer to clean that up in a subsequent patch.

Just noticed this... it seems just as easy to copy-and-paste this into the
nacl_defines target. I'm not sure why this would be easier to clean up in a
separate step.

[Dirk Pranke, 2015-08-28 20:08:32]

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/BUILD.gn
File build/config/nacl/BUILD.gn (right):

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/BUILD...
build/config/nacl/BUILD.gn:10: include_dirs = [ "//." ]
On 2015/08/28 18:25:52, brettw wrote:
> This should come from //build/config/compiler:default_include_dirs which
should
> be applied by default to all targets. So you can remove it.

Acknowledged.

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/nacl_...
File build/config/nacl/nacl_defines.gni (right):

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/nacl_...
build/config/nacl/nacl_defines.gni:5: nacl_defines = [
On 2015/08/28 18:25:52, brettw wrote:
> I don't see why this is in a .gni file and not just applied inline in the
> nacl_defines config. Unless you can see an obvious use, we should just move
this
> there.

Acknowledged.

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/nacl_...
build/config/nacl/nacl_defines.gni:12: "_LARGEFILE64_SOURCE=1",
On 2015/08/28 18:25:52, brettw wrote:
> Looks like this is already defined in build/config/compiler:compiler for all
> posix builds. I think you can remove this line.

Acknowledged.

https://codereview.chromium.org/1312813003/diff/20001/build/config/nacl/nacl_...
build/config/nacl/nacl_defines.gni:14: "__STDC_FORMAT_MACROS=1",
On 2015/08/28 18:25:52, brettw wrote:
> __STDC_FORMAT_MACROS is set automatically by chromium_code. I think the
relevant
> nacl targets should be compiled with this config, so you can remove it.

Acknowledged.

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

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl/BU...
build/toolchain/nacl/BUILD.gn:9: nacl_toolchain_dir =
rebase_path("//native_client/toolchain", root_build_dir)
On 2015/08/28 18:25:52, brettw wrote:
> Is this ever set outside of the build today? I can't imagine it is. If I'm
> correct, can you remove the declare_args?
> 
> If we need this, the current definition needs documentation and also changing.
> The current definition is very weird, you have to specify a path relative to
> your build directory to the toolchain. Instead, we should probably take an
input
> as a regular path and then rebase it to be relative to the build directory
when
> needed.

I'd defer this one to the NaCl folks ...

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
File build/toolchain/nacl_toolchain.gni (right):

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
build/toolchain/nacl_toolchain.gni:9: # The template defines both a "raw" and
"cooked" toolchain.  These toolchains
On 2015/08/28 18:25:52, brettw wrote:
> I don't understand this comment, I see only one toolchain in this template
> definition.

I think this comment might be out of date. I will check and confirm.

[Dirk Pranke, 2015-08-28 20:37:08]

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

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl/BU...
build/toolchain/nacl/BUILD.gn:9: nacl_toolchain_dir =
rebase_path("//native_client/toolchain", root_build_dir)
ok, after checking with them, we think that we will need some way in the nacl
builds to specify that we want to build with custom toolchains, but this may not
be the right mechanism for doing so.

I will move this out of the declare_args() block for now.

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
File build/toolchain/nacl_toolchain.gni (right):

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
build/toolchain/nacl_toolchain.gni:9: # The template defines both a "raw" and
"cooked" toolchain.  These toolchains
On 2015/08/28 20:08:32, Dirk Pranke wrote:
> I think this comment might be out of date. I will check and confirm.

Confirmed. Will clean it up.

[Dirk Pranke, 2015-08-28 20:53:14]

updated; please take another look?

[Roland McGrath, 2015-08-28 20:58:40]

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

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl/BU...
build/toolchain/nacl/BUILD.gn:34: nacl_toolchain("newlib_arm") {
When I started hacking on this stuff, I made nacl_toolchain take more args
(name="newlib_arm", rev=nacl_arm_newlib_rev, cpu="arm", tuple="arm-nacl",
is_clang) so thre was much less repeated boilerplate.  Only pnacl really needs
more code of its own.

Also, the current norm is to use nacl-clang rather than gcc for all these newlib
cases.  It's toolprefix + "clang"/"clang++" and is_clang=true but otherwise the
same as you have here.

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl/BU...
build/toolchain/nacl/BUILD.gn:140: link_irt =
rebase_path("//native_client/build/link_irt.py", root_build_dir)
This needs comments about what it is and why it exists.

Also, I used a template("nacl_irt_toolchain") to reduce this boilerplate.

These are cleanups I can make afterwards if you prefer.

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
File build/toolchain/nacl_toolchain.gni (right):

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
build/toolchain/nacl_toolchain.gni:31: if (defined(invoker.is_clang)) {
Use forward_variables_from for this, postlink, and link_outputs.
Less code to use it for cc et al too (below), and equivalent after the asserts
above.

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
build/toolchain/nacl_toolchain.gni:52: # NaCl toolchains do not support shared
libraries and so they don't support
NaCl glibc toolchains do support shared libraries.  Perhaps we don't want to
support component builds even for those, but this statement is not entirely
accurate.

https://codereview.chromium.org/1312813003/diff/20001/third_party/mojo/src/na...
File third_party/mojo/src/nacl_bindings/BUILD.gn (right):

https://codereview.chromium.org/1312813003/diff/20001/third_party/mojo/src/na...
third_party/mojo/src/nacl_bindings/BUILD.gn:10: "mojo_syscall.cc",
This change seems unrelated.  Why is it in this CL?

[Dirk Pranke, 2015-08-28 21:17:05]

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

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl/BU...
build/toolchain/nacl/BUILD.gn:34: nacl_toolchain("newlib_arm") {
On 2015/08/28 20:58:40, Roland McGrath wrote:
> When I started hacking on this stuff, I made nacl_toolchain take more args
> (name="newlib_arm", rev=nacl_arm_newlib_rev, cpu="arm", tuple="arm-nacl",
> is_clang) so thre was much less repeated boilerplate.  Only pnacl really needs
> more code of its own.
> 
> Also, the current norm is to use nacl-clang rather than gcc for all these
newlib
> cases.  It's toolprefix + "clang"/"clang++" and is_clang=true but otherwise
the
> same as you have here.

Acknowledged.

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl/BU...
build/toolchain/nacl/BUILD.gn:140: link_irt =
rebase_path("//native_client/build/link_irt.py", root_build_dir)
On 2015/08/28 20:58:40, Roland McGrath wrote:
> This needs comments about what it is and why it exists.
> 
> Also, I used a template("nacl_irt_toolchain") to reduce this boilerplate.
> 
> These are cleanups I can make afterwards if you prefer.

Acknowledged. I would prefer it if you can make those changes afterwards.

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
File build/toolchain/nacl_toolchain.gni (right):

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
build/toolchain/nacl_toolchain.gni:31: if (defined(invoker.is_clang)) {
On 2015/08/28 20:58:40, Roland McGrath wrote:
> Use forward_variables_from for this, postlink, and link_outputs.
> Less code to use it for cc et al too (below), and equivalent after the asserts
> above.

Ack. I debated whether to change that or not as part of this CL, but was erring
on the side
of "minimize changes to the file until after we've de-forked".

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
build/toolchain/nacl_toolchain.gni:52: # NaCl toolchains do not support shared
libraries and so they don't support
On 2015/08/28 20:58:40, Roland McGrath wrote:
> NaCl glibc toolchains do support shared libraries.  Perhaps we don't want to
> support component builds even for those, but this statement is not entirely
> accurate.

I see. I have a vague memory of ncbray telling me otherwise. I'm happy to change
to 
something else; what would you prefer?

https://codereview.chromium.org/1312813003/diff/20001/third_party/mojo/src/na...
File third_party/mojo/src/nacl_bindings/BUILD.gn (right):

https://codereview.chromium.org/1312813003/diff/20001/third_party/mojo/src/na...
third_party/mojo/src/nacl_bindings/BUILD.gn:10: "mojo_syscall.cc",
On 2015/08/28 20:58:40, Roland McGrath wrote:
> This change seems unrelated.  Why is it in this CL?

It's just a sorting change caused by running `gn format` on the file (which
Chromium has as a presubmit trigger.

[brettw, 2015-08-28 22:22:28]

lgtm

[Roland McGrath, 2015-08-28 22:35:58]

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
File build/toolchain/nacl_toolchain.gni (right):

https://codereview.chromium.org/1312813003/diff/20001/build/toolchain/nacl_to...
build/toolchain/nacl_toolchain.gni:52: # NaCl toolchains do not support shared
libraries and so they don't support
On 2015/08/28 21:17:05, Dirk Pranke wrote:
> On 2015/08/28 20:58:40, Roland McGrath wrote:
> > NaCl glibc toolchains do support shared libraries.  Perhaps we don't want to
> > support component builds even for those, but this statement is not entirely
> > accurate.
> 
> I see. I have a vague memory of ncbray telling me otherwise. I'm happy to
change
> to 
> something else; what would you prefer?

ncbray disclaims all knowledge of this particular bit of history, and the
history shows it came from https://codereview.chromium.org/1121643002 and ncbray
was never involved!  So there.  That said, and with some latent pedanticism
addressed locally, it's fine enough to leave this as it is for now.  I think the
conclusion "we never want NaCl toolchains to build component builds" is probably
correct regardless.

[Dirk Pranke, 2015-08-31 19:25:31]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-08-31 19:25:31]

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/1312813003/#ps60001
(title: "tweak component build wording in comment")

[commit-bot: I haz the power, 2015-08-31 19:25:46]

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

[commit-bot: I haz the power, 2015-08-31 19:27:38]

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

[commit-bot: I haz the power, 2015-08-31 19:27:39]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Dirk Pranke, 2015-08-31 19:45:21]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-08-31 19:45:22]

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/1312813003/#ps80001
(title: "merge to #346439")

[commit-bot: I haz the power, 2015-08-31 19:45:37]

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

[commit-bot: I haz the power, 2015-08-31 20:18:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-31 20:18:59]

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