Introduce create_nonsfi_test_nmf.py to simplify nacl_test_data.gyp

ppapi/tests/create_nonsfi_test_nmf.py

Index: ppapi/tests/create_nonsfi_test_nmf.py
diff --git a/ppapi/tests/create_nonsfi_test_nmf.py b/ppapi/tests/create_nonsfi_test_nmf.py
new file mode 100755
index 0000000000000000000000000000000000000000..5860ebfcb92c76cec6776ca9c56c4178eefa8188
--- /dev/null
+++ b/ppapi/tests/create_nonsfi_test_nmf.py
@@ -0,0 +1,85 @@
+#!/usr/bin/env python

[Mark Seaborn, 2014/05/19 18:55:29]

Can you remove "nonsfi" from the file's name, perhaps, e.g.
"create_test_nmf.py"?

It would be good to see the NMF generation become more consistent, later on.  In
later changes, we could use this to generate NMFs for all cases in which we
generate statically-linked nexes (using either PNaCl or GCC toolchains), and it
would be easier if we didn't have to rename this file. :-)

[hidehiko, 2014/05/20 05:51:47]

I'm not still very sure if this should be reused for generating other nmf files.
This looks currently simple, but if we need to support either 1) multi-arch 2)
.so linked nmf and 3) runnable-ld.so, the code would rapidly become gigantic,
complicated, and looked just yet-another implementation of create_nmf.py.
(Actually, I tried :-)

IIUC, actually these are used for testing. For 1), as you said, Win uses it. 2)
and 3) are necessary for glibc testing now.

If we could replace all create_nmf.py, it would be an option. However, it is not
the case. And, to be honest, using create_nmf.py sounds better approach, because
it is closer to what users (developers) does. Moreover, create_nmf.py does more
than nmf creation. E.g., it also stages the needed files.

Do we want to use this for other nmf file generation, although it should be
supported by create_nmf.py?

I added "nonsfi" to the file name in the sense that; this script is used only
for nonsfi because create_nmf.py does not support generating nmf files for
nonsfi.

WDYT?

[Mark Seaborn, 2014/05/21 00:08:39]

OK, let's not get bogged down in this review with discussing how subsequent
changes could be done. :-)  We can talk about that after.  Sticking with the
name "create_nonsfi_test_nmf.py" is fine for this change.  The change is fine
since it does make the Gyp logic more regular.

[hidehiko, 2014/05/21 13:25:32]

Acknowledged. Thanks for reply.

+# Copyright 2014 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Simple tool to generate NMF file from given arguments for nonsfi."""

[Mark Seaborn, 2014/05/19 18:55:29]

Can you add a comment saying why this tool exists, comparing it to create_nmf.py
in the SDK?  How is it similar and how does it differ?

[hidehiko, 2014/05/20 05:51:47]

Could you tell me the future plans as I asked above?
I'll write more comments based on them. Currently, this is just a kind of lesser
copy of create_nmf.py only for nonsfi.

[Mark Seaborn, 2014/05/21 00:08:39]

The comment doesn't need to describe future plans, just the current state.

How about something like:

This tool is similar to native_client_sdk/src/tools/create_nmf.py. 
create_nmf.py handles most cases, with the exception of Non-SFI nexes. 
create_nmf.py tries to auto-detect nexe and pexe types based on their contents,
but this does not work for Non-SFI nexes (which don't have a marker to
distinguish them from SFI nexes).

create_nonsfi_test_nmf.py is a simpler tool which essentially just reformats its
command line arguments, converting them to JSON.

[hidehiko, 2014/05/21 13:25:32]

Done. Thank you for suggestion. How about this?

+
+import argparse
+import collections
+import json
+import logging
+import os
+
+_FILES_KEY = 'files'
+_PORTABLE_KEY = 'portable'
+_PROGRAM_KEY = 'program'
+_URL_KEY = 'url'
+_X86_32_NONSFI_KEY = 'x86-32-nonsfi'
+
+
+def ParseArgs():
+  parser = argparse.ArgumentParser()
+  parser.add_argument(
+      '--program', metavar='FILE', help='Main program file')

[Mark Seaborn, 2014/05/19 18:55:29]

Maybe s/file/nexe/ to be more specific?

[hidehiko, 2014/05/20 05:51:47]

Done.

+  # To keep compatibility with create_nmf.py, we use -x and --extra-files
+  # as flags.
+  parser.add_argument(
+      '-x', '--extra-files', action='append', metavar='KEY:FILE', default=[],
+      help=('Add extra key:file tuple to the "files"' +
+            ' esection of the .nmf'))

[Mark Seaborn, 2014/05/19 18:55:29]

Typo: "section"

[hidehiko, 2014/05/20 05:51:47]

Done.

+  parser.add_argument(
+      '--output', metavar='FILE', help='Path to the output nmf file.')
+
+  return parser.parse_args()
+
+
+def BuildNmfMap(root_path, program, extra_files):
+  """Build simple map representing nmf json."""
+  result = {
+    _PROGRAM_KEY: {
+      _X86_32_NONSFI_KEY: {
+        # The program path is relative to the root_path.
+        _URL_KEY: os.path.relpath(program, root_path)
+      }
+    }
+  }
+
+  if extra_files:
+    files = {}
+    for named_file in extra_files:
+      name, path = named_file.split(':', 1)
+      files[name] = {
+        _PORTABLE_KEY: {
+          # Note use path as is, unlike program path.

[Mark Seaborn, 2014/05/19 18:55:29]

Add colon -- "Note:"?

[hidehiko, 2014/05/20 05:51:47]

Done.

+          _URL_KEY: path
+        }
+      }
+    if files:
+      result[_FILES_KEY] = files
+
+  return result
+
+
+def OutputNmf(nmf_map, output_path):
+  """Writes the nmf to an output file at given path in JSON format."""
+  with open(output_path, 'w') as output:
+    json.dump(nmf_map, output, indent=2)
+
+
+def main():
+  args = ParseArgs()
+  if not args.program:
+    logging.error('--program is not specified.')
+    sys.exit(1)
+  if not args.output:
+    logging.error('--output is not specified.')
+    sys.exit(1)
+
+  nmf_map = BuildNmfMap(os.path.dirname(args.output),
+                        args.program, args.extra_files)
+  OutputNmf(nmf_map, args.output)
+
+
+if __name__ == '__main__':
+  main()