From 2982099e5cc7f128691de4b75328f718b49b7e9f Mon Sep 17 00:00:00 2001 From: dymart Date: Mon, 13 Dec 2021 10:41:53 -0800 Subject: [PATCH] fix(): remove _repository_args from nodejs_binary --- docs/Built-ins.md | 16 +---- e2e/bazel_managed_deps/BUILD.bazel | 1 + e2e/fine_grained_symlinks/BUILD.bazel | 1 + e2e/jasmine/BUILD.bazel | 2 + .../WORKSPACE | 2 +- e2e/node_loader_preserve_symlinks/BUILD.bazel | 1 + e2e/nodejs_image/BUILD.bazel | 58 ++++++++++++++++++- e2e/nodejs_image/WORKSPACE | 22 ++++++- e2e/nodejs_image/binary_version.js | 1 + e2e/nodejs_image/rules_docker.patch | 30 ++++++++++ e2e/nodejs_repository/BUILD.bazel | 1 + e2e/packages/BUILD.bazel | 15 ++++- e2e/symlinked_node_modules_npm/BUILD | 1 + e2e/symlinked_node_modules_yarn/BUILD | 1 + e2e/webapp/BUILD.bazel | 1 + examples/angular/WORKSPACE | 8 ++- examples/angular/rules_docker.patch | 30 ++++++++++ examples/angular/src/BUILD.bazel | 1 + examples/nestjs/WORKSPACE | 8 ++- examples/nestjs/rules_docker.patch | 30 ++++++++++ examples/nestjs/src/BUILD.bazel | 1 + .../bazel_integration_test/test_runner.js | 1 - internal/node/launcher.sh | 7 +-- internal/node/node.bzl | 9 +-- internal/node/node_patches.js | 6 +- nodejs/BUILD.bazel | 12 ++++ nodejs/private/nodejs_repo_host_os_alias.bzl | 1 - .../private/providers/user_build_settings.bzl | 3 + nodejs/private/user_build_settings.bzl | 33 +++++++++++ nodejs/providers.bzl | 5 ++ nodejs/repositories.bzl | 31 +--------- packages/node-patches/src/subprocess.ts | 6 +- 32 files changed, 270 insertions(+), 75 deletions(-) create mode 100644 e2e/nodejs_image/binary_version.js create mode 100644 e2e/nodejs_image/rules_docker.patch create mode 100644 examples/angular/rules_docker.patch create mode 100644 examples/nestjs/rules_docker.patch create mode 100644 nodejs/private/providers/user_build_settings.bzl create mode 100644 nodejs/private/user_build_settings.bzl diff --git a/docs/Built-ins.md b/docs/Built-ins.md index 050c279c87..19b8548e80 100755 --- a/docs/Built-ins.md +++ b/docs/Built-ins.md @@ -19,8 +19,8 @@ This is necessary to bootstrap Bazel to run the package manager to download othe 
 node_repositories(name, node_download_auth, node_repositories, node_urls, node_version,
-                  package_json, platform, preserve_symlinks, repo_mapping, use_nvmrc, vendored_node,
-                  vendored_yarn, yarn_download_auth, yarn_repositories, yarn_urls, yarn_version)
+                  package_json, platform, repo_mapping, use_nvmrc, vendored_node, vendored_yarn,
+                  yarn_download_auth, yarn_repositories, yarn_urls, yarn_version)
To be run in user's WORKSPACE to install rules_nodejs dependencies. @@ -184,18 +184,6 @@ Defaults to `[]` Defaults to `""` - - -(*Boolean*): Turn on --node_options=--preserve-symlinks for nodejs_binary and nodejs_test rules. - -When this option is turned on, node will preserve the symlinked path for resolves instead of the default -behavior of resolving to the real path. This means that all required files must be in be included in your -runfiles as it prevents the default behavior of potentially resolving outside of the runfiles. For example, -all required files need to be included in your node_modules filegroup. This option is desirable as it gives -a stronger guarantee of hermeticity which is required for remote execution. - -Defaults to `True` -

repo_mapping

(*Dictionary: String -> String, mandatory*): A dictionary from local repository name to global repository name. This allows controls over workspace dependency resolution for dependencies of this repository.

