fix: separate nodejs require patches from loader and —require them first

examples/BUILD.bazel

@@ -64,7 +64,7 @@ example_integration_test(
     name = "examples_react_webpack",
     # TODO: add some tests in the example
     bazel_commands = ["build ..."],
-    # TODO(alexeagle): somehow this is broken by the new node-patches based bazel_require_script
+    # TODO(alexeagle): somehow this is broken by the new node-patches based node_patches script
     # ERROR: D:/temp/tmp-6900sejcsrcttpdb/BUILD.bazel:37:1: output 'app.bundle.js' was not created
     tags = ["no-bazelci-windows"],
 )

internal/linker/link_node_modules.bzl

@@ -69,7 +69,7 @@ def write_node_modules_manifest(ctx, extra_data = []):
             mappings[k] = v
 
     # Write the result to a file, and use the magic node option --bazel_node_modules_manifest
-    # The node_launcher.sh will peel off this argument and pass it to the linker rather than the program.
+    # The launcher.sh will peel off this argument and pass it to the linker rather than the program.
     modules_manifest = ctx.actions.declare_file("_%s.module_mappings.json" % ctx.label.name)
     content = {
         "bin": ctx.bin_dir.path,

internal/linker/test/integration/run_program.sh

@@ -13,8 +13,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-# This shell script is a minimal fixture for the node_launcher.sh script
-# with a critical difference: instead of calling the node_loader.js script
+# This shell script is a minimal fixture for the launcher.sh script
+# with a critical difference: instead of calling the loader.js script
 # with the users program passed as an argument (allowing us to patch the node
 # loader), this one just runs vanilla node with the users program as the argument
 # which lets us assert that the linker is the reason the program works.

internal/node/BUILD.bazel

@@ -30,8 +30,8 @@ bzl_library(
 exports_files([
     "node.bzl",  # Exported to be consumed for generating stardoc.
     "node_repositories.bzl",  # Exported to be consumed for generating stardoc.
-    "node_launcher.sh",
-    "node_loader.js",
+    "launcher.sh",
+    "loader.js",
 ])
 
 filegroup(
@@ -49,5 +49,5 @@ filegroup(
 golden_file_test(
     name = "checked_in_node_patches",
     actual = "//packages/node-patches:bundle",
-    golden = "bazel_require_script.js",
+    golden = "node_patches.js",
 )

internal/node/node_launcher.sh → internal/node/launcher.sh

@@ -143,46 +143,58 @@ 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
+# Export the location of the require patch script as it can be used to boostrap
 # node require patch if needed
-export BAZEL_NODE_PATCH_REQUIRE=$(rlocation "TEMPLATED_loader_path")
+export BAZEL_NODE_PATCH_REQUIRE=$(rlocation "TEMPLATED_require_patch_script")
+
+# The main entry point
+MAIN=$(rlocation "TEMPLATED_loader_script")
 
 readonly repository_args=$(rlocation "TEMPLATED_repository_args")
-MAIN=$(rlocation "TEMPLATED_loader_path")
 readonly link_modules_script=$(rlocation "TEMPLATED_link_modules_script")
-bazel_require_script=$(rlocation "TEMPLATED_bazel_require_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 "${bazel_require_script}" in
+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
-  *           ) bazel_require_script="./${bazel_require_script}" ;;
+  *           ) require_patch_script="./${require_patch_script}" ;;
 esac
 
 source $repository_args
 
 ARGS=()
-NODE_OPTIONS=()
+LAUNCHER_NODE_OPTIONS=( "--require" "$require_patch_script" )
+USER_NODE_OPTIONS=()
 ALL_ARGS=(TEMPLATED_args $NODE_REPOSITORY_ARGS "$@")
 for ARG in "${ALL_ARGS[@]:-}"; do
   case "$ARG" in
     --bazel_node_modules_manifest=*) MODULES_MANIFEST="${ARG#--bazel_node_modules_manifest=}" ;;
     --nobazel_patch_module_resolver)
       MAIN="TEMPLATED_script_path"
-      NODE_OPTIONS+=( "--require" "$bazel_require_script" )
+      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")}
       ;;
-    --node_options=*) NODE_OPTIONS+=( "${ARG#--node_options=}" ) ;;
+    --node_options=*) USER_NODE_OPTIONS+=( "${ARG#--node_options=}" ) ;;
     *) ARGS+=( "$ARG" )
   esac
 done
@@ -192,7 +204,7 @@ if [[ -n "${MODULES_MANIFEST:-}" ]]; then
   "${node}" "${link_modules_script}" "${MODULES_MANIFEST:-}"
 fi
 
