Build PPAPI NaCl Glibc tests with GN

Build PPAPI NaCl Glibc tests with GN

Support building PPAPI NaCl tests with GCC/Glibc-based toolchain.

BUG=462791
Committed: https://crrev.com/70a330fb376e614a401e738f6e7442674b0052bd
Cr-Commit-Position: refs/heads/master@{#350467}

[Petr Hosek, 2015-09-09 16:25:42]

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

[Petr Hosek, 2015-09-09 16:37:06]

PTAL, by the way is there a way to run trybots with custom gn arguments (i.e.
enable_nacl_browsertests)?

[brettw, 2015-09-09 17:41:59]

To do the trybots, send a patch with the default value changed in the .gni file.
Just remember not to check it in.

[Dirk Pranke, 2015-09-09 20:00:35]

mostly looks okay, but I'd like Brett to take a good look as well.

You can also test changes on the trybots by modifying the flags in
//tools/mb/mb_config.pyl, but Brett's suggestion is easier.

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn
File ppapi/BUILD.gn (right):

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn#newcode162
ppapi/BUILD.gn:162: cflags = [ "-fPIC" ]
Presumably this is a linux/unix- specific flag and we'll need something
different on windows?

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn#newcode212
ppapi/BUILD.gn:212: ldflags = [ "-pthread" ]
same comment: presumably this will also need to change on windows?

[brettw, 2015-09-09 20:52:31]

Didn't look at the ppapi file yet, will do so shortly...

https://codereview.chromium.org/1333673002/diff/1/build/config/nacl/nacl.gni
File build/config/nacl/nacl.gni (right):

https://codereview.chromium.org/1333673002/diff/1/build/config/nacl/nacl.gni#...
build/config/nacl/nacl.gni:5: nacl_toolchain_dir =
rebase_path("//native_client/toolchain")
I prefer keeping all variables in GN-style and then having the users rebase as
needed. This gives a bit more flexibility since future users may want this
directory relative to different places.

So I would keep this as:
  nacl_toolchain_dir = "//native_client/toolchain"
in the toolchain file where you need the absolute path frequently, maybe make a
new "abs_nacl_toolchain" var, and in the ppapi file when you compute
glibc_toolchain_dir, just put the rebase path in there.

[Petr Hosek, 2015-09-09 20:55:55]

Do you have any recommendation for naming libraries which are being built both
as static and shared? I've added the `_shared` suffix to the shared version but
I'm not sure whether this follows the Chrome conventions.

Regarding the toolchain path, one possible solution I thought of would be to
define a `toolchain_path` variable in BUILDCONFIG.gn and then assign it inside
the `toolchain_args` block (if defined). The path could then be used to access
tools provided by the toolchain (e.g. objdump) which is needed for building
PPAPI tests under NaCl. What do you think of that? Would this be acceptable?

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn
File ppapi/BUILD.gn (right):

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn#newcode162
ppapi/BUILD.gn:162: cflags = [ "-fPIC" ]
On 2015/09/09 20:00:35, Dirk Pranke wrote:
> Presumably this is a linux/unix- specific flag and we'll need something
> different on windows?
> 

Actually, this is NaCl specific. After checking the GYP files, it seems it's not
being used in non-NaCl code, so it might be worth moving this into the `is_nacl`
block.

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn#newcode212
ppapi/BUILD.gn:212: ldflags = [ "-pthread" ]
On 2015/09/09 20:00:35, Dirk Pranke wrote:
> same comment: presumably this will also need to change on windows?

ditto

[brettw, 2015-09-09 21:08:46]

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn
File ppapi/BUILD.gn (right):

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn#newcode156
ppapi/BUILD.gn:156: configs -= [ "//build/config/gcc:symbol_visibility_hidden" ]
This needs to be inside
  if (is_posix) { ... }
to match the case in BUILDCONFIG where it is added.

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn#newcode162
ppapi/BUILD.gn:162: cflags = [ "-fPIC" ]
This should be set already for Linux so you can just delete it.

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn#newcode204
ppapi/BUILD.gn:204: "..",
Remove this, it's automatic in GN. Can you also remove it from the target above
(where I presume you copied it from)?

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn#newcode257
ppapi/BUILD.gn:257: script = create_nmf
Since you only use this var once, inline it here. Also, you don't need the
rebase_path command, this takes a GN path. So delete the declaration above and
do:
  script = "//native_client_sdk/src/tools/create_nmf.py"

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn#newcode276
ppapi/BUILD.gn:276: ] + create_nmf_flags + glibc_nmf_flags +
Since this is only used once, can you delete the create_nmf_flags variable and
inline it here?

