refactor: always link

We now run the linker always, regardless of the configuration of the patch_module_resolver option. This creates some new flags to the launcher.sh - `--bazel_patch_module_resolver`/`--nobazel_patch_module_resolver` turns on/off the monkey-patches for require(). Still defaults to true (behavior unchanged) - `--nobazel_node_patches` disables the patches that make symlinks stay inside the execroot. Undocumented escape valve - `--nobazel_run_linker` disables running the linker, since it is now on by default. Undocumented escape valve As a later step we'll change default for patch_module_resolver to false (could be for 2.0 or 3.0 depending on how breaking it looks)
bazel-contrib · Jun 16, 2020 · 2e6bc72 · 2e6bc72
1 parent 45f9443
commit 2e6bc72
Show file tree

Hide file tree

Showing 12 changed files with 194 additions and 45 deletions.
diff --git a/e2e/BUILD.bazel b/e2e/BUILD.bazel
@@ -42,6 +42,8 @@ e2e_integration_test(
 
 e2e_integration_test(
     name = "e2e_packages",
+    # TODO(alex): figure out why this is broken by turning on linker
+    tags = ["no-bazelci-windows"],
 )
 
 e2e_integration_test(

diff --git a/e2e/node_loader_no_preserve_symlinks/BUILD.bazel b/e2e/node_loader_no_preserve_symlinks/BUILD.bazel
@@ -9,4 +9,5 @@ nodejs_test(
     ],
     entry_point = ":node_loader_test.spec.js",
     node_modules = "@npm//:node_modules",
+    templated_args = ["--nobazel_node_patches"],
 )
diff --git a/e2e/packages/npm1/package-lock.json b/e2e/packages/npm1/package-lock.json
diff --git a/e2e/packages/npm2/package-lock.json b/e2e/packages/npm2/package-lock.json
diff --git a/e2e/packages/yarn1/yarn.lock b/e2e/packages/yarn1/yarn.lock
@@ -36,6 +36,18 @@ glob@^7.0.6:
     once "^1.3.0"
     path-is-absolute "^1.0.0"
 
+glob@^7.1.3:
+  version "7.1.6"
+  resolved "https://registry.yarnpkg.com/glob/-/glob-7.1.6.tgz#141f33b81a7c2492e125594307480c46679278a6"
+  integrity sha512-LwaxwyZ72Lk7vZINtNNrywX0ZuLyStrdDtabefZKAY5ZGJhVtgdznluResxNmPitE0SAO+O26sWTHeKSI2wMBA==
+  dependencies:
+    fs.realpath "^1.0.0"
+    inflight "^1.0.4"
+    inherits "2"
+    minimatch "^3.0.4"
+    once "^1.3.0"
+    path-is-absolute "^1.0.0"
+
 inflight@^1.0.4:
   version "1.0.6"
   resolved "https://registry.yarnpkg.com/inflight/-/inflight-1.0.6.tgz#49bd6331d7d02d0c09bc910a1075ba8165b56df9"
@@ -79,6 +91,20 @@ path-is-absolute@^1.0.0:
   version "1.0.1"
   resolved "https://registry.yarnpkg.com/path-is-absolute/-/path-is-absolute-1.0.1.tgz#174b9268735534ffbc7ace6bf53a5a9e1b5c5f5f"
 
+rimraf@^2.6.3:
+  version "2.7.1"
+  resolved "https://registry.yarnpkg.com/rimraf/-/rimraf-2.7.1.tgz#35797f13a7fdadc566142c29d4f07ccad483e3ec"
+  integrity sha512-uWjbaKIK3T1OSVptzX7Nl6PvQ3qAGtKEtVRjRuazjfL3Bx5eI409VZSqgND+4UNnmzLVdPj9FqFJNPqBZFve4w==
+  dependencies:
+    glob "^7.1.3"
+
+tmp@0.1.0:
+  version "0.1.0"
+  resolved "https://registry.yarnpkg.com/tmp/-/tmp-0.1.0.tgz#ee434a4e22543082e294ba6201dcc6eafefa2877"
+  integrity sha512-J7Z2K08jbGcdA1kkQpJSqLF6T0tdQqpR2pnSUXsIchbPdTI9v3e85cLW0d6WDhwuAleOV71j2xWs8qMPfK7nKw==
+  dependencies:
+    rimraf "^2.6.3"
+
 wrappy@1:
   version "1.0.2"
   resolved "https://registry.yarnpkg.com/wrappy/-/wrappy-1.0.2.tgz#b5243d8f3ec1aa35f1364605bc0d1036e30ab69f"
diff --git a/e2e/packages/yarn2/yarn.lock b/e2e/packages/yarn2/yarn.lock
@@ -36,6 +36,18 @@ glob@^7.0.6:
     once "^1.3.0"
     path-is-absolute "^1.0.0"
 
+glob@^7.1.3:
+  version "7.1.6"
+  resolved "https://registry.yarnpkg.com/glob/-/glob-7.1.6.tgz#141f33b81a7c2492e125594307480c46679278a6"
+  integrity sha512-LwaxwyZ72Lk7vZINtNNrywX0ZuLyStrdDtabefZKAY5ZGJhVtgdznluResxNmPitE0SAO+O26sWTHeKSI2wMBA==
+  dependencies:
+    fs.realpath "^1.0.0"
+    inflight "^1.0.4"
+    inherits "2"
+    minimatch "^3.0.4"
+    once "^1.3.0"
+    path-is-absolute "^1.0.0"
+
 inflight@^1.0.4:
   version "1.0.6"
   resolved "https://registry.yarnpkg.com/inflight/-/inflight-1.0.6.tgz#49bd6331d7d02d0c09bc910a1075ba8165b56df9"
@@ -79,6 +91,20 @@ path-is-absolute@^1.0.0:
   version "1.0.1"
   resolved "https://registry.yarnpkg.com/path-is-absolute/-/path-is-absolute-1.0.1.tgz#174b9268735534ffbc7ace6bf53a5a9e1b5c5f5f"
 
+rimraf@^2.6.3:
+  version "2.7.1"
+  resolved "https://registry.yarnpkg.com/rimraf/-/rimraf-2.7.1.tgz#35797f13a7fdadc566142c29d4f07ccad483e3ec"
+  integrity sha512-uWjbaKIK3T1OSVptzX7Nl6PvQ3qAGtKEtVRjRuazjfL3Bx5eI409VZSqgND+4UNnmzLVdPj9FqFJNPqBZFve4w==
+  dependencies:
+    glob "^7.1.3"
+
+tmp@0.1.0:
+  version "0.1.0"
+  resolved "https://registry.yarnpkg.com/tmp/-/tmp-0.1.0.tgz#ee434a4e22543082e294ba6201dcc6eafefa2877"
+  integrity sha512-J7Z2K08jbGcdA1kkQpJSqLF6T0tdQqpR2pnSUXsIchbPdTI9v3e85cLW0d6WDhwuAleOV71j2xWs8qMPfK7nKw==
+  dependencies:
+    rimraf "^2.6.3"
+
 wrappy@1:
   version "1.0.2"
   resolved "https://registry.yarnpkg.com/wrappy/-/wrappy-1.0.2.tgz#b5243d8f3ec1aa35f1364605bc0d1036e30ab69f"
diff --git a/examples/angular/tools/angular_prerender.bzl b/examples/angular/tools/angular_prerender.bzl
@@ -46,6 +46,7 @@ def ng_prerender(name, index, prerender_roots = [], **kwargs):
             "@npm//reflect-metadata",
         ],
         entry_point = "//src:prerender.ts",
