It2Me native messaging: GYP and source refactoring

remoting/remoting.gyp

Index: remoting/remoting.gyp
diff --git a/remoting/remoting.gyp b/remoting/remoting.gyp
index c268d9997355f982a96db0fdf3ea708d47a95571..5786d1eacd14e67201681a95af0b638f3dd0b856 100644
--- a/remoting/remoting.gyp
+++ b/remoting/remoting.gyp
@@ -529,6 +529,23 @@
         },  # end of target 'remoting_host'
 
         {
+          'target_name': 'remoting_native_messaging_base',
+          'type': 'static_library',
+          'variables': { 'enable_wexit_time_destructors': 1, },
+          'dependencies': [
+            '../base/base.gyp:base',
+          ],
+          'sources': [
+            'host/native_messaging/native_messaging_channel.cc',
+            'host/native_messaging/native_messaging_channel.h',
+            'host/native_messaging/native_messaging_reader.cc',
+            'host/native_messaging/native_messaging_reader.h',
+            'host/native_messaging/native_messaging_writer.cc',
+            'host/native_messaging/native_messaging_writer.h',
+          ],
+        },  # end of target 'remoting_native_messaging_base'
+
+        {
           'target_name': 'remoting_me2me_host_static',
           'type': 'static_library',
           'variables': { 'enable_wexit_time_destructors': 1, },
@@ -589,14 +606,6 @@
             'host/setup/daemon_installer_win.h',
             'host/setup/host_starter.cc',
             'host/setup/host_starter.h',
-            'host/setup/native_messaging_channel.cc',
-            'host/setup/native_messaging_channel.h',
-            'host/setup/native_messaging_host.cc',
-            'host/setup/native_messaging_host.h',
-            'host/setup/native_messaging_reader.cc',
-            'host/setup/native_messaging_reader.h',
-            'host/setup/native_messaging_writer.cc',
-            'host/setup/native_messaging_writer.h',
             'host/setup/oauth_client.cc',
             'host/setup/oauth_client.h',
             'host/setup/oauth_helper.cc',
@@ -623,6 +632,26 @@
         },  # end of target 'remoting_host_setup_base'
 
         {
+          'target_name': 'remoting_me2me_native_messaging_host_static',

[Sergey Ulanov, 2013/10/30 22:51:01]

Do we really need this target? I think you can just include
me2me_native_messaging_host.* in remoting_host_setup_base and trust the linker
to strip that code from binaries that don't use it.

[weitao, 2013/10/31 17:59:34]

I think this makes a cleaner componentization. setup_base includes all the
DaemonController functionality that is shared between the NPAPI plugin and the
native messaging host.

One good example is Jamie's fix for the Mac issue (293036). Since
native_messaging_host.cc is included in the setup_base target almost all
remoting targets need to be rebuilt. Now that it is moved
me2me_native_messaging_host_static, only the targets that include native
messaging (me2me_native_messaging_host and remoting_core) need to be rebuilt.

Initially I didn't have this me2me_native_messaging_host_static target and
included me2me_native_messaging_host.* directly in
remoting_me2me_native_messaging_host. But that broke the Windows build which
combines the me2me host and the NM host in one binary. So I created this static
target to accommodate that.

WDYT?

On 2013/10/30 22:51:01, Sergey Ulanov wrote:

[Sergey Ulanov, 2013/10/31 19:20:42]

Problem is that each additional target adds complexity in this file making it
harder for humans to read and edit it. It's already freaking 3000 lines long!

On 2013/10/31 17:59:34, weitaosu wrote:
Having multiple targets may make build faster in some cases, but it makes it
slower in all other cases, so it's not clear if it is a win overall. Each
additional target requires separate library file to be generated. It also means
that GYP will take longer to process this file (e.g. a separate .ninja file is
generated for each build target).
Right, that's why I think it should be part of remoting_host_setup_base.

[weitao, 2013/10/31 20:59:16]

Done.

+          'type': 'static_library',
+          'variables': { 'enable_wexit_time_destructors': 1, },
+          'dependencies': [
+            '../base/base.gyp:base',
+            'remoting_host',
+            'remoting_host_logging',
+            'remoting_native_messaging_base',
+            'remoting_host_setup_base',
+          ],
+          'defines': [
+            'VERSION=<(version_full)',
+          ],
+          'sources': [
+            'host/setup/me2me_native_messaging_host.cc',
+            'host/setup/me2me_native_messaging_host.h',
+          ],
+        },  # end of target 'remoting_me2me_native_messaging_host_static'
+
+        {
           'target_name': 'remoting_host_plugin',
           'type': 'loadable_module',
           'variables': { 'enable_wexit_time_destructors': 1, },
@@ -748,8 +777,8 @@
             'remoting_resources',
           ],
           'sources': [
-            'host/it2me/it2me_impl.cc',
-            'host/it2me/it2me_impl.h',
+            'host/it2me/it2me_host.cc',
+            'host/it2me/it2me_host.h',
           ],
         },  # end of target 'remoting_it2me_host_static'
         {
@@ -899,7 +928,8 @@
               'dependencies': [
                 'remoting_me2me_host',
                 'remoting_start_host',
-                'remoting_native_messaging_host',
+                'remoting_me2me_native_messaging_host',
+                'remoting_me2me_native_messaging_manifest',
               ],
               'actions': [
                 {
@@ -1021,21 +1051,14 @@
           ],  # end of 'conditions'
         },  # end of target 'remoting_me2me_host'
         {
-          'target_name': 'remoting_native_messaging_host',
+          'target_name': 'remoting_me2me_native_messaging_host',
           'type': 'executable',
           'variables': { 'enable_wexit_time_destructors': 1, },
           'dependencies': [
-            '../base/base.gyp:base',
-            'remoting_host',
-            'remoting_host_logging',
-            'remoting_host_setup_base',
-            'remoting_native_messaging_manifest',
-          ],
-          'defines': [
-            'VERSION=<(version_full)',
+            'remoting_me2me_native_messaging_host_static',
           ],
           'sources': [
-            'host/setup/native_messaging_host_main.cc',
+            'host/setup/me2me_native_messaging_host_main.cc',
           ],
           'conditions': [
             ['OS=="linux" and linux_use_tcmalloc==1', {
@@ -1044,7 +1067,7 @@
               ],
             }],
           ],
-        },  # end of target 'remoting_native_messaging_host'
+        },  # end of target 'remoting_me2me_native_messaging_host'
       ],  # end of 'targets'
     }],  # 'OS!="win" and enable_remoting_host==1'
 
