Issue 5535 - Replace our module system with webpack

packagerChrome.py

Index: packagerChrome.py
diff --git a/packagerChrome.py b/packagerChrome.py
index bf28457acf544c84b276cab5f71fb089bfc4ba5d..eef29614668b7d5f4b8634b5b1fe70c44c7d74cb 100644
--- a/packagerChrome.py
+++ b/packagerChrome.py
@@ -7,12 +7,10 @@ import io
 import json
 import os
 import re
-import shutil
 from StringIO import StringIO
 import struct
 import subprocess
 import sys
-import tempfile
 
 from ensure_dependencies import read_deps
 from packager import (readMetadata, getDefaultFileName, getBuildVersion,
@@ -142,6 +140,13 @@ def createManifest(params, files):
     return manifest.encode('utf-8')
 
 
+def toJson(data):
+    return json.dumps(
+        data, ensure_ascii=False, sort_keys=True,
+        indent=2, separators=(',', ': ')
+    ).encode('utf-8') + '\n'
+
+
 def create_bundles(params, files):
     base_extension_path = params['baseDir']
     info_templates = {
@@ -149,62 +154,49 @@ def create_bundles(params, files):
         'edge': 'edgeInfo.js.tmpl',
         'gecko-webext': 'geckoInfo.js.tmpl'
     }
-    info_module = None
 
     # Historically we didn't use relative paths when requiring modules, so in
     # order for webpack to know where to find them we need to pass in a list of
     # resolve paths. Going forward we should always use relative paths, once we
     # do that consistently this can be removed. See issues 5760, 5761 and 5762.
-    resolve_paths = ' '.join(
-        [os.path.join(base_extension_path, dir, 'lib')
-         for dir in ['', 'adblockpluscore', 'adblockplusui']]
-    )
-
-    temp_dir = tempfile.mkdtemp()
-    try:
-        info_module = os.path.join(temp_dir, 'info.js')
-        template = getTemplate(info_templates[params['type']])
-        with open(info_module, 'w') as info_file:
-            info_file.write(
-                template.render(
-                    basename=params['metadata'].get('general', 'basename'),
-                    version=params['metadata'].get('general', 'version')
-                ).encode('utf-8')
-            )
+    resolve_paths = [os.path.join(base_extension_path, dir, 'lib')
+                     for dir in ['', 'adblockpluscore', 'adblockplusui']]
 
-        for item in params['metadata'].items('bundles'):
-            name, value = item
-            base_item_path = os.path.dirname(item.source)
-
-            bundle_file = os.path.relpath(os.path.join(base_item_path, name),
-                                          base_extension_path)
-            entry_files = [os.path.join(base_item_path, module_path)
-                           for module_path in value.split()]
-            subprocess.check_call(
-                ['npm', 'run-script', 'webpack', '--silent'],
-                cwd=os.path.dirname(__file__),
-                env={
-                    'EXTENSION_PATH': base_extension_path,
-                    'ENTRY_POINTS': ' '.join(entry_files),
-                    'OUTPUT_PATH': temp_dir,
-                    'BUNDLE_NAME': bundle_file,
-                    'RESOLVE_PATHS': resolve_paths,
-                    'INFO_PATH': info_module,
-                    'PATH': os.environ['PATH']
-                }
-            )
-            for file_name in [bundle_file, bundle_file + '.map']:
-                with open(os.path.join(temp_dir, file_name), 'r') as f:
-                    files[file_name] = f.read()
-    finally:
-        shutil.rmtree(temp_dir)
+    info_template = getTemplate(info_templates[params['type']])
+    info_module = info_template.render(
+        basename=params['metadata'].get('general', 'basename'),
+        version=params['metadata'].get('general', 'version')
+    ).encode('utf-8')
 
+    boundary = '=============================WEBPACK-BOUNDARY'
+    configuration = {
+        'BOUNDARY': boundary,
+        'BUNDLES': [],
+        'EXTENSION_PATH': base_extension_path,
+        'INFO_MODULE': info_module,
+        'RESOLVE_PATHS': resolve_paths,

[Wladimir Palant, 2017/10/09 10:54:54]

Nit: it's a JSON structure, why are properties all-caps?

[kzar, 2017/10/09 13:52:14]

Done.

+    }
 
-def toJson(data):
-    return json.dumps(
-        data, ensure_ascii=False, sort_keys=True,
-        indent=2, separators=(',', ': ')
-    ).encode('utf-8') + '\n'
+    for item in params['metadata'].items('bundles'):
+        name, value = item
+        base_item_path = os.path.dirname(item.source)
+
+        bundle_file = os.path.relpath(os.path.join(base_item_path, name),
+                                      base_extension_path)
+        entry_files = [os.path.join(base_item_path, module_path)
+                       for module_path in value.split()]
+        configuration['BUNDLES'].append({
+            'BUNDLE_NAME': bundle_file,
+            'ENTRY_POINTS': entry_files,
+        })
+
+    output = subprocess.check_output(
+        ['node',
+         os.path.join(os.path.dirname(__file__), 'webpack_runner.js'),
+         toJson(configuration)]

[Wladimir Palant, 2017/10/09 10:54:55]

Passing the configuration as parameter has an ugly side-effect: if the command
fails, the error output is massive due to quoting the command line parameters.
Also, the size of command line parameters might be limited on some platforms
(Windows?). You could use subprocess.communicate() to pass data to the process
via stdin. Note also that info_loader.js doesn't need to parse input separately
- webpack_runner.js could require("./info_loader") and call some method to set
info module source.

[kzar, 2017/10/09 13:52:15]

Done, you're right the error output looks way nicer now. (We could also catch
the subprocess.CalledProcessError, and re-throw with the configuration removed
from the command I suppose. That would be simpler but seems like kind of a
hack.)

+    ).split(boundary)
+    for i in range(0, len(output) - 1, 2):
+        files[output[i].strip()] = output[i + 1].strip()
 
 
 def import_string_webext(data, key, source):