+        templated_args = ["--nobazel_run_linker"],
     )
 
     root_at = "_prerender/" + native.package_name()

diff --git a/internal/linker/index.js b/internal/linker/index.js
@@ -74,7 +74,7 @@ function resolveRoot(root, startCwd, isExecroot, runfiles) {
         if (isExecroot) {
             return root ? `${startCwd}/external/${root}` : `${startCwd}/node_modules`;
         }
-        const match = startCwd.match(/\/bazel-out\//);
+        const match = startCwd.match(/(\/bazel-out\/|\/bazel-~1\/x64_wi~1\/)/);
         if (!match) {
             panic(`No 'bazel-out' folder found in path '${startCwd}'!`);
             return '';

diff --git a/internal/linker/link_node_modules.ts b/internal/linker/link_node_modules.ts
@@ -121,7 +121,9 @@ async function resolveRoot(
   // runfiles on rbe, bazel runs the process in a directory such as
   // `/b/f/w/bazel-out/k8-fastbuild/bin/path/to/pkg/some_test.sh.runfiles/my_wksp`. From here we can
   // determine the execroot `b/f/w` by finding the first instance of bazel-out.
-  const match = startCwd.match(/\/bazel-out\//);
+  // NB: on windows thanks to legacy 8-character path segments it might be like
+  // c:/b/ojvxx6nx/execroot/build_~1/bazel-~1/x64_wi~1/bin/internal/npm_in~1/test
+  const match = startCwd.match(/(\/bazel-out\/|\/bazel-~1\/x64_wi~1\/)/);
   if (!match) {
     panic(`No 'bazel-out' folder found in path '${startCwd}'!`);
     return '';

diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh
@@ -152,79 +152,100 @@ if [[ "${BAZEL_NODE_RUNFILES_HELPER}" != /* ]] && [[ ! "${BAZEL_NODE_RUNFILES_HE
   export BAZEL_NODE_RUNFILES_HELPER=$(pwd)/${BAZEL_NODE_RUNFILES_HELPER}
 fi
 
-# Export the location of the require patch script as it can be used to boostrap
+# Export the location of the require patch script as it can be used to bootstrap
 # node require patch if needed
 export BAZEL_NODE_PATCH_REQUIRE=$(rlocation "TEMPLATED_require_patch_script")
 if [[ "${BAZEL_NODE_PATCH_REQUIRE}" != /* ]] && [[ ! "${BAZEL_NODE_PATCH_REQUIRE}" =~ ^[A-Z]:[\\/] ]]; then
   export BAZEL_NODE_PATCH_REQUIRE=$(pwd)/${BAZEL_NODE_PATCH_REQUIRE}
 fi
 
-# The main entry point
-MAIN=$(rlocation "TEMPLATED_loader_script")
-
 readonly repository_args=$(rlocation "TEMPLATED_repository_args")
-readonly link_modules_script=$(rlocation "TEMPLATED_link_modules_script")
 readonly lcov_merger_script=$(rlocation "TEMPLATED_lcov_merger_script")
-node_patches_script=$(rlocation "TEMPLATED_node_patches_script")
-require_patch_script=${BAZEL_NODE_PATCH_REQUIRE}
-
-# Node's --require option assumes that a non-absolute path not starting with `.` is
-# a module, so that you can do --require=source-map-support/register
-# So if the require script is not absolute, we must make it so
-case "${node_patches_script}" in
-  # Absolute path on unix
-  /*          ) ;;
-  # Absolute path on Windows, e.g. C:/path/to/thing
-  [a-zA-Z]:/* ) ;;
-  # Otherwise it needs to be made relative
-  *           ) node_patches_script="./${node_patches_script}" ;;
-esac
-case "${require_patch_script}" in
-  # Absolute path on unix
-  /*          ) ;;
-  # Absolute path on Windows, e.g. C:/path/to/thing
-  [a-zA-Z]:/* ) ;;
-  # Otherwise it needs to be made relative
-  *           ) require_patch_script="./${require_patch_script}" ;;
-esac
 
 source $repository_args
 
 ARGS=()
-LAUNCHER_NODE_OPTIONS=( "--require" "$require_patch_script" )
+LAUNCHER_NODE_OPTIONS=()
 USER_NODE_OPTIONS=()
 ALL_ARGS=(TEMPLATED_args $NODE_REPOSITORY_ARGS "$@")
 STDOUT_CAPTURE=""
 STDERR_CAPTURE=""
 
+RUN_LINKER=true
+NODE_PATCHES=true
+# TODO(alex): change the default to false
+PATCH_REQUIRE=true
 for ARG in "${ALL_ARGS[@]:-}"; do
   case "$ARG" in
+    # Supply custom linker arguments for first-party dependencies
     --bazel_node_modules_manifest=*) MODULES_MANIFEST="${ARG#--bazel_node_modules_manifest=}" ;;
     --bazel_capture_stdout=*) STDOUT_CAPTURE="${ARG#--bazel_capture_stdout=}" ;;
     --bazel_capture_stderr=*) STDERR_CAPTURE="${ARG#--bazel_capture_stderr=}" ;;
-    --nobazel_patch_module_resolver)
-      MAIN=TEMPLATED_entry_point_execroot_path
-      LAUNCHER_NODE_OPTIONS=( "--require" "$node_patches_script" )
-
-      # In this case we should always run the linker
-      # For programs which are called with bazel run or bazel test, there will be no additional runtime
-      # dependencies to link, so we use the default modules_manifest which has only the static dependencies
-      # of the binary itself
-      MODULES_MANIFEST=${MODULES_MANIFEST:-$(rlocation "TEMPLATED_modules_manifest")}
-      ;;
+    # Disable the node_loader.js monkey patches for require()
+    # Note that this means you need an explicit runfiles helper library
+    --nobazel_patch_module_resolver) PATCH_REQUIRE=false ;;
+    # Enable the node_loader.js monkey patches for require()
+    # Currently a no-op, but specifying this makes the behavior unchanged when we update
+    # the default for PATCH_REQUIRE above
+    --bazel_patch_module_resolver) PATCH_REQUIRE=true ;;
+    # Disable the --require node-patches (undocumented and unused; only here as an escape value)
+    --nobazel_node_patches) NODE_PATCHES=false ;;
+    # Disable the linker pre-process (undocumented and unused; only here as an escape value)
+    --nobazel_run_linker) RUN_LINKER=false ;;
+    # Let users pass through arguments to node itself
     --node_options=*) USER_NODE_OPTIONS+=( "${ARG#--node_options=}" ) ;;
+    # Remaining argv is collected to pass to the program
     *) ARGS+=( "$ARG" )
   esac
 done
 
 # Link the first-party modules into node_modules directory before running the actual program
-if [[ -n "${MODULES_MANIFEST:-}" ]]; then
+if [ "$RUN_LINKER" = true ]; then
+  link_modules_script=$(rlocation "TEMPLATED_link_modules_script")
+  # For programs which are called with bazel run or bazel test, there will be no additional runtime
+  # dependencies to link, so we use the default modules_manifest which has only the static dependencies
+  # of the binary itself.
+  MODULES_MANIFEST=${MODULES_MANIFEST:-$(rlocation "TEMPLATED_modules_manifest")}
   "${node}" "${link_modules_script}" "${MODULES_MANIFEST:-}"
 fi
 
-# TODO: after we link-all-bins we should not need this extra lookup
-if [[ ! -f "$MAIN" ]]; then
-  MAIN=TEMPLATED_entry_point_manifest_path
+if [ "$NODE_PATCHES" = true ]; then
+  node_patches_script=$(rlocation "TEMPLATED_node_patches_script")
+  # Node's --require option assumes that a non-absolute path not starting with `.` is
+  # a module, so that you can do --require=source-map-support/register
+  # So if the require script is not absolute, we must make it so
+  case "${node_patches_script}" in
+    # Absolute path on unix
+    /*          ) ;;
+    # Absolute path on Windows, e.g. C:/path/to/thing
+    [a-zA-Z]:/* ) ;;
+    # Otherwise it needs to be made relative
+    *           ) node_patches_script="./${node_patches_script}" ;;
+  esac
+  LAUNCHER_NODE_OPTIONS+=( "--require" "$node_patches_script" )
+fi
+
+if [ "$PATCH_REQUIRE" = true ]; then
+  require_patch_script=${BAZEL_NODE_PATCH_REQUIRE}
+  # See comment above
+  case "${require_patch_script}" in
+    # Absolute path on unix
+    /*          ) ;;
+    # Absolute path on Windows, e.g. C:/path/to/thing
+    [a-zA-Z]:/* ) ;;
+    # Otherwise it needs to be made relative
+    *           ) require_patch_script="./${require_patch_script}" ;;
+  esac
+  LAUNCHER_NODE_OPTIONS+=( "--require" "$require_patch_script" )
+  # Change the entry point to be the loader.js script so we run code before node
+  MAIN=$(rlocation "TEMPLATED_loader_script")
+else
+  # Entry point is the user-supplied script
+  MAIN=TEMPLATED_entry_point_execroot_path  
+  # TODO: after we link-all-bins we should not need this extra lookup
+  if [[ ! -f "$MAIN" ]]; then
+    MAIN=TEMPLATED_entry_point_manifest_path
+  fi
 fi
 
 # Tell the node_patches_script that programs should not escape the execroot

diff --git a/packages/labs/protobufjs/BUILD.bazel b/packages/labs/protobufjs/BUILD.bazel
@@ -44,6 +44,7 @@ nodejs_binary(
         "@build_bazel_rules_typescript_protobufs_compiletime_deps//estraverse",
     ],
     entry_point = "@build_bazel_rules_typescript_protobufs_compiletime_deps//:node_modules/protobufjs/bin/pbjs",
+    templated_args = ["--nobazel_run_linker"],
 )
 
 nodejs_binary(
@@ -66,6 +67,7 @@ nodejs_binary(
         "@build_bazel_rules_typescript_protobufs_compiletime_deps//estraverse",
     ],
     entry_point = "@build_bazel_rules_typescript_protobufs_compiletime_deps//:node_modules/protobufjs/bin/pbts",
+    templated_args = ["--nobazel_run_linker"],
 )
 
 # Runtime libraries needed by the protobufjs library.

diff --git a/packages/node-patches/BUILD.bazel b/packages/node-patches/BUILD.bazel
@@ -60,7 +60,7 @@ nodejs_test(
     ],
     entry_point = "@npm_node_patches//:node_modules/mocha/bin/mocha",
     tags = ["fix-windows"],
-    templated_args = ["$$(rlocation $(rootpath %s))" % s for s in test_js],
+    templated_args = ["$$(rlocation $(rootpath %s))" % s for s in test_js] + ["--nobazel_node_patches"],
 )
 
 rollup_bundle(