refactor(builtin): cleanup and optimizations in nodejs_binary & linker (

#1799)
bazel-contrib · Apr 7, 2020 · 1667d50 · 1667d50
1 parent 830f946
commit 1667d50
Show file tree

Hide file tree

Showing 5 changed files with 35 additions and 51 deletions.
diff --git a/internal/linker/BUILD.bazel b/internal/linker/BUILD.bazel
@@ -4,7 +4,9 @@ load("@npm_bazel_typescript//:index.from_src.bzl", "checked_in_ts_library")
 
 # We can't bootstrap the ts_library rule using the linker itself,
 # because the implementation of ts_library depends on the linker so that would be a cycle.
-# So we compile it to JS and check in the result as index.js
+# So we compile it to JS and check in the result as index.js.
+# To update index.js run:
+#   bazel run //internal/linker:linker_lib_check_compiled.accept
 checked_in_ts_library(
     name = "linker_lib",
     srcs = ["link_node_modules.ts"],

diff --git a/internal/linker/index.js b/internal/linker/index.js
@@ -172,10 +172,9 @@ 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['BAZEL_WORKSPACE'];
-            if (!!wksp) {
-                this.workspace = wksp;
-            }
+            // Bazel starts actions with pwd=execroot/my_wksp
+            this.workspaceDir = path.resolve('.');
+            this.workspace = path.basename(this.workspaceDir);
             // If target is from an external workspace such as @npm//rollup/bin:rollup
             // resolvePackageRelative is not supported since package is in an external
             // workspace.
@@ -238,17 +237,11 @@ 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.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');
+                throw new Error('package could not be determined from the environment; make sure BAZEL_TARGET is set');
             }
             return this.resolve(path.posix.join(this.workspace, this.package, modulePath));
         }
@@ -432,17 +425,15 @@ Include as much of the build output as you can without disclosing anything confi
             modules = modules || {};
             log_verbose('manifest file', modulesManifest);
             log_verbose('manifest contents', JSON.stringify({ workspace, bin, root, modules }, null, 2));
-            // Bazel starts actions with pwd=execroot/my_wksp
-            const workspaceDir = path.resolve('.');
-            // resolveRoot will change the cwd when under runfiles
+            // NB: resolveRoot will change the cwd when under runfiles to the runfiles root
             const rootDir = yield resolveRoot(root, runfiles);
-            log_verbose('resolved root', root, 'to', rootDir);
+            log_verbose('resolved node_modules root', root, 'to', rootDir);
             log_verbose('cwd', process.cwd());
-            // Create the $pwd/node_modules directory that node will resolve from
+            // Create the node_modules symlink to the node_modules root that node will resolve from
             yield symlink(rootDir, 'node_modules');
+            // Change directory to the node_modules root directory so that all subsequent
+            // symlinks will be created under node_modules
             process.chdir(rootDir);