If we have another create nmf script invocation, we should make a template that
encapsulates this rather than duplicating most of this logic in a similar
variant. So inlining everything for now won't set us back.

[brettw, 2015-09-09 21:19:19]

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn
File ppapi/BUILD.gn (right):

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn#newcode162
ppapi/BUILD.gn:162: cflags = [ "-fPIC" ]
Actually, this whole target is suspicious. ppapi_sources.cpp_source_files is
already compiled in the ppapi_cpp_lib source set. I think what you're trying to
do here is make a NaCl-specific shared library with those sources.

If that's the case, move the shared library to within the nacl block, then
depend on ":ppapi_cpp_lib".

-fPIC *isn't* automatically set for all compiles on NaCl. Maybe it should be?

[Roland McGrath, 2015-09-09 21:24:42]

On 2015/09/09 21:19:19, brettw wrote:
> -fPIC *isn't* automatically set for all compiles on NaCl. Maybe it should be?

It's only correct for objects that are going into a shared library.

[brettw, 2015-09-09 21:40:56]

On 2015/09/09 21:24:42, Roland McGrath wrote:
> On 2015/09/09 21:19:19, brettw wrote:
> > -fPIC *isn't* automatically set for all compiles on NaCl. Maybe it should
be?
> 
> It's only correct for objects that are going into a shared library.

In that case, the existing source set should probably have an if (is_nacl) {
cflags = [ "-fPIC" ] } with a comment about why this is necessary. Then we
should be OK.

[Petr Hosek, 2015-09-09 22:14:50]

https://codereview.chromium.org/1333673002/diff/1/build/config/nacl/nacl.gni
File build/config/nacl/nacl.gni (right):

https://codereview.chromium.org/1333673002/diff/1/build/config/nacl/nacl.gni#...
build/config/nacl/nacl.gni:5: nacl_toolchain_dir =
rebase_path("//native_client/toolchain")
On 2015/09/09 20:52:30, brettw wrote:
> I prefer keeping all variables in GN-style and then having the users rebase as
> needed. This gives a bit more flexibility since future users may want this
> directory relative to different places.
> 
> So I would keep this as:
>   nacl_toolchain_dir = "//native_client/toolchain"
> in the toolchain file where you need the absolute path frequently, maybe make
a
> new "abs_nacl_toolchain" var, and in the ppapi file when you compute
> glibc_toolchain_dir, just put the rebase path in there.

Done.

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn
File ppapi/BUILD.gn (right):

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn#newcode156
ppapi/BUILD.gn:156: configs -= [ "//build/config/gcc:symbol_visibility_hidden" ]
On 2015/09/09 21:08:45, brettw wrote:
> This needs to be inside
>   if (is_posix) { ... }
> to match the case in BUILDCONFIG where it is added.

Done.

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn#newcode162
ppapi/BUILD.gn:162: cflags = [ "-fPIC" ]
On 2015/09/09 21:19:19, brettw wrote:
> Actually, this whole target is suspicious. ppapi_sources.cpp_source_files is
> already compiled in the ppapi_cpp_lib source set. I think what you're trying
to
> do here is make a NaCl-specific shared library with those sources.
> 
> If that's the case, move the shared library to within the nacl block, then
> depend on ":ppapi_cpp_lib".
> 
> -fPIC *isn't* automatically set for all compiles on NaCl. Maybe it should be?

Done.

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn#newcode204
ppapi/BUILD.gn:204: "..",
On 2015/09/09 21:08:45, brettw wrote:
> Remove this, it's automatic in GN. Can you also remove it from the target
above
> (where I presume you copied it from)?

