Build PPAPI PNaCl Newlib tests with GN

Build PPAPI PNaCl Newlib tests with GN

Support building PPAPI NaCl tests with PNaCl/Newlib-based toolchain.

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

[Petr Hosek, 2015-09-30 19:03:25]

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

[Petr Hosek, 2015-09-30 19:03:45]

PTAL

[Dirk Pranke, 2015-09-30 19:40:24]

this looks like correctly-written GN code :).

I leave it to Roland to say whether it is doing the right thing.

lgtm.

[Roland McGrath, 2015-09-30 20:26:34]

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

https://codereview.chromium.org/1365973004/diff/60001/ppapi/BUILD.gn#newcode214
ppapi/BUILD.gn:214: nacl_toolchain_dir =
rebase_path("//native_client/toolchain")
Hoist these *_dir variables up into the if (is_nacl) {...} scope so that you
don't have to repeat them here and in generate_nmf.

Better yet, figure out a way to just snarf these from
build/toolchain/nacl/BUILD.gn.  That probably requires moving them into some
.gni file like nacl_toolchain.gni, but that should be fine.

https://codereview.chromium.org/1365973004/diff/60001/ppapi/BUILD.gn#newcode216
ppapi/BUILD.gn:216: script =
"${os_toolchain_dir}/pnacl_newlib/bin/pydir/loader.py"
Can you put script next to sources and outputs and separate those from the local
variables here?  That would make it more clear that scripts, outputs, and
sources are GN's "action" parameters, while the rest are just local variables.

https://codereview.chromium.org/1365973004/diff/60001/ppapi/BUILD.gn#newcode236
ppapi/BUILD.gn:236: "pnacl-translate",
Please have dschuff review the details of calling pnacl-translate.

https://codereview.chromium.org/1365973004/diff/60001/ppapi/BUILD.gn#newcode244
ppapi/BUILD.gn:244:
"//ppapi/native_client/src/untrusted/pnacl_irt_shim:aot(//build/toolchain/nacl:clang_newlib_${target_cpu})",
Put this long label into a variable and use the variable here and in data_deps. 
Also needs a comment somewhere saying what it's for.

https://codereview.chromium.org/1365973004/diff/60001/ppapi/BUILD.gn#newcode261
ppapi/BUILD.gn:261: suffix = "glibc_${target_cpu}"
This needs to be merged with the nmf_cpu logic below so that the nexe file name
continues to be what it has been in the GYP build.  As written it will now be
"ppapi_nacl_tests_newlib_x86_x32.nexe" for example, but
"ppapi_nacl_tests_newlib_x32.nexe" is what works.

https://codereview.chromium.org/1365973004/diff/60001/ppapi/BUILD.gn#newcode296
ppapi/BUILD.gn:296: toolchain_dir = "${os_toolchain_dir}/pnacl_newlib"
The toolchain definitions already have toolchain_package from which this can be
derived without repeated conditionals.  Can we figure out a way to just make
that available here?

https://codereview.chromium.org/1365973004/diff/60001/ppapi/BUILD.gn#newcode349
ppapi/BUILD.gn:349: ":generate_nmf(//build/toolchain/nacl:newlib_pnacl)",
What is the purpose of doing generate_nmf for newlib_pnacl?
It's not done for the other newlib cases.  Why not just add a pnacl case to the
static ppapi_nacl_tests_newlib.nmf?

[Petr Hosek, 2015-09-30 23:22:03]

https://chromiumcodereview.appspot.com/1365973004/diff/60001/ppapi/BUILD.gn
File ppapi/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1365973004/diff/60001/ppapi/BUILD.gn#n...
ppapi/BUILD.gn:214: nacl_toolchain_dir =
rebase_path("//native_client/toolchain")
On 2015/09/30 20:26:33, Roland McGrath wrote:
> Hoist these *_dir variables up into the if (is_nacl) {...} scope so that you
> don't have to repeat them here and in generate_nmf.
> 
> Better yet, figure out a way to just snarf these from
> build/toolchain/nacl/BUILD.gn.  That probably requires moving them into some
> .gni file like nacl_toolchain.gni, but that should be fine.

Done, I've moved `nacl_toolchain_dir` and `os_toolchain_dir` to
build/config/nacl/config.gni.

https://chromiumcodereview.appspot.com/1365973004/diff/60001/ppapi/BUILD.gn#n...
ppapi/BUILD.gn:216: script =
"${os_toolchain_dir}/pnacl_newlib/bin/pydir/loader.py"
On 2015/09/30 20:26:33, Roland McGrath wrote:
> Can you put script next to sources and outputs and separate those from the
local
> variables here?  That would make it more clear that scripts, outputs, and
> sources are GN's "action" parameters, while the rest are just local variables.

