From 7955dbc9025fbbc21d1fac02571e960772c4127a Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Fri, 10 Apr 2020 15:23:11 -0700 Subject: [PATCH] fix(builtin): look in the execroot for nodejs_binary source entry_points In a6e29c2add19dd487035b0951f5271053a0a11e2 we added support for generated entry_point by adding a secondary lookup. In 863c7de48046c35dfe29f6a4f625ca3e7057b030 we reversed the order of the lookups to make rollup work in a certain use case. Neither of these was principled, because we know ahead of time whether the entry_point is generated. We can use a single lookup. This also makes sure that programs run in the location where the linker will put them (note that the logic in question runs before the linker has created the node_modules directory in the execroot.) This is why #1787 observes that some node programs were broken - we were running the entry point as bazel-out/host/bin/external/npm/@graphql-codegen/cli/bin/graphql-codegen.sh.runfiles/npm/node_modules/@graphql-codegen/cli/bin.js when it should have been node_modules/@graphql-codegen/cli/bin.js Fixes #1787 --- internal/node/launcher.sh | 5 +---- internal/node/node.bzl | 11 ++++++++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh index ba149e03d4..89149785a2 100644 --- a/internal/node/launcher.sh +++ b/internal/node/launcher.sh @@ -191,10 +191,7 @@ for ARG in "${ALL_ARGS[@]:-}"; do case "$ARG" in --bazel_node_modules_manifest=*) MODULES_MANIFEST="${ARG#--bazel_node_modules_manifest=}" ;; --nobazel_patch_module_resolver) - declare MAIN="TEMPLATED_entry_point_execroot_path" - if [[ ! -f "$MAIN" ]]; then - MAIN=$(rlocation "TEMPLATED_entry_point_manifest_path") - fi + MAIN=TEMPLATED_script_path LAUNCHER_NODE_OPTIONS=( "--require" "$node_patches_script" ) # In this case we should always run the linker diff --git a/internal/node/node.bzl b/internal/node/node.bzl index 858bfa738d..f9f78338c5 100644 --- a/internal/node/node.bzl +++ b/internal/node/node.bzl @@ -135,12 +135,13 @@ def _to_manifest_path(ctx, file): def _to_execroot_path(ctx, file): parts = file.path.split("/") - if parts[0] == "external": if parts[2] == "node_modules": # external/npm/node_modules -> node_modules/foo # the linker will make sure we can resolve node_modules from npm return "/".join(parts[2:]) + else: + return "/".join([".."] + parts[1:]) return file.path def _nodejs_binary_impl(ctx): @@ -246,8 +247,6 @@ fi # Need a smarter split operation than `expanded_arg.split(" ")` as it will split # up args with intentional spaces and it will fail for expanded files with spaces. "TEMPLATED_args": " ".join(expanded_args), - "TEMPLATED_entry_point_execroot_path": _to_execroot_path(ctx, ctx.file.entry_point), - "TEMPLATED_entry_point_manifest_path": _to_manifest_path(ctx, ctx.file.entry_point), "TEMPLATED_env_vars": env_vars, "TEMPLATED_expected_exit_code": str(expected_exit_code), "TEMPLATED_link_modules_script": _to_manifest_path(ctx, ctx.file._link_modules_script), @@ -259,6 +258,12 @@ fi "TEMPLATED_runfiles_helper_script": _to_manifest_path(ctx, ctx.file._runfiles_helper_script), "TEMPLATED_vendored_node": "" if is_builtin else strip_external(ctx.file._node.path), } + + if ctx.file.entry_point.is_source: + substitutions["TEMPLATED_script_path"] = "\"%s\"" % _to_execroot_path(ctx, ctx.file.entry_point) + else: + substitutions["TEMPLATED_script_path"] = "$(rlocation \"%s\")" % _to_manifest_path(ctx, ctx.file.entry_point) + ctx.actions.expand_template( template = ctx.file._launcher_template, output = ctx.outputs.launcher_sh,