@@ -1158,7 +1181,8 @@
             'remoting_host_prefpane',
             'remoting_host_uninstaller',
             'remoting_me2me_host',
-            'remoting_native_messaging_host',
+            'remoting_me2me_native_messaging_host',
+            'remoting_me2me_native_messaging_manifest',
           ],
           'variables': {
             'host_name': '<!(python <(version_py_path) -f <(branding_path) -t "@HOST_PLUGIN_FILE_NAME@")',
@@ -1188,7 +1212,7 @@
                 'PrivilegedHelperTools/org.chromium.chromoting.me2me_host.app',
                 'Applications/<(host_uninstaller_name).app',
                 'PrivilegedHelperTools/org.chromium.chromoting.me2me_host.app/Contents/MacOS/native_messaging_host',
-		'Config/com.google.chrome.remote_desktop.json',
+                'Config/com.google.chrome.remote_desktop.json',
               ],
               'source_files': [
                 '<@(remoting_host_installer_mac_files)',
@@ -1536,6 +1560,8 @@
             'remoting_lib_ps',
             'remoting_lib_rc',
             'remoting_me2me_host_static',
+            'remoting_me2me_native_messaging_host_static',
+            'remoting_native_messaging_base',
             'remoting_protocol',
             'remoting_version_resources',
           ],
@@ -1570,6 +1596,7 @@
             'host/remoting_me2me_host.cc',
             'host/sas_injector.h',
             'host/sas_injector_win.cc',
+            'host/setup/me2me_native_messaging_host_main.cc',
             'host/verify_config_window_win.cc',
             'host/verify_config_window_win.h',
             'host/win/chromoting_module.cc',
@@ -1949,7 +1976,7 @@
             'remoting_core',
             'remoting_desktop',
             'remoting_host_exe',
-            'remoting_native_messaging_manifest',
+            'remoting_me2me_native_messaging_manifest',
           ],
           'compiled_inputs': [
             '<(PRODUCT_DIR)/remoting_core.dll',
@@ -2306,16 +2333,16 @@
       ],
     }, # end of target 'remoting_webapp'
 
