Refactor compile_js.gypi to support script_args and closure_args

remoting/app_remoting_webapp.gyp

Index: remoting/app_remoting_webapp.gyp
diff --git a/remoting/app_remoting_webapp.gyp b/remoting/app_remoting_webapp.gyp
index fad6f428f1a8e6b97b49d804fe0a417f192c85b7..9c41bc1748f80cfe9cf7ae2bd1cc7c036d6211eb 100644
--- a/remoting/app_remoting_webapp.gyp
+++ b/remoting/app_remoting_webapp.gyp
@@ -6,98 +6,18 @@
   'includes': [
     '../remoting/remoting_locales.gypi',
     '../remoting/app_remoting_webapp_build.gypi',
-  ],
-
+   ],

[Theresa, 2015/06/12 20:36:50]

will fix whitespace

[Theresa, 2015/06/12 22:12:15]

Done.

   'target_defaults': {
     'variables': {
       'ar_internal': 0,
-      'compiler_flags': [
-        '--strict',
-        '--no-single-file',
-        '--externs',
-        '../third_party/closure_compiler/externs/chrome_extensions.js',
-      ],
     },
-
-    'conditions': [
-      ['run_jscompile != 0', {
-        'actions': [
-          {
-            'action_name': 'Verify >(ar_app_name) main.html',
-            'variables': {
-              'success_stamp': '<(PRODUCT_DIR)/>(_target_name)_main_jscompile.stamp',
-            },
-            'inputs': [
-              '<@(ar_main_js_files)',
-              '<@(remoting_webapp_js_proto_files)',
-              # Include zip as input so that this action is run after the build.
-              '<(zip_path)',
-            ],
-            'outputs': [
-              '<(success_stamp)',
-            ],
-            'action': [
-              'python', '../third_party/closure_compiler/compile.py',
-              '<@(compiler_flags)',
-              '--success-stamp', '<(success_stamp)',
-              '<@(ar_main_js_files)',
-              '<@(remoting_webapp_js_proto_files)',
-            ],
-          },
-          {
-            'action_name': 'Verify >(ar_app_name) background.js',
-            'variables': {
-              'success_stamp': '<(PRODUCT_DIR)/>(_target_name)_background_jscompile.stamp',
-            },
-            'inputs': [
-              '<@(ar_background_js_files)',
-              '<@(remoting_webapp_js_proto_files)',
-              # Include zip as input so that this action is run after the build.
-              '<(zip_path)',
-            ],
-            'outputs': [
-              '<(success_stamp)',
-            ],
-            'action': [
-              'python', '../third_party/closure_compiler/compile.py',
-              '<@(compiler_flags)',
-              '--success-stamp', '<(success_stamp)',
-              '<@(ar_background_js_files)',
-              '<@(remoting_webapp_js_proto_files)',
-            ],
-          },
-          {
-            'action_name': 'Verify >(ar_app_name) feedback_consent.html',
-            'variables': {
-              'success_stamp': '<(PRODUCT_DIR)/>(_target_name)_feedback_consent_jscompile.stamp',
-            },
-            'inputs': [
-              '<@(ar_feedback_consent_html_all_js_files)',
-              '<@(remoting_webapp_js_proto_files)',
-              # Include zip as input so that this action is run after the build.
-              '<(zip_path)',
-            ],
-            'outputs': [
-              '<(success_stamp)',
-            ],
-            'action': [
-              'python', '../third_party/closure_compiler/compile.py',
-              '<@(compiler_flags)',
-              '--success-stamp', '<(success_stamp)',
-              '<@(ar_feedback_consent_html_all_js_files)',
-              '<@(remoting_webapp_js_proto_files)',
-            ],
-          },
-        ],  # actions
-      }],
-    ],  # conditions
   },
-
   'targets': [
     {
       # GN version: //remoting/webapp:ar_sample_app
       # Sample AppRemoting app.
       'target_name': 'ar_sample_app',
+      'type': 'none',

[garykac, 2015/06/16 00:32:42]

This change shouldn't be needed since it comes from app_remoting_webapp_build's
target_defaults. If it's not already defined here, then something odd is going
on.

[Theresa, 2015/06/16 17:49:45]

Done.

       'app_key': 'Sample_App',
       'app_id': 'ljacajndfccfgnfohlgkdphmbnpkjflk',
       'app_client_id': 'sample_client_id',
@@ -105,6 +25,17 @@
       'app_description': 'App Remoting client',
       'app_capabilities': ['GOOGLE_DRIVE'],
       'manifest_key': 'remotingdevbuild',
+      'conditions': [

[garykac, 2015/06/16 00:32:42]

This needs to be in 'target_defaults' (where it was) because there is more than
one AppRemoting target and they need to share the build rules and conditions
rather than repeat them in each target. The 'target' should contain the minimal
set of info that differs for each target.

Most of the targets are internal - ar_sample_app is the public sample app that
acts as a sanity check to make sure that the builds don't break.

You should be building at least one of the internal apps to verify your changes.
I can give you more info separately.

[Theresa, 2015/06/16 17:49:45]

Moved to target_defaults.

Yes, please send instructions for an internal app that is easy to test. I work
on Chrome for Android, so assume my setup is from scratch wrt to testing
remoting.

+        ['run_jscompile != 0', {
+          'dependencies': [
+            'app_remoting_webapp_compile.gypi:*'

[garykac, 2015/06/16 00:32:42]

Note that use of the '*' has been deprecated in GYP because this feature does
not exist in GN. The workaround is to explicitly list each target.

[Theresa, 2015/06/16 17:49:45]

Done.

+          ],
+        }],
+      ], # conditions
+#       'inputs': [
+#         # Include zip as input so that this action is run after the build.
+#         '<(zip_path)',

[Theresa, 2015/06/12 20:36:50]

ninja is not at all happy with this variable. It complains that it's undefined,
even though it's set in the target_defaults section of
app_remoting_webapp_build.gypi, which is in the includes list above.

Any thoughts on why it's not picking it up? Is this a critical piece to keep 
(why does this set of actions need to run after the build? after the build of
what?)?

[garykac, 2015/06/16 00:32:42]

No. But the previous code was working when it was in the target_defaults
section. Since this needs to go back there, you should do that before
investigating further.
I don't think that part is critical anymore, and we may need to update how we
jscompile anyway (since we our build script writes into the JS files - we want
to modify it to check the output of that rather than the checked-in files). So
you can omit this.

[Theresa, 2015/06/16 17:49:45]

It works now that it's back in target_defaults. Would you prefer if I leave it
in or omit it?

+#       ],
     },
   ],  # end of targets
 }