Skip to content

Commit

Permalink
fix(builtin): look in the execroot for nodejs_binary source entry_points
Browse files Browse the repository at this point in the history
In a6e29c2 we added support for generated entry_point by adding a secondary lookup.
In 863c7de 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.
However that depends on running the linker on all generated bin rules, which is a breaking change, so for now we just defer the FS check until after the linker runs.

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
  • Loading branch information
alexeagle committed Apr 11, 2020
1 parent 7e3f9b1 commit 3d6e003
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 7 deletions.
10 changes: 6 additions & 4 deletions internal/node/launcher.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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_entry_point_execroot_path
LAUNCHER_NODE_OPTIONS=( "--require" "$node_patches_script" )

# In this case we should always run the linker
Expand All @@ -213,6 +210,11 @@ if [[ -n "${MODULES_MANIFEST:-}" ]]; then
"${node}" "${link_modules_script}" "${MODULES_MANIFEST:-}"
fi

# TODO: after we link-all-bins we should not need this extra lookup
if [[ ! -f "$MAIN" ]]; then
MAIN=TEMPLATED_entry_point_manifest_path
fi

# Tell the node_patches_script that programs should not escape the execroot
# Bazel always sets the PWD to execroot/my_wksp so we go up one directory.
export BAZEL_PATCH_ROOT=$(dirname $PWD)
Expand Down
14 changes: 11 additions & 3 deletions internal/node/node.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,12 @@ 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:])

return file.path

def _nodejs_binary_impl(ctx):
Expand Down Expand Up @@ -246,8 +246,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),
Expand All @@ -259,6 +257,16 @@ 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),
}

# TODO when we have "link_all_bins" we will only need to look in one place for the entry point
#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)
# For now we need to look in both places
substitutions["TEMPLATED_entry_point_execroot_path"] = "\"%s\"" % _to_execroot_path(ctx, ctx.file.entry_point)
substitutions["TEMPLATED_entry_point_manifest_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,
Expand Down

0 comments on commit 3d6e003

Please sign in to comment.