For example, an entry `"@foo": "@bar"` declares that, for any time this repository depends on `@foo` (such as a dependency on `@foo//some:target`, it should actually resolve that dependency within globally-declared `@bar` (`@bar//some:target`). diff --git a/e2e/bazel_managed_deps/BUILD.bazel b/e2e/bazel_managed_deps/BUILD.bazel index a93cfe4870..f1ad0145d0 100644 --- a/e2e/bazel_managed_deps/BUILD.bazel +++ b/e2e/bazel_managed_deps/BUILD.bazel @@ -7,6 +7,7 @@ jasmine_node_test( name = "fine_grained_test", srcs = glob(["*.spec.js"]), data = ["@npm//:bin_files"], + templated_args = ["--node_options=--preserve-symlinks"], deps = [ "@npm//jasmine", "@npm//typescript", diff --git a/e2e/fine_grained_symlinks/BUILD.bazel b/e2e/fine_grained_symlinks/BUILD.bazel index 341c961dee..39426bbf9c 100644 --- a/e2e/fine_grained_symlinks/BUILD.bazel +++ b/e2e/fine_grained_symlinks/BUILD.bazel @@ -9,4 +9,5 @@ nodejs_test( "@npm//webpack", ], entry_point = ":index.spec.js", + templated_args = ["--node_options=--preserve-symlinks"], ) diff --git a/e2e/jasmine/BUILD.bazel b/e2e/jasmine/BUILD.bazel index 550db6e5d2..94bc286f66 100644 --- a/e2e/jasmine/BUILD.bazel +++ b/e2e/jasmine/BUILD.bazel @@ -3,6 +3,7 @@ load("@npm//@bazel/jasmine:index.bzl", "jasmine_node_test") jasmine_node_test( name = "test", srcs = ["test.spec.js"], + templated_args = ["--node_options=--preserve-symlinks"], ) jasmine_node_test( @@ -11,6 +12,7 @@ jasmine_node_test( data = ["jasmine_shared_env_bootstrap.js"], templated_args = [ "--node_options=--require=$$(rlocation $(rootpath :jasmine_shared_env_bootstrap.js))", + "--node_options=--preserve-symlinks", ], deps = [ "@npm//jasmine", diff --git a/e2e/node_loader_no_preserve_symlinks/WORKSPACE b/e2e/node_loader_no_preserve_symlinks/WORKSPACE index a9afeca45b..aad72269d9 100644 --- a/e2e/node_loader_no_preserve_symlinks/WORKSPACE +++ b/e2e/node_loader_no_preserve_symlinks/WORKSPACE @@ -19,7 +19,7 @@ rules_nodejs_dependencies() load("@build_bazel_rules_nodejs//:index.bzl", "node_repositories", "yarn_install") -node_repositories(preserve_symlinks = False) +node_repositories() yarn_install( name = "npm", diff --git a/e2e/node_loader_preserve_symlinks/BUILD.bazel b/e2e/node_loader_preserve_symlinks/BUILD.bazel index a50c3b926e..cfdcecc411 100644 --- a/e2e/node_loader_preserve_symlinks/BUILD.bazel +++ b/e2e/node_loader_preserve_symlinks/BUILD.bazel @@ -9,4 +9,5 @@ nodejs_test( "@npm//:node_modules", ], entry_point = ":node_loader_test.spec.js", + templated_args = ["--node_options=--preserve-symlinks"], ) diff --git a/e2e/nodejs_image/BUILD.bazel b/e2e/nodejs_image/BUILD.bazel index b3574ce082..7d6ead07ac 100644 --- a/e2e/nodejs_image/BUILD.bazel +++ b/e2e/nodejs_image/BUILD.bazel @@ -1,6 +1,7 @@ load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary") load("@io_bazel_rules_docker//contrib:test.bzl", "container_test") load("@io_bazel_rules_docker//nodejs:image.bzl", "nodejs_image") +load("@bazel_skylib//rules:write_file.bzl", "write_file") nodejs_binary( name = "main", @@ -11,10 +12,11 @@ nodejs_binary( entry_point = "main.js", ) -# bazel run --platforms=@rules_nodejs//nodejs:linux_amd64 //:nodejs_image +# bazel run --platforms=@build_bazel_rules_nodejs//toolchains/node:linux_amd64 //:nodejs_image nodejs_image( name = "nodejs_image", binary = ":main", + include_node_repo_args = False, ) container_test( @@ -22,3 +24,57 @@ container_test( configs = [":nodejs_image.yaml"], image = ":nodejs_image", ) + +# this creates: +# bazel run main_node15_linux_amd64 +# bazel run main_node16_linux_amd64 +# bazel run nodejs_image_node15_linux_amd64 +# bazel run nodejs_image_node16_linux_amd64 +# bazel test nodejs_image_test_node15_linux_amd64 +# bazel test nodejs_image_test_node16_linux_amd64 +[ + [ + # Trivial test fixture: a nodejs program that writes to a file + write_file( + name = "test_" + id, + out = "image_%s.yaml" % id, + content = [ + "schemaVersion: 2.0.0", + "metadataTest:", + " entrypoint: ['/app//main_%s']" % id, + """ workdir: "/app//main_%s.runfiles/e2e_nodejs_image" """ % id, + ], + ), + nodejs_binary( + name = "main_" + id, + data = [ + "//foolib", + "@npm//date-fns", + ], + entry_point = "binary_version.js", + node = toolchain, + toolchain = toolchain, + ), + nodejs_image( + name = "nodejs_image_" + id, + binary = ":main_" + id, + include_node_repo_args = False, + node_repository_name = id, + ), + container_test( + name = "nodejs_image_test_" + id, + configs = ["test_" + id], + image = ":nodejs_image_" + id, + ), + ] + for id, toolchain in zip( + [ + "node15_linux_amd64", + "node16_linux_amd64", + ], + [ + "@node15_linux_amd64//:node_toolchain", + "@node16_linux_amd64//:node_toolchain", + ], + ) +] diff --git a/e2e/nodejs_image/WORKSPACE b/e2e/nodejs_image/WORKSPACE index d1cb269633..4aa2fb0c58 100644 --- a/e2e/nodejs_image/WORKSPACE +++ b/e2e/nodejs_image/WORKSPACE @@ -43,9 +43,11 @@ yarn_install( http_archive( name = "io_bazel_rules_docker", - sha256 = "20dfc1d9bc6a251455c4f306d3dea030b323ea3f22e0600698fab9665eb273b8", - strip_prefix = "rules_docker-0086a29d19b124b04a864e3913cfa7a2cba46349", - urls = ["https://github.com/bazelbuild/rules_docker/archive/0086a29d19b124b04a864e3913cfa7a2cba46349.tar.gz"], + patch_args = ["-p1"], + patches = ["//:rules_docker.patch"], + sha256 = "9f4948332e5fa34255c71e605ed49cc6fb684315f713cdff18ecb1fac21b67f2", + strip_prefix = "rules_docker-12de489abc70d7bb1ed00ab4b2cf431d46eccd00", + urls = ["https://github.com/bazelbuild/rules_docker/archive/12de489abc70d7bb1ed00ab4b2cf431d46eccd00.tar.gz"], ) load("@io_bazel_rules_docker//repositories:repositories.bzl", container_repositories = "repositories") @@ -59,3 +61,17 @@ container_deps() load("@io_bazel_rules_docker//nodejs:image.bzl", nodejs_image_repositories = "repositories") nodejs_image_repositories() + +load("@rules_nodejs//nodejs:repositories.bzl", "nodejs_register_toolchains") + +# The order matters because Bazel will provide the first registered toolchain when a rule asks Bazel to select it +# This applies to the resolved_toolchain +nodejs_register_toolchains( + name = "node16", + node_version = "16.5.0", +) + +nodejs_register_toolchains( + name = "node15", + node_version = "15.14.0", +) diff --git a/e2e/nodejs_image/binary_version.js b/e2e/nodejs_image/binary_version.js new file mode 100644 index 0000000000..bf96b9dbe5 --- /dev/null +++ b/e2e/nodejs_image/binary_version.js @@ -0,0 +1 @@ +console.log(process.version) \ No newline at end of file diff --git a/e2e/nodejs_image/rules_docker.patch b/e2e/nodejs_image/rules_docker.patch new file mode 100644 index 0000000000..5afb94b067 --- /dev/null +++ b/e2e/nodejs_image/rules_docker.patch @@ -0,0 +1,30 @@ +diff --git a/nodejs/image.bzl b/nodejs/image.bzl +index 7c72596..0dac854 100644 +--- a/nodejs/image.bzl ++++ b/nodejs/image.bzl +@@ -117,6 +117,8 @@ def nodejs_image( + binary = None, + launcher = None, + launcher_args = None, ++ node_repository_name = "nodejs", ++ include_node_repo_args = True, + **kwargs): + """Constructs a container image wrapping a nodejs_binary target. + +@@ -142,11 +144,13 @@ def nodejs_image( + + nodejs_layers = [ + # Put the Node binary into its own layers. +- "@nodejs//:node", +- "@nodejs//:node_files", +- "@nodejs//:bin/node_repo_args.sh", ++ "@%s//:node" % node_repository_name, ++ "@%s//:node_files" % node_repository_name, + ] + ++ if include_node_repo_args: ++ nodejs_layers.append("@%s//:bin/node_repo_args.sh" % node_repository_name) ++ + all_layers = nodejs_layers + layers + + visibility = kwargs.get("visibility", None) diff --git a/e2e/nodejs_repository/BUILD.bazel b/e2e/nodejs_repository/BUILD.bazel index cb42cd13f1..caac6813f1 100644 --- a/e2e/nodejs_repository/BUILD.bazel +++ b/e2e/nodejs_repository/BUILD.bazel @@ -21,4 +21,5 @@ nodejs_test( "@nodejs//:yarn_node_repositories", ], entry_point = ":index.spec.js", + templated_args = ["--node_options=--preserve-symlinks"], ) diff --git a/e2e/packages/BUILD.bazel b/e2e/packages/BUILD.bazel index 57f8e60c65..c9347a3796 100644 --- a/e2e/packages/BUILD.bazel +++ b/e2e/packages/BUILD.bazel @@ -27,7 +27,10 @@ VARIANTS = [ # syscall: 'mkdir', # path: 'C:\\users\\b\\_bazel_b\\p4se2lwa\\execroot\\e2e_packages\\node_modules' # } - templated_args = ["--nobazel_run_linker"], + templated_args = [ + "--nobazel_run_linker", + "--node_options=--preserve-symlinks", + ], ) for variant in VARIANTS] nodejs_test( @@ -41,7 +44,10 @@ nodejs_test( # TODO(gregmagolan): fix this test on windows tags = ["fix-windows"], # TODO: use runfiles - templated_args = ["--bazel_patch_module_resolver"], + templated_args = [ + "--bazel_patch_module_resolver", + "--node_options=--preserve-symlinks", + ], ) nodejs_test( @@ -55,5 +61,8 @@ nodejs_test( # TODO(gregmagolan): fix this test on windows tags = ["fix-windows"], # TODO: use runfiles - templated_args = ["--bazel_patch_module_resolver"], + templated_args = [ + "--bazel_patch_module_resolver", + "--node_options=--preserve-symlinks", + ], ) diff --git a/e2e/symlinked_node_modules_npm/BUILD b/e2e/symlinked_node_modules_npm/BUILD index 8476401c46..4534e0c80c 100644 --- a/e2e/symlinked_node_modules_npm/BUILD +++ b/e2e/symlinked_node_modules_npm/BUILD @@ -6,4 +6,5 @@ nodejs_test( "@npm//typescript", ], entry_point = ":main.js", + templated_args = ["--node_options=--preserve-symlinks"], ) diff --git a/e2e/symlinked_node_modules_yarn/BUILD b/e2e/symlinked_node_modules_yarn/BUILD index 8476401c46..4534e0c80c 100644 --- a/e2e/symlinked_node_modules_yarn/BUILD +++ b/e2e/symlinked_node_modules_yarn/BUILD @@ -6,4 +6,5 @@ nodejs_test( "@npm//typescript", ], entry_point = ":main.js", + templated_args = ["--node_options=--preserve-symlinks"], ) diff --git a/e2e/webapp/BUILD.bazel b/e2e/webapp/BUILD.bazel index af96a1e97f..5c0ea3a4b1 100644 --- a/e2e/webapp/BUILD.bazel +++ b/e2e/webapp/BUILD.bazel @@ -21,4 +21,5 @@ nodejs_test( name = "test", data = ["out.min"], entry_point = ":test.js", + templated_args = ["--node_options=--preserve-symlinks"], ) diff --git a/examples/angular/WORKSPACE b/examples/angular/WORKSPACE index f424ebf6e8..bd0b3b6e23 100644 --- a/examples/angular/WORKSPACE +++ b/examples/angular/WORKSPACE @@ -117,9 +117,11 @@ http_archive( http_archive( name = "io_bazel_rules_docker", - sha256 = "c27ab432594e793eb864604ec0e4cfd708285218da663b805eefdd479378da93", - strip_prefix = "rules_docker-2b35b2dd56f0be6cc6b8df957332a31435f6b3ce", - urls = ["https://github.com/bazelbuild/rules_docker/archive/2b35b2dd56f0be6cc6b8df957332a31435f6b3ce.tar.gz"], + patch_args = ["-p1"], + patches = ["//:rules_docker.patch"], + sha256 = "9f4948332e5fa34255c71e605ed49cc6fb684315f713cdff18ecb1fac21b67f2", + strip_prefix = "rules_docker-12de489abc70d7bb1ed00ab4b2cf431d46eccd00", + urls = ["https://github.com/bazelbuild/rules_docker/archive/12de489abc70d7bb1ed00ab4b2cf431d46eccd00.tar.gz"], ) load("@io_bazel_rules_docker//repositories:repositories.bzl", container_repositories = "repositories") diff --git a/examples/angular/rules_docker.patch b/examples/angular/rules_docker.patch new file mode 100644 index 0000000000..5afb94b067 --- /dev/null +++ b/examples/angular/rules_docker.patch @@ -0,0 +1,30 @@ +diff --git a/nodejs/image.bzl b/nodejs/image.bzl +index 7c72596..0dac854 100644 +--- a/nodejs/image.bzl ++++ b/nodejs/image.bzl +@@ -117,6 +117,8 @@ def nodejs_image( + binary = None, + launcher = None, + launcher_args = None, ++ node_repository_name = "nodejs", ++ include_node_repo_args = True, + **kwargs): + """Constructs a container image wrapping a nodejs_binary target. + +@@ -142,11 +144,13 @@ def nodejs_image( + + nodejs_layers = [ + # Put the Node binary into its own layers. +- "@nodejs//:node", +- "@nodejs//:node_files", +- "@nodejs//:bin/node_repo_args.sh", ++ "@%s//:node" % node_repository_name, ++ "@%s//:node_files" % node_repository_name, + ] + ++ if include_node_repo_args: ++ nodejs_layers.append("@%s//:bin/node_repo_args.sh" % node_repository_name) ++ + all_layers = nodejs_layers + layers + + visibility = kwargs.get("visibility", None) diff --git a/examples/angular/src/BUILD.bazel b/examples/angular/src/BUILD.bazel index 618ec4ca02..e5f79450c1 100644 --- a/examples/angular/src/BUILD.bazel +++ b/examples/angular/src/BUILD.bazel @@ -223,6 +223,7 @@ history_server( nodejs_image( name = "nodejs_image", binary = ":prodserver", + include_node_repo_args = False, # Actions created by this rule are I/O-bound, # so there is no benefit to running them remotely tags = ["local"], diff --git a/examples/nestjs/WORKSPACE b/examples/nestjs/WORKSPACE index 1e72c9467e..a18ee41113 100644 --- a/examples/nestjs/WORKSPACE +++ b/examples/nestjs/WORKSPACE @@ -43,9 +43,11 @@ yarn_install( http_archive( name = "io_bazel_rules_docker", - sha256 = "c27ab432594e793eb864604ec0e4cfd708285218da663b805eefdd479378da93", - strip_prefix = "rules_docker-2b35b2dd56f0be6cc6b8df957332a31435f6b3ce", - urls = ["https://github.com/bazelbuild/rules_docker/archive/2b35b2dd56f0be6cc6b8df957332a31435f6b3ce.tar.gz"], + patch_args = ["-p1"], + patches = ["//:rules_docker.patch"], + sha256 = "9f4948332e5fa34255c71e605ed49cc6fb684315f713cdff18ecb1fac21b67f2", + strip_prefix = "rules_docker-12de489abc70d7bb1ed00ab4b2cf431d46eccd00", + urls = ["https://github.com/bazelbuild/rules_docker/archive/12de489abc70d7bb1ed00ab4b2cf431d46eccd00.tar.gz"], ) load( diff --git a/examples/nestjs/rules_docker.patch b/examples/nestjs/rules_docker.patch new file mode 100644 index 0000000000..5afb94b067 --- /dev/null +++ b/examples/nestjs/rules_docker.patch @@ -0,0 +1,30 @@ +diff --git a/nodejs/image.bzl b/nodejs/image.bzl +index 7c72596..0dac854 100644 +--- a/nodejs/image.bzl ++++ b/nodejs/image.bzl +@@ -117,6 +117,8 @@ def nodejs_image( + binary = None, + launcher = None, + launcher_args = None, ++ node_repository_name = "nodejs", ++ include_node_repo_args = True, + **kwargs): + """Constructs a container image wrapping a nodejs_binary target. + +@@ -142,11 +144,13 @@ def nodejs_image( + + nodejs_layers = [ + # Put the Node binary into its own layers. +- "@nodejs//:node", +- "@nodejs//:node_files", +- "@nodejs//:bin/node_repo_args.sh", ++ "@%s//:node" % node_repository_name, ++ "@%s//:node_files" % node_repository_name, + ] + ++ if include_node_repo_args: ++ nodejs_layers.append("@%s//:bin/node_repo_args.sh" % node_repository_name) ++ + all_layers = nodejs_layers + layers + + visibility = kwargs.get("visibility", None) diff --git a/examples/nestjs/src/BUILD.bazel b/examples/nestjs/src/BUILD.bazel index b4058db55e..2b4d32dec8 100644 --- a/examples/nestjs/src/BUILD.bazel +++ b/examples/nestjs/src/BUILD.bazel @@ -77,4 +77,5 @@ jasmine_node_test( nodejs_image( name = "docker", binary = ":server", + include_node_repo_args = False, ) diff --git a/internal/bazel_integration_test/test_runner.js b/internal/bazel_integration_test/test_runner.js index 81a1521f57..5d6579150e 100644 --- a/internal/bazel_integration_test/test_runner.js +++ b/internal/bazel_integration_test/test_runner.js @@ -384,7 +384,6 @@ for (const bazelCommand of config.bazelCommands) { 'GTEST_TMP_DIR', 'INIT_CWD', 'JAVA_RUNFILES', - 'NODE_REPOSITORY_ARGS', 'OLDPWD', 'PYTHON_RUNFILES', 'RUN_UNDER_RUNFILES', diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh index fd832d0203..e154c5ddf8 100644 --- a/internal/node/launcher.sh +++ b/internal/node/launcher.sh @@ -170,13 +170,11 @@ if [[ "${BAZEL_NODE_PATCH_REQUIRE}" != /* ]] && [[ ! "${BAZEL_NODE_PATCH_REQUIRE export BAZEL_NODE_PATCH_REQUIRE=$(pwd)/${BAZEL_NODE_PATCH_REQUIRE} fi -readonly repository_args=$(rlocation "TEMPLATED_repository_args") readonly lcov_merger_script=$(rlocation "TEMPLATED_lcov_merger_script") -source $repository_args - ARGS=() -LAUNCHER_NODE_OPTIONS=($NODE_REPOSITORY_ARGS) +# this flag can be set on the command line as needed +LAUNCHER_NODE_OPTIONS=("TEMPLATED_node_args") USER_NODE_OPTIONS=() ALL_ARGS=(TEMPLATED_args "$@") STDOUT_CAPTURE="" @@ -390,7 +388,6 @@ if [[ -n "$NODE_WORKING_DIR" ]]; then cd "$NODE_WORKING_DIR" fi set +e - if [[ -n "${STDOUT_CAPTURE}" ]] && [[ -n "${STDERR_CAPTURE}" ]]; then "${node}" ${LAUNCHER_NODE_OPTIONS[@]+"${LAUNCHER_NODE_OPTIONS[@]}"} ${USER_NODE_OPTIONS[@]+"${USER_NODE_OPTIONS[@]}"} "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} <&0 >$STDOUT_CAPTURE 2>$STDERR_CAPTURE & elif [[ -n "${STDOUT_CAPTURE}" ]]; then diff --git a/internal/node/node.bzl b/internal/node/node.bzl index a874c54c90..493a4406dc 100644 --- a/internal/node/node.bzl +++ b/internal/node/node.bzl @@ -28,6 +28,7 @@ load("//internal/common:path_utils.bzl", "strip_external") load("//internal/common:preserve_legacy_templated_args.bzl", "preserve_legacy_templated_args") load("//internal/common:windows_utils.bzl", "create_windows_native_launcher_script", "is_windows") load("//internal/linker:link_node_modules.bzl", "LinkerPackageMappingInfo", "module_mappings_aspect", "write_node_modules_manifest") +load("//nodejs:providers.bzl", "UserBuildSettingInfo") def _trim_package_node_modules(package_name): # trim a package name down to its path prior to a node_modules @@ -267,7 +268,6 @@ fi runfiles.extend(ctx.files._bash_runfile_helper) runfiles.append(ctx.outputs.loader_script) runfiles.append(ctx.outputs.require_patch_script) - runfiles.append(ctx.file._repository_args) # First replace any instances of "$(rlocation " with "$$(rlocation " to preserve # legacy uses of "$(rlocation" @@ -316,10 +316,10 @@ fi "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_repository_args": _to_manifest_path(ctx, ctx.file._repository_args), "TEMPLATED_require_patch_script": _to_manifest_path(ctx, ctx.outputs.require_patch_script), "TEMPLATED_runfiles_helper_script": _to_manifest_path(ctx, ctx.file._runfile_helpers_main), "TEMPLATED_vendored_node": strip_external(node_toolchain.nodeinfo.target_tool_path), + "TEMPLATED_node_args": ctx.attr._node_args[UserBuildSettingInfo].value, } # TODO when we have "link_all_bins" we will only need to look in one place for the entry point @@ -594,10 +594,7 @@ Predefined genrule variables are not supported in this context. allow_single_file = True, ), "toolchain": attr.label(), - "_repository_args": attr.label( - default = Label("@nodejs//:bin/node_repo_args.sh"), - allow_single_file = True, - ), + "_node_args": attr.label(default = "//nodejs:default_args"), "_node_patches_script": attr.label( default = Label("//internal/node:node_patches.js"), allow_single_file = True, diff --git a/internal/node/node_patches.js b/internal/node/node_patches.js index 7b13064769..239bb5f581 100644 --- a/internal/node/node_patches.js +++ b/internal/node/node_patches.js @@ -570,7 +570,7 @@ const patcher = (requireScriptName, nodeDir) => { fs__default['default'].writeFileSync(nodeEntry, `@if not defined DEBUG_HELPER @ECHO OFF set NP_SUBPROCESS_NODE_DIR=${nodeDir} set Path=${nodeDir};%Path% -"${process.execPath}" ${process.env.NODE_REPOSITORY_ARGS} --require "${requireScriptName}" %* +"${process.execPath}" --require "${requireScriptName}" %* `); } } @@ -581,9 +581,9 @@ set Path=${nodeDir};%Path% export NP_SUBPROCESS_NODE_DIR="${nodeDir}" export PATH="${nodeDir}":\$PATH if [[ ! "\${@}" =~ "${file}" ]]; then - exec ${process.execPath} ${process.env.NODE_REPOSITORY_ARGS} --require "${requireScriptName}" "$@" + exec ${process.execPath} --require "${requireScriptName}" "$@" else - exec ${process.execPath} ${process.env.NODE_REPOSITORY_ARGS} "$@" + exec ${process.execPath} "$@" fi `, { mode: 0o777 }); } diff --git a/nodejs/BUILD.bazel b/nodejs/BUILD.bazel index ddef6c8e90..87220019cd 100644 --- a/nodejs/BUILD.bazel +++ b/nodejs/BUILD.bazel @@ -1,6 +1,10 @@ load("@bazel_skylib//:bzl_library.bzl", "bzl_library") load("//nodejs/private:toolchains_repo.bzl", "PLATFORMS") +# bazel test --//nodejs:default_args="" //... +# allowed args ["--preserve-symlinks", ""] +load("//nodejs/private:user_build_settings.bzl", "user_args") + filegroup( name = "package_contents", srcs = glob(["*"]) + [ @@ -36,3 +40,11 @@ toolchain_type( ) for key, values in PLATFORMS.items() ] + +# User set flags that are passed to the build + +user_args( + name = "default_args", + build_setting_default = "--preserve-symlinks", + visibility = ["//visibility:public"], +) diff --git a/nodejs/private/nodejs_repo_host_os_alias.bzl b/nodejs/private/nodejs_repo_host_os_alias.bzl index 38b95be7f9..814a96bea1 100644 --- a/nodejs/private/nodejs_repo_host_os_alias.bzl +++ b/nodejs/private/nodejs_repo_host_os_alias.bzl @@ -9,7 +9,6 @@ package(default_visibility = ["//visibility:public"]) # aliases for exports_files alias(name = "run_npm.sh.template", actual = "@{node_repository}_{os_name}//:run_npm.sh.template") alias(name = "run_npm.bat.template", actual = "@{node_repository}_{os_name}//:run_npm.bat.template") -alias(name = "bin/node_repo_args.sh", actual = "@{node_repository}_{os_name}//:bin/node_repo_args.sh") # aliases for other aliases alias(name = "node_bin", actual = "@{node_repository}_{os_name}//:node_bin") diff --git a/nodejs/private/providers/user_build_settings.bzl b/nodejs/private/providers/user_build_settings.bzl new file mode 100644 index 0000000000..1d5c4b3d14 --- /dev/null +++ b/nodejs/private/providers/user_build_settings.bzl @@ -0,0 +1,3 @@ +"""To pass information about the user defined flags +""" +UserBuildSettingInfo = provider(fields = ["value"]) diff --git a/nodejs/private/user_build_settings.bzl b/nodejs/private/user_build_settings.bzl new file mode 100644 index 0000000000..fcc01eb529 --- /dev/null +++ b/nodejs/private/user_build_settings.bzl @@ -0,0 +1,33 @@ +"""Function for user args to be set on the command line +""" + +load("//nodejs/private/providers:user_build_settings.bzl", "UserBuildSettingInfo") + +# this can be used to limit user input for flags and also provide useful error messages for incorrect flags +user_args_allowed = ["--preserve-symlinks", ""] + +def _impl(ctx): + # use `ctx.build_setting_value` to access the raw value + # of this build setting. This value is either derived from + # the default value set on the target or from the setting + # being set somewhere on the command line/in a transition, etc. + raw_user_args = ctx.build_setting_value + + # Things you can do in a build setting implementation function + # include more advanced type checking + if raw_user_args not in user_args_allowed: + fail(str(ctx.label) + " build setting allowed to take values {" + + ", ".join(user_args_allowed) + "} but was set to unallowed value " + + raw_user_args) + + # Returns a provider like a normal rule + return UserBuildSettingInfo(value = raw_user_args) + +user_args = rule( + implementation = _impl, + # This line separates a build setting from a regular target, by using + # the `build_setting` atttribute, you mark this rule as a build setting + # including what raw type it is and if it can be used on the command + # line or not (if yes, you must set `flag = True`) + build_setting = config.string(flag = True), +) diff --git a/nodejs/providers.bzl b/nodejs/providers.bzl index 39efbcbd1c..84f9473c92 100644 --- a/nodejs/providers.bzl +++ b/nodejs/providers.bzl @@ -20,6 +20,10 @@ load( "//nodejs/private/providers:directory_file_path_info.bzl", _DirectoryFilePathInfo = "DirectoryFilePathInfo", ) +load( + "//nodejs/private/providers:user_build_settings.bzl", + _UserBuildSettingInfo = "UserBuildSettingInfo", +) DeclarationInfo = _DeclarationInfo declaration_info = _declaration_info @@ -27,3 +31,4 @@ JSModuleInfo = _JSModuleInfo js_module_info = _js_module_info LinkablePackageInfo = _LinkablePackageInfo DirectoryFilePathInfo = _DirectoryFilePathInfo +UserBuildSettingInfo = _UserBuildSettingInfo diff --git a/nodejs/repositories.bzl b/nodejs/repositories.bzl index 09b3ababae..da70281d16 100644 --- a/nodejs/repositories.bzl +++ b/nodejs/repositories.bzl @@ -165,16 +165,6 @@ If set then also set node_version to the version found in the .nvmrc file.""", doc = "Internal use only. Which platform to install as a toolchain. If unset, we assume the repository is named nodejs_[platform]", values = BUILT_IN_NODE_PLATFORMS, ), - "preserve_symlinks": attr.bool( - default = True, - doc = """Turn on --node_options=--preserve-symlinks for nodejs_binary and nodejs_test rules. - -When this option is turned on, node will preserve the symlinked path for resolves instead of the default -behavior of resolving to the real path. This means that all required files must be in be included in your -runfiles as it prevents the default behavior of potentially resolving outside of the runfiles. For example, -all required files need to be included in your node_modules filegroup. This option is desirable as it gives -a stronger guarantee of hermeticity which is required for remote execution.""", - ), "vendored_node": attr.label( allow_single_file = True, doc = """the local path to a pre-installed NodeJS runtime. @@ -435,11 +425,6 @@ def _prepare_node(repository_ctx): npm_script_relative = npm_script if repository_ctx.attr.vendored_node else paths.relativize(npm_script, "bin") yarn_script_relative = yarn_script if repository_ctx.attr.vendored_yarn else paths.relativize(yarn_script, "bin") - if repository_ctx.attr.preserve_symlinks: - node_args = "--preserve-symlinks" - else: - node_args = "" - # The entry points for node for osx/linux and windows if not is_windows: # Sets PATH and runs the application @@ -449,11 +434,10 @@ def _prepare_node(repository_ctx): set -e {get_script_dir} export PATH="$SCRIPT_DIR":$PATH -exec "$SCRIPT_DIR/{node}" {args} "$@" +exec "$SCRIPT_DIR/{node}" "$@" """.format( get_script_dir = GET_SCRIPT_DIR, node = node_bin_relative, - args = node_args, )) else: # Sets PATH for node, npm & yarn and run user script @@ -461,16 +445,8 @@ exec "$SCRIPT_DIR/{node}" {args} "$@" @echo off SET SCRIPT_DIR=%~dp0 SET PATH=%SCRIPT_DIR%;%PATH% -CALL "%SCRIPT_DIR%\\{node}" {args} %* -""".format(node = node_bin_relative, args = node_args)) - - # Shell script to set repository arguments for node used by nodejs_binary & nodejs_test launcher - repository_ctx.file("bin/node_repo_args.sh", content = """#!/usr/bin/env bash -# Immediately exit if any command fails. -set -e -# Generated by node_repositories.bzl -export NODE_REPOSITORY_ARGS="{args}" -""".format(args = node_args), executable = True) +CALL "%SCRIPT_DIR%\\{node}" %* +""".format(node = node_bin_relative)) # The entry points for npm for osx/linux and windows # Runs npm using appropriate node entry point @@ -642,7 +618,6 @@ package(default_visibility = ["//visibility:public"]) exports_files([ "run_npm.sh.template", "run_npm.bat.template", - "bin/node_repo_args.sh",{node_bin_export}{npm_bin_export}{npx_bin_export}{yarn_bin_export} "{node_entry}", "{npm_entry}", "{yarn_entry}", diff --git a/packages/node-patches/src/subprocess.ts b/packages/node-patches/src/subprocess.ts index 8242f734c2..71f7afa6a5 100644 --- a/packages/node-patches/src/subprocess.ts +++ b/packages/node-patches/src/subprocess.ts @@ -22,7 +22,7 @@ export const patcher = (requireScriptName: string, nodeDir?: string) => { fs.writeFileSync(nodeEntry, `@if not defined DEBUG_HELPER @ECHO OFF set NP_SUBPROCESS_NODE_DIR=${nodeDir} set Path=${nodeDir};%Path% -"${process.execPath}" ${process.env.NODE_REPOSITORY_ARGS} --require "${requireScriptName}" %* +"${process.execPath}" --require "${requireScriptName}" %* `); } } else { @@ -33,9 +33,9 @@ set Path=${nodeDir};%Path% export NP_SUBPROCESS_NODE_DIR="${nodeDir}" export PATH="${nodeDir}":\$PATH if [[ ! "\${@}" =~ "${file}" ]]; then - exec ${process.execPath} ${process.env.NODE_REPOSITORY_ARGS} --require "${requireScriptName}" "$@" + exec ${process.execPath} --require "${requireScriptName}" "$@" else - exec ${process.execPath} ${process.env.NODE_REPOSITORY_ARGS} "$@" + exec ${process.execPath} "$@" fi `, {mode: 0o777});