Introduce create_nonsfi_test_nmf.py to simplify nacl_test_data.gyp

ppapi/ppapi_nacl_test_common.gypi

Index: ppapi/ppapi_nacl_test_common.gypi
diff --git a/ppapi/ppapi_nacl_test_common.gypi b/ppapi/ppapi_nacl_test_common.gypi
index 0d9b676884821509868a0cb8600822b1f6d9e91c..4f029521de5a5b3f95fbd950b97b3c708e384fdb 100644
--- a/ppapi/ppapi_nacl_test_common.gypi
+++ b/ppapi/ppapi_nacl_test_common.gypi
@@ -35,6 +35,7 @@
           'out_pnacl_newlib%': '>(nacl_pnacl_newlib_out_dir)/>(nexe_target)_newlib_pnacl.pexe',
           'nmf_pnacl_newlib%': '>(nacl_pnacl_newlib_out_dir)/>(nexe_target).nmf',
           'out_pnacl_newlib_x86_32_nonsfi_nexe': '>(nacl_pnacl_newlib_nonsfi_out_dir)/>(nexe_target)_pnacl_newlib_x32_nonsfi.nexe',
+          'nmf_pnacl_newlib_nonsfi%': '>(nacl_pnacl_newlib_nonsfi_out_dir)/>(nexe_target).nmf',
         }],
       ],
     },
@@ -64,8 +65,8 @@
           },
         ],
       }],
-      # Nonsfi pnacl copy is covered below. Currently, these are exclusive.
-      ['test_files!=[] and build_pnacl_newlib==1 and disable_pnacl==0 and enable_x86_32_nonsfi==0', {
+      # Nonsfi pnacl copy is covered below.
+      ['test_files!=[] and build_pnacl_newlib==1 and disable_pnacl==0 and (enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or enable_mips==1)', {

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

Can you assume that "enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or
enable_mips==1" is true, please?

See comment below.

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

Acknowledged.

         'copies': [
           {
             'destination': '>(nacl_pnacl_newlib_out_dir)',
@@ -106,6 +107,7 @@
           ],
           'create_nmf': '<(DEPTH)/native_client_sdk/src/tools/create_nmf.py',
           'create_nmf_args_portable%': [],
+          'create_nonsfi_test_nmf': '<(DEPTH)/ppapi/tests/create_nonsfi_test_nmf.py',
         },
         'target_conditions': [
           ['generate_nmf==1 and build_newlib==1', {
@@ -188,7 +190,7 @@
               },
             ],
           }],
-          ['generate_nmf==1 and build_pnacl_newlib==1 and disable_pnacl==0', {
+          ['generate_nmf==1 and build_pnacl_newlib==1 and disable_pnacl==0 and (enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or enable_mips==1)', {

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

I don't understand the reason for changing this.

In what case would "enable_x86_32==1 or enable_x86_64==1 or enable_arm==1 or
enable_mips==1" be false?

I think it would be better to assume that's always true.  That would reduce the
number of places in which we have to list all supported architectures.

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

Conceptually, we should, for irt_manifest_file, if possible.

Currently build_pnacl_newlib is used for both;
1) building a pexe binary
2) building a nexe binary for x86-32-nonsfi
These are conceptually independent, but nonsfi nexe binary is translated from
pexe binary so the flag is needed, as you know.

Due to some reason, we may want to enable only 2). Current irt_manifest_file is
an example, I think. (Indeed, ideally, we probably want to enable both 1) and 2)
for irt_manifest_file, though)
It may not actually cause a problem, but building unnecessary targets looked
confusing to me, so I just tried to get rid of it.

Currently we have four categories for tests, which are newlib, glibc, pnacl and
nonsfi. Before adding nonsfi, build_newlib, build_glibc, build_pnacl_newlib was
one-to-one-onto mapping for them. However, nonsfi is a bit tricky as it is
"build_pnacl_newlib = 1 && enable_x86_32_nonsfi". So probably, an alternative
approach would be introducing new variables, e.g.;
enable_nacl_newlib_test
enable_nacl_glibc_test
enable_nacl_pnacl_test
enable_nacl_nonsfi_test

WDYT?
Please let me keep this as is in this patch set. I'll revisit here, upon your
reply.

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

...
OK, let's leave this as is.

My main problem with this extra condition was that it's not very clear what it's
for -- whether it is:

 * just an optimisation (avoiding building something we don't need);
 * needed for correctness (maybe the build fails if we try this when no inputs
are present).

I'm not sure whether it's worth commenting this in the gyp file.
If we need labels like this, they would be clearer as:

enable_pnacl_pexe_test
enable_pnacl_sfi_nexe_test
enable_pnacl_nonsfi_nexe_test

So that it's clearer whether the NMFs list pexes or nexes.  But this is just an
aside which doesn't need to affect your current change.

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

Seems like we want to control sfi glibc and sfi newlib test building separately?
Anyway, I'll try to work on it as another clean up in a separate CL.

             'actions': [
               {
                 'action_name': 'Generate PNACL NEWLIB NMF',
@@ -206,6 +208,26 @@
               },
             ],
           }],
+          ['generate_nmf==1 and build_pnacl_newlib==1 and disable_pnacl==0 and enable_x86_32_nonsfi', {

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

Should this have "enable_x86_32_nonsfi==1" (add "==1") for consistency?

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

Done.

+            'actions': [
+              {
+                'action_name': 'Generate PNACL NEWLIB nonsfi NMF',
+                # If we support arm, we should split the dependency to

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

Nit: "If we add support for ARM"?  "dependency on" rather than "dependency to"?

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

Done.

+                # out_pnacl_newlib_x86_32_nonsfi_nexe to 'target_conditions',
+                # similar to build_newlib=1 config declared above.
+                'inputs': ['>(create_nonsfi_test_nmf)',
+                           '>(out_pnacl_newlib_x86_32_nonsfi_nexe)'],
+                'outputs': ['>(nmf_pnacl_newlib_nonsfi)'],
+                'action': [
+                  'python',
+                  '>(create_nonsfi_test_nmf)',
+                  '--output=>(nmf_pnacl_newlib_nonsfi)',
+                  '--program=>(out_pnacl_newlib_x86_32_nonsfi_nexe)',
+                  '>@(create_nmf_args_portable)'
+                ],
+              },
+            ],
+          }],
         ],
       }],
     ],