fix: fix nodejs_binary cross-platform RBE issue bazel-contrib#1305

* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazel-contrib#1305

internal/node/node.bzl

@@ -139,6 +139,9 @@ def _to_execroot_path(ctx, file):
 
     return ("<ERROR> _to_execroot_path not yet implemented for " + file.path)
 
+def _strip_external(path):
+    return path[len("external/"):] if path.startswith("external/") or path.startswith("external\\") else path
+
 def _nodejs_binary_impl(ctx):
     node_modules = depset(ctx.files.node_modules)
 
@@ -173,18 +176,11 @@ def _nodejs_binary_impl(ctx):
     if hasattr(ctx.attr, "expected_exit_code"):
         expected_exit_code = ctx.attr.expected_exit_code
 
-    node_tool_info = ctx.toolchains["@build_bazel_rules_nodejs//toolchains/node:toolchain_type"].nodeinfo
-
-    # Make a copy so we don't try to mutate a frozen object
-    node_tool_files = node_tool_info.tool_files[:]
-    if node_tool_info.target_tool_path == "":
-        # If tool_path is empty and tool_target is None then there is no local
-        # node tool, we will just print a nice error message if the user
-        # attempts to do bazel run
-        fail("The node toolchain was not properly configured so %s cannot be executed. Make sure that target_tool_path or target_tool is set." % ctx.attr.name)
-
-    node_tool_files.append(ctx.file._link_modules_script)
-    node_tool_files.append(ctx.file._bazel_require_script)
+    node_tool_files = [
+        ctx.file._link_modules_script,
+        ctx.file._bazel_require_script,
+    ]
+    node_tool_files.extend(ctx.files._node)
 
     if not ctx.outputs.templated_args_file:
         templated_args = ctx.attr.templated_args
@@ -221,7 +217,9 @@ def _nodejs_binary_impl(ctx):
         "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_node": node_tool_info.target_tool_path,
+        "TEMPLATED_node_darwin": _strip_external(ctx.file._node_darwin.path),
+        "TEMPLATED_node_linux": _strip_external(ctx.file._node_linux.path),
+        "TEMPLATED_node_windows": _strip_external(ctx.file._node_windows.path),
         "TEMPLATED_repository_args": _to_manifest_path(ctx, ctx.file._repository_args),
         "TEMPLATED_script_path": _to_execroot_path(ctx, ctx.file.entry_point),
     }
@@ -466,6 +464,22 @@ jasmine_node_test(
         default = Label("//internal/node:node_loader.js"),
         allow_single_file = True,
     ),
+    "_node": attr.label(
+        default = Label("@nodejs//:node_bin"),
+        allow_single_file = True,
+    ),
+    "_node_darwin": attr.label(
+        default = Label("@nodejs_darwin_amd64//:node_bin"),
+        allow_single_file = True,
+    ),
+    "_node_linux": attr.label(
+        default = Label("@nodejs_linux_amd64//:node_bin"),
+        allow_single_file = True,
+    ),
+    "_node_windows": attr.label(
+        default = Label("@nodejs_windows_amd64//:node_bin"),
+        allow_single_file = True,
+    ),
     "_repository_args": attr.label(
         default = Label("@nodejs//:bin/node_repo_args.sh"),
         allow_single_file = True,

internal/node/node_launcher.sh

@@ -114,7 +114,24 @@ TEMPLATED_env_vars
 # This redirects to stderr so it doesn't interfere with Bazel's worker protocol
 # find . -name thingImLookingFor 1>&2
 
-readonly node=$(rlocation "TEMPLATED_node")
+# Check environment
+unameOut="$(uname -s)"
+case "${unameOut}" in
+    Linux*)     machine=linux;;
+    Darwin*)    machine=darwin;;
+    CYGWIN*)    machine=windows;;
+    MINGW*)     machine=windows;;
+    MSYS_NT*)   machine=windows;;
+    *)          machine=linux;;
+esac
+
+case "${machine}" in
+  linux) readonly node=$(rlocation "TEMPLATED_node_linux");;
+  darwin) readonly node=$(rlocation "TEMPLATED_node_darwin");;
+  windows) readonly node=$(rlocation "TEMPLATED_node_windows");;
+  *) readonly node=$(rlocation "TEMPLATED_node_linux");;
+esac
+
 readonly repository_args=$(rlocation "TEMPLATED_repository_args")
 MAIN=$(rlocation "TEMPLATED_loader_path")
 readonly link_modules_script=$(rlocation "TEMPLATED_link_modules_script")

internal/node/test/BUILD.bazel

@@ -2,9 +2,6 @@ load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary", "nodejs_test", "n
 load("//internal/js_library:js_library.bzl", "js_library")
 load("//third_party/github.com/bazelbuild/bazel-skylib:rules/copy_file.bzl", "copy_file")
 load("//third_party/github.com/bazelbuild/bazel-skylib:rules/write_file.bzl", "write_file")
-load(":nodejs_toolchain_test.bzl", "nodejs_binary_test_suite")
-
-nodejs_binary_test_suite()
 
 # You can have a nodejs_binary with no node_modules attribute
 # and no fine grained deps