Build PPAPI PNaCl Newlib tests with GN

ppapi/BUILD.gn

Index: ppapi/BUILD.gn
diff --git a/ppapi/BUILD.gn b/ppapi/BUILD.gn
index 2a982027a1ea45612a60e984c78e47913d32a02a..51dfb864fe4e0808ac530e6439c7efaf7dd958cc 100644
--- a/ppapi/BUILD.gn
+++ b/ppapi/BUILD.gn
@@ -209,14 +209,60 @@ if (enable_nacl) {
       }
     }
 
+    if (current_cpu == "pnacl") {
+      action("translate_pexe_to_nexe") {
+        nacl_toolchain_dir = rebase_path("//native_client/toolchain")

[Roland McGrath, 2015/09/30 20:26:33]

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.

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

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

+        os_toolchain_dir = "${nacl_toolchain_dir}/${host_os}_x86"
+        script = "${os_toolchain_dir}/pnacl_newlib/bin/pydir/loader.py"

[Roland McGrath, 2015/09/30 20:26:33]

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.

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

Done.

+        pexe = "${root_out_dir}/ppapi_nacl_tests.pexe"
+        nexe = "${root_out_dir}/ppapi_nacl_tests.nexe"
+        sources = [
+          pexe,
+        ]
+        outputs = [
+          nexe,
+        ]
+
+        # TODO(phosek): remove the following once change 1360243003 is rolled
+        # into Chrome and use $target_cpu directly.
+        if (target_cpu == "x86") {
+          arch = "i686"
+        } else if (target_cpu == "x64") {
+          arch = "x86-64"
+        } else if (target_cpu == "arm") {
+          arch = "armv7"
+        }
+        args = [
+          "pnacl-translate",

[Roland McGrath, 2015/09/30 20:26:33]

Please have dschuff review the details of calling pnacl-translate.

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

Done, Derek already helped me debugging this.

+          rebase_path(pexe),
+          "-o",
+          rebase_path(nexe),
+          "-arch",
+          arch,
+          "-Wl,-L" + rebase_path(
+                  get_label_info(
+                      "//ppapi/native_client/src/untrusted/pnacl_irt_shim:aot(//build/toolchain/nacl:clang_newlib_${target_cpu})",

[Roland McGrath, 2015/09/30 20:26:33]

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.

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

Done.

+                      "target_out_dir")),
+        ]
+        deps = [
+          ":ppapi_nacl_tests",
+        ]
+        data_deps = [
+          "//ppapi/native_client/src/untrusted/pnacl_irt_shim:aot(//build/toolchain/nacl:clang_newlib_${target_cpu})",
+        ]
+      }
+    }
+
     copy("nacl_tests_copy") {
       sources = [
         "${root_out_dir}/ppapi_nacl_tests.nexe",
       ]
       if (is_nacl_glibc) {
-        suffix = "glibc"
+        suffix = "glibc_${target_cpu}"

[Roland McGrath, 2015/09/30 20:26:33]

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.

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

Done.

+      } else if (current_cpu == "pnacl") {
+        suffix = "pnacl"
       } else {
-        suffix = "newlib"
+        suffix = "newlib_${target_cpu}"
       }
 
       # The CPU names used in tests/ppapi_nacl_tests_newlib.nmf
@@ -229,9 +275,15 @@ if (enable_nacl) {
       outputs = [
         "${root_build_dir}/{{source_name_part}}_${suffix}_${nmf_cpu}.nexe",
       ]
-      deps = [
-        ":ppapi_nacl_tests",
-      ]
+      if (current_cpu == "pnacl") {
+        deps = [
+          ":translate_pexe_to_nexe",
+        ]
+      } else {
+        deps = [
+          ":ppapi_nacl_tests",
+        ]
+      }
     }
 
     action("generate_nmf") {
@@ -240,6 +292,9 @@ if (enable_nacl) {
       if (is_nacl_glibc) {
         toolchain_dir = "${os_toolchain_dir}/nacl_x86_glibc"
         nmf = "${root_build_dir}/ppapi_nacl_tests_glibc.nmf"
+      } else if (current_cpu == "pnacl") {
+        toolchain_dir = "${os_toolchain_dir}/pnacl_newlib"

[Roland McGrath, 2015/09/30 20:26:33]

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?

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

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?

+        nmf = "${root_build_dir}/ppapi_nacl_tests_pnacl.nmf"
       } else {
         toolchain_dir = "${os_toolchain_dir}/nacl_x86_newlib"
         nmf = "${root_build_dir}/ppapi_nacl_tests_newlib.nmf"
@@ -291,6 +346,7 @@ if (enable_nacl) {
         ":copy_test_files",
         ":nacl_tests_copy(//build/toolchain/nacl:clang_newlib_${target_cpu})",
         ":generate_nmf(//build/toolchain/nacl:glibc_${target_cpu})",
+        ":generate_nmf(//build/toolchain/nacl:newlib_pnacl)",

[Roland McGrath, 2015/09/30 20:26:33]

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]

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.

       ]
     }
   }