feat(builtin): use linker for all generated :bin targets

rather than loader.js module-resolver hacks for generated targets, we now expect that tools will use the linker to set up their node_modules tree, or will explicitly use a runfiles helper library to resolve files in the runfiles, like other languages in Bazel. BREAKING CHANGE: If you use the generated nodejs_binary or nodejs_test rules in the npm workspace, for example @npm//typescript/bin:tsc, your custom rule must now link the node_modules directory into that process. A typical way to do this is with the run_node helper. See updates to examples in this commit.
bazel-contrib · Apr 11, 2020 · 007a8f6 · 007a8f6
1 parent 55d132f
commit 007a8f6
Show file tree

Hide file tree

Showing 16 changed files with 86 additions and 32 deletions.
diff --git a/examples/BUILD.bazel b/examples/BUILD.bazel
@@ -91,6 +91,10 @@ example_integration_test(
     npm_packages = {
         "//packages/jasmine:npm_package": "@bazel/jasmine",
     },
+    # Parcel spawns a subprocess which requires our node-patches
+    # but we don't yet have a mechanism on Windows for spawned processes
+    # to inherit the --require script needed to install the patches
+    tags = ["no-bazelci-windows"],
 )
 
 example_integration_test(

diff --git a/examples/parcel/parcel.bzl b/examples/parcel/parcel.bzl
@@ -17,6 +17,8 @@
 This is not a full-featured Parcel bazel rule, just enough to demonstrate how to write one.
 """
 
+load("@build_bazel_rules_nodejs//:providers.bzl", "run_node")
+
 def _parcel_impl(ctx):
     """The "implementation function" for our rule.
 
@@ -31,9 +33,12 @@ def _parcel_impl(ctx):
     args += ["--out-dir", ctx.outputs.bundle.dirname]
     args += ["--out-file", ctx.outputs.bundle.basename]
 
-    ctx.actions.run(
+    # We use the run_node helper rather than ctx.actions.run so that the npm package
+    # gets automatically included in the action inputs
+    run_node(
+        ctx = ctx,
         inputs = ctx.files.srcs + [ctx.file.entry_point],
-        executable = ctx.executable.parcel,
+        executable = "parcel",
         outputs = [ctx.outputs.bundle, ctx.outputs.sourcemap],
         arguments = args,
         env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]},

diff --git a/internal/linker/index.js b/internal/linker/index.js
@@ -143,12 +143,16 @@ class Runfiles {
     }
     resolve(modulePath) {
         if (this.manifest) {
-            return this.lookupDirectory(modulePath);
+            const result = this.lookupDirectory(modulePath);
+            if (result)
+                return result;
         }
-        if (exports.runfiles.dir) {
+        if (exports.runfiles.dir && fs.existsSync(path.join(exports.runfiles.dir, modulePath))) {
             return path.join(exports.runfiles.dir, modulePath);
         }
-        throw new Error(`could not resolve modulePath ${modulePath}`);
+        const e = new Error(`could not resolve modulePath ${modulePath}`);
+        e.code = 'MODULE_NOT_FOUND';
+        throw e;
     }
     resolveWorkspaceRelative(modulePath) {
         if (!this.workspace) {
@@ -345,7 +349,12 @@ function main(args, runfiles) {
                             else {
                                 runfilesPath = `${workspace}/${runfilesPath}`;
                             }
-                            target = runfiles.resolve(runfilesPath) || '<runfiles resolution failed>';
+                            try {
+                                target = runfiles.resolve(runfilesPath);
+                            }
+                            catch (_a) {
+                                target = '<runfiles resolution failed>';
+                            }
                             break;
                     }
                     yield symlink(target, m.name);

diff --git a/internal/linker/link_node_modules.ts b/internal/linker/link_node_modules.ts
@@ -245,12 +245,15 @@ export class Runfiles {
   resolve(modulePath: string) {
     // Look in the runfiles first
     if (this.manifest) {
-      return this.lookupDirectory(modulePath);
+      const result = this.lookupDirectory(modulePath);
+      if (result) return result;
     }
-    if (runfiles.dir) {
+    if (runfiles.dir && fs.existsSync(path.join(runfiles.dir, modulePath))) {
       return path.join(runfiles.dir, modulePath);
     }
-    throw new Error(`could not resolve modulePath ${modulePath}`);
+    const e = new Error(`could not resolve modulePath ${modulePath}`);
+    (e as any).code = 'MODULE_NOT_FOUND';
+    throw e;
   }
 
   resolveWorkspaceRelative(modulePath: string) {
@@ -569,7 +572,11 @@ export async function main(args: string[], runfiles: Runfiles) {
           } else {
             runfilesPath = `${workspace}/${runfilesPath}`;
           }
-          target = runfiles.resolve(runfilesPath) || '<runfiles resolution failed>';
+          try {
+            target = runfiles.resolve(runfilesPath);
+          } catch {
+            target = '<runfiles resolution failed>';
+          }
           break;
       }
 

diff --git a/internal/node/node_patches.js b/internal/node/node_patches.js
@@ -593,7 +593,10 @@ fi
         process.env.PATH = nodeDir + path.delimiter + process.env.PATH;
     }
     // fix execPath so folks use the proxy node
-    process.argv[0] = process.execPath = path.join(nodeDir, 'node');
+    if (process.platform == 'win32') ;
+    else {
+        process.argv[0] = process.execPath = path.join(nodeDir, 'node');
+    }
     // replace any instances of require script in execArgv with the absolute path to the script.
     // example: bazel-require-script.js
     process.execArgv.map(v => {

diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts
@@ -1074,7 +1074,8 @@ nodejs_binary(
     name = "${name}",
     entry_point = "//:node_modules/${pkg._dir}/${path}",
     install_source_map_support = False,
-    data = [${data.map(p => `"${p}"`).join(', ')}],${additionalAttributes(pkg, name)}
+    data = [${data.map(p => `"${p}"`).join(', ')}],
+    templated_args = ["--nobazel_patch_module_resolver"],${additionalAttributes(pkg, name)}
 )
 `;
     }

diff --git a/internal/npm_install/index.js b/internal/npm_install/index.js
@@ -645,7 +645,8 @@ nodejs_binary(
     name = "${name}",
     entry_point = "//:node_modules/${pkg._dir}/${path}",
     install_source_map_support = False,
-    data = [${data.map(p => `"${p}"`).join(', ')}],${additionalAttributes(pkg, name)}
+    data = [${data.map(p => `"${p}"`).join(', ')}],
+    templated_args = ["--nobazel_patch_module_resolver"],${additionalAttributes(pkg, name)}
 )
 `;
         }

diff --git a/internal/npm_install/test/golden/@gregmagolan/test-a/bin/BUILD.bazel.golden b/internal/npm_install/test/golden/@gregmagolan/test-a/bin/BUILD.bazel.golden
@@ -6,4 +6,5 @@ nodejs_binary(
     entry_point = "//:node_modules/@gregmagolan/test-a/@bin/test.js",
     install_source_map_support = False,
     data = ["//@gregmagolan/test-a:test-a"],
+    templated_args = ["--nobazel_patch_module_resolver"],
 )
diff --git a/internal/npm_install/test/golden/jasmine/bin/BUILD.bazel.golden b/internal/npm_install/test/golden/jasmine/bin/BUILD.bazel.golden
@@ -6,4 +6,5 @@ nodejs_binary(
     entry_point = "//:node_modules/jasmine/bin/jasmine.js",
     install_source_map_support = False,
     data = ["//jasmine:jasmine"],
+    templated_args = ["--nobazel_patch_module_resolver"],
 )
diff --git a/internal/providers/node_runtime_deps_info.bzl b/internal/providers/node_runtime_deps_info.bzl
@@ -74,7 +74,6 @@ def run_node(ctx, inputs, arguments, executable, **kwargs):
 
     modules_manifest = write_node_modules_manifest(ctx, link_data)
     add_arg(arguments, "--bazel_node_modules_manifest=%s" % modules_manifest.path)
-    inputs.append(modules_manifest)
 
     # By using the run_node helper, you suggest that your program
     # doesn't implicitly use runfiles to require() things
@@ -98,7 +97,7 @@ def run_node(ctx, inputs, arguments, executable, **kwargs):
     env["BAZEL_NODE_MODULES_ROOT"] = _compute_node_modules_root(ctx)
 
     ctx.actions.run(
-        inputs = inputs + extra_inputs,
+        inputs = inputs + extra_inputs + [modules_manifest],
         arguments = arguments,
         executable = exec_exec,
         env = env,

diff --git a/packages/karma/src/karma.conf.js b/packages/karma/src/karma.conf.js
@@ -5,6 +5,7 @@ try {
   const path = require('path');
   const tmp = require('tmp');
   const child_process = require('child_process');
+  const runfiles = require(process.env['BAZEL_NODE_RUNFILES_HELPER']);
 
   const VERBOSE_LOGS = !!process.env['VERBOSE_LOGS'];
 
@@ -230,7 +231,7 @@ try {
       // In Windows, the runfile will probably not be symlinked. Se we need to
       // serve the real file through karma, and proxy calls to the expected file
       // location in the runfiles to the real file.
-      const resolvedFile = require.resolve(f);
+      const resolvedFile = runfiles.resolve(f);
       conf.files.push({pattern: resolvedFile, included: false});
       // Prefixing the proxy path with '/absolute' allows karma to load local
       // files. This doesn't see to be an official API.
@@ -249,14 +250,14 @@ try {
       let filePath = null;
       if (f.startsWith('NODE_MODULES/')) {
         try {
-          // attempt to resolve in @bazel/typescript nested node_modules first
-          filePath = require.resolve(f.replace(/^NODE_MODULES\//, '@bazel/karma/node_modules/'));
+          // attempt to resolve in nested node_modules first
+          filePath = runfiles.resolve(f.replace(/^NODE_MODULES\//, '@bazel/karma/node_modules/'));
         } catch (e) {
           // if that failed then attempt to resolve in root node_modules
-          filePath = require.resolve(f.replace(/^NODE_MODULES\//, ''));
+          filePath = runfiles.resolve(f.replace(/^NODE_MODULES\//, ''));
         }
       } else {
-        filePath = require.resolve(f);
+        filePath = runfiles.resolve(f);
       }
 
       conf.files.push(filePath);
@@ -308,7 +309,7 @@ try {
     overrideConfigValue(conf, 'browsers', []);
     overrideConfigValue(conf, 'customLaunchers', null);
 
-    const webTestMetadata = require(process.env['WEB_TEST_METADATA']);
+    const webTestMetadata = require(runfiles.resolve(process.env['WEB_TEST_METADATA']));
     log_verbose(`WEB_TEST_METADATA: ${JSON.stringify(webTestMetadata, null, 2)}`);
     if (webTestMetadata['environment'] === 'local') {
       // When a local chrome or firefox browser is chosen such as
@@ -327,7 +328,11 @@ try {
             process.env.CHROME_BIN =
                 extractWebArchive(extractExe, archiveFile, webTestNamedFiles['CHROMIUM']);
           } else {
-            process.env.CHROME_BIN = require.resolve(webTestNamedFiles['CHROMIUM']);
+            try {
+              process.env.CHROME_BIN = runfiles.resolve(webTestNamedFiles['CHROMIUM']);
+            } catch {
+              // Ignore; karma may still find Chrome another way
+            }
           }
           const browser = process.env['DISPLAY'] ? 'Chrome' : 'ChromeHeadless';
           if (!supportChromeSandboxing()) {
@@ -345,7 +350,11 @@ try {
             process.env.FIREFOX_BIN =
                 extractWebArchive(extractExe, archiveFile, webTestNamedFiles['FIREFOX']);
           } else {
-            process.env.FIREFOX_BIN = require.resolve(webTestNamedFiles['FIREFOX']);
+            try {
+              process.env.FIREFOX_BIN = runfiles.resolve(webTestNamedFiles['FIREFOX']);
+            } catch {
+              // Ignore; karma may still find Firefox another way
+            }
           }
           conf.browsers.push(process.env['DISPLAY'] ? 'Firefox' : 'FirefoxHeadless');
         }
@@ -403,7 +412,7 @@ try {
 
     // Import the user's base karma configuration if specified
     if (configPath) {
-      const baseConf = require(configPath);
+      const baseConf = require(runfiles.resolve(configPath));
       if (typeof baseConf !== 'function') {
         throw new Error(
             'Invalid base karma configuration. Expected config function to be exported.');

diff --git a/packages/karma/src/karma_web_test.bzl b/packages/karma/src/karma_web_test.bzl
@@ -110,6 +110,12 @@ def _write_amd_names_shim(ctx):
 def _filter_js(files):
     return [f for f in files if f.extension == "js" or f.extension == "mjs"]
 
+def _find_dep(ctx, suffix):
+    for d in ctx.files.deps:
+        if (d.path.endswith(suffix)):
+            return _to_manifest_path(ctx, d)
+    fail("couldn't find file %s in the deps" % suffix)
+
 # Generates the karma configuration file for the rule
 def _write_karma_config(ctx, files, amd_names_shim):
     configuration = ctx.actions.declare_file(
@@ -149,8 +155,8 @@ def _write_karma_config(ctx, files, amd_names_shim):
     # for a priority require of nested `@bazel/typescript/node_modules` before
     # looking in root node_modules.
     bootstrap_entries += [
-        "NODE_MODULES/requirejs/require.js",
-        "NODE_MODULES/karma-requirejs/lib/adapter.js",
+        "NODE_MODULES/%s" % _find_dep(ctx, "requirejs/require.js"),
+        "NODE_MODULES/%s" % _find_dep(ctx, "karma-requirejs/lib/adapter.js"),
         "/".join([ctx.workspace_name, amd_names_shim.short_path]),
     ]
 

diff --git a/packages/karma/test/karma_typescript/user_files.spec.js b/packages/karma/test/karma_typescript/user_files.spec.js
@@ -18,8 +18,8 @@ describe('karma_web_test_suite', () => {
       return l.trim().slice(1, -1);
     }).filter(l => !!l);
     expect(files).toEqual([
-      'NODE_MODULES/requirejs/require.js',
-      'NODE_MODULES/karma-requirejs/lib/adapter.js',
+      'NODE_MODULES/npm/node_modules/requirejs/require.js',
+      'NODE_MODULES/npm/node_modules/karma-requirejs/lib/adapter.js',
       'build_bazel_rules_nodejs/packages/karma/test/karma_typescript/_testing_wrapped_test.amd_names_shim.js',
     ]);
   });

diff --git a/packages/node-patches/src/subprocess.ts b/packages/node-patches/src/subprocess.ts
@@ -47,7 +47,12 @@ fi
   }
 
   // fix execPath so folks use the proxy node
-  process.argv[0] = process.execPath = path.join(nodeDir, 'node');
+  if (process.platform == 'win32') {
+    // FIXME: need to make an exe, or run in a shell so we can use .bat
+  } else {
+    process.argv[0] = process.execPath = path.join(nodeDir, 'node');
+  }
+
 
   // replace any instances of require script in execArgv with the absolute path to the script.
   // example: bazel-require-script.js

diff --git a/packages/terser/src/index.js b/packages/terser/src/index.js
@@ -72,7 +72,7 @@ function terserDirectory(input, output, residual, terserBinary) {
       ...directoryArgs(residual, inputFile, outputFile)
     ];
 
-    spawn(process.execPath, args)
+    spawn(process.execPath, [...process.execArgv, ...args])
         .then(
             (data) => {
               if (data.code) {

diff --git a/packages/terser/src/terser_minified.bzl b/packages/terser/src/terser_minified.bzl
@@ -14,6 +14,8 @@
 
 "Rule to run the terser binary under bazel"
 
+load("@build_bazel_rules_nodejs//:providers.bzl", "run_node")
+
 _DOC = """Run the terser minifier.
 
 Typical example:
@@ -166,10 +168,11 @@ def _terser(ctx):
     args.add_all(["--config-file", opts.path])
     args.add_all(ctx.attr.args)
 
-    ctx.actions.run(
+    run_node(
+        ctx,
         inputs = inputs,
         outputs = outputs,
-        executable = ctx.executable.terser_bin,
+        executable = "terser_bin",
         arguments = [args],
         env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]},
         progress_message = "Minifying JavaScript %s [terser]" % (outputs[0].short_path),