Done.

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn#newcode257
ppapi/BUILD.gn:257: script = create_nmf
On 2015/09/09 21:08:45, brettw wrote:
> Since you only use this var once, inline it here. Also, you don't need the
> rebase_path command, this takes a GN path. So delete the declaration above and
> do:
>   script = "//native_client_sdk/src/tools/create_nmf.py"

I'm going to be using it in the future for PNaCl action which is going to use
the same script and flags but I can move it to a variable later.

https://codereview.chromium.org/1333673002/diff/1/ppapi/BUILD.gn#newcode276
ppapi/BUILD.gn:276: ] + create_nmf_flags + glibc_nmf_flags +
On 2015/09/09 21:08:45, brettw wrote:
> Since this is only used once, can you delete the create_nmf_flags variable and
> inline it here?
> 
> If we have another create nmf script invocation, we should make a template
that
> encapsulates this rather than duplicating most of this logic in a similar
> variant. So inlining everything for now won't set us back.

There is going to be another invocation in the future, but I can factor this out
into a template then as you suggested.

[brettw, 2015-09-10 05:02:50]

LGTM

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

https://codereview.chromium.org/1333673002/diff/20001/ppapi/BUILD.gn#newcode160
ppapi/BUILD.gn:160: "//build/config/nacl:nacl_base",
I think you mean to put this inside the is_nacl block.

https://codereview.chromium.org/1333673002/diff/20001/ppapi/BUILD.gn#newcode234
ppapi/BUILD.gn:234: "${root_build_dir}/{{source_name_part}}_${target_cpu}.nexe",
Actually, I just realized how this should really work. Sorry I didn't notice
this in the first pass.

In the nacl build, you'll have different toolchains for the glibc and newlib
builds. So you would never express a "glibc" target or have different variants
of the nmf action. You would have only one, and inside conditionally check flags
like CPU or the glibc/newlib variant and change things accordingly.

We'll need to add new builds args so it's easy for code to check the
glibc/newlib-ness of the current toolchain.

If you feel like this patch is a good first step, we can land as-is and fix in a
followup. But basically any target name with "glibc" in it is wrong.

[Petr Hosek, 2015-09-10 06:25:31]

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

https://codereview.chromium.org/1333673002/diff/20001/ppapi/BUILD.gn#newcode234
ppapi/BUILD.gn:234: "${root_build_dir}/{{source_name_part}}_${target_cpu}.nexe",
On 2015/09/10 05:02:50, brettw wrote:
> Actually, I just realized how this should really work. Sorry I didn't notice
> this in the first pass.
> 
> In the nacl build, you'll have different toolchains for the glibc and newlib
> builds. So you would never express a "glibc" target or have different variants
> of the nmf action. You would have only one, and inside conditionally check
flags
> like CPU or the glibc/newlib variant and change things accordingly.
> 
> We'll need to add new builds args so it's easy for code to check the
> glibc/newlib-ness of the current toolchain.
> 
> If you feel like this patch is a good first step, we can land as-is and fix in
a
> followup. But basically any target name with "glibc" in it is wrong.

I'm fine to fix it even in this change. Shall I introduce new args: is_glibc,
is_newlib; and set these accordingly inside the toolchain?

[brettw, 2015-09-10 17:20:09]

On 2015/09/10 06:25:31, Petr Hosek wrote:
> I'm fine to fix it even in this change. Shall I introduce new args: is_glibc,
> is_newlib; and set these accordingly inside the toolchain?

Yeah. There are a few steps (sorry):

- Make a new .gni file that declares these somewhere. I'm thinking
//build/config/nacl/config.gni. I'm assuming we can get away with just one
nacl/newlib switch (this will make a little less work later):

  if (is_nacl) {
    declare_args() {
      # Write some good documentation about what this is and what true and false
means.
      is_nacl_glibc = false
    }
  }

Import this file when you need the parameter.

