From 38d0b3d64037f73509ca494ea11c458e5c3aa986 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 fix: node --- internal/common/path_utils.bzl | 21 ++++++++ internal/node/node.bzl | 30 +++++++---- internal/node/node_launcher.sh | 45 ++++++++++++++++- internal/node/node_repositories.bzl | 74 ++++++++++++++++++---------- internal/npm_package/npm_package.bzl | 3 +- 5 files changed, 136 insertions(+), 37 deletions(-) create mode 100644 internal/common/path_utils.bzl diff --git a/internal/common/path_utils.bzl b/internal/common/path_utils.bzl new file mode 100644 index 0000000000..182a5b32fb --- /dev/null +++ b/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 diff --git a/internal/node/node.bzl b/internal/node/node.bzl index ed135aec32..dba72ae965 100644 --- a/internal/node/node.bzl +++ b/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, diff --git a/internal/node/node_launcher.sh b/internal/node/node_launcher.sh index 6e4b4ca5e0..7272a62680 100644 --- a/internal/node/node_launcher.sh +++ b/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") diff --git a/internal/node/node_repositories.bzl b/internal/node/node_repositories.bzl index 63a5074a58..dc3272ce35 100644 --- a/internal/node/node_repositories.bzl +++ b/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,6 +673,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 +682,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 +696,7 @@ def node_repositories(package_json = [], **kwargs): _maybe( _nodejs_repo_host_os_alias, name = "nodejs", + vendored_node = vendored_node, ) _maybe( diff --git a/internal/npm_package/npm_package.bzl b/internal/npm_package/npm_package.bzl index a6f9bba863..5f4e895232 100644 --- a/internal/npm_package/npm_package.bzl +++ b/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]