-            // Symlinks to packages need to reach back to the workspace/runfiles directory
-            const workspaceAbs = path.resolve(workspaceDir);
             function linkModules(m) {
                 return __awaiter(this, void 0, void 0, function* () {
                     // ensure the parent directory exist
@@ -454,7 +445,7 @@ Include as much of the build output as you can without disclosing anything confi
                         switch (root) {
                             case 'execroot':
                                 if (runfiles.execroot) {
-                                    target = path.posix.join(workspaceAbs, modulePath);
+                                    target = path.posix.join(runfiles.workspaceDir, modulePath);
                                 }
                                 else {
                                     // If under runfiles, convert from execroot path to runfiles path.
@@ -470,7 +461,7 @@ Include as much of the build output as you can without disclosing anything confi
                                     if (runfilesPath.startsWith(externalPrefix)) {
                                         runfilesPath = `../${runfilesPath.slice(externalPrefix.length)}`;
                                     }
-                                    target = path.posix.join(workspaceAbs, runfilesPath);
+                                    target = path.posix.join(runfiles.workspaceDir, runfilesPath);
                                 }
                                 break;
                             case 'runfiles':

diff --git a/internal/linker/link_node_modules.ts b/internal/linker/link_node_modules.ts
@@ -131,10 +131,8 @@ export class Runfiles {
   manifest: Map<string, string>|undefined;
   dir: string|undefined;
   execroot: boolean;
-  /**
-   * If the environment gives us enough hints, we can know the workspace name
-   */
-  workspace: string|undefined;
+  workspace: string;
+  workspaceDir: string;
   /**
    * If the environment gives us enough hints, we can know the package path
    */
@@ -166,10 +164,9 @@ export class Runfiles {
                  If you want to test runfiles manifest behavior, add
                  --spawn_strategy=standalone to the command line.`);
     }
-    const wksp = env['BAZEL_WORKSPACE'];
-    if (!!wksp) {
-      this.workspace = wksp;
-    }
+    // Bazel starts actions with pwd=execroot/my_wksp
+    this.workspaceDir = path.resolve('.');
+    this.workspace = path.basename(this.workspaceDir);
     // If target is from an external workspace such as @npm//rollup/bin:rollup
     // resolvePackageRelative is not supported since package is in an external
     // workspace.
@@ -239,18 +236,13 @@ export class Runfiles {
   }
 
   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.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');
+      throw new Error(
+          'package could not be determined from the environment; make sure BAZEL_TARGET is set');
     }
     return this.resolve(path.posix.join(this.workspace, this.package, modulePath));
   }
@@ -493,20 +485,17 @@ export async function main(args: string[], runfiles: Runfiles) {
   log_verbose('manifest file', modulesManifest);
   log_verbose('manifest contents', JSON.stringify({workspace, bin, root, modules}, null, 2));
 
-  // Bazel starts actions with pwd=execroot/my_wksp
-  const workspaceDir = path.resolve('.');
-
-  // resolveRoot will change the cwd when under runfiles
+  // NB: resolveRoot will change the cwd when under runfiles to the runfiles root
   const rootDir = await resolveRoot(root, runfiles);
-  log_verbose('resolved root', root, 'to', rootDir);
+  log_verbose('resolved node_modules root', root, 'to', rootDir);
   log_verbose('cwd', process.cwd());
 
-  // Create the $pwd/node_modules directory that node will resolve from
+  // Create the node_modules symlink to the node_modules root that node will resolve from
   await symlink(rootDir, 'node_modules');
-  process.chdir(rootDir);
 
-  // Symlinks to packages need to reach back to the workspace/runfiles directory
-  const workspaceAbs = path.resolve(workspaceDir);
+  // Change directory to the node_modules root directory so that all subsequent
+  // symlinks will be created under node_modules
+  process.chdir(rootDir);
 
   async function linkModules(m: LinkerTreeElement) {
     // ensure the parent directory exist
@@ -519,7 +508,7 @@ export async function main(args: string[], runfiles: Runfiles) {
       switch (root) {
         case 'execroot':
           if (runfiles.execroot) {
-            target = path.posix.join(workspaceAbs, modulePath);
+            target = path.posix.join(runfiles.workspaceDir, modulePath);
           } else {
             // If under runfiles, convert from execroot path to runfiles path.
             // First strip the bin portion if it exists:
@@ -533,7 +522,7 @@ export async function main(args: string[], runfiles: Runfiles) {
             if (runfilesPath.startsWith(externalPrefix)) {
               runfilesPath = `../${runfilesPath.slice(externalPrefix.length)}`;
             }
-            target = path.posix.join(workspaceAbs, runfilesPath);
+            target = path.posix.join(runfiles.workspaceDir, runfilesPath);
           }
           break;
         case 'runfiles':

diff --git a/internal/node/node.bzl b/internal/node/node.bzl
@@ -179,7 +179,6 @@ def _nodejs_binary_impl(ctx):
     _write_loader_script(ctx)
 
     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:
         # Check ctx.var first & if env var not in there then check
         # ctx.configuration.default_shell_env. The former will contain values from --define=FOO=BAR

diff --git a/third_party/github.com/bazelbuild/bazel/tools/bash/runfiles/runfiles.bash b/third_party/github.com/bazelbuild/bazel/tools/bash/runfiles/runfiles.bash
@@ -135,9 +135,12 @@ function rlocation() {
     return 1
   else
     # --- begin rules_nodejs custom code ---
-    # Normalize ${BAZEL_WORKSPACE}/$1.
+    # Bazel always sets the PWD to execroot/my_wksp so we derive the workspace
+    # from the PWD.
+    export bazel_workspace=$(basename $PWD)
+    # Normalize ${bazel_workspace}/$1.
     # If $1 is a $(rootpath) this will convert it to the runfiles manifest path
-    readonly from_rootpath=$(normpath ${BAZEL_WORKSPACE:-/dev/null}/$1)
+    readonly from_rootpath=$(normpath ${bazel_workspace}/$1)
     # --- end rules_nodejs custom code ---
     if [[ -e "${RUNFILES_DIR:-/dev/null}/$1" ]]; then
       if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then
@@ -148,7 +151,7 @@ function rlocation() {
     # If $1 is a rootpath then check if the converted rootpath to runfiles manifest path file is found
     elif [[ -e "${RUNFILES_DIR:-/dev/null}/${from_rootpath}" ]]; then
       if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then
-        echo >&2 "INFO[runfiles.bash]: rlocation($1): found under RUNFILES_DIR/BAZEL_WORKSPACE ($RUNFILES_DIR/$BAZEL_WORKSPACE), return"
+        echo >&2 "INFO[runfiles.bash]: rlocation($1): found under RUNFILES_DIR/WKSP ($RUNFILES_DIR/$bazel_workspace), return"
       fi
       echo "${RUNFILES_DIR}/${from_rootpath}"
     # --- end rules_nodejs custom code ---