- Now we need to propagate this flag to the corresponding toolchain definition.
This is a bit annoying! NaCl ends up using the gcc toolchain template in
//build/toolchain/gcc_toolchain.gni, and they need to end up in the
toolchain_args block there:

  toolchain_args() {
    ...
    if (defined(invoker.is_nacl_glibc)) {
      is_nacl_glibc = invoker.is_nacl_glibc
    }
  }

Then need to forward this from the nacl_toolchain template in
//build/toolchain/nacl_toolchain.gni. I think this template should assume the
flag is set so it doesn't need to check for the defined condition. In the
gcc_toolchain invocation there, add:

  is_nacl_glibc = invoker.is_nacl_glibc

Then in the nacl_toolchain invocations in //build/toolchain/nacl/BUILD.gn just
set "is_nacl_glibc = true" (or false).

[Dirk Pranke, 2015-09-10 22:27:21]

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

https://codereview.chromium.org/1333673002/diff/20001/ppapi/BUILD.gn#newcode234
ppapi/BUILD.gn:234: "${root_build_dir}/{{source_name_part}}_${target_cpu}.nexe",
On 2015/09/10 05:02:50, brettw wrote:
> But basically any target name with "glibc" in it is wrong.

You'll still need the target outputs to have "glibs" and "newlib" in the names,
though, right? So you can distinguish the two toolchains?

[Dirk Pranke, 2015-09-10 22:28:04]

+mcgrathr since we're talking about how the toolchains might work.

[Petr Hosek, 2015-09-10 22:49:19]

