Issue 5535 - Replace our module system with webpack

packagerChrome.py

Index: packagerChrome.py
diff --git a/packagerChrome.py b/packagerChrome.py
index d75203d17b9e63262e69f44e5112cfc729005d8d..9eaeee10e5a5deb96776f0cdec555c4b6f86a443 100644
--- a/packagerChrome.py
+++ b/packagerChrome.py
@@ -7,10 +7,12 @@ import io
 import json
 import os
 import re
+import shutil
 from StringIO import StringIO
 import struct
+import subprocess
 import sys
-import collections
+import tempfile
 
 from packager import (readMetadata, getDefaultFileName, getBuildVersion,
                       getTemplate, Files)
@@ -139,48 +141,65 @@ def createManifest(params, files):
     return manifest.encode('utf-8')
 
 
-def convertJS(params, files):
-    output_files = collections.OrderedDict()
-    args = {}
-
-    for item in params['metadata'].items('convert_js'):
-        name, value = item
-        filename, arg = re.search(r'^(.*?)(?:\[(.*)\])?$', name).groups()
-        if arg is None:
-            output_files[filename] = (value.split(), item.source)
-        else:
-            args.setdefault(filename, {})[arg] = value
-
-    template = getTemplate('modules.js.tmpl')
-
-    for filename, (input_files, origin) in output_files.iteritems():
-        if '/' in filename and not files.isIncluded(filename):
-            continue
-
-        current_args = args.get(filename, {})
-        current_args['autoload'] = [module for module in
-                                    current_args.get('autoload', '').split(',')
-                                    if module != '']
-
-        base_dir = os.path.dirname(origin)
-        modules = []
-
-        for input_filename in input_files:
-            module_name = os.path.splitext(os.path.basename(input_filename))[0]
-            prefix = os.path.basename(os.path.dirname(input_filename))
-            if prefix != 'lib':
-                module_name = '{}_{}'.format(prefix, module_name)
-            with open(os.path.join(base_dir, input_filename), 'r') as file:
-                modules.append((module_name, file.read().decode('utf-8')))
-            files.pop(input_filename, None)
+def create_bundles(params, files):
+    base_extension_path = params['baseDir']
+    info_templates = {
+        'chrome': 'chromeInfo.js.tmpl',
+        'edge': 'edgeInfo.js.tmpl',
+        'gecko-webext': 'geckoInfo.js.tmpl'
+    }
+    info_module = None
 
-        files[filename] = template.render(
-            args=current_args,
-            basename=params['metadata'].get('general', 'basename'),
-            modules=modules,
-            type=params['type'],
-            version=params['metadata'].get('general', 'version')
-        ).encode('utf-8')
+    # Once we use relative paths when requiring modules we can remove these,
+    # but in the mean time Webpack needs to know where to look.
+    resolve_paths = ' '.join(['./lib',
+                              './adblockpluscore/lib',
+                              './adblockplusui/lib'])

[Sebastian Noack, 2017/09/22 19:28:02]

Mind filing a follow up issue (and referring to it here), to replace absolute
paths across adblockpluschrome/ablockplusui/andblockpluscore?

For the time being, I'd prefer to look in 'lib' + '*/lib', rather than
hard-coding these paths. This however, would not work because 'buildtools/lib'
which includes conflicting scripts, which are only used in legacy Gecko
extensions. But we should remove support for legacy Gecko builds (as well as for
Safari builds) from buildtools, anyway, regardless of this change. Can you file
an issue for that as well?

[Sebastian Noack, 2017/09/22 22:21:12]

Never mind, these issues have already been filed, apparently:
https://issues.adblockplus.org/ticket/5751
https://issues.adblockplus.org/ticket/5752

[kzar, 2017/09/23 20:26:36]

OK since we're now considering removing buildtools/lib a blocker for this issue
I figured we could do that. I ran into two problems though. Firstly devenv.*/lib
directories were included, which broke everything. So I reused the
ensure_dependencies logic to only check directories which match our
dependencies. Second problem I hit was that elemHideEmulation started resolving
to adblockplus/lib/elemHideEmulation.js which broke everything too.

So now the logic says check the lib directory of all our immediate dependencies
unless they're called "adblockplus". I do wonder if it's worth the trouble
however, perhaps hardcoding them was better after all?

[Sebastian Noack, 2017/09/23 20:49:01]

It seems you missed this part?
Dang. FWIW, we also want to get rid of the "adblockplus" dependency. This
repository is deprecated now, and even before, since "adblockpluscore" moved to
a separate repository, we only kept the dependency on "adblockplus" as temporary
hack to import strings until we moved them where they belong. There should be an
issue for that, otherwise we should file one! However, since the issue tracker
is currently down, I cannot look into it.

Anyway, I do not necessarily want to block this review by an endless lists of
cleanups that are overdue, in particular since the code here is a temporary
workaround anyway. Feel free to go back to your initial approach just
whitelsiting the relevant paths, for now, and remove the dependency on #5751.
(Still I would love to see, #5751 and #5752 move forward independently.)

[kzar, 2017/09/24 09:47:53]

Oh, I forgot to mention that I tried to file the issues but the tracker was
down. I've done that now see #5760, #5761 and #5762.
Yea I agree, Done.

+    try:
+        temp_dir = tempfile.mkdtemp()

[Sebastian Noack, 2017/09/22 19:28:02]

This should be outside of the try-finally block. Otherwise, if it fails, the
code in the finally block would fail as well.

[kzar, 2017/09/23 20:26:35]

Done.

+        template = getTemplate(info_templates[params['type']])
+        with tempfile.NamedTemporaryFile(delete=False,

[Sebastian Noack, 2017/09/22 19:28:02]

Do we need to use NamedTemporaryFile here, or can't we just hard-code the
filename, since we are inside a temporary folder anyway?

[kzar, 2017/09/23 20:26:35]

Done. (But I wonder if the file name is more likely to clash now.)

+                                         dir=temp_dir) as info_file:
+            info_file.write(
+                template.render(
+                    basename=params['metadata'].get('general', 'basename'),
+                    version=params['metadata'].get('general', 'version')
+                ).encode('utf-8')
+            )
+            info_module = info_file.name
+
+        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(
+                    '.',
+                    os.path.relpath(os.path.join(base_item_path, module_path),
+                                    base_extension_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)
 
 
 def toJson(data):
@@ -389,8 +408,8 @@ def createBuild(baseDir, type='chrome', outFile=None, buildNum=None, releaseBuil
     files.readMappedFiles(mapped)
     files.read(baseDir, skip=[opt for opt, _ in mapped])
 
-    if metadata.has_section('convert_js'):
-        convertJS(params, files)
+    if metadata.has_section('bundles'):
+        create_bundles(params, files)
 
     if metadata.has_section('preprocess'):
         files.preprocess(