fix: fix nodejs_binary cross-platform RBE issue #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 #1305

fix: node

internal/common/path_utils.bzl

@@ -0,0 +1,21 @@
+# 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.
+
+"""Path utils
+
+Helper functions for path manipulations.
+"""
+
+def strip_external(path):
+    return path[len("external/"):] if path.startswith("external/") else path

internal/node/node.bzl

@@ -23,7 +23,9 @@ a `module_name` attribute can be `require`d by that name.
 load("@build_bazel_rules_nodejs//:providers.bzl", "JSNamedModuleInfo", "NodeRuntimeDepsInfo", "NpmPackageInfo", "node_modules_aspect")
 load("//internal/common:expand_into_runfiles.bzl", "expand_location_into_runfiles")
 load("//internal/common:module_mappings.bzl", "module_mappings_runtime_aspect")
+load("//internal/common:path_utils.bzl", "strip_external")
 load("//internal/common:windows_utils.bzl", "create_windows_native_launcher_script", "is_windows")
+load("//internal/node:node_repositories.bzl", "BUILT_IN_NODE_PLATFORMS")
 
 def _trim_package_node_modules(package_name):
     # trim a package name down to its path prior to a node_modules
@@ -173,15 +175,17 @@ 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)
+    # Add both the node executable for the user's local machine which is in ctx.files._node and comes
+    # from @nodejs//:node_bin and the node executable from the selected node --platform which comes from
+    # ctx.toolchains["@build_bazel_rules_nodejs//toolchains/node:toolchain_type"].nodeinfo.
+    # In most cases these are the same files but for RBE and when explitely setting --platform for cross-compilation
+    # any given nodejs_binary should be able to run on both the user's local machine and on the RBE or selected
+    # platform.
+    #
+    # Rules such as nodejs_image should use only ctx.toolchains["@build_bazel_rules_nodejs//toolchains/node:toolchain_type"].nodeinfo
+    # when building the image as that will reflect the selected --platform.
+    node_tool_files = ctx.files._node[:]
+    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._bazel_require_script)
@@ -211,6 +215,8 @@ def _nodejs_binary_impl(ctx):
         # also be sure to include the params file in the program inputs
         node_tool_files.append(ctx.outputs.templated_args_file)
 
+    is_builtin = ctx.attr._node.label.workspace_name in ["nodejs_%s" % p for p in BUILT_IN_NODE_PLATFORMS]
+
     substitutions = {
         "TEMPLATED_args": " ".join([
             expand_location_into_runfiles(ctx, a)
@@ -221,9 +227,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_repository_args": _to_manifest_path(ctx, ctx.file._repository_args),
         "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,
@@ -466,6 +472,10 @@ 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,
+    ),
     "_repository_args": attr.label(
         default = Label("@nodejs//:bin/node_repo_args.sh"),
         allow_single_file = True,

internal/node/node_launcher.sh

@@ -114,7 +114,50 @@ 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")
+readonly vendored_node="TEMPLATED_vendored_node"
+
+if [ -n "${vendored_node}" ]; then
+  # Use the vendored node path
+  readonly node=$(rlocation "${vendored_node}")
+
+  if [ ! -f "${node}" ]; then
+      printf "\n>>>> FAIL: The vendored node binary '${vendored_node}' not found in runfiles. <<<<\n\n" >&2
+      exit 1
+  fi
+else
+  # Check environment for which node path to use
+  unameOut="$(uname -s)"
+  case "${unameOut}" in
+      Linux*)     machine=linux ;;
+      Darwin*)    machine=darwin ;;
+      CYGWIN*)    machine=windows ;;
+      MINGW*)     machine=windows ;;
+      MSYS_NT*)   machine=windows ;;
+      *)          machine=linux
+                  printf "\nUnrecongized uname '${unameOut}'; defaulting to use node for linux.\n" >&2
+                  printf "Please file an issue to https://github.com/bazelbuild/rules_nodejs/issues if \n" >&2
+                  printf "you would like to add your platform to the supported rules_nodejs node platforms.\n\n" >&2
+                  ;;
+  esac
+
+  case "${machine}" in
+    # The following paths must match up with _download_node in node_repositories
+    darwin) readonly node_toolchain="nodejs_darwin_amd64/bin/nodejs/bin/node" ;;
+    windows) readonly node_toolchain="nodejs_windows_amd64/bin/nodejs/node.exe" ;;
+    *) readonly node_toolchain="nodejs_linux_amd64/bin/nodejs/bin/node" ;;
+  esac
+
+  readonly node=$(rlocation "${node_toolchain}")
+
+  if [ ! -f "${node}" ]; then
+      printf "\n>>>> FAIL: The node binary '${node_toolchain}' not found in runfiles.\n" >&2
+      printf "This node toolchain was chosen based on your uname '${unameOut}'.\n" >&2
+      printf "Please file an issue to https://github.com/bazelbuild/rules_nodejs/issues if \n" >&2
+      printf "you would like to add your platform to the supported rules_nodejs node platforms. <<<<\n\n" >&2
+      exit 1
+  fi
+fi
+
 readonly repository_args=$(rlocation "TEMPLATED_repository_args")
 MAIN=$(rlocation "TEMPLATED_loader_path")
 readonly link_modules_script=$(rlocation "TEMPLATED_link_modules_script")

