Land prep work to enable NaCl in the Linux x64 GN builds.

Land prep work to enable NaCl in the Linux x64 GN builds.

This should get most of NaCl and the PPAPI stuff needed
for NaCl building and linking. There is more work to be
done to get some of the test binaries working (and
probably fill out parts of the NaCl SDK) and possibly
some pnacl support work remaining as well.

NaCl is still disabled by default (set enable_nacl=true
to change). Enabling nacl is still mostly untested and
likely doesn't work at all :).

R=ncbray@chromium.org, brettw@chromium.org
BUG=432959
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:android_chromium_gn_compile_dbg,android_chromium_gn_compile_rel;tryserver.chromium.win:win8_chromium_gn_rel,win8_chromium_gn_dbg;tryserver.chromium.mac:mac_chromium_gn_rel,mac_chromium_gn_dbg

Committed: https://crrev.com/6065cf751340a8a99b670357b2053ed6df12a7af
Cr-Commit-Position: refs/heads/master@{#318180}

[Dirk Pranke, 2015-02-05 00:00:14]

https://codereview.chromium.org/877553008/diff/20001/third_party/harfbuzz-ng/...
File third_party/harfbuzz-ng/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/third_party/harfbuzz-ng/...
third_party/harfbuzz-ng/BUILD.gn:17: 
We need this change to keep GN from complaining that the arg might be defined in
multiple different places w/ multiple different default values.

This edit should produce no actual changes in the logic, I just rewrote it to
(hopefully) be a little clearer.

[Dirk Pranke, 2015-02-05 00:09:00]

Also, this patch needs https://codereview.chromium.org/888903003/ and
https://codereview.chromium.org/893993005/ to land first in order to actually
work.

[brettw, 2015-02-05 00:20:36]

lgtm

https://codereview.chromium.org/877553008/diff/20001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/chrome/test/BUILD.gn#new...
chrome/test/BUILD.gn:713: # "../ppapi/ppapi_nacl.gyp:ppapi_nacl_tests",
You kept these commented out but removed the TODOs. Can you put the TODOs back?
I like to be able to gpre for "TODO(GYP)" to find a list of deps that are
missing.

https://codereview.chromium.org/877553008/diff/20001/chrome/test/BUILD.gn#new...
chrome/test/BUILD.gn:744: # "//components/nacl_nonsfi.gyp:nacl_helper_nonsfi",
Ditto

https://codereview.chromium.org/877553008/diff/20001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/components/nacl/BUILD.gn...
components/nacl/BUILD.gn:1: import("//build/config/features.gni")
Need copyright.

https://codereview.chromium.org/877553008/diff/20001/components/nacl/renderer...
File components/nacl/renderer/plugin/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/components/nacl/renderer...
components/nacl/renderer/plugin/BUILD.gn:1:
import("//components/nacl/nacl_defines.gni")
Ditto

https://codereview.chromium.org/877553008/diff/20001/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/mojo/nacl/BUILD.gn#newco...
mojo/nacl/BUILD.gn:37: include_dirs = [ "//third_party/mojo/src" ]
It seems that this should reference a config rather than hardcoding directories.
It looks like internal mojo targets have the public config
//third_party/mojo/public/build/config:mojo_sdk. If you made that a
public_config of mojo_nacl_codegen, then this target and the "mojo" target would
get this automatically.

":mojo_nacl_codegen" should in turn be a public dep in "mojo" so it gets
forwarded to "monacl_test".

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/BUILD.gn:1:
import("//components/nacl/nacl_defines.gni")
Copyright.

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/untrusted/irt_stub/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/untrusted/irt_stub/BUILD.gn:1: # Copyright 2014 The
Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/untrusted/pnacl_support_extension/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/untrusted/pnacl_support_extension/BUILD.gn:1:
import("//build/config/features.gni")
Copyright (I'm going to stop commenting on these)

[Dirk Pranke, 2015-02-05 00:26:16]

Will fix the TODO's and copyrights (meant to, but missed some, obviously).

https://codereview.chromium.org/877553008/diff/20001/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/mojo/nacl/BUILD.gn#newco...
mojo/nacl/BUILD.gn:37: include_dirs = [ "//third_party/mojo/src" ]
On 2015/02/05 00:20:36, brettw wrote:
> It seems that this should reference a config rather than hardcoding
directories.
> It looks like internal mojo targets have the public config
> //third_party/mojo/public/build/config:mojo_sdk. If you made that a
> public_config of mojo_nacl_codegen, then this target and the "mojo" target
would
> get this automatically.

Yeah, I thought about doing that but at the time was lazy and looking at other
things. Will do.
> 
> ":mojo_nacl_codegen" should in turn be a public dep in "mojo" so it gets
> forwarded to "monacl_test".

ok.

[cpu_(ooo_6.6-7.5), 2015-02-05 02:31:23]

third party is TBR unless new libraries are added.

[Nick Bray (chromium), 2015-02-05 23:21:46]

https://codereview.chromium.org/877553008/diff/20001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/BUILD.gn#newcode167
BUILD.gn:167: if (is_linux && cpu_arch == "x64") {
Are you not supporting x86 at this point?  TODO?

https://codereview.chromium.org/877553008/diff/20001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/base/BUILD.gn#newcode759
base/BUILD.gn:759: set_sources_assignment_filter([])
Why?  Comment.

https://codereview.chromium.org/877553008/diff/20001/base/BUILD.gn#newcode803
base/BUILD.gn:803: deps += [ "//base/third_party/dynamic_annotations" ]
Nit: comment why?

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

https://codereview.chromium.org/877553008/diff/20001/build/config/BUILDCONFIG...
build/config/BUILDCONFIG.gn:521: host_toolchain =
"//build/toolchain/linux:clang_x64"
Note: this hack would cause the NaCl bots to blow up if we blindly de-duped
BUILDCONFIG.gn, now.  Not sure if there's an AI, but it bothers me a little.

https://codereview.chromium.org/877553008/diff/20001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/877553008/diff/20001/build/config/features.gn...
build/config/features.gni:29: enable_nacl = is_linux || is_nacl
Nit: it seems strange is_nacl is needed.  Why?

https://codereview.chromium.org/877553008/diff/20001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/chrome/test/BUILD.gn#new...
chrome/test/BUILD.gn:9: import("//components/nacl/nacl_defines.gni")
Unlike the other CL, I think this is correct.  (Different repo, different
rules.)

https://codereview.chromium.org/877553008/diff/20001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/components/nacl/BUILD.gn...
components/nacl/BUILD.gn:39:
"//ppapi/native_client:nacl_irt(//native_client/build/toolchain/nacl:irt_x64)",
For the toolchain:
:irt_${cpu_arch}
?

Or TODO supporting other arches.

https://codereview.chromium.org/877553008/diff/20001/components/nacl/renderer...
File components/nacl/renderer/plugin/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/components/nacl/renderer...
components/nacl/renderer/plugin/BUILD.gn:20: configs += [
"//native_client/build/config/nacl:nacl_defines" ]
A third way to get the defines?  You have the chrome-style import above, why not
use that?

https://codereview.chromium.org/877553008/diff/20001/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/mojo/nacl/BUILD.gn#newco...
mojo/nacl/BUILD.gn:37: include_dirs = [ "//third_party/mojo/src" ]
On 2015/02/05 00:26:16, Dirk Pranke wrote:
> On 2015/02/05 00:20:36, brettw wrote:
> > It seems that this should reference a config rather than hardcoding
> directories.
> > It looks like internal mojo targets have the public config
> > //third_party/mojo/public/build/config:mojo_sdk. If you made that a
> > public_config of mojo_nacl_codegen, then this target and the "mojo" target
> would
> > get this automatically.
> 
> Yeah, I thought about doing that but at the time was lazy and looking at other
> things. Will do.
> > 
> > ":mojo_nacl_codegen" should in turn be a public dep in "mojo" so it gets
> > forwarded to "monacl_test".
> 
> ok.

This file should really be coming from the Mojo repo.  I suspect this issue
needs to be fixed upstream.

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/BUILD.gn:20: configs += [
"//native_client/build/config/nacl:nacl_defines" ]
Dito.

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/untrusted/irt_stub/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/untrusted/irt_stub/BUILD.gn:7: if (enable_nacl &&
enable_nacl_untrusted) {
Nit: technically not needed?  If we're including _any_ file in the untrusted
directory without enable_nacl_untrusted being true, something is seriously
wrong.  I wouldn't mind leaving this in as a sanity check, however, if you
comment it.

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/untrusted/pnacl_irt_shim/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/untrusted/pnacl_irt_shim/BUILD.gn:5:
import("//build/config/features.gni")
Not needed unless you want to paranoidly gate out based on
enable_nacl_untrusted?

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/untrusted/pnacl_support_extension/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/untrusted/pnacl_support_extension/BUILD.gn:3: if
(enable_nacl && enable_nacl_untrusted && enable_pnacl) {
Dito

[Dirk Pranke, 2015-02-05 23:52:20]

https://codereview.chromium.org/877553008/diff/20001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/BUILD.gn#newcode167
BUILD.gn:167: if (is_linux && cpu_arch == "x64") {
On 2015/02/05 23:21:45, Nick Bray (chromium) wrote:
> Are you not supporting x86 at this point?  TODO?

Correct (only x64). I will add a TODO.

https://codereview.chromium.org/877553008/diff/20001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/base/BUILD.gn#newcode759
base/BUILD.gn:759: set_sources_assignment_filter([])
On 2015/02/05 23:21:45, Nick Bray (chromium) wrote:
> Why?  Comment.

I will add a comment, but we need to include the _linux file that
sources_assignment_filter will otherwise strip out when is_nacl is true.

https://codereview.chromium.org/877553008/diff/20001/base/BUILD.gn#newcode803
base/BUILD.gn:803: deps += [ "//base/third_party/dynamic_annotations" ]
On 2015/02/05 23:21:45, Nick Bray (chromium) wrote:
> Nit: comment why?

I'm not sure what I would say in a comment here; we don't use dynamic_annotation
in a nacl build, which is pretty much what that says ... Would you want
something saying *why* we don't use dynamic_annotations? (I don't actually know
the reason, but I can find out).

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

https://codereview.chromium.org/877553008/diff/20001/build/config/BUILDCONFIG...
build/config/BUILDCONFIG.gn:521: host_toolchain =
"//build/toolchain/linux:clang_x64"
On 2015/02/05 23:21:45, Nick Bray (chromium) wrote:
> Note: this hack would cause the NaCl bots to blow up if we blindly de-duped
> BUILDCONFIG.gn, now.  Not sure if there's an AI, but it bothers me a little.

True. However, I need to rework this whole section of the file in the next
couple days, which will fix this and, I expect, happen far before we attempt to
dedup things.

https://codereview.chromium.org/877553008/diff/20001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/877553008/diff/20001/build/config/features.gn...
build/config/features.gni:29: enable_nacl = is_linux || is_nacl
On 2015/02/05 23:21:45, Nick Bray (chromium) wrote:
> Nit: it seems strange is_nacl is needed.  Why?

Because a lot of the conditions are if (enable_nacl && enable_nacl_untrusted).
When os=="nacl" (which is where you want most of the untrusted code compiled),
this would otherwise be false.

I can add a comment to that extent.

It might be better to have much of the code protected by if
(enable_nacl_untrusted) to actually be if (is_nacl), but that would diverge more
from how we did things in GYP.

https://codereview.chromium.org/877553008/diff/20001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/chrome/test/BUILD.gn#new...
chrome/test/BUILD.gn:9: import("//components/nacl/nacl_defines.gni")
On 2015/02/05 23:21:46, Nick Bray (chromium) wrote:
> Unlike the other CL, I think this is correct.  (Different repo, different
> rules.)

right.

https://codereview.chromium.org/877553008/diff/20001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/components/nacl/BUILD.gn...
components/nacl/BUILD.gn:39:
"//ppapi/native_client:nacl_irt(//native_client/build/toolchain/nacl:irt_x64)",
On 2015/02/05 23:21:46, Nick Bray (chromium) wrote:
> For the toolchain:
> :irt_${cpu_arch}
> ?
> 
> Or TODO supporting other arches.

Will add a TODO.

https://codereview.chromium.org/877553008/diff/20001/components/nacl/renderer...
File components/nacl/renderer/plugin/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/components/nacl/renderer...
components/nacl/renderer/plugin/BUILD.gn:20: configs += [
"//native_client/build/config/nacl:nacl_defines" ]
On 2015/02/05 23:21:46, Nick Bray (chromium) wrote:
> A third way to get the defines?  You have the chrome-style import above, why
not
> use that?

Good question. Will fix one way or another.

https://codereview.chromium.org/877553008/diff/20001/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/mojo/nacl/BUILD.gn#newco...
mojo/nacl/BUILD.gn:37: include_dirs = [ "//third_party/mojo/src" ]
On 2015/02/05 23:21:46, Nick Bray (chromium) wrote:
> On 2015/02/05 00:26:16, Dirk Pranke wrote:
> > On 2015/02/05 00:20:36, brettw wrote:
> > > It seems that this should reference a config rather than hardcoding
> > directories.
> > > It looks like internal mojo targets have the public config
> > > //third_party/mojo/public/build/config:mojo_sdk. If you made that a
> > > public_config of mojo_nacl_codegen, then this target and the "mojo" target
> > would
> > > get this automatically.
> > 
> > Yeah, I thought about doing that but at the time was lazy and looking at
other
> > things. Will do.
> > > 
> > > ":mojo_nacl_codegen" should in turn be a public dep in "mojo" so it gets
> > > forwarded to "monacl_test".
> > 
> > ok.
> 
> This file should really be coming from the Mojo repo.  I suspect this issue
> needs to be fixed upstream.

Right, that's what brett is suggesting.

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/BUILD.gn:20: configs += [
"//native_client/build/config/nacl:nacl_defines" ]
On 2015/02/05 23:21:46, Nick Bray (chromium) wrote:
> Dito.

Will fix.

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/untrusted/irt_stub/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/untrusted/irt_stub/BUILD.gn:7: if (enable_nacl &&
enable_nacl_untrusted) {
On 2015/02/05 23:21:46, Nick Bray (chromium) wrote:
> Nit: technically not needed?  If we're including _any_ file in the untrusted
> directory without enable_nacl_untrusted being true, something is seriously
> wrong.  I wouldn't mind leaving this in as a sanity check, however, if you
> comment it.

This is probably better changed to an assert() then. Will do so.

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/untrusted/pnacl_irt_shim/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/untrusted/pnacl_irt_shim/BUILD.gn:5:
import("//build/config/features.gni")
On 2015/02/05 23:21:46, Nick Bray (chromium) wrote:
> Not needed unless you want to paranoidly gate out based on
> enable_nacl_untrusted?

Will remove. It was probably a cut & paste thing ...

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/untrusted/pnacl_support_extension/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/untrusted/pnacl_support_extension/BUILD.gn:3: if
(enable_nacl && enable_nacl_untrusted && enable_pnacl) {
On 2015/02/05 23:21:46, Nick Bray (chromium) wrote:
> Dito

Will make same fix.

[Nick Bray (chromium), 2015-02-06 01:18:00]

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/untrusted/irt_stub/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/untrusted/irt_stub/BUILD.gn:7: if (enable_nacl &&
enable_nacl_untrusted) {
On 2015/02/05 23:52:19, Dirk Pranke wrote:
> On 2015/02/05 23:21:46, Nick Bray (chromium) wrote:
> > Nit: technically not needed?  If we're including _any_ file in the untrusted
> > directory without enable_nacl_untrusted being true, something is seriously
> > wrong.  I wouldn't mind leaving this in as a sanity check, however, if you
> > comment it.
> 
> This is probably better changed to an assert() then. Will do so.

asserting is_nacl would be the cleanest approach, I think.

[Dirk Pranke, 2015-02-06 02:34:11]

dpranke@chromium.org changed reviewers:
- cpu@chromium.org

[Dirk Pranke, 2015-02-06 02:57:27]

https://codereview.chromium.org/877553008/diff/20001/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/mojo/nacl/BUILD.gn#newco...
mojo/nacl/BUILD.gn:37: include_dirs = [ "//third_party/mojo/src" ]
On 2015/02/05 23:52:19, Dirk Pranke wrote:
> On 2015/02/05 23:21:46, Nick Bray (chromium) wrote:
> > On 2015/02/05 00:26:16, Dirk Pranke wrote:
> > > On 2015/02/05 00:20:36, brettw wrote:
> > > > It seems that this should reference a config rather than hardcoding
> > > directories.
> > > > It looks like internal mojo targets have the public config
> > > > //third_party/mojo/public/build/config:mojo_sdk. If you made that a
> > > > public_config of mojo_nacl_codegen, then this target and the "mojo"
target
> > > would
> > > > get this automatically.
> > > 
> > > Yeah, I thought about doing that but at the time was lazy and looking at
> other
> > > things. Will do.
> > > > 
> > > > ":mojo_nacl_codegen" should in turn be a public dep in "mojo" so it gets
> > > > forwarded to "monacl_test".
> > > 
> > > ok.
> > 
> > This file should really be coming from the Mojo repo.  I suspect this issue
> > needs to be fixed upstream.
> 
> Right, that's what brett is suggesting.

Brett, Nick, are you two okay with me leaving this as a TODO w/ the appropriate
comments and letting ncbray sort this out, since he's the guy working on mojo +
nacl ?

[Dirk Pranke, 2015-02-06 03:21:26]

New patchsets have been uploaded after l-g-t-m from brettw@chromium.org

[Dirk Pranke, 2015-02-06 03:24:29]

Please take another look? I think I've addressed all of the feedback except for
the mojo change (which I'd like to defer to ncbray in another patch and will add
a TODO for).

[brettw, 2015-02-06 06:06:15]

https://codereview.chromium.org/877553008/diff/20001/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/20001/mojo/nacl/BUILD.gn#newco...
mojo/nacl/BUILD.gn:37: include_dirs = [ "//third_party/mojo/src" ]
I'm just concerned that if it gets moved it will be copied-and-pasted from here.
Since it's only a few line change, I think we should just make it here.

[Nick Bray (chromium), 2015-02-07 02:37:47]

LGTM, modulo sorting out the Mojo issues.  (Mojo-side changes + include dir.) 
Let's touch base Monday.

https://codereview.chromium.org/877553008/diff/40001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/40001/chrome/test/BUILD.gn#new...
chrome/test/BUILD.gn:332: if (enable_nacl && enable_nacl_untrusted) {
Unless one of these targets has a transitive dependency on the IRT that itsn't
properly gated, there should be no reason to gate this on enable_nacl_untrusted?

https://codereview.chromium.org/877553008/diff/40001/chrome/test/BUILD.gn#new...
chrome/test/BUILD.gn:712:
"//ppapi/native_client:nacl_irt(//native_client/build/toolchain/nacl:irt_x64)",
Comment need to architecture support.  There could be a silent weirdness where
we build a 64 bit IRT on 32 bit platforms through this path if this target is
not updated (but others are).  I suspect we actually need an
"build_required_irt" pseudo target that chooses the appropriate toolchain in one
place rather than letting everything that wants the IRT built select the
toolchain.

Your choice to do it in this CL or punt.

https://codereview.chromium.org/877553008/diff/40001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/40001/components/nacl/BUILD.gn...
components/nacl/BUILD.gn:10: nacl_sources = [
Why not inline?

https://codereview.chromium.org/877553008/diff/40001/ppapi/BUILD.gn
File ppapi/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/40001/ppapi/BUILD.gn#newcode107
ppapi/BUILD.gn:107: ]
gn format is happy with this?

https://codereview.chromium.org/877553008/diff/40001/ppapi/native_client/src/...
File ppapi/native_client/src/untrusted/irt_stub/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/40001/ppapi/native_client/src/...
ppapi/native_client/src/untrusted/irt_stub/BUILD.gn:5:
import("//build/config/features.gni")
Not needed

https://codereview.chromium.org/877553008/diff/40001/ppapi/native_client/src/...
File ppapi/native_client/src/untrusted/pnacl_irt_shim/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/40001/ppapi/native_client/src/...
ppapi/native_client/src/untrusted/pnacl_irt_shim/BUILD.gn:5:
import("//build/config/features.gni")
Dito.

[Dirk Pranke, 2015-02-07 02:48:04]

https://codereview.chromium.org/877553008/diff/40001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/40001/chrome/test/BUILD.gn#new...
chrome/test/BUILD.gn:332: if (enable_nacl && enable_nacl_untrusted) {
On 2015/02/07 02:37:46, Nick Bray (chromium) wrote:
> Unless one of these targets has a transitive dependency on the IRT that itsn't
> properly gated, there should be no reason to gate this on
enable_nacl_untrusted?

Done.

https://codereview.chromium.org/877553008/diff/40001/chrome/test/BUILD.gn#new...
chrome/test/BUILD.gn:712:
"//ppapi/native_client:nacl_irt(//native_client/build/toolchain/nacl:irt_x64)",

TODO added.

https://codereview.chromium.org/877553008/diff/40001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/40001/components/nacl/BUILD.gn...
components/nacl/BUILD.gn:10: nacl_sources = [
On 2015/02/07 02:37:46, Nick Bray (chromium) wrote:
> Why not inline?

No idea :). Will fix.

https://codereview.chromium.org/877553008/diff/40001/ppapi/BUILD.gn
File ppapi/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/40001/ppapi/BUILD.gn#newcode107
ppapi/BUILD.gn:107: ]
On 2015/02/07 02:37:46, Nick Bray (chromium) wrote:
> gn format is happy with this?

Apparently.

https://codereview.chromium.org/877553008/diff/40001/ppapi/native_client/src/...
File ppapi/native_client/src/untrusted/irt_stub/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/40001/ppapi/native_client/src/...
ppapi/native_client/src/untrusted/irt_stub/BUILD.gn:5:
import("//build/config/features.gni")
On 2015/02/07 02:37:46, Nick Bray (chromium) wrote:
> Not needed

Done.

https://codereview.chromium.org/877553008/diff/40001/ppapi/native_client/src/...
File ppapi/native_client/src/untrusted/pnacl_irt_shim/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/40001/ppapi/native_client/src/...
ppapi/native_client/src/untrusted/pnacl_irt_shim/BUILD.gn:5:
import("//build/config/features.gni")
On 2015/02/07 02:37:46, Nick Bray (chromium) wrote:
> Dito.

Done.

[Dirk Pranke, 2015-02-07 02:48:40]

New patchsets have been uploaded after l-g-t-m from ncbray@chromium.org

[Dirk Pranke, 2015-02-07 02:56:25]

Okay, at this point I think I've updated w/ all of the feedback from both ncbray
and brettw.

I think the patch can land as-is and we can sort out the mojo stuff down the
road. Let me know 
if anyone objects (of course, I still need to land the runhooks fix and the
nacl-side change
first).

[Nick Bray (chromium), 2015-02-07 05:21:52]

The Mojo repo should be (?) the authoritative source for mojo/nacl/*

If you commit a change directly to the Chrome repo, it will probably cause the
DEPS roll scripts to do bad things.

[Dirk Pranke, 2015-02-07 20:56:19]

On 2015/02/07 05:21:52, Nick Bray (chromium) wrote:
> The Mojo repo should be (?) the authoritative source for mojo/nacl/*
> 
> If you commit a change directly to the Chrome repo, it will probably cause the
> DEPS roll scripts to do bad things.

blundell@ sent a note out on 1/30 saying that all of the stuff that was rolled
in now lives under //third_party, and the stuff under //mojo should be
chromium-specific. Maybe that doesn't apply to //mojo/nacl yet, or we need to
move it, or something.

At any rate, we can discuss on Monday and I'm not landing anything before then
regardless.

[Dirk Pranke, 2015-02-25 03:38:04]

Patchset #7 (id:110001) has been deleted

[Dirk Pranke, 2015-02-25 03:39:58]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-02-25 03:39:59]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
ncbray@chromium.org
Link to the patchset: https://codereview.chromium.org/877553008/#ps130001
(title: "merge to #317924")

[commit-bot: I haz the power, 2015-02-25 03:40:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877553008/130001

[Dirk Pranke, 2015-02-25 03:41:53]

The CQ bit was unchecked by dpranke@chromium.org

[Dirk Pranke, 2015-02-25 21:08:46]

Please take another look ...

Brett, can you make sure I merged in your ppapi changes properly?

Nick, can you look at the two comments I have? Also, I have a couple questions
related to how we need to handle the build failures I got on the try jobs.

https://codereview.chromium.org/877553008/diff/130001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/BUILD.gn#newcode170
BUILD.gn:170: # TODO(dpranke) needed? deps += [
"//ppapi:ppapi_cpp_lib(//native_client/build/toolchain/nacl:clang_newlib_x64)" ]
It looks like we don't need to explicitly build this at this point, so I'll
remove this line ...

https://codereview.chromium.org/877553008/diff/130001/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/mojo/nacl/BUILD.gn#newc...
mojo/nacl/BUILD.gn:27: [
"//third_party/mojo/src/mojo/public/build/config:mojo_sdk" ]
I'm not sure if this is the right way to handle this ...

https://codereview.chromium.org/877553008/diff/130001/ppapi/proxy/BUILD.gn
File ppapi/proxy/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/ppapi/proxy/BUILD.gn#ne...
ppapi/proxy/BUILD.gn:287: ]
or this ...

https://codereview.chromium.org/877553008/diff/130001/third_party/harfbuzz-ng...
File third_party/harfbuzz-ng/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/third_party/harfbuzz-ng...
third_party/harfbuzz-ng/BUILD.gn:29: "value")
Brett, can you double-check that this is right?

The prior version of this causes GN to complain about use_system_harfbuzz being
declared multiple times. I had originally worked around this by declaring
use_system_harfbuzz = false and the updating the value, and I was pretty sure
that was working at one point, but now GN won't let me do that either.

[brettw, 2015-02-25 21:27:30]

https://codereview.chromium.org/877553008/diff/130001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/BUILD.gn#newcode170
BUILD.gn:170: # TODO(dpranke) needed? deps += [
"//ppapi:ppapi_cpp_lib(//native_client/build/toolchain/nacl:clang_newlib_x64)" ]
On 2015/02/25 21:08:46, Dirk Pranke wrote:
> It looks like we don't need to explicitly build this at this point, so I'll
> remove this line ...

Yes, remove this.

https://codereview.chromium.org/877553008/diff/130001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/base/BUILD.gn#newcode813
base/BUILD.gn:813: deps += [ "//base/third_party/dynamic_annotations" ]
Does dynamic_annotations ever make sense in nacl? If not, I think we should have
an if in base that maps it to an empty group in nacl so we don't need these
conditions.

https://codereview.chromium.org/877553008/diff/130001/base/BUILD.gn#newcode1060
base/BUILD.gn:1060: deps += [ "//base/third_party/dynamic_annotations" ]
Ditto

https://codereview.chromium.org/877553008/diff/130001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:707: # "test/data/nacl:shared_test_files",
TODO(GYP) comment?

https://codereview.chromium.org/877553008/diff/130001/extensions/shell/BUILD.gn
File extensions/shell/BUILD.gn (left):

https://codereview.chromium.org/877553008/diff/130001/extensions/shell/BUILD....
extensions/shell/BUILD.gn:61: sources =
rebase_path(app_shell_gypi_values.app_shell_lib_sources,
Why did you change this?

https://codereview.chromium.org/877553008/diff/130001/ipc/BUILD.gn
File ipc/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/ipc/BUILD.gn#newcode96
ipc/BUILD.gn:96: "//base/third_party/dynamic_annotations",
Ditto about dynamic annotations.

https://codereview.chromium.org/877553008/diff/130001/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/mojo/nacl/BUILD.gn#newc...
mojo/nacl/BUILD.gn:27: [
"//third_party/mojo/src/mojo/public/build/config:mojo_sdk" ]
Doesn't look obviously wrong to me.

https://codereview.chromium.org/877553008/diff/130001/mojo/nacl/BUILD.gn#newc...
mojo/nacl/BUILD.gn:87: include_dirs = [ "//third_party/mojo/src" ]
We shouldn't have to specify this kind of thing manually. It should come from
some appropriate config. What's the error you're trying to solve?

https://codereview.chromium.org/877553008/diff/130001/mojo/nacl/BUILD.gn#newc...
mojo/nacl/BUILD.gn:103: include_dirs = [ "$root_build_dir/gen" ]
You can delete this line, it should be there by default.

https://codereview.chromium.org/877553008/diff/130001/ppapi/native_client/src...
File ppapi/native_client/src/untrusted/irt_stub/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/ppapi/native_client/src...
ppapi/native_client/src/untrusted/irt_stub/BUILD.gn:8:
static_library("ppapi_stub_lib") {
Almost all static libraries in GYP should be converted to source_set in GN. Some
exceptions are if we're distributing the actual static library. Can you check
the static libs you added to see if you're sure they need to be static
libraries?

https://codereview.chromium.org/877553008/diff/130001/ppapi/proxy/BUILD.gn
File ppapi/proxy/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/ppapi/proxy/BUILD.gn#ne...
ppapi/proxy/BUILD.gn:287: ]
The gen dir should already be implicit, the mojo one should come from a mojo
config.

https://codereview.chromium.org/877553008/diff/130001/ppapi/shared_impl/BUILD.gn
File ppapi/shared_impl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/ppapi/shared_impl/BUILD...
ppapi/shared_impl/BUILD.gn:167: if (!is_nacl) {
I think jam's patch just deletes this dependency in case you get a merge
conflict on this.

https://codereview.chromium.org/877553008/diff/130001/ppapi/shared_impl/BUILD...
ppapi/shared_impl/BUILD.gn:191: "//base/third_party/dynamic_annotations",
Ditto about dynamic annotations.

https://codereview.chromium.org/877553008/diff/130001/third_party/harfbuzz-ng...
File third_party/harfbuzz-ng/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/third_party/harfbuzz-ng...
third_party/harfbuzz-ng/BUILD.gn:29: "value")
Seems OK.

[brettw, 2015-02-25 21:28:03]

To reduce risk, you may want to land this patch with nacl still off by default,
then flip it to on in a separate small patch.

[Dirk Pranke, 2015-02-25 21:37:41]

https://codereview.chromium.org/877553008/diff/130001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/base/BUILD.gn#newcode813
base/BUILD.gn:813: deps += [ "//base/third_party/dynamic_annotations" ]
On 2015/02/25 21:27:29, brettw wrote:
> Does dynamic_annotations ever make sense in nacl? If not, I think we should
have
> an if in base that maps it to an empty group in nacl so we don't need these
> conditions.

I don't think so; from what I could tell, we never build dynamic_annotations in
nacl in the GYP builds. will change.

https://codereview.chromium.org/877553008/diff/130001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:707: # "test/data/nacl:shared_test_files",
On 2015/02/25 21:27:29, brettw wrote:
> TODO(GYP) comment?

Will do.

https://codereview.chromium.org/877553008/diff/130001/extensions/shell/BUILD.gn
File extensions/shell/BUILD.gn (left):

https://codereview.chromium.org/877553008/diff/130001/extensions/shell/BUILD....
extensions/shell/BUILD.gn:61: sources =
rebase_path(app_shell_gypi_values.app_shell_lib_sources,
On 2015/02/25 21:27:29, brettw wrote:
> Why did you change this?

I suspect that was unintentional and came in automatically in my merge to HEAD.
I'll put it back.

https://codereview.chromium.org/877553008/diff/130001/ipc/BUILD.gn
File ipc/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/ipc/BUILD.gn#newcode96
ipc/BUILD.gn:96: "//base/third_party/dynamic_annotations",
On 2015/02/25 21:27:29, brettw wrote:
> Ditto about dynamic annotations.

Acknowledged.

https://codereview.chromium.org/877553008/diff/130001/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/mojo/nacl/BUILD.gn#newc...
mojo/nacl/BUILD.gn:27: [
"//third_party/mojo/src/mojo/public/build/config:mojo_sdk" ]
On 2015/02/25 21:27:29, brettw wrote:
> Doesn't look obviously wrong to me.

Acknowledged.

https://codereview.chromium.org/877553008/diff/130001/mojo/nacl/BUILD.gn#newc...
mojo/nacl/BUILD.gn:87: include_dirs = [ "//third_party/mojo/src" ]
On 2015/02/25 21:27:29, brettw wrote:
> We shouldn't have to specify this kind of thing manually. It should come from
> some appropriate config. What's the error you're trying to solve?

Agreed, which is partially why I had the comment above. However, per ncbray we
can actually just delete these targets as they shouldn't be in chromium at all.

https://codereview.chromium.org/877553008/diff/130001/mojo/nacl/BUILD.gn#newc...
mojo/nacl/BUILD.gn:103: include_dirs = [ "$root_build_dir/gen" ]
On 2015/02/25 21:27:29, brettw wrote:
> You can delete this line, it should be there by default.

It didn't seem to be, but I'll double check. Maybe I misred the error somewhere.

https://codereview.chromium.org/877553008/diff/130001/ppapi/native_client/src...
File ppapi/native_client/src/untrusted/irt_stub/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/ppapi/native_client/src...
ppapi/native_client/src/untrusted/irt_stub/BUILD.gn:8:
static_library("ppapi_stub_lib") {
On 2015/02/25 21:27:29, brettw wrote:
> Almost all static libraries in GYP should be converted to source_set in GN.
Some
> exceptions are if we're distributing the actual static library. Can you check
> the static libs you added to see if you're sure they need to be static
> libraries?

Will do.

https://codereview.chromium.org/877553008/diff/130001/ppapi/proxy/BUILD.gn
File ppapi/proxy/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/ppapi/proxy/BUILD.gn#ne...
ppapi/proxy/BUILD.gn:287: ]
On 2015/02/25 21:27:30, brettw wrote:
> The gen dir should already be implicit, the mojo one should come from a mojo
> config.

Hence the TODO, I'll make sure it's TODO(GYP), also, and double-check the gen
dir.

https://codereview.chromium.org/877553008/diff/130001/ppapi/shared_impl/BUILD.gn
File ppapi/shared_impl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/130001/ppapi/shared_impl/BUILD...
ppapi/shared_impl/BUILD.gn:167: if (!is_nacl) {
On 2015/02/25 21:27:30, brettw wrote:
> I think jam's patch just deletes this dependency in case you get a merge
> conflict on this.

Yeah, I thought I deleted all these, I guess I missed one.

https://codereview.chromium.org/877553008/diff/130001/ppapi/shared_impl/BUILD...
ppapi/shared_impl/BUILD.gn:191: "//base/third_party/dynamic_annotations",
On 2015/02/25 21:27:30, brettw wrote:
> Ditto about dynamic annotations.

Acknowledged.

[Dirk Pranke, 2015-02-25 22:08:35]

Patchset #8 (id:150001) has been deleted

[Dirk Pranke, 2015-02-25 22:08:44]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-02-25 22:08:44]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
ncbray@chromium.org
Link to the patchset: https://codereview.chromium.org/877553008/#ps170001
(title: "update w/ review feedback")

[commit-bot: I haz the power, 2015-02-25 22:08:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877553008/170001

[Dirk Pranke, 2015-02-25 22:08:56]

New patch posted. Please take another look?

[Dirk Pranke, 2015-02-25 22:09:45]

The CQ bit was unchecked by dpranke@chromium.org

[brettw, 2015-02-25 22:22:25]

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:9: static_library("nacl") {
Use source_set instead

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:36: include_dirs = [ "//third_party/mojo/src" ]
We shouldn't be hardcoding these and this isn't even in GYP. Is GYP getting this
from a dependency? It looks like the deps are fairly different in GYP from this
list (crypto, monacl_syscall, service_runtime, mojo_system_impl are all
missing). Probably one of these targets has dependent settings.

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:52: static_library("nacl_browser") {
source_set

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:85:
"//components/nacl/zygote/nacl_fork_delegate_linux.cc",
Just delete "//components/nacl/" from the beginning of these files.

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:93: static_library("nacl_renderer") {
source_set

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:160: static_library("nacl_linux") {
source_set

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:226: static_library("nacl_switches") {
source_set

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:233: static_library("nacl_common") {
source_set

https://codereview.chromium.org/877553008/diff/170001/components/nacl/rendere...
File components/nacl/renderer/plugin/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/components/nacl/rendere...
components/nacl/renderer/plugin/BUILD.gn:5:
static_library("nacl_trusted_plugin") {
source_set

https://codereview.chromium.org/877553008/diff/170001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/BUILD.gn:5:
static_library("nacl_trusted_plugin") {
source_set

https://codereview.chromium.org/877553008/diff/170001/ppapi/native_client/src...
File ppapi/native_client/src/untrusted/pnacl_irt_shim/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/ppapi/native_client/src...
ppapi/native_client/src/untrusted/pnacl_irt_shim/BUILD.gn:8:
static_library("aot") {
source_set (also twice below)

[Dirk Pranke, 2015-02-25 22:26:30]

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:36: include_dirs = [ "//third_party/mojo/src" ]
On 2015/02/25 22:22:25, brettw wrote:
> We shouldn't be hardcoding these and this isn't even in GYP. Is GYP getting
this
> from a dependency? It looks like the deps are fairly different in GYP from
this
> list (crypto, monacl_syscall, service_runtime, mojo_system_impl are all
> missing). Probably one of these targets has dependent settings.

It may be that we don't actually need this now. I know a lot of the mojo/nacl
integration stuff has been changing. Will verify.

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:85:
"//components/nacl/zygote/nacl_fork_delegate_linux.cc",
On 2015/02/25 22:22:25, brettw wrote:
> Just delete "//components/nacl/" from the beginning of these files.

Ah, right.

[Nick Bray (chromium), 2015-02-25 22:52:40]

LGTM, although some of the dependency stuff seems sketchy.

https://codereview.chromium.org/877553008/diff/170001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:708:
"//ppapi/native_client:nacl_irt(//native_client/build/toolchain/nacl:irt_x64)",
assert(current_cpu == 'x64') ?  I think there's another place in BUILDCONFIG you
could do this?

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:36: include_dirs = [ "//third_party/mojo/src" ]
On 2015/02/25 22:26:30, Dirk Pranke wrote:
> On 2015/02/25 22:22:25, brettw wrote:
> > We shouldn't be hardcoding these and this isn't even in GYP. Is GYP getting
> this
> > from a dependency? It looks like the deps are fairly different in GYP from
> this
> > list (crypto, monacl_syscall, service_runtime, mojo_system_impl are all
> > missing). Probably one of these targets has dependent settings.
> 
> It may be that we don't actually need this now. I know a lot of the mojo/nacl
> integration stuff has been changing. Will verify.

I believe there should be a dependency on monacl_syscall, hereabouts?  If there
is a dependancy elsewhere, I would guess it is in the wrong place.

https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl.gy...

https://codereview.chromium.org/877553008/diff/170001/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/mojo/nacl/BUILD.gn#newc...
mojo/nacl/BUILD.gn:27: [
"//third_party/mojo/src/mojo/public/build/config:mojo_sdk" ]
I think this should be on monacl_sel, instead?

https://codereview.chromium.org/877553008/diff/170001/ppapi/native_client/BUI...
File ppapi/native_client/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/ppapi/native_client/BUI...
ppapi/native_client/BUILD.gn:10: "src/untrusted/irt_stub:ppapi_stub_lib",
Wat?  Why no abspath?  (And below.)

[Dirk Pranke, 2015-02-25 23:23:40]

ninth patchset is the charm?

https://codereview.chromium.org/877553008/diff/170001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:708:
"//ppapi/native_client:nacl_irt(//native_client/build/toolchain/nacl:irt_x64)",
On 2015/02/25 22:52:40, Nick Bray (chromium) wrote:
> assert(current_cpu == 'x64') ?  I think there's another place in BUILDCONFIG
you
> could do this?

ok.

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:9: static_library("nacl") {
On 2015/02/25 22:22:25, brettw wrote:
> Use source_set instead

Done.

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:36: include_dirs = [ "//third_party/mojo/src" ]
On 2015/02/25 22:52:40, Nick Bray (chromium) wrote:
> On 2015/02/25 22:26:30, Dirk Pranke wrote:
> > On 2015/02/25 22:22:25, brettw wrote:
> > > We shouldn't be hardcoding these and this isn't even in GYP. Is GYP
getting
> > this
> > > from a dependency? It looks like the deps are fairly different in GYP from
> > this
> > > list (crypto, monacl_syscall, service_runtime, mojo_system_impl are all
> > > missing). Probably one of these targets has dependent settings.
> > 
> > It may be that we don't actually need this now. I know a lot of the
mojo/nacl
> > integration stuff has been changing. Will verify.
> 
> I believe there should be a dependency on monacl_syscall, hereabouts?  If
there
> is a dependancy elsewhere, I would guess it is in the wrong place.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl.gy...

Acknowledged.

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:52: static_library("nacl_browser") {
On 2015/02/25 22:22:25, brettw wrote:
> source_set

Done.

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:85:
"//components/nacl/zygote/nacl_fork_delegate_linux.cc",
On 2015/02/25 22:26:30, Dirk Pranke wrote:
> On 2015/02/25 22:22:25, brettw wrote:
> > Just delete "//components/nacl/" from the beginning of these files.
> 
> Ah, right.

Done.

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:93: static_library("nacl_renderer") {
On 2015/02/25 22:22:25, brettw wrote:
> source_set

Done.

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:160: static_library("nacl_linux") {
On 2015/02/25 22:22:25, brettw wrote:
> source_set

Done.

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:226: static_library("nacl_switches") {
On 2015/02/25 22:22:25, brettw wrote:
> source_set

Done.

https://codereview.chromium.org/877553008/diff/170001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:233: static_library("nacl_common") {
On 2015/02/25 22:22:25, brettw wrote:
> source_set

Done.

https://codereview.chromium.org/877553008/diff/170001/components/nacl/rendere...
File components/nacl/renderer/plugin/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/components/nacl/rendere...
components/nacl/renderer/plugin/BUILD.gn:5:
static_library("nacl_trusted_plugin") {
On 2015/02/25 22:22:25, brettw wrote:
> source_set

Done.

https://codereview.chromium.org/877553008/diff/170001/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/mojo/nacl/BUILD.gn#newc...
mojo/nacl/BUILD.gn:27: [
"//third_party/mojo/src/mojo/public/build/config:mojo_sdk" ]
On 2015/02/25 22:52:40, Nick Bray (chromium) wrote:
> I think this should be on monacl_sel, instead?

Done.

https://codereview.chromium.org/877553008/diff/170001/ppapi/native_client/BUI...
File ppapi/native_client/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/ppapi/native_client/BUI...
ppapi/native_client/BUILD.gn:10: "src/untrusted/irt_stub:ppapi_stub_lib",
On 2015/02/25 22:52:40, Nick Bray (chromium) wrote:
> Wat?  Why no abspath?  (And below.)

Done.

https://codereview.chromium.org/877553008/diff/170001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/BUILD.gn:5:
static_library("nacl_trusted_plugin") {
On 2015/02/25 22:22:25, brettw wrote:
> source_set

Done.

https://codereview.chromium.org/877553008/diff/170001/ppapi/native_client/src...
File ppapi/native_client/src/untrusted/pnacl_irt_shim/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/170001/ppapi/native_client/src...
ppapi/native_client/src/untrusted/pnacl_irt_shim/BUILD.gn:8:
static_library("aot") {
On 2015/02/25 22:22:25, brettw wrote:
> source_set (also twice below)

Done.

[Dirk Pranke, 2015-02-25 23:32:49]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-02-25 23:32:50]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
ncbray@chromium.org
Link to the patchset: https://codereview.chromium.org/877553008/#ps190001
(title: "update w/ more review feedback")

[commit-bot: I haz the power, 2015-02-25 23:33:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877553008/190001

[Dirk Pranke, 2015-02-25 23:35:43]

The CQ bit was unchecked by dpranke@chromium.org

[brettw, 2015-02-25 23:36:54]

LGTM if you check the below. Not sure if there are other cases.

https://codereview.chromium.org/877553008/diff/190001/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/190001/ui/events/BUILD.gn#newc...
ui/events/BUILD.gn:354: static_library("latency_info") {
Please check for static libraries again. This needs to be source_set.

[jvoung (off chromium), 2015-02-26 00:01:42]

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

[jvoung (off chromium), 2015-02-26 00:01:44]

https://codereview.chromium.org/877553008/diff/190001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/190001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/BUILD.gn:5:
source_set("nacl_trusted_plugin") {
driveby -- I think the files moved to components/nacl/renderer/plugin

[Dirk Pranke, 2015-02-26 00:21:29]

https://codereview.chromium.org/877553008/diff/190001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/190001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/BUILD.gn:5:
source_set("nacl_trusted_plugin") {
On 2015/02/26 00:01:44, jvoung wrote:
> driveby -- I think the files moved to components/nacl/renderer/plugin

You're right! It looks like this whole file is no longer needed.

https://codereview.chromium.org/877553008/diff/190001/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/877553008/diff/190001/ui/events/BUILD.gn#newc...
ui/events/BUILD.gn:354: static_library("latency_info") {
On 2015/02/25 23:36:54, brettw wrote:
> Please check for static libraries again. This needs to be source_set.

Sigh. Yeah, this was the only one I missed.

[Dirk Pranke, 2015-02-26 00:24:26]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-02-26 00:24:26]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
ncbray@chromium.org
Link to the patchset: https://codereview.chromium.org/877553008/#ps210001
(title: "yet more feedback")

[commit-bot: I haz the power, 2015-02-26 00:25:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877553008/210001

[Dirk Pranke, 2015-02-26 00:27:32]

The CQ bit was unchecked by dpranke@chromium.org

[commit-bot: I haz the power, 2015-02-26 00:28:02]

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

[commit-bot: I haz the power, 2015-02-26 00:28:04]

Try jobs failed on following builders:
  ios_rel_device_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Dirk Pranke, 2015-02-26 01:39:06]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2015-02-26 01:40:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877553008/210001

[commit-bot: I haz the power, 2015-02-26 02:13:12]

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

[commit-bot: I haz the power, 2015-02-26 02:13:15]

Failed to commit the patch.

[commit-bot: I haz the power, 2015-02-26 02:13:23]

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

[Dirk Pranke, 2015-02-26 02:16:27]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2015-02-26 02:17:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877553008/210001

[commit-bot: I haz the power, 2015-02-26 02:20:37]

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

[commit-bot: I haz the power, 2015-02-26 02:20:39]

Failed to commit the patch.

[Dirk Pranke, 2015-02-26 03:28:42]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2015-02-26 03:29:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877553008/210001

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

Committed patchset #10 (id:210001)

[commit-bot: I haz the power, 2015-02-26 03:32:08]

Patchset 10 (id:??) landed as
https://crrev.com/6065cf751340a8a99b670357b2053ed6df12a7af
Cr-Commit-Position: refs/heads/master@{#318180}