Done.

https://chromiumcodereview.appspot.com/1365973004/diff/60001/ppapi/BUILD.gn#n...
ppapi/BUILD.gn:236: "pnacl-translate",
On 2015/09/30 20:26:33, Roland McGrath wrote:
> Please have dschuff review the details of calling pnacl-translate.

Done, Derek already helped me debugging this.

https://chromiumcodereview.appspot.com/1365973004/diff/60001/ppapi/BUILD.gn#n...
ppapi/BUILD.gn:244:
"//ppapi/native_client/src/untrusted/pnacl_irt_shim:aot(//build/toolchain/nacl:clang_newlib_${target_cpu})",
On 2015/09/30 20:26:33, Roland McGrath wrote:
> Put this long label into a variable and use the variable here and in
data_deps. 
> Also needs a comment somewhere saying what it's for.

Done.

https://chromiumcodereview.appspot.com/1365973004/diff/60001/ppapi/BUILD.gn#n...
ppapi/BUILD.gn:261: suffix = "glibc_${target_cpu}"
On 2015/09/30 20:26:33, Roland McGrath wrote:
> This needs to be merged with the nmf_cpu logic below so that the nexe file
name
> continues to be what it has been in the GYP build.  As written it will now be
> "ppapi_nacl_tests_newlib_x86_x32.nexe" for example, but
> "ppapi_nacl_tests_newlib_x32.nexe" is what works.

Done.

https://chromiumcodereview.appspot.com/1365973004/diff/60001/ppapi/BUILD.gn#n...
ppapi/BUILD.gn:296: toolchain_dir = "${os_toolchain_dir}/pnacl_newlib"
On 2015/09/30 20:26:33, Roland McGrath wrote:
> The toolchain definitions already have toolchain_package from which this can
be
> derived without repeated conditionals.  Can we figure out a way to just make
> that available here?

I'm thinking about turning `toolchain_package` and `toolchain_revision` into
arguments and declaring them in sameplace. This could in fact completely replace
`is_nacl_glibc` since you can then simply check whether `toolchain_package ==
"nacl_x86_glibc"`. Would that be fine with you?

https://chromiumcodereview.appspot.com/1365973004/diff/60001/ppapi/BUILD.gn#n...
ppapi/BUILD.gn:349: ":generate_nmf(//build/toolchain/nacl:newlib_pnacl)",
On 2015/09/30 20:26:33, Roland McGrath wrote:
> What is the purpose of doing generate_nmf for newlib_pnacl?
> It's not done for the other newlib cases.  Why not just add a pnacl case to
the
> static ppapi_nacl_tests_newlib.nmf?

The goal is to match the GYP behavior and possibly improve this in the future
when GYP build is deprecated

One thing I've realized though is that since we're generating the nmf files, we
don't need to copy nexe's into build root and instead just point create_nmf.py
to the right file so I've made that change which makes the rule more generic and
saves one extra operation.

[Roland McGrath, 2015-10-01 17:16:20]

lgtm

[brettw, 2015-10-01 17:27:36]

https://chromiumcodereview.appspot.com/1365973004/diff/80001/build/config/nac...
File build/config/nacl/config.gni (right):

https://chromiumcodereview.appspot.com/1365973004/diff/80001/build/config/nac...
build/config/nacl/config.gni:6: os_toolchain_dir =
"${nacl_toolchain_dir}/${host_os}_x86"
Since this is going to be basically a global variable now, I think it would be
best to have "nacl" in the name somewhere. I'm not sure enough if what this is
to be able to suggest a good name, though. Can these also have comments, for the
same reason?

I normally put declare_args at the top, and other stuff below it.

[Petr Hosek, 2015-10-03 05:25:27]

https://chromiumcodereview.appspot.com/1365973004/diff/80001/build/config/nac...
File build/config/nacl/config.gni (right):

https://chromiumcodereview.appspot.com/1365973004/diff/80001/build/config/nac...
build/config/nacl/config.gni:6: os_toolchain_dir =
"${nacl_toolchain_dir}/${host_os}_x86"
On 2015/10/01 17:27:36, brettw wrote:
> Since this is going to be basically a global variable now, I think it would be
> best to have "nacl" in the name somewhere. I'm not sure enough if what this is
> to be able to suggest a good name, though. Can these also have comments, for
the
> same reason?
> 
> I normally put declare_args at the top, and other stuff below it.