-# Tell the bazel_require_script that programs should not escape the execroot
+# Tell the node_patches_script that programs should not escape the execroot
 # Bazel always sets the PWD to execroot/my_wksp so we go up one directory.
 export BAZEL_PATCH_ROOT=$(dirname $PWD)
 
@@ -206,12 +218,12 @@ if [ "${EXPECTED_EXIT_CODE}" -eq "0" ]; then
   # handled by the node process.
   # If we had merely forked a child process here, we'd be responsible
   # for forwarding those OS interactions.
-  exec "${node}" "${NODE_OPTIONS[@]:-}" "${MAIN}" "${ARGS[@]:-}"
+  exec "${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}" "${ARGS[@]:-}"
   # exec terminates execution of this shell script, nothing later will run.
 fi
 
 set +e
-"${node}" "${NODE_OPTIONS[@]:-}" "${MAIN}" "${ARGS[@]:-}"
+"${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}" "${ARGS[@]:-}"
 RESULT="$?"
 set -e
 

internal/node/loader.js

@@ -0,0 +1,37 @@
+/**
+ * @license
+ * Copyright 2017 The Bazel Authors. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ *
+ * You may obtain a copy of the License at
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+/**
+ * @fileoverview NodeJS module loader for bazel.
+ */
+'use strict';
+
+// Ensure that node is added to the path for any subprocess calls
+process.env.PATH = [require('path').dirname(process.execPath), process.env.PATH].join(
+    /^win/i.test(process.platform) ? ';' : ':');
+
+if (require.main === module) {
+  // Set the actual entry point in the arguments list.
+  // argv[0] == node, argv[1] == entry point.
+  // NB: 'TEMPLATED_entry_point' below is replaced during the build process.
+  var mainScript = process.argv[1] = 'TEMPLATED_entry_point';
+  try {
+    module.constructor._load(mainScript, this, /*isMain=*/true);
+  } catch (e) {
+    console.error(e.stack || e);
+    process.exit(1);
+  }
+}

internal/node/node.bzl

@@ -78,7 +78,7 @@ def _compute_node_modules_root(ctx):
         ] if f])
     return node_modules_root
 
-def _write_loader_script(ctx):
+def _write_require_patch_script(ctx):
     # Generates the JavaScript snippet of module roots mappings, with each entry
     # in the form:
     #   {module_name: /^mod_name\b/, module_root: 'path/to/mod_name'}
@@ -92,6 +92,22 @@ def _write_loader_script(ctx):
 
     node_modules_root = _compute_node_modules_root(ctx)
 
+    ctx.actions.expand_template(
+        template = ctx.file._require_patch_template,
+        output = ctx.outputs.require_patch_script,
+        substitutions = {
+            "TEMPLATED_bin_dir": ctx.bin_dir.path,
+            "TEMPLATED_gen_dir": ctx.genfiles_dir.path,
+            "TEMPLATED_install_source_map_support": str(ctx.attr.install_source_map_support).lower(),
+            "TEMPLATED_module_roots": "\n  " + ",\n  ".join(module_mappings),
+            "TEMPLATED_node_modules_root": node_modules_root,
+            "TEMPLATED_target": str(ctx.label),
+            "TEMPLATED_user_workspace_name": ctx.workspace_name,
+        },
+        is_executable = True,
+    )
+
+def _write_loader_script(ctx):
     if len(ctx.attr.entry_point.files.to_list()) != 1:
         fail("labels in entry_point must contain exactly one file")
 
@@ -106,17 +122,8 @@ def _write_loader_script(ctx):
 
     ctx.actions.expand_template(
         template = ctx.file._loader_template,
-        output = ctx.outputs.loader,
-        substitutions = {
-            "TEMPLATED_bin_dir": ctx.bin_dir.path,
-            "TEMPLATED_entry_point": entry_point_path,
-            "TEMPLATED_gen_dir": ctx.genfiles_dir.path,
-            "TEMPLATED_install_source_map_support": str(ctx.attr.install_source_map_support).lower(),
-            "TEMPLATED_module_roots": "\n  " + ",\n  ".join(module_mappings),
-            "TEMPLATED_node_modules_root": node_modules_root,
-            "TEMPLATED_target": str(ctx.label),
-            "TEMPLATED_user_workspace_name": ctx.workspace_name,
-        },
+        output = ctx.outputs.loader_script,
+        substitutions = {"TEMPLATED_entry_point": entry_point_path},
         is_executable = True,
     )
 
@@ -165,6 +172,7 @@ def _nodejs_binary_impl(ctx):
             sources_depsets.append(d.files)
     sources = depset(transitive = sources_depsets)
 
+    _write_require_patch_script(ctx)
     _write_loader_script(ctx)
 
     env_vars = "export BAZEL_TARGET=%s\n" % ctx.label
