Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(builtin): remove unnecessary loader script #3495

Merged
merged 1 commit into from
Jun 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions examples/angular/WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ http_archive(
patches = [
# Updates @bazel/work dep to 4.0.0 inside rules_sass so it is compatible
"//:io_bazel_rules_sass.patch",
# Add missing extension to entrypoint (https://github.com/bazelbuild/rules_sass/pull/142)
"//:io_bazel_rules_sass_entrypoint.patch",
],
sha256 = "5313032124ff191eed68efcfbdc6ee9b5198093b2b80a8e640ea34feabbffc69",
strip_prefix = "rules_sass-1.34.0",
Expand Down
12 changes: 12 additions & 0 deletions examples/angular/io_bazel_rules_sass_entrypoint.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
index cfa8b8b..394f6c2 100644
--- a/sass/BUILD
+++ b/sass/BUILD
@@ -11,7 +11,7 @@ exports_files([
# Executable for the sass_binary rule
nodejs_binary(
name = "sass",
- entry_point = "sass_wrapper",
+ entry_point = "sass_wrapper.js",
data = [
":sass_wrapper.js",
"@build_bazel_rules_sass_deps//sass",
2 changes: 0 additions & 2 deletions internal/node/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ package(default_visibility = ["//visibility:public"])
exports_files([
"node_patches.cjs",
"require_patch.cjs",
"loader.cjs",
])

bzl_library(
Expand All @@ -40,7 +39,6 @@ exports_files([
"node.bzl", # Exported to be consumed for generating stardoc.
"node_repositories.bzl", # Exported to be consumed for generating stardoc.
"launcher.sh",
"loader.cjs",
])

filegroup(
Expand Down
39 changes: 19 additions & 20 deletions internal/node/launcher.sh
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,11 @@ for ARG in ${ALL_ARGS[@]+"${ALL_ARGS[@]}"}; do
--bazel_capture_exit_code=*) EXIT_CODE_CAPTURE="${PWD}/${ARG#--bazel_capture_exit_code=}" ;;
# Outputs nothing on success
--bazel_silent_on_success=*) SILENT_ON_SUCCESS=true ;;
# Disable the node_loader.cjs monkey patches for require()
# Disable the require_patch.cjs monkey patches for require()
# Note that this means you need an explicit runfiles helper library
# This flag is now a no-op since the default is also false
--nobazel_patch_module_resolver) PATCH_REQUIRE=false ;;
# Enable the node_loader.cjs monkey patches for require()
# Enable the require_patch.cjs monkey patches for require()
--bazel_patch_module_resolver) PATCH_REQUIRE=true ;;
# Disable the --require node-patches (undocumented and unused; only here as an escape value)
--nobazel_node_patches) NODE_PATCHES=false ;;
Expand Down Expand Up @@ -331,26 +331,24 @@ if [ "$PATCH_REQUIRE" = true ]; then
* ) require_patch_script="${PWD}/${require_patch_script}" ;;
esac
LAUNCHER_NODE_OPTIONS+=( "--require" "$require_patch_script" )
# Change the entry point to be the loader.cjs script so we run code before node
MAIN=$(rlocation "TEMPLATED_loader_script")
else
# Entry point is the user-supplied script
MAIN="${PWD}/"TEMPLATED_entry_point_execroot_path
# TODO: after we link-all-bins we should not need this extra lookup
if [[ ! -e "$MAIN" ]]; then
if [ "$FROM_EXECROOT" = true ]; then
MAIN="$EXECROOT/"TEMPLATED_entry_point_execroot_path
else
MAIN=TEMPLATED_entry_point_manifest_path
fi
fi
# Always set up source-map-support using our vendored copy, just like the require_patch_script
register_source_map_support=$(rlocation build_bazel_rules_nodejs/third_party/github.com/source-map-support/register.js)
LAUNCHER_NODE_OPTIONS+=( "--require" "${register_source_map_support}" )
if [[ -n "TEMPLATED_entry_point_main" ]]; then
MAIN="${MAIN}/"TEMPLATED_entry_point_main
fi

# Entry point is the user-supplied script
MAIN="${PWD}/"TEMPLATED_entry_point_execroot_path
# TODO: after we link-all-bins we should not need this extra lookup
if [[ ! -e "$MAIN" ]]; then
if [ "$FROM_EXECROOT" = true ]; then
MAIN="$EXECROOT/"TEMPLATED_entry_point_execroot_path
else
MAIN=TEMPLATED_entry_point_manifest_path
fi
fi
# Always set up source-map-support using our vendored copy, just like the require_patch_script
register_source_map_support=$(rlocation build_bazel_rules_nodejs/third_party/github.com/source-map-support/register.js)
LAUNCHER_NODE_OPTIONS+=( "--require" "${register_source_map_support}" )
if [[ -n "TEMPLATED_entry_point_main" ]]; then
MAIN="${MAIN}/"TEMPLATED_entry_point_main
fi

if [ "${SILENT_ON_SUCCESS:-}" = true ]; then
if [[ -z "${STDOUT_CAPTURE}" ]]; then
Expand Down Expand Up @@ -484,3 +482,4 @@ if [[ -n "${EXIT_CODE_CAPTURE}" ]]; then
else
exit ${RESULT}
fi

39 changes: 0 additions & 39 deletions internal/node/loader.cjs

This file was deleted.

25 changes: 0 additions & 25 deletions internal/node/node.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -117,21 +117,6 @@ def _get_entry_point_file(ctx):
return ctx.attr.entry_point[DirectoryFilePathInfo].directory
fail("entry_point must either be a file, or provide DirectoryFilePathInfo")

def _write_loader_script(ctx):
substitutions = {}
substitutions["TEMPLATED_entry_point_path"] = _ts_to_js(_to_manifest_path(ctx, _get_entry_point_file(ctx)))
if DirectoryFilePathInfo in ctx.attr.entry_point:
substitutions["TEMPLATED_entry_point_main"] = ctx.attr.entry_point[DirectoryFilePathInfo].path
else:
substitutions["TEMPLATED_entry_point_main"] = ""

ctx.actions.expand_template(
template = ctx.file._loader_template,
output = ctx.outputs.loader_script,
substitutions = substitutions,
is_executable = True,
)

# Avoid using non-normalized paths (workspace/../other_workspace/path)
def _to_manifest_path(ctx, file):
if file.short_path.startswith("../"):
Expand Down Expand Up @@ -208,8 +193,6 @@ def _nodejs_binary_impl(ctx, data = [], runfiles = [], expanded_args = []):
node_modules_root = "build_bazel_rules_nodejs/node_modules"
_write_require_patch_script(ctx, data, node_modules_root)

_write_loader_script(ctx)

# Provide the target name as an environment variable avaiable to all actions for the
# runfiles helpers to use.
env_vars = "export BAZEL_TARGET=%s\n" % ctx.label
Expand Down Expand Up @@ -276,7 +259,6 @@ fi
runfiles = runfiles[:]
runfiles.extend(node_tool_files)
runfiles.extend(ctx.files._bash_runfile_helper)
runfiles.append(ctx.outputs.loader_script)
runfiles.append(ctx.outputs.require_patch_script)

# First replace any instances of "$(rlocation " with "$$(rlocation " to preserve
Expand Down Expand Up @@ -331,7 +313,6 @@ if (process.cwd() !== __dirname) {
"TEMPLATED_expected_exit_code": str(expected_exit_code),
"TEMPLATED_lcov_merger_script": _to_manifest_path(ctx, ctx.file._lcov_merger_script),
"TEMPLATED_link_modules_script": _to_manifest_path(ctx, ctx.file._link_modules_script),
"TEMPLATED_loader_script": _to_manifest_path(ctx, ctx.outputs.loader_script),
"TEMPLATED_modules_manifest": _to_manifest_path(ctx, node_modules_manifest),
"TEMPLATED_node_patches_script": _to_manifest_path(ctx, ctx.file._node_patches_script),
"TEMPLATED_require_patch_script": _to_manifest_path(ctx, ctx.outputs.require_patch_script),
Expand Down Expand Up @@ -378,7 +359,6 @@ if (process.cwd() !== __dirname) {
runfiles = ctx.runfiles(
transitive_files = depset(runfiles),
files = node_tool_files + [
ctx.outputs.loader_script,
ctx.outputs.require_patch_script,
] + ctx.files._source_map_support_files +

Expand Down Expand Up @@ -608,10 +588,6 @@ Predefined genrule variables are not supported in this context.
default = Label("//internal/linker:index.js"),
allow_single_file = True,
),
"_loader_template": attr.label(
default = Label("//internal/node:loader.cjs"),
allow_single_file = True,
),
"toolchain": attr.label(),
"_node_args": attr.label(default = "@rules_nodejs//nodejs:default_args"),
"_node_patches_script": attr.label(
Expand Down Expand Up @@ -642,7 +618,6 @@ Predefined genrule variables are not supported in this context.

_NODEJS_EXECUTABLE_OUTPUTS = {
"launcher_sh": "%{name}.sh",
"loader_script": "%{name}_loader.cjs",
"require_patch_script": "%{name}_require_patch.cjs",
}

Expand Down
15 changes: 14 additions & 1 deletion internal/node/test/esm/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary")
load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary", "npm_package_bin")

nodejs_binary(
name = "has_deps",
Expand All @@ -12,3 +12,16 @@ nodejs_binary(
name = "no_deps",
entry_point = ":no-deps.mjs",
)

nodejs_binary(
name = "no_deps_linker_disabled",
entry_point = ":no-deps-create-file.mjs",
templated_args = ["--nobazel_run_linker"],
)

npm_package_bin(
name = "run_with_linker_disabled",
outs = ["out.txt"],
args = ["$@"],
tool = ":no_deps_linker_disabled",
)
4 changes: 4 additions & 0 deletions internal/node/test/esm/no-deps-create-file.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import {writeFileSync} from "fs";

const outputFile = process.argv[2];
writeFileSync(outputFile, "foobar\n");