Done.

[brettw, 2015-10-05 18:20:40]

Sorry to do another pass but I noticed some other things...

https://chromiumcodereview.appspot.com/1365973004/diff/100001/ppapi/BUILD.gn
File ppapi/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1365973004/diff/100001/ppapi/BUILD.gn#...
ppapi/BUILD.gn:217: script = rebase_path(
You can delete the whole rebase_path call and just say:
  script = "${nacl_host_os_toolchain_dir}/pnacl_newlib/bin/pydir/loader.py"

The script variable is something GN is processing. You only need rebase_path
when passing arguments to scripts to make the inputs match the script's concept
of how files are referenced.

https://chromiumcodereview.appspot.com/1365973004/diff/100001/ppapi/BUILD.gn#...
ppapi/BUILD.gn:246: rebase_path(pexe),
For these two calls, they'll generate absolute paths. If you need that, this
code is correct. Most scripts can also take files relative to the current
directory, which makes the command lines nicer for debugging. If your script can
handle that, can you replace these two rebase_path calls with:
  rebase_path(..., root_build_dir)

https://chromiumcodereview.appspot.com/1365973004/diff/100001/ppapi/BUILD.gn#...
ppapi/BUILD.gn:325: "",
In my last pass, I almost asked you to not call rebase_path when declaring
nacl_host_os_toolchain_dir, since mostly global paths should be source-absolute
("//foo/bar") and then GN code that needs to use it rebases it to what it wants.
But it seemed OK since your code was only needing the
relative-to-the-build-directory verison.

But I missed this. This rebase_path call is pretty weird to me (and several like
it below). I think this would be much clearer if we did my above suggestion.
Then the directory doesn't have to be magically treated as relative to the build
directory and you can delete the third argument.

[Petr Hosek, 2015-10-06 04:55:55]

I've also merged nacl_host_os_toolchain_dir and nacl_toolchain_dir in
build/config/nacl/config.gni since the latter was not really used anywhere.

https://codereview.chromium.org/1365973004/diff/100001/ppapi/BUILD.gn
File ppapi/BUILD.gn (right):

https://codereview.chromium.org/1365973004/diff/100001/ppapi/BUILD.gn#newcode217
ppapi/BUILD.gn:217: script = rebase_path(
On 2015/10/05 18:20:40, brettw wrote:
> You can delete the whole rebase_path call and just say:
>   script = "${nacl_host_os_toolchain_dir}/pnacl_newlib/bin/pydir/loader.py"
> 
> The script variable is something GN is processing. You only need rebase_path
> when passing arguments to scripts to make the inputs match the script's
concept
> of how files are referenced.

Done.

https://codereview.chromium.org/1365973004/diff/100001/ppapi/BUILD.gn#newcode246
ppapi/BUILD.gn:246: rebase_path(pexe),
On 2015/10/05 18:20:40, brettw wrote:
> For these two calls, they'll generate absolute paths. If you need that, this
> code is correct. Most scripts can also take files relative to the current
> directory, which makes the command lines nicer for debugging. If your script
can
> handle that, can you replace these two rebase_path calls with:
>   rebase_path(..., root_build_dir)

Done.

https://codereview.chromium.org/1365973004/diff/100001/ppapi/BUILD.gn#newcode325
ppapi/BUILD.gn:325: "",
On 2015/10/05 18:20:40, brettw wrote:
> In my last pass, I almost asked you to not call rebase_path when declaring
> nacl_host_os_toolchain_dir, since mostly global paths should be
source-absolute
> ("//foo/bar") and then GN code that needs to use it rebases it to what it
wants.
> But it seemed OK since your code was only needing the
> relative-to-the-build-directory verison.
> 
> But I missed this. This rebase_path call is pretty weird to me (and several
like
> it below). I think this would be much clearer if we did my above suggestion.
> Then the directory doesn't have to be magically treated as relative to the
build
> directory and you can delete the third argument.

Done.

[brettw, 2015-10-06 19:19:32]

lgtm

[Petr Hosek, 2015-10-06 19:43:55]

The CQ bit was checked by phosek@chromium.org

[Petr Hosek, 2015-10-06 19:43:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, mcgrathr@chromium.org
Link to the patchset: https://codereview.chromium.org/1365973004/#ps120001
(title: "Address additional feedback")

[commit-bot: I haz the power, 2015-10-06 19:44:28]

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

[commit-bot: I haz the power, 2015-10-06 20:35:05]

Committed patchset #7 (id:120001)

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

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