Yes, unless we want to change tests so they load nexe's from the respective
toolchain subdirectory. This is something we might want to consider since it'd
obviate the need to copy nexe's into the build root. On the other hand, it'd
break the GYP build (not sure if that's a problem).

[Dirk Pranke, 2015-09-10 22:50:28]

On 2015/09/10 22:49:19, Petr Hosek wrote:
> Yes, unless we want to change tests so they load nexe's from the respective
> toolchain subdirectory. This is something we might want to consider since it'd
> obviate the need to copy nexe's into the build root. On the other hand, it'd
> break the GYP build (not sure if that's a problem).

We probably shouldn't break the GYP build.

[brettw, 2015-09-10 23:00:15]

The only newlib/glibc thing that should appear would be in the output file of
the copy step. It woudl look at the current toolchain flag and adjust its name
to compute what Chrome expects that target to be named in the main output
directory.

[brettw, 2015-09-11 20:41:34]

On 2015/09/10 23:00:15, brettw wrote:
> The only newlib/glibc thing that should appear would be in the output file of
> the copy step. It woudl look at the current toolchain flag and adjust its name
> to compute what Chrome expects that target to be named in the main output
> directory.

When this patch lands:
https://codereview.chromium.org/1338803004
It should be a bit simpler. You can just list your variable in the appropriate
list in a forward_variables_from call to thread the values through. This
implicitly does the if (defined)...set code for all variables.

[Petr Hosek, 2015-09-11 22:10:50]

I have just uploaded the new version, but I'll update the code to use the
forward_variables_from.

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

https://codereview.chromium.org/1333673002/diff/20001/ppapi/BUILD.gn#newcode160
ppapi/BUILD.gn:160: "//build/config/nacl:nacl_base",
On 2015/09/10 05:02:50, brettw wrote:
> I think you mean to put this inside the is_nacl block.

Done.

https://codereview.chromium.org/1333673002/diff/20001/ppapi/BUILD.gn#newcode234
ppapi/BUILD.gn:234: "${root_build_dir}/{{source_name_part}}_${target_cpu}.nexe",
On 2015/09/10 05:02:50, brettw wrote:
> Actually, I just realized how this should really work. Sorry I didn't notice
> this in the first pass.
> 
> In the nacl build, you'll have different toolchains for the glibc and newlib
> builds. So you would never express a "glibc" target or have different variants
> of the nmf action. You would have only one, and inside conditionally check
flags
> like CPU or the glibc/newlib variant and change things accordingly.
> 
> We'll need to add new builds args so it's easy for code to check the
> glibc/newlib-ness of the current toolchain.
> 
> If you feel like this patch is a good first step, we can land as-is and fix in
a
> followup. But basically any target name with "glibc" in it is wrong.

Done.

[Roland McGrath, 2015-09-14 20:19:51]

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

[Roland McGrath, 2015-09-14 20:19:51]

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

https://codereview.chromium.org/1333673002/diff/40001/ppapi/BUILD.gn#newcode150
ppapi/BUILD.gn:150: configs -= [ "//build/config/gcc:symbol_visibility_hidden" ]
Why?  I don't see why this is needed.  There should be a comment to justify it.

https://codereview.chromium.org/1333673002/diff/40001/ppapi/BUILD.gn#newcode158
ppapi/BUILD.gn:158: cflags = [ "-fPIC" ]
This really does not seem like it belongs here.  You should build with -fPIC
when building a shared library and not otherwise.  But that is not specific to
this library, and should be handled in some generic fashion.

https://codereview.chromium.org/1333673002/diff/40001/ppapi/BUILD.gn#newcode189
ppapi/BUILD.gn:189: configs -= [ "//build/config/gcc:symbol_visibility_hidden" ]
Why is this here?  It needs a comment.

https://codereview.chromium.org/1333673002/diff/40001/ppapi/BUILD.gn#newcode200
ppapi/BUILD.gn:200: ldflags = [ "-pthread" ]
Don't all the NaCl toolchains accept -pthread?

[Petr Hosek, 2015-09-14 23:50:44]

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

https://codereview.chromium.org/1333673002/diff/40001/ppapi/BUILD.gn#newcode150
ppapi/BUILD.gn:150: configs -= [ "//build/config/gcc:symbol_visibility_hidden" ]
On 2015/09/14 20:19:51, Roland McGrath wrote:
> Why?  I don't see why this is needed.  There should be a comment to justify
it.

Done.

https://codereview.chromium.org/1333673002/diff/40001/ppapi/BUILD.gn#newcode158
ppapi/BUILD.gn:158: cflags = [ "-fPIC" ]
On 2015/09/14 20:19:51, Roland McGrath wrote:
> This really does not seem like it belongs here.  You should build with -fPIC
> when building a shared library and not otherwise.  But that is not specific to
> this library, and should be handled in some generic fashion.

Is there a clean way to check whether the source set is being deps'd into a
shared library and adjust the flags accordingly?

https://codereview.chromium.org/1333673002/diff/40001/ppapi/BUILD.gn#newcode189
ppapi/BUILD.gn:189: configs -= [ "//build/config/gcc:symbol_visibility_hidden" ]
On 2015/09/14 20:19:51, Roland McGrath wrote:
> Why is this here?  It needs a comment.

Done.

https://codereview.chromium.org/1333673002/diff/40001/ppapi/BUILD.gn#newcode200
ppapi/BUILD.gn:200: ldflags = [ "-pthread" ]
On 2015/09/14 20:19:51, Roland McGrath wrote:
> Don't all the NaCl toolchains accept -pthread?

True, it doesn't hurt to have it here unconditionally.

[Roland McGrath, 2015-09-17 18:16:52]

LGTM though it's going to need some rebasing

https://codereview.chromium.org/1333673002/diff/60001/ppapi/BUILD.gn
File ppapi/BUILD.gn (right):

https://codereview.chromium.org/1333673002/diff/60001/ppapi/BUILD.gn#newcode195
ppapi/BUILD.gn:195: # defined by the test executable.
Fine enough I guess, though it seems like it might be both cleaner and easier to
grok if the needed exports were just marked explicitly in the source.

[Petr Hosek, 2015-09-17 21:24:50]

Patchset #5 (id:80001) has been deleted

[Petr Hosek, 2015-09-17 21:54:09]

https://codereview.chromium.org/1333673002/diff/60001/ppapi/BUILD.gn
File ppapi/BUILD.gn (right):

https://codereview.chromium.org/1333673002/diff/60001/ppapi/BUILD.gn#newcode195
ppapi/BUILD.gn:195: # defined by the test executable.
On 2015/09/17 18:16:52, Roland McGrath wrote:
> Fine enough I guess, though it seems like it might be both cleaner and easier
to
> grok if the needed exports were just marked explicitly in the source.

Done.

[Petr Hosek, 2015-09-17 23:49:42]

Patchset #9 (id:180001) has been deleted

[Petr Hosek, 2015-09-17 23:49:50]

Patchset #8 (id:160001) has been deleted

[Petr Hosek, 2015-09-17 23:49:57]

Patchset #7 (id:140001) has been deleted

[Petr Hosek, 2015-09-17 23:50:03]

Patchset #6 (id:120001) has been deleted

[Petr Hosek, 2015-09-21 22:46:31]

Patchset #8 (id:240001) has been deleted

[Petr Hosek, 2015-09-22 23:36:12]

Patchset #8 (id:260001) has been deleted

[Petr Hosek, 2015-09-22 23:36:19]

Patchset #8 (id:280001) has been deleted

[Petr Hosek, 2015-09-23 16:23:26]

Patchset #8 (id:300001) has been deleted

[Petr Hosek, 2015-09-23 17:28:34]

Patchset #9 (id:340001) has been deleted

[Petr Hosek, 2015-09-23 17:30:17]

Patchset #9 (id:360001) has been deleted

[Petr Hosek, 2015-09-23 18:12:57]

The CQ bit was checked by phosek@chromium.org

[Petr Hosek, 2015-09-23 18:12:57]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
mcgrathr@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1333673002/#ps380001 (title: "Rebase")

[commit-bot: I haz the power, 2015-09-23 18:15:47]

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

[Roland McGrath, 2015-09-23 18:33:36]

https://codereview.chromium.org/1333673002/diff/380001/build/toolchain/nacl/B...
File build/toolchain/nacl/BUILD.gn (right):

https://codereview.chromium.org/1333673002/diff/380001/build/toolchain/nacl/B...
build/toolchain/nacl/BUILD.gn:114: is_nacl_glibc = false
This is superfluous, right?  The default is false.
If this were needed, then nacl_clang_toolchain would need it too.

[Petr Hosek, 2015-09-23 18:35:14]

The CQ bit was unchecked by phosek@chromium.org

[Petr Hosek, 2015-09-23 18:37:22]

The CQ bit was checked by phosek@chromium.org

[Petr Hosek, 2015-09-23 18:37:23]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
mcgrathr@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1333673002/#ps400001 (title: "Removed
unnecessary assignments from toolchain definitions")

[Petr Hosek, 2015-09-23 18:37:28]

https://chromiumcodereview.appspot.com/1333673002/diff/380001/build/toolchain...
File build/toolchain/nacl/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1333673002/diff/380001/build/toolchain...
build/toolchain/nacl/BUILD.gn:114: is_nacl_glibc = false
On 2015/09/23 18:33:36, Roland McGrath wrote:
> This is superfluous, right?  The default is false.
> If this were needed, then nacl_clang_toolchain would need it too.

Removed.

[commit-bot: I haz the power, 2015-09-23 18:39:34]

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

[commit-bot: I haz the power, 2015-09-23 19:44:01]

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

[commit-bot: I haz the power, 2015-09-23 19:44:03]

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

[Roland McGrath, 2015-09-23 21:54:39]

The CQ bit was checked by mcgrathr@chromium.org

[Roland McGrath, 2015-09-23 21:54:39]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
mcgrathr@chromium.org
Link to the patchset: https://codereview.chromium.org/1333673002/#ps420001
(title: "Rebase")

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

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

[Petr Hosek, 2015-09-23 23:53:32]

Patchset #11 (id:420001) has been deleted

[Dirk Pranke, 2015-09-24 02:45:00]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-09-24 02:45:00]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
mcgrathr@chromium.org
Link to the patchset: https://codereview.chromium.org/1333673002/#ps440001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-24 02:45:33]

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

[commit-bot: I haz the power, 2015-09-24 03:23:43]

Committed patchset #11 (id:440001)

[commit-bot: I haz the power, 2015-09-24 03:24:51]

Patchset 11 (id:??) landed as
https://crrev.com/70a330fb376e614a401e738f6e7442674b0052bd
Cr-Commit-Position: refs/heads/master@{#350467}