feat(builtin): allow patching require in bootstrap scripts

* export BAZEL_WORKSPACE, BAZEL_NODE_LINKER & BAZEL_NODE_PATCH_REQUIRE env variables from node launcher * linker now users BAZEL_WORKSPACE & BAZEL_TARGET so that its runfiles resolve support works with non-test targets that don't have TEST_WORKSPACE & TEST_TARGET envs * adds resolveWorkspaceRelative() & patchRequire() to linker runfiles resolve support. The former is useful if you want to resolve without specifying the workspace. The latter is useful for nodejs_binary bootstrap scripts that want to patch require via the linker (such as e2e/jasmine/jasmine_shared_env_bootstrap.js) * tests targets can safely use TEST_TARGET & TEST_WORKSPACE build: s
bazel-contrib · Jan 3, 2020 · 842dfb4 · 842dfb4
1 parent 8a931d8
commit 842dfb4
Show file tree

Hide file tree

Showing 11 changed files with 137 additions and 48 deletions.
diff --git a/e2e/jasmine/BUILD.bazel b/e2e/jasmine/BUILD.bazel
@@ -5,17 +5,11 @@ jasmine_node_test(
     srcs = ["test.spec.js"],
 )
 
-# This requires the linker as jasmine_shared_env_bootstrap.js requires @bazel/jasmine
-# which only works with the linker as the --require script doesn't get the require.resolve
-# patches.
 jasmine_node_test(
     name = "shared_env_test",
     srcs = ["jasmine_shared_env_test.spec.js"],
     data = ["jasmine_shared_env_bootstrap.js"],
-    # TODO(gregmagolan): fix Windows error: Error: Cannot find module 'e2e_jasmine/shared_env_test_devmode_srcs.MF'
-    tags = ["fix-windows"],
     templated_args = [
-        "--nobazel_patch_module_resolver",
         "--node_options=--require=$(rlocation $(location :jasmine_shared_env_bootstrap.js))",
     ],
     deps = [

diff --git a/e2e/jasmine/jasmine_shared_env_bootstrap.js b/e2e/jasmine/jasmine_shared_env_bootstrap.js
@@ -1,3 +1,9 @@
+// bootstrap the bazel require patch since this bootstrap script is loaded with
+// `--node_options=--require=$(rlocation $(location :jasmine_shared_env_bootstrap.js))`
+if (process.env['BAZEL_NODE_RUNFILES_HELPER']) {
+  require(process.env['BAZEL_NODE_RUNFILES_HELPER']).patchRequire();
+}
+
 global.foobar = 1;
 
 require('zone.js/dist/zone-node.js');

diff --git a/internal/golden_file_test/bin.js b/internal/golden_file_test/bin.js
@@ -29,7 +29,7 @@ ${prettyDiff}
 Update the golden file:
 
             bazel run ${debugBuild ? '--compilation_mode=dbg ' : ''}${
-          process.env['BAZEL_TARGET'].replace(/_bin$/, '')}.accept
+          process.env['TEST_TARGET'].replace(/_bin$/, '')}.accept
 `);
     } else {
       throw new Error('unknown mode', mode);

diff --git a/internal/linker/BUILD.bazel b/internal/linker/BUILD.bazel
@@ -20,7 +20,10 @@ bzl_library(
 )
 
 # END-INTERNAL
-exports_files(["index.js"])
+exports_files([
+    "index.js",
+    "runfiles_helper.js",
+])
 
 filegroup(
     name = "package_contents",

diff --git a/internal/linker/index.js b/internal/linker/index.js
@@ -165,16 +165,21 @@ Include as much of the build output as you can without disclosing anything confi
                  If you want to test runfiles manifest behavior, add
                  --spawn_strategy=standalone to the command line.`);
             }