-    # Generates 'native_messaging_manifest.json' to be included in the
+    # Generates 'me2me_native_messaging_manifest.json' to be included in the
     # installation.
     {
-      'target_name': 'remoting_native_messaging_manifest',
+      'target_name': 'remoting_me2me_native_messaging_manifest',
       'type': 'none',
       'dependencies': [
         'remoting_resources',
       ],
       'variables': {
-        'input': 'host/setup/native_messaging_manifest.json',
+        'input': 'host/setup/me2me_native_messaging_manifest.json',
         'output': '<(PRODUCT_DIR)/remoting/com.google.chrome.remote_desktop.json',
       },
       'target_conditions': [
@@ -2323,19 +2350,19 @@
           'conditions': [
             [ 'OS == "win"', {
               'variables': {
-                'native_messaging_host_path': 'remoting_host.exe',
+                'me2me_native_messaging_host_path': 'remoting_host.exe',
               },
             }], [ 'OS == "mac"', {
               'variables': {
-                'native_messaging_host_path': '/Library/PrivilegedHelperTools/org.chromium.chromoting.me2me_host.app/Contents/MacOS/native_messaging_host',
+                'me2me_native_messaging_host_path': '/Library/PrivilegedHelperTools/org.chromium.chromoting.me2me_host.app/Contents/MacOS/native_messaging_host',
               },
             }], ['OS == "linux"', {
               'variables': {
-                'native_messaging_host_path': '/opt/google/chrome-remote-desktop/native-messaging-host',
+                'me2me_native_messaging_host_path': '/opt/google/chrome-remote-desktop/native-messaging-host',
               },
             }], ['OS != "linux" and OS != "mac" and OS != "win"', {
               'variables': {
-                'native_messaging_host_path': '/opt/google/chrome-remote-desktop/native-messaging-host',
+                'me2me_native_messaging_host_path': '/opt/google/chrome-remote-desktop/native-messaging-host',
               },
             }],
           ],  # conditions
@@ -2352,7 +2379,7 @@
               'action': [
                 'python',
                 '<(remoting_localize_path)',
-                '--define', 'NATIVE_MESSAGING_HOST_PATH=<(native_messaging_host_path)',
+                '--define', 'ME2ME_NATIVE_MESSAGING_HOST_PATH=<(me2me_native_messaging_host_path)',
                 '--locale_dir', '<(webapp_locale_dir)',
                 '--template', '<(input)',
                 '--locale_output',
@@ -2365,7 +2392,7 @@
         },
        ],
       ],  # target_conditions
-    },  # end of target 'remoting_native_messaging_manifest'
+    },  # end of target 'remoting_me2me_native_messaging_manifest'
     {
       'target_name': 'remoting_resources',
       'type': 'none',
@@ -2772,6 +2799,8 @@
         'remoting_host_event_logger',
         'remoting_host_setup_base',
         'remoting_jingle_glue',
+        'remoting_me2me_native_messaging_host_static',
+        'remoting_native_messaging_base',
         'remoting_protocol',
         'remoting_resources',
         '../third_party/webrtc/modules/modules.gyp:desktop_capture',
@@ -2836,6 +2865,8 @@
         'host/linux/x_server_clipboard_unittest.cc',
         'host/local_input_monitor_unittest.cc',
         'host/log_to_server_unittest.cc',
+        'host/native_messaging/native_messaging_reader_unittest.cc',
+        'host/native_messaging/native_messaging_writer_unittest.cc',
         'host/pairing_registry_delegate_linux_unittest.cc',
         'host/pairing_registry_delegate_win_unittest.cc',
         'host/pin_hash_unittest.cc',
@@ -2851,9 +2882,7 @@
         'host/screen_capturer_fake.h',
         'host/screen_resolution_unittest.cc',
         'host/server_log_entry_unittest.cc',
-        'host/setup/native_messaging_host_unittest.cc',
-        'host/setup/native_messaging_reader_unittest.cc',
-        'host/setup/native_messaging_writer_unittest.cc',
+        'host/setup/me2me_native_messaging_host_unittest.cc',
         'host/setup/oauth_helper_unittest.cc',
         'host/setup/pin_validator_unittest.cc',
         'host/token_validator_factory_impl_unittest.cc',
@@ -2956,6 +2985,8 @@
           'dependencies!': [
             'remoting_host',
             'remoting_host_setup_base',
+            'remoting_me2me_native_messaging_host_static',
+            'remoting_native_messaging_base',
           ],
           'sources/': [
             ['exclude', 'codec/*'],