Skip to content

Commit

Permalink
fix: fix nodejs_binary cross-platform RBE issue bazel-contrib#1305
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
gregmagolan committed Nov 7, 2019
1 parent 7653a22 commit 22c196a
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 18 deletions.
23 changes: 12 additions & 11 deletions internal/node/node.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
27 changes: 26 additions & 1 deletion internal/node/node_launcher.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
29 changes: 23 additions & 6 deletions internal/node/node_repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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.
Expand All @@ -652,14 +666,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])
Expand All @@ -673,6 +689,7 @@ def node_repositories(package_json = [], **kwargs):
_maybe(
_nodejs_repo_host_os_alias,
name = "nodejs",
vendored_node = vendored_node,
)

_maybe(
Expand Down

0 comments on commit 22c196a

Please sign in to comment.