-            const wksp = env['TEST_WORKSPACE'];
-            const target = env['TEST_TARGET'];
-            if (!!wksp && !!target) {
-                // //path/to:target -> //path/to
-                const pkg = target.split(':')[0];
-                this.packagePath = path.posix.join(wksp, pkg);
-            }
-            // If under runfiles (and not unit testing) then don't add a bin to taget for bin links
-            this.noBin = !!env['TEST_TARGET'] && wksp !== 'build_bazel_rules_nodejs' &&
-                target !== '//internal/linker/test:unit_tests';
+            const wksp = env['BAZEL_WORKSPACE'];
+            if (!!wksp) {
+                this.workspace = wksp;
+            }
+            // If target is from an external workspace such as @npm//rollup/bin:rollup
+            // resolvePackageRelative is not supported since package is in an external
+            // workspace.
+            const target = env['BAZEL_TARGET'];
+            if (!!target && !target.startsWith('@')) {
+                // //path/to:target -> path/to
+                this.package = target.split(':')[0].replace(/^\/\//, '');
+            }
+            // We can derive if the process is being run in the execroot
+            // if there is a bazel-out folder at the cwd.
+            this.execroot = existsSync('bazel-out');
         }
         lookupDirectory(dir) {
             if (!this.manifest)
@@ -225,11 +230,27 @@ Include as much of the build output as you can without disclosing anything confi
             }
             throw new Error(`could not resolve modulePath ${modulePath}`);
         }
+        resolveWorkspaceRelative(modulePath) {
+            if (!this.workspace) {
+                throw new Error('workspace could not be determined from the environment');
+            }
+            return this.resolve(path.posix.join(this.workspace, modulePath));
+        }
         resolvePackageRelative(modulePath) {
-            if (!this.packagePath) {
-                throw new Error('packagePath could not be determined from the environment');
+            if (!this.workspace) {
+                throw new Error('workspace could not be determined from the environment');
+            }
+            if (!this.package) {
+                throw new Error('package could not be determined from the environment');
+            }
+            return this.resolve(path.posix.join(this.workspace, this.package, modulePath));
+        }
+        patchRequire() {
+            const requirePatch = process.env['BAZEL_NODE_PATCH_REQUIRE'];
+            if (!requirePatch) {
+                throw new Error('require patch location could not be determined from the environment');
             }
-            return this.resolve(path.posix.join(this.packagePath, modulePath));
+            require(requirePatch);
         }
     }
     exports.Runfiles = Runfiles;
@@ -249,6 +270,18 @@ Include as much of the build output as you can without disclosing anything confi
             }
         });
     }
+    function existsSync(p) {
+        try {
+            fs.statSync(p);
+            return true;
+        }
+        catch (e) {
+            if (e.code === 'ENOENT') {
+                return false;
+            }
+            throw e;
+        }
+    }
     /**
      * Given a set of module aliases returns an array of recursive `LinkerTreeElement`.
      *
@@ -424,9 +457,11 @@ Include as much of the build output as you can without disclosing anything confi
                         let target = '<package linking failed>';
                         switch (root) {
                             case 'bin':
+                                // If we are in the execroot then add the bin path to the target; otherwise
+                                // we are in runfiles and the bin path should be omitted.
                                 // FIXME(#1196)
-                                target = runfiles.noBin ? path.join(workspaceAbs, toWorkspaceDir(modulePath)) :
-                                    path.join(workspaceAbs, bin, toWorkspaceDir(modulePath));
+                                target = runfiles.execroot ? path.join(workspaceAbs, bin, toWorkspaceDir(modulePath)) :
+                                    path.join(workspaceAbs, toWorkspaceDir(modulePath));
                                 break;
                             case 'src':
                                 target = path.join(workspaceAbs, toWorkspaceDir(modulePath));

diff --git a/internal/linker/link_node_modules.ts b/internal/linker/link_node_modules.ts
@@ -126,12 +126,15 @@ async function resolveRoot(root: string|undefined, runfiles: Runfiles) {
 export class Runfiles {
   manifest: Map<string, string>|undefined;
   dir: string|undefined;
-  noBin: boolean;
+  execroot: boolean;
   /**
-   * If the environment gives us enough hints, we can know the path to the package
-   * in the form workspace_name/path/to/package
+   * If the environment gives us enough hints, we can know the workspace name
    */
