From 22c196a9729c00f844ccd44e6ba1ba550e5ba383 Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Fri, 1 Nov 2019 00:14:29 -0700 Subject: [PATCH] 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 https://github.com/bazelbuild/rules_nodejs/issues/1305 --- internal/node/node.bzl | 23 ++++++++++++----------- internal/node/node_launcher.sh | 27 ++++++++++++++++++++++++++- internal/node/node_repositories.bzl | 29 +++++++++++++++++++++++------ 3 files changed, 61 insertions(+), 18 deletions(-) diff --git a/internal/node/node.bzl b/internal/node/node.bzl index ed135aec32..0a90a26f4e 100644 --- a/internal/node/node.bzl +++ b/internal/node/node.bzl @@ -139,6 +139,9 @@ def _to_execroot_path(ctx, file): return (" _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,16 +176,8 @@ 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 = 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 +206,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_vendored_node = (ctx.attr._node.label.workspace_name != "nodejs_darwin_amd64" and ctx.attr._node.label.workspace_name != "nodenodejs_linux_amd64js_darwin_amd64" and ctx.attr._node.label.workspace_name != "nodejs_windows_amd64") + substitutions = { "TEMPLATED_args": " ".join([ expand_location_into_runfiles(ctx, a) @@ -221,9 +218,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": _strip_external(ctx.file._node.path) if is_vendored_node else "", } ctx.actions.expand_template( template = ctx.file._launcher_template, @@ -466,6 +463,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, diff --git a/internal/node/node_launcher.sh b/internal/node/node_launcher.sh index 6e4b4ca5e0..3f7c06e9da 100644 --- a/internal/node/node_launcher.sh +++ b/internal/node/node_launcher.sh @@ -114,7 +114,32 @@ 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") +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;; + esac + + case "${machine}" in + # The following paths must match up with _download_node in node_repositories + linux) readonly node=$(rlocation "nodejs_linux_amd64/bin/nodejs/bin/node");; + darwin) readonly node=$(rlocation "nodejs_darwin_amd64/bin/nodejs/bin/node");; + windows) readonly node=$(rlocation "nodejs_windows_amd64/bin/nodejs/node.exe");; + *) readonly node=$(rlocation "nodejs_linux_amd64/bin/nodejs/bin/node");; + esac +fi + readonly repository_args=$(rlocation "TEMPLATED_repository_args") MAIN=$(rlocation "TEMPLATED_loader_path") readonly link_modules_script=$(rlocation "TEMPLATED_link_modules_script") diff --git a/internal/node/node_repositories.bzl b/internal/node/node_repositories.bzl index 63a5074a58..a2139ad4c2 100644 --- a/internal/node/node_repositories.bzl +++ b/internal/node/node_repositories.bzl @@ -609,18 +609,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_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 +643,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,6 +666,8 @@ 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) @@ -659,7 +675,7 @@ def node_repositories(package_json = [], **kwargs): _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 +689,7 @@ def node_repositories(package_json = [], **kwargs): _maybe( _nodejs_repo_host_os_alias, name = "nodejs", + vendored_node = vendored_node, ) _maybe(