@@ -191,7 +199,7 @@ def _nodejs_binary_impl(ctx):
 
     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(ctx.file._node_patches_script)
     node_tool_files.append(node_modules_manifest)
 
     is_builtin = ctx.attr._node.label.workspace_name in ["nodejs_%s" % p for p in BUILT_IN_NODE_PLATFORMS]
@@ -201,35 +209,37 @@ def _nodejs_binary_impl(ctx):
             expand_location_into_runfiles(ctx, a, ctx.attr.data)
             for a in ctx.attr.templated_args
         ]),
-        "TEMPLATED_bazel_require_script": _to_manifest_path(ctx, ctx.file._bazel_require_script),
         "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": _to_manifest_path(ctx, ctx.outputs.loader),
+        "TEMPLATED_loader_script": _to_manifest_path(ctx, ctx.outputs.loader_script),
         "TEMPLATED_modules_manifest": _to_manifest_path(ctx, node_modules_manifest),
+        "TEMPLATED_node_patches_script": _to_manifest_path(ctx, ctx.file._node_patches_script),
         "TEMPLATED_repository_args": _to_manifest_path(ctx, ctx.file._repository_args),
+        "TEMPLATED_require_patch_script": _to_manifest_path(ctx, ctx.outputs.require_patch_script),
         "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),
     }
     ctx.actions.expand_template(
         template = ctx.file._launcher_template,
-        output = ctx.outputs.script,
+        output = ctx.outputs.launcher_sh,
         substitutions = substitutions,
         is_executable = True,
     )
 
     runfiles = []
     runfiles.extend(node_tool_files)
     runfiles.extend(ctx.files._bash_runfile_helpers)
-    runfiles.append(ctx.outputs.loader)
+    runfiles.append(ctx.outputs.loader_script)
+    runfiles.append(ctx.outputs.require_patch_script)
     runfiles.append(ctx.file._repository_args)
 
     if is_windows(ctx):
-        runfiles.append(ctx.outputs.script)
-        executable = create_windows_native_launcher_script(ctx, ctx.outputs.script)
+        runfiles.append(ctx.outputs.launcher_sh)
+        executable = create_windows_native_launcher_script(ctx, ctx.outputs.launcher_sh)
     else:
-        executable = ctx.outputs.script
+        executable = ctx.outputs.launcher_sh
 
     # entry point is only needed in runfiles if it is a .js file
     if ctx.file.entry_point.extension == "js":
@@ -241,7 +251,8 @@ def _nodejs_binary_impl(ctx):
             runfiles = ctx.runfiles(
                 transitive_files = depset(runfiles),
                 files = node_tool_files + [
-                            ctx.outputs.loader,
+                            ctx.outputs.loader_script,
+                            ctx.outputs.require_patch_script,
                         ] + ctx.files._source_map_support_files +
 
                         # We need this call to the list of Files.
@@ -426,30 +437,34 @@ jasmine_node_test(
         """,
     ),
     "_bash_runfile_helpers": attr.label(default = Label("@bazel_tools//tools/bash/runfiles")),
-    "_bazel_require_script": attr.label(
-        default = Label("//internal/node:bazel_require_script.js"),
-        allow_single_file = True,
-    ),
     "_launcher_template": attr.label(
-        default = Label("//internal/node:node_launcher.sh"),
+        default = Label("//internal/node:launcher.sh"),
         allow_single_file = True,
     ),
     "_link_modules_script": attr.label(
         default = Label("//internal/linker:index.js"),
         allow_single_file = True,
     ),
     "_loader_template": attr.label(
-        default = Label("//internal/node:node_loader.js"),
+        default = Label("//internal/node:loader.js"),
         allow_single_file = True,
     ),
     "_node": attr.label(
         default = Label("@nodejs//:node_bin"),
         allow_single_file = True,
     ),
+    "_node_patches_script": attr.label(
+        default = Label("//internal/node:node_patches.js"),
+        allow_single_file = True,
+    ),
     "_repository_args": attr.label(
         default = Label("@nodejs//:bin/node_repo_args.sh"),
         allow_single_file = True,
     ),
+    "_require_patch_template": attr.label(
+        default = Label("//internal/node:require_patch.js"),
+        allow_single_file = True,
+    ),
     "_runfiles_helper_script": attr.label(
         default = Label("//internal/linker:runfiles_helper.js"),
         allow_single_file = True,
@@ -465,8 +480,9 @@ jasmine_node_test(
 }
 
 _NODEJS_EXECUTABLE_OUTPUTS = {
-    "loader": "%{name}_loader.js",
-    "script": "%{name}.sh",
+    "launcher_sh": "%{name}.sh",
+    "loader_script": "%{name}_loader.js",
+    "require_patch_script": "%{name}_require_patch.js",
 }
 
 # The name of the declared rule appears in