internal/node/node_repositories.bzl

@@ -243,8 +243,14 @@ and expect the file to have sha256sum `09bea8f4ec41e9079fa03093d3b2db7ac5c533185
     ),
 }
 
-NODE_DIR = "bin/nodejs"
-YARN_DIR = "bin/yarnpkg"
+BUILT_IN_NODE_PLATFORMS = [
+    "darwin_amd64",
+    "linux_amd64",
+    "windows_amd64",
+]
+
+NODE_EXTRACT_DIR = "bin/nodejs"
+YARN_EXTRACT_DIR = "bin/yarnpkg"
 
 GET_SCRIPT_DIR = """
 # From stackoverflow.com
@@ -268,24 +274,25 @@ def _download_node(repository_ctx):
     """
     if repository_ctx.attr.vendored_node:
         return
-    if repository_ctx.name == "nodejs":
-        host = os_name(repository_ctx)
-    else:
-        host = repository_ctx.name.split("nodejs_", 1)[1]
+
+    # The host is baked into the repository name by design.
+    # Current these workspaces are:
+    # @nodejs_PLATFORM where PLATFORM is one of BUILT_IN_NODE_PLATFORMS
+    host_os = repository_ctx.name.split("nodejs_", 1)[1]
+
     node_version = repository_ctx.attr.node_version
     node_repositories = repository_ctx.attr.node_repositories
     node_urls = repository_ctx.attr.node_urls
 
     # Download node & npm
-    node_host_version = "{}-{}".format(node_version, host)
-    if node_host_version in node_repositories:
-        filename, strip_prefix, sha256 = node_repositories[node_host_version]
-    else:
-        fail("Unknown NodeJS host/version {}".format(node_host_version))
+    version_host_os = "%s-%s" % (node_version, host_os)
+    if not version_host_os in node_repositories:
+        fail("Unknown NodeJS version-host %s" % version_host_os)
+    filename, strip_prefix, sha256 = node_repositories[version_host_os]
 
     repository_ctx.download_and_extract(
         url = [url.format(version = node_version, filename = filename) for url in node_urls],
-        output = NODE_DIR,
+        output = NODE_EXTRACT_DIR,
         stripPrefix = strip_prefix,
         sha256 = sha256,
     )
@@ -306,11 +313,11 @@ def _download_yarn(repository_ctx):
     if yarn_version in yarn_repositories:
         filename, strip_prefix, sha256 = yarn_repositories[yarn_version]
     else:
-        fail("Unknown Yarn version {}".format(yarn_version))
+        fail("Unknown Yarn version %s" % yarn_version)
 
     repository_ctx.download_and_extract(
         url = [url.format(version = yarn_version, filename = filename) for url in yarn_urls],
-        output = YARN_DIR,
+        output = YARN_EXTRACT_DIR,
         stripPrefix = strip_prefix,
         sha256 = sha256,
     )
@@ -353,8 +360,8 @@ def _prepare_node(repository_ctx):
             "lib/node_modules/npm/bin/npm-cli.js" if not is_windows else "node_modules/npm/bin/npm-cli.js",
         ] if f])
     else:
-        node_exec = "{}/bin/node".format(NODE_DIR) if not is_windows else "{}/node.exe".format(NODE_DIR)
-        npm_script = "{}/lib/node_modules/npm/bin/npm-cli.js".format(NODE_DIR) if not is_windows else "{}/node_modules/npm/bin/npm-cli.js".format(NODE_DIR)
+        node_exec = ("%s/bin/node" % NODE_EXTRACT_DIR) if not is_windows else ("%s/node.exe" % NODE_EXTRACT_DIR)
+        npm_script = ("%s/lib/node_modules/npm/bin/npm-cli.js" % NODE_EXTRACT_DIR) if not is_windows else ("%s/node_modules/npm/bin/npm-cli.js" % NODE_EXTRACT_DIR)
         node_exec_label = node_exec
     if repository_ctx.attr.vendored_yarn:
         yarn_script = "/".join([f for f in [
@@ -365,7 +372,7 @@ def _prepare_node(repository_ctx):
             "bin/yarn.js",
         ] if f])
     else:
-        yarn_script = "{}/bin/yarn.js".format(YARN_DIR)
+        yarn_script = "%s/bin/yarn.js" % YARN_EXTRACT_DIR
     node_entry = "bin/node" if not is_windows else "bin/node.cmd"
     npm_node_repositories_entry = "bin/npm_node_repositories" if not is_windows else "bin/npm_node_repositories.cmd"
     yarn_node_repositories_entry = "bin/yarn_node_repositories" if not is_windows else "bin/yarn_node_repositories.cmd"
@@ -420,8 +427,8 @@ CALL "%SCRIPT_DIR%\\{node}" {args} %*
 # Immediately exit if any command fails.
 set -e
 # Generated by node_repositories.bzl
-export NODE_REPOSITORY_ARGS={}
-""".format(node_repo_args), executable = True)
+export NODE_REPOSITORY_ARGS={args}
+""".format(args = node_repo_args), executable = True)
 
     # The entry points for npm for osx/linux and windows
     # Runs npm using appropriate node entry point
@@ -609,18 +616,29 @@ node_repositories_rule = repository_rule(
 )
 
 def _nodejs_host_os_alias_impl(repository_ctx):
-    host_os = os_name(repository_ctx)
-    node_repository = "@nodejs_%s" % host_os
     is_windows_host = is_windows_os(repository_ctx)
     file_ending = ".cmd" if is_windows_host else ""
-    actual_node_bin = "bin/nodejs/node.exe" if is_windows_host else "bin/nodejs/bin/node"
+    node_repository = "@nodejs_%s" % os_name(repository_ctx)
+    if repository_ctx.attr.vendored_node:
+        node_bin_repository = "@%s" % repository_ctx.attr.vendored_node.workspace_name
+        actual_node_bin = "/".join([f for f in [
+            repository_ctx.attr.vendored_node.package,
+            repository_ctx.attr.vendored_node.name,
+            "bin/node" if not is_windows_host else "node.exe",
+        ] if f])
+    else:
+        node_bin_repository = node_repository
+        actual_node_bin = "%s/%s" % (
+            NODE_EXTRACT_DIR,
+            "node.exe" if is_windows_host else "bin/node",
+        )
     repository_ctx.template(
         "BUILD.bazel",
         Label("@build_bazel_rules_nodejs//internal/node:BUILD.nodejs_host_os_alias.tpl"),
         substitutions = {
             "TEMPLATE__npm_node_repositories": "%s//:bin/npm_node_repositories%s" % (node_repository, file_ending),
             "TEMPLATE__yarn_node_repositories": "%s//:bin/yarn_node_repositories%s" % (node_repository, file_ending),
-            "TEMPLATE_actual_node_bin": "%s//:%s" % (node_repository, actual_node_bin),
+            "TEMPLATE_actual_node_bin": "%s//:%s" % (node_bin_repository, actual_node_bin),
             "TEMPLATE_node_repo_args": "%s//:bin/node_repo_args.sh" % node_repository,
             "TEMPLATE_npm": "%s//:bin/npm%s" % (node_repository, file_ending),
             "TEMPLATE_run_npm": "%s//:run_npm.sh.template" % node_repository,
@@ -632,9 +650,12 @@ def _nodejs_host_os_alias_impl(repository_ctx):
 
 _nodejs_repo_host_os_alias = repository_rule(
     _nodejs_host_os_alias_impl,
+    attrs = {
+        "vendored_node": attr.label(allow_single_file = True),
+    },
 )
 
-def node_repositories(package_json = [], **kwargs):
+def node_repositories(**kwargs):
     """
     Wrapper macro around node_repositories_rule to call it for each platform, register bazel toolchains,
     and make other convenience repositories.
@@ -652,14 +673,16 @@ def node_repositories(package_json = [], **kwargs):
         minimum_bazel_version = "0.21.0",
     )
 
+    vendored_node = kwargs.pop("vendored_node", None)
+
     # This needs to be setup so toolchains can access nodejs for all different versions
     for os_arch_name in OS_ARCH_NAMES:
         os_name = "_".join(os_arch_name)
         node_repository_name = "nodejs_%s" % os_name
         _maybe(
             node_repositories_rule,
             name = node_repository_name,
-            package_json = package_json,
+            vendored_node = vendored_node,
             **kwargs
         )
         native.register_toolchains("@build_bazel_rules_nodejs//toolchains/node:node_%s_toolchain" % os_arch_name[0])
@@ -673,6 +696,7 @@ def node_repositories(package_json = [], **kwargs):
     _maybe(
         _nodejs_repo_host_os_alias,
         name = "nodejs",
+        vendored_node = vendored_node,
     )
 
     _maybe(

internal/npm_package/npm_package.bzl

@@ -7,6 +7,7 @@ to the `deps` of one of their targets.
 """
 
 load("@build_bazel_rules_nodejs//:providers.bzl", "DeclarationInfo", "JSNamedModuleInfo")
+load("//internal/common:path_utils.bzl", "strip_external")
 
 # Takes a depset of files and returns a corresponding list of file paths without any files
 # that aren't part of the specified package path. Also include files from external repositories
@@ -65,7 +66,7 @@ def create_package(ctx, deps_sources, nested_packages):
     args.add("1" if ctx.attr.rename_build_files else "0")
 
     # require.resolve expects the path to start with the workspace name and not "external"
-    run_npm_template_path = ctx.file._run_npm_template.path[len("external") + 1:] if ctx.file._run_npm_template.path.startswith("external") else ctx.file._run_npm_template.path
+    run_npm_template_path = strip_external(ctx.file._run_npm_template.path)
     args.add(run_npm_template_path)
 
     inputs = ctx.files.srcs + deps_sources + nested_packages + [ctx.file._run_npm_template]