GN: Fix NaCl IRT build options

GN: Fix NaCl IRT build options

The NaCl IRT is a special case and needs to be built with specific
code-generation and optimization options (we also always want debug
symbols).  This plumbs through an is_nacl_irt variable that configs
can test, and uses that to make the standard configs do the right
thing.  The resulting compiler and linker flags are a close
approximation of what GYP uses.

BUG=531702
R=dpranke@chromium.org, dschuff@chromium.org

Committed: https://crrev.com/0b9de3622acdbc1fba625094b369090f65145b78
Cr-Commit-Position: refs/heads/master@{#353385}

[Dirk Pranke, 2015-10-09 00:17:49]

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

[Dirk Pranke, 2015-10-09 00:17:49]

It seems like we could avoid having to declare is_nacl_irt as an arg if you
modify build/config/nacl/config.gni so that is_nacl_irt is just a variable set
to true depending on the value of the current toolchain.

That would keep us from cluttering up the arg namespace with something that'll
never be explicitly set on the command line, which I tend to prefer. The
downside is that it would require hardcoding toolchain names. 

Brett, do you have any thoughts on the right way to do this?

The change looks fine otherwise.

[Roland McGrath, 2015-10-09 16:54:12]

On 2015/10/09 00:17:49, Dirk Pranke wrote:
> It seems like we could avoid having to declare is_nacl_irt as an arg if you
> modify build/config/nacl/config.gni so that is_nacl_irt is just a variable set
> to true depending on the value of the current toolchain.
> 
> That would keep us from cluttering up the arg namespace with something that'll
> never be explicitly set on the command line, which I tend to prefer. The
> downside is that it would require hardcoding toolchain names. 

I followed the model of is_nacl_glibc.  I think the review discussion for that
covered these same issues and came to the conclusion of using the toolchain_args
approach.  But I'm happy to do it however you like.  I'm sure I can get the
alternate approach working easily enough.

[Roland McGrath, 2015-10-09 18:22:25]

PTAL, now using current_toolchain test rather than toolchain_args for
is_nacl_irt value

[Dirk Pranke, 2015-10-09 19:25:53]

The second patchset lgtm as well, apart from the one nit I noticed.

I remember having a discussion about whether declare_args() and toolchain_args()
should be preferred or not, but I remember nothing about the content of the
discussion or the conclusion :(.

Let me see if I can sync up w/ brett to reach a conclusion. You should feel free
to land either patch in the meantime if you like (we can presumably always fix
things later if we decide it matters and reach a different stance).

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

https://codereview.chromium.org/1397953002/diff/20001/build/config/nacl/BUILD...
build/config/nacl/BUILD.gn:5: import("config.gni")
imports should always be src-relative paths, i.e.,
import("//build/config/nacl/config.gni").

[Dirk Pranke, 2015-10-09 19:44:23]

okay, brettw and I chatted and we both prefer the non-declare_arg() version
(i.e., avoid 
adding new args when it's reasonable to do so). In this case, since it's one
line of code
to avoid needing the arg, we should therefore avoid the arg.

As I noted, I don't remember the contents of the previous discussion. If anyone
does
and can point to good counterarguments, though, I think we're flexible on this.

[Roland McGrath, 2015-10-09 20:35:26]

I was thinking of https://codereview.chromium.org/1333673002/ but the discussion
there doesn't seem to include any suggestions contrary to the is_nacl_glibc
variable begin in declare_args/toolchain_args.  Perhaps I misremembered or
perhaps that discussion was somewhere else.  Petr might recall for our
edification.

But in the meantime I'll just land the version that tests current_toolchain.

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

https://codereview.chromium.org/1397953002/diff/20001/build/config/nacl/BUILD...
build/config/nacl/BUILD.gn:5: import("config.gni")
On 2015/10/09 19:25:53, Dirk Pranke wrote:
> imports should always be src-relative paths, i.e.,
> import("//build/config/nacl/config.gni").

Done.

[Roland McGrath, 2015-10-09 20:35:44]

The CQ bit was checked by mcgrathr@chromium.org

[Roland McGrath, 2015-10-09 20:35:44]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1397953002/#ps40001
(title: "review nit")

[commit-bot: I haz the power, 2015-10-09 20:36:02]

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

[Roland McGrath, 2015-10-09 20:44:55]

+bbudge for ppapi/OWNERS review

I'm not at all sure why that change is necessary now when it wasn't before.  I
guess GN changed?  I got an error about nmf_cpu being unreferenced after it was
set, which was always happening in the pnacl toolchain instantiation but didn't
cause a GN error when that code first went in.

[commit-bot: I haz the power, 2015-10-09 20:46:00]

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

[commit-bot: I haz the power, 2015-10-09 20:46:01]

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

[bbudge, 2015-10-09 20:59:11]

bbudge@chromium.org changed reviewers:
+ bbudge@chromium.org

[bbudge, 2015-10-09 20:59:11]

ppapi lgtm

[Roland McGrath, 2015-10-09 20:59:43]

The CQ bit was checked by mcgrathr@chromium.org

[commit-bot: I haz the power, 2015-10-09 21:03:17]

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

[commit-bot: I haz the power, 2015-10-09 21:23:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-09 21:23:58]

Patchset 3 (id:??) landed as
https://crrev.com/0b9de3622acdbc1fba625094b369090f65145b78
Cr-Commit-Position: refs/heads/master@{#353385}