Factor out nmf generation into a GN template

build/config/nacl/rules.gni

+# Copyright 2015 The Native Client Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import("//build/config/features.gni")
+import("//build/config/nacl/config.gni")
+
+template("generate_nmf") {

[brettw, 2015/11/11 21:20:12]

All templates should have comments at the top listing what they do (a quick
description of what an NMF is would be good since not everybody will know), and
all the arguments that they expect.

Same for the template below.

[Petr Hosek, 2015/11/11 23:35:03]

Done.

+  assert(defined(invoker.executable), "Must define executable")
+  assert(defined(invoker.nmf), "Must define nmf")
+  objdump = rebase_path("${nacl_toolprefix}objdump")

[brettw, 2015/11/11 21:20:12]

You only use this once, so I'd just inline the rebase call in the args below.

[Petr Hosek, 2015/11/11 23:35:03]

Done.

+
+  action(target_name) {
+    forward_variables_from(invoker,
+                           [
+                             "deps",
+                             "data_deps",
+                             "executable",
+                             "nmf",
+                             "nmfflags",
+                             "public_deps",
+                             "testonly",
+                           ])
+
+    script = "//native_client_sdk/src/tools/create_nmf.py"
+    sources = [
+      executable,
+    ]
+    outputs = [
+      nmf,
+    ]
+    args = [
+      "--no-default-libpath",
+      "--objdump=" + objdump,
+      "--output=" + rebase_path(nmf, root_build_dir),
+    ]
+    if (is_nacl_glibc) {
+      args += [ "--library-path=" + rebase_path(root_out_dir) ]

[Roland McGrath, 2015/11/11 21:44:31]

Maybe the library path dir should be listed in inputs?

[brettw, 2015/11/11 22:32:56]

You can't list directories as inputs, only files. If the script's behavior
depends on anything in here, though, it should be listed in the inputs and the
action should depend on whatever generated that stuff.

[Petr Hosek, 2015/11/11 23:35:03]

I could remove this and it'll be up to the invoker to append that path to
nmfflags if it's needed.

+      if (current_cpu == "x86") {
+        args += [ "--library-path=" +
+                  rebase_path("${nacl_toolchain_tooldir}/lib32") ]
+        data = [
+          "$root_build_dir/lib32/",

[brettw, 2015/11/11 21:20:12]

It seems surprising to me that depending on a single nacl nmf then inserts every
nacl output into my data files. Instead, this should list only what's actually
needed (same for the other architectures).

Also, this doesn't match the code that writes into it. Above you specify a
library path of "${nacl_toolchain_tooldir}/lib32" which is not necessarily the
same as the value you write in the outputs here.

[Petr Hosek, 2015/11/11 23:35:03]

That's intentional because those are two different things. The library path is
where the runtime libraries are looked for. The scripts then takes all the
dependencies and drops them into the lib32/lib64 folder (relative to the current
directory). That path can be overridden by passing an extra argument, so I can
check whether that argument is being passed in and if so use that path instead
of root_build_dir. However, there is no way to find out what those libraries
will be ahead of time so I cannot list them (we'd need another script to do that
and parse it's output from GN which is probably doable but it's an extra work).

[brettw, 2015/11/12 17:40:58]

Okay, I guess the analogy would be that normal programs all depend on random
stuff in /usr/lib. So this seems fine.

+        ]
+      } else if (target_cpu == "x64" || (target_cpu == "x86" && is_win)) {

[brettw, 2015/11/11 21:20:12]

I'm confused by these conditions. The first if tests the current_cpu, this one
tests the target.

I don't think you want to use target_cpu anywhere in this file, since that will
only allow you to generate a single type of NMF that matches the current output.
If my understanding is correct, I think on Windows you want to be able to
generate both 32-bit and 64-bit ones in the same build, which means everything
should key off of current_cpu.

[Roland McGrath, 2015/11/11 21:44:32]

Why is Windows special?  If this is the proper condition, it needs a comment. 
But it seems like this should just depend on current_cpu, and the whole thing
will be instantiated twice for split 32/64 Windows.

[Petr Hosek, 2015/11/11 23:35:03]

This is taken from GYP and I haven't found any comment as to why it was setup
this way, but I guess we can remove this for now and see if it's still needed
after we get the Windows GN build working.

+        args +=
+            [ "--library-path=" + rebase_path("${nacl_toolchain_tooldir}/lib") ]
+        data = [
+          "$root_build_dir/lib64/",
+        ]
+      } else if (current_cpu == "arm") {

[Roland McGrath, 2015/11/11 21:44:31]

Just make this the default 'else' unless you are going to assert that
current_cpu is one of the recognized set.

[Petr Hosek, 2015/11/11 23:35:03]

Done.

+        args +=
+            [ "--library-path=" + rebase_path("${nacl_toolchain_tooldir}/lib") ]
+        data = [
+          "$root_build_dir/lib/",
+        ]
+      }
+    }
+    args += nmfflags + rebase_path(sources, root_build_dir)
+    if (is_nacl_glibc && current_cpu == "arm") {
+      deps += [ "//native_client/src/untrusted/elf_loader:elf_loader" ]
+    }
+  }
+}
+
+template("generate_nonsfi_test_nmf") {
+  assert(defined(invoker.executable), "Must define executable")
+  assert(defined(invoker.nmf), "Must define nmf")
+
+  action(target_name) {
+    forward_variables_from(invoker,
+                           [
+                             "deps",
+                             "data_deps",
+                             "executable",
+                             "nmf",
+                             "testonly",
+                             "public_deps",
+                           ])
+
+    script = "//ppapi/tests/create_nonsfi_test_nmf.py"
+    sources = [
+      executable,
+    ]
+    outputs = [
+      nmf,
+    ]
+    if (target_cpu == "x86") {
+      arch = "x86-32"
+    } else if (target_cpu == "x64") {
+      arch = "x86-64"
+    } else if (target_cpu == "arm") {

[Roland McGrath, 2015/11/11 21:44:32]

This can just default to arch = current_cpu.
All of these should be current_cpu instead of target_cpu.

[Petr Hosek, 2015/11/11 23:35:03]

Done.

[Petr Hosek, 2015/11/13 00:37:38]

Actually, in this case we really want target_cpu and not current_cpu because
current_cpu will always be pnacl for nonsfi while we're targeting for the target
platform (i.e. the cpu of the default toolchain).

+      arch = "arm"
+    }
+    args = [
+      "--program=" + rebase_path(executable, root_build_dir),
+      "--arch=${arch}",
+      "--output=" + rebase_path(nmf, root_build_dir),
+    ]
+  }
+}