-  packagePath: string|undefined;
+  workspace: string|undefined;
+  /**
+   * If the environment gives us enough hints, we can know the package path
+   */
+  package: string|undefined;
 
   constructor(env: typeof process.env) {
     // If Bazel sets a variable pointing to a runfiles manifest,
@@ -159,18 +162,21 @@ export class Runfiles {
                  If you want to test runfiles manifest behavior, add
                  --spawn_strategy=standalone to the command line.`);
     }
-
-    const wksp = env['TEST_WORKSPACE'];
-    const target = env['TEST_TARGET'];
-    if (!!wksp && !!target) {
-      // //path/to:target -> //path/to
-      const pkg = target.split(':')[0];
-      this.packagePath = path.posix.join(wksp, pkg);
+    const wksp = env['BAZEL_WORKSPACE'];
+    if (!!wksp) {
+      this.workspace = wksp;
     }
-
-    // If under runfiles (and not unit testing) then don't add a bin to taget for bin links
-    this.noBin = !!env['TEST_TARGET'] && wksp !== 'build_bazel_rules_nodejs' &&
-        target !== '//internal/linker/test:unit_tests';
+    // If target is from an external workspace such as @npm//rollup/bin:rollup
+    // resolvePackageRelative is not supported since package is in an external
+    // workspace.
+    const target = env['BAZEL_TARGET'];
+    if (!!target && !target.startsWith('@')) {
+      // //path/to:target -> path/to
+      this.package = target.split(':')[0].replace(/^\/\//, '');
+    }
+    // We can derive if the process is being run in the execroot
+    // if there is a bazel-out folder at the cwd.
+    this.execroot = existsSync('bazel-out');
   }
 
   lookupDirectory(dir: string): string|undefined {
@@ -228,11 +234,29 @@ export class Runfiles {
     throw new Error(`could not resolve modulePath ${modulePath}`);
   }
 
+  resolveWorkspaceRelative(modulePath: string) {
+    if (!this.workspace) {
+      throw new Error('workspace could not be determined from the environment');
+    }
+    return this.resolve(path.posix.join(this.workspace, modulePath));
+  }
+
   resolvePackageRelative(modulePath: string) {
-    if (!this.packagePath) {
-      throw new Error('packagePath could not be determined from the environment');
+    if (!this.workspace) {
+      throw new Error('workspace could not be determined from the environment');
+    }
+    if (!this.package) {
+      throw new Error('package could not be determined from the environment');
+    }
+    return this.resolve(path.posix.join(this.workspace, this.package, modulePath));
+  }
+
+  patchRequire() {
+    const requirePatch = process.env['BAZEL_NODE_PATCH_REQUIRE'];
+    if (!requirePatch) {
+      throw new Error('require patch location could not be determined from the environment');
     }
-    return this.resolve(path.posix.join(this.packagePath, modulePath));
+    require(requirePatch);
   }
 }
 
@@ -257,6 +281,18 @@ async function exists(p: string) {
   }
 }
 
+function existsSync(p: string) {
+  try {
+    fs.statSync(p)
+    return true;
+  } catch (e) {
+    if (e.code === 'ENOENT') {
+      return false;
+    }
+    throw e;
+  }
+}
+
 /**
  * Given a set of module aliases returns an array of recursive `LinkerTreeElement`.
  *
@@ -491,9 +527,11 @@ export async function main(args: string[], runfiles: Runfiles) {
       let target: string = '<package linking failed>';
       switch (root) {
         case 'bin':
+          // If we are in the execroot then add the bin path to the target; otherwise
+          // we are in runfiles and the bin path should be omitted.
           // FIXME(#1196)
-          target = runfiles.noBin ? path.join(workspaceAbs, toWorkspaceDir(modulePath)) :
-                                    path.join(workspaceAbs, bin, toWorkspaceDir(modulePath));
+          target = runfiles.execroot ? path.join(workspaceAbs, bin, toWorkspaceDir(modulePath)) :
+                                       path.join(workspaceAbs, toWorkspaceDir(modulePath));
           break;
         case 'src':
           target = path.join(workspaceAbs, toWorkspaceDir(modulePath));

diff --git a/internal/linker/runfiles_helper.js b/internal/linker/runfiles_helper.js
@@ -0,0 +1 @@
+module.exports = require('./index.js').runfiles;
diff --git a/internal/node/node.bzl b/internal/node/node.bzl
@@ -167,9 +167,8 @@ def _nodejs_binary_impl(ctx):
 
     _write_loader_script(ctx)
 
-    script_path = _to_manifest_path(ctx, ctx.outputs.loader)
-
     env_vars = "export BAZEL_TARGET=%s\n" % ctx.label
+    env_vars += "export BAZEL_WORKSPACE=%s\n" % ctx.workspace_name
     for k in ctx.attr.configuration_env_vars + ctx.attr.default_env_vars:
         if k in ctx.var.keys():
             env_vars += "export %s=\"%s\"\n" % (k, ctx.var[k])
@@ -191,6 +190,7 @@ def _nodejs_binary_impl(ctx):
     node_tool_files.extend(ctx.toolchains["@build_bazel_rules_nodejs//toolchains/node:toolchain_type"].nodeinfo.tool_files)
 
     node_tool_files.append(ctx.file._link_modules_script)
+    node_tool_files.append(ctx.file._runfiles_helper_script)
     node_tool_files.append(ctx.file._bazel_require_script)
     node_tool_files.append(node_modules_manifest)
 
@@ -205,9 +205,10 @@ def _nodejs_binary_impl(ctx):
         "TEMPLATED_env_vars": env_vars,
         "TEMPLATED_expected_exit_code": str(expected_exit_code),
         "TEMPLATED_link_modules_script": _to_manifest_path(ctx, ctx.file._link_modules_script),
-        "TEMPLATED_loader_path": script_path,
+        "TEMPLATED_loader_path": _to_manifest_path(ctx, ctx.outputs.loader),
         "TEMPLATED_modules_manifest": _to_manifest_path(ctx, node_modules_manifest),
         "TEMPLATED_repository_args": _to_manifest_path(ctx, ctx.file._repository_args),
+        "TEMPLATED_runfiles_helper_script": _to_manifest_path(ctx, ctx.file._runfiles_helper_script),
         "TEMPLATED_script_path": _to_execroot_path(ctx, ctx.file.entry_point),
         "TEMPLATED_vendored_node": "" if is_builtin else strip_external(ctx.file._node.path),
     }
@@ -449,6 +450,10 @@ jasmine_node_test(
         default = Label("@nodejs//:bin/node_repo_args.sh"),
         allow_single_file = True,
     ),
+    "_runfiles_helper_script": attr.label(
+        default = Label("//internal/linker:runfiles_helper.js"),
+        allow_single_file = True,
+    ),
     "_source_map_support_files": attr.label_list(
         default = [
             Label("//third_party/github.com/buffer-from:contents"),

diff --git a/internal/node/node_launcher.sh b/internal/node/node_launcher.sh
@@ -158,6 +158,13 @@ else
   fi
 fi
 
+# Export the location of the runfiles helpers script
+export BAZEL_NODE_RUNFILES_HELPER=$(rlocation "TEMPLATED_runfiles_helper_script")
+
+# Export the location of the loader script as it can be used to boostrap
+# node require patch if needed
+export BAZEL_NODE_PATCH_REQUIRE=$(rlocation "TEMPLATED_loader_path")
+
 readonly repository_args=$(rlocation "TEMPLATED_repository_args")
 MAIN=$(rlocation "TEMPLATED_loader_path")
 readonly link_modules_script=$(rlocation "TEMPLATED_link_modules_script")

diff --git a/internal/npm_install/test/check.js b/internal/npm_install/test/check.js
@@ -40,7 +40,7 @@ ${prettyDiff}
 
 Update the golden file:
 
-      bazel run ${process.env['BAZEL_TARGET']}.accept`);
+      bazel run ${process.env['TEST_TARGET']}.accept`);
     }
   }
 }

diff --git a/packages/jasmine/src/jasmine_runner.js b/packages/jasmine/src/jasmine_runner.js
@@ -191,7 +191,7 @@ function main(args) {
     // Special case!
     // To allow us to test sharding, always run the specs in the order they are declared
     if (process.env['TEST_WORKSPACE'] === 'build_bazel_rules_nodejs' &&
-        process.env['BAZEL_TARGET'] === '//packages/jasmine/test:sharding_test') {
+        process.env['TEST_TARGET'] === '//packages/jasmine/test:sharding_test') {
       jrunner.randomizeTests(false);
     }
   }