From f96cfde7f7dc3fb25e3e7a010619e4e3705c9b1f Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Thu, 9 Jun 2022 09:40:46 -0700 Subject: [PATCH] chore: remove usage of managed_directories (#3466) * chore: remove usage of managed_directories We discouraged users from using this as of rnj 5.0, and now Bazel is removing the feature. Fixes #3465 * Also remove symlink_node_modules = True usage --- WORKSPACE | 7 ---- docs/Built-ins.md | 10 ++++-- docs/dependencies.md | 34 +++++++++---------- e2e/BUILD.bazel | 7 ---- .../.bazelignore | 1 - e2e/node_loader_no_preserve_symlinks/.bazelrc | 3 -- .../BUILD.bazel | 13 ------- .../WORKSPACE | 32 ----------------- .../node_loader_test.spec.js | 24 ------------- .../package.json | 12 ------- .../yarn.lock | 32 ----------------- internal/npm_install/npm_install.bzl | 5 ++- npm_deps.bzl | 4 --- 13 files changed, 28 insertions(+), 156 deletions(-) delete mode 100644 e2e/node_loader_no_preserve_symlinks/.bazelignore delete mode 100644 e2e/node_loader_no_preserve_symlinks/.bazelrc delete mode 100644 e2e/node_loader_no_preserve_symlinks/BUILD.bazel delete mode 100644 e2e/node_loader_no_preserve_symlinks/WORKSPACE delete mode 100644 e2e/node_loader_no_preserve_symlinks/node_loader_test.spec.js delete mode 100644 e2e/node_loader_no_preserve_symlinks/package.json delete mode 100644 e2e/node_loader_no_preserve_symlinks/yarn.lock diff --git a/WORKSPACE b/WORKSPACE index fd99df20e5..0781c6b756 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -14,13 +14,6 @@ workspace( name = "build_bazel_rules_nodejs", - managed_directories = { - # cypress_deps must be a managed directory to ensure it is downloaded before cypress_repositories is run. - "@cypress_deps": ["packages/cypress/test/node_modules"], - "@internal_npm_install_test_patches_npm_symlinked": ["internal/npm_install/test/patches_npm_symlinked/node_modules"], - "@internal_npm_install_test_patches_yarn_symlinked": ["internal/npm_install/test/patches_yarn_symlinked/node_modules"], - "@npm": ["node_modules"], - }, ) # diff --git a/docs/Built-ins.md b/docs/Built-ins.md index 40ac77dfa2..f12011ef65 100755 --- a/docs/Built-ins.md +++ b/docs/Built-ins.md @@ -872,7 +872,10 @@ Defaults to `True` -(*Boolean*): Turn symlinking of node_modules on +(*Boolean*): Turn symlinking of node_modules on. + +**Use of this feature is not recommended, as Bazel is removing `managed_directories`. +See https://github.com/bazelbuild/bazel/issues/15463 When False, the package manager will run in the external repository created by this rule. @@ -1537,7 +1540,10 @@ Defaults to `True` -(*Boolean*): Turn symlinking of node_modules on +(*Boolean*): Turn symlinking of node_modules on. + +**Use of this feature is not recommended, as Bazel is removing `managed_directories`. +See https://github.com/bazelbuild/bazel/issues/15463 When False, the package manager will run in the external repository created by this rule. diff --git a/docs/dependencies.md b/docs/dependencies.md index 3336cd0f50..fa4f88c10e 100644 --- a/docs/dependencies.md +++ b/docs/dependencies.md @@ -64,6 +64,22 @@ npm_install( ### symlink_node_modules and managed_directories +**The `managed_directories` feature is being removed from Bazel.** +See https://github.com/bazelbuild/bazel/issues/15463. +Using `managed_directories` is not recommended. + +Furthermore, as of rules_nodejs 5.0, `symlink_node_modules` defaults to `False`. We've found that the benefits of using +`symlink_node_modules`, which allows Bazel to use a `node_modules` directory +that is in the user workspace, do not outweigh the downsides of the repository +rule not defining all of their inputs and of having to re-run the repository rule +if the user's `node_modules` folder is deleted. On persistent CI machines, that +will delete the `node_modules` folder when cleaning the clone between jobs, the +repository rule will run for every job when `symlink_node_modules` is enabled. +With `symlink_node_modules` disabled, the repository rule will only re-run if +its inputs change between jobs. + +The remaining documentation in this section is for legacy usage only. + Set `symlink_node_modules` to `True` to configure `npm_install` and/or `yarn_install` to install `node_modules` inside the user workspace and have Bazel use the `node_modules` folder in the user workspace for the build via a @@ -92,24 +108,6 @@ workspace( ) ``` -As of rules_nodejs 5.0, `symlink_node_modules` defaults to `False` and using -`managed_directories` is not recommended. We've found that the benefits of using -`symlink_node_modules`, which allows Bazel to use a `node_modules` directory -that is in the user workspace, do not outweigh the downsides of the repository -rule not defining all of their inputs and of having to re-run the repository rule -if the user's `node_modules` folder is deleted. On persistent CI machines, that -will delete the `node_modules` folder when cleaning the clone between jobs, the -repository rule will run for every job when `symlink_node_modules` is enabled. -With `symlink_node_modules` disabled, the repository rule will only re-run if -its inputs change between jobs. - -NB: On older versions of Bazel you may have to add the following flag to your -`.bazelrc` to enable managed directories. - -``` -common --experimental_allow_incremental_repository_updates -``` - ### yarn_install vs. npm_install `yarn_install` is the preferred rule for setting up Bazel-managed dependencies for a number of reasons: diff --git a/e2e/BUILD.bazel b/e2e/BUILD.bazel index 5b43f8bbb9..e698ec8715 100644 --- a/e2e/BUILD.bazel +++ b/e2e/BUILD.bazel @@ -75,13 +75,6 @@ e2e_integration_test( "4.0.x", ]] -e2e_integration_test( - name = "e2e_node_loader_no_preserve_symlinks", - # TODO: figure out why this fails on Windows since setting - # symlink_node_modules to False in the test WORKSPACE - tags = ["no-bazelci-windows"], -) - e2e_integration_test( name = "e2e_node_loader_preserve_symlinks", # TODO: figure out why this fails on Windows since setting diff --git a/e2e/node_loader_no_preserve_symlinks/.bazelignore b/e2e/node_loader_no_preserve_symlinks/.bazelignore deleted file mode 100644 index b512c09d47..0000000000 --- a/e2e/node_loader_no_preserve_symlinks/.bazelignore +++ /dev/null @@ -1 +0,0 @@ -node_modules \ No newline at end of file diff --git a/e2e/node_loader_no_preserve_symlinks/.bazelrc b/e2e/node_loader_no_preserve_symlinks/.bazelrc deleted file mode 100644 index ec45b8b929..0000000000 --- a/e2e/node_loader_no_preserve_symlinks/.bazelrc +++ /dev/null @@ -1,3 +0,0 @@ -import %workspace%/../../common.bazelrc - -build --@rules_nodejs//nodejs:default_args="" diff --git a/e2e/node_loader_no_preserve_symlinks/BUILD.bazel b/e2e/node_loader_no_preserve_symlinks/BUILD.bazel deleted file mode 100644 index 26d3bb8155..0000000000 --- a/e2e/node_loader_no_preserve_symlinks/BUILD.bazel +++ /dev/null @@ -1,13 +0,0 @@ -load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_test") - -package(default_visibility = ["//visibility:public"]) - -nodejs_test( - name = "test", - data = [ - "node_loader_test.spec.js", - "@npm//:node_modules", - ], - entry_point = ":node_loader_test.spec.js", - templated_args = ["--nobazel_node_patches"], -) diff --git a/e2e/node_loader_no_preserve_symlinks/WORKSPACE b/e2e/node_loader_no_preserve_symlinks/WORKSPACE deleted file mode 100644 index e71af560f2..0000000000 --- a/e2e/node_loader_no_preserve_symlinks/WORKSPACE +++ /dev/null @@ -1,32 +0,0 @@ -workspace( - name = "e2e_node_loader_no_preserve_symlinks", - managed_directories = { - "@npm": ["node_modules"], - }, -) - -load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") - -http_archive( - name = "build_bazel_rules_nodejs", - sha256 = "0fad45a9bda7dc1990c47b002fd64f55041ea751fafc00cd34efb96107675778", - urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/5.5.0/rules_nodejs-5.5.0.tar.gz"], -) - -load("@build_bazel_rules_nodejs//:repositories.bzl", "build_bazel_rules_nodejs_dependencies") - -build_bazel_rules_nodejs_dependencies() - -load("@rules_nodejs//nodejs:repositories.bzl", "nodejs_register_toolchains") - -nodejs_register_toolchains(name = "nodejs") - -load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install") - -yarn_install( - name = "npm", - package_json = "//:package.json", - # TODO: figure out why this test fails with symlink_node_modules disabled - symlink_node_modules = True, - yarn_lock = "//:yarn.lock", -) diff --git a/e2e/node_loader_no_preserve_symlinks/node_loader_test.spec.js b/e2e/node_loader_no_preserve_symlinks/node_loader_test.spec.js deleted file mode 100644 index 5cea4532e7..0000000000 --- a/e2e/node_loader_no_preserve_symlinks/node_loader_test.spec.js +++ /dev/null @@ -1,24 +0,0 @@ -try { - require('minimist'); - console.error('should fail to resolve minimist'); - process.exitCode = 1; -} catch { -} - - -// should resolve tmp -require('tmp'); - -const testA = require('@gregmagolan/test-a'); -if (testA !== 'test-a-0.0.2') { - console.error('should resolve @gregmagolan/test-a to version 0.0.2 but was', testA); - process.exitCode = 1; -} - -const testB = require('@gregmagolan/test-b'); -if (testB !== 'test-b-0.0.2/test-a-0.0.1') { - console.error( - 'should resolve @gregmagolan/test-b to version 0.0.2 with a @gregmagolan/test-a dependency of 0.0.1 but was', - testB); - process.exitCode = 1; -} diff --git a/e2e/node_loader_no_preserve_symlinks/package.json b/e2e/node_loader_no_preserve_symlinks/package.json deleted file mode 100644 index 15554a6fad..0000000000 --- a/e2e/node_loader_no_preserve_symlinks/package.json +++ /dev/null @@ -1,12 +0,0 @@ -{ - "description": "e2e node_loader testing dependencies", - "devDependencies": {}, - "dependencies": { - "@gregmagolan/test-a": "0.0.2", - "@gregmagolan/test-b": "0.0.2", - "tmp": "0.0.33" - }, - "scripts": { - "test": "bazel test ..." - } -} diff --git a/e2e/node_loader_no_preserve_symlinks/yarn.lock b/e2e/node_loader_no_preserve_symlinks/yarn.lock deleted file mode 100644 index d1c15d0801..0000000000 --- a/e2e/node_loader_no_preserve_symlinks/yarn.lock +++ /dev/null @@ -1,32 +0,0 @@ -# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. -# yarn lockfile v1 - - -"@gregmagolan/test-a@0.0.1": - version "0.0.1" - resolved "https://registry.yarnpkg.com/@gregmagolan/test-a/-/test-a-0.0.1.tgz#c90ebc0676f13b34400da8d1e55ffe5aa76655b4" - integrity sha512-nMZ3MKkXZ+uYbrm8R3dfdt3v1gOOLtf88CdDciWxMYGLr29oVjQG11y2fz4IRBR6R7hI2Gj+G9sHZ69wLTnjfA== - -"@gregmagolan/test-a@0.0.2": - version "0.0.2" - resolved "https://registry.yarnpkg.com/@gregmagolan/test-a/-/test-a-0.0.2.tgz#86ab79a55f44860b02d8275e22d282a1f82b8768" - integrity sha512-u8pW4cm5Xk58fQeA1tGER3KwCfHZ1sBEx0YDWs5Spdi7tiw/21DN7btDvGqx4sG3d2UnUdAnvXPsdwArjNGdmg== - -"@gregmagolan/test-b@0.0.2": - version "0.0.2" - resolved "https://registry.yarnpkg.com/@gregmagolan/test-b/-/test-b-0.0.2.tgz#7549fb9f39895c0d744c2c24c98b0e5817e46bd7" - integrity sha512-h+LeJUbUued9XyQwxKMUdklGiGxPYJ1RvTAK9612ctCiMS2Fn0wu/Au5kHsMHxm8l4bOfpgAWmQ0OQQy7wUBCg== - dependencies: - "@gregmagolan/test-a" "0.0.1" - -os-tmpdir@~1.0.2: - version "1.0.2" - resolved "https://registry.yarnpkg.com/os-tmpdir/-/os-tmpdir-1.0.2.tgz#bbe67406c79aa85c5cfec766fe5734555dfa1274" - integrity sha1-u+Z0BseaqFxc/sdm/lc0VV36EnQ= - -tmp@0.0.33: - version "0.0.33" - resolved "https://registry.yarnpkg.com/tmp/-/tmp-0.0.33.tgz#6d34335889768d21b2bcda0aa277ced3b1bfadf9" - integrity sha512-jRCJlojKnZ3addtTOjdIqoRuPEKBvNXcGYqzO6zWZX8KfKEpnGY5jfggJQ3EjKuu8D4bJRr0y+cYJFmYbImXGw== - dependencies: - os-tmpdir "~1.0.2" diff --git a/internal/npm_install/npm_install.bzl b/internal/npm_install/npm_install.bzl index 23a40385c2..dc161d9637 100644 --- a/internal/npm_install/npm_install.bzl +++ b/internal/npm_install/npm_install.bzl @@ -309,7 +309,10 @@ NB: replaces specified are performed after preinstall_patches so if you are usin to the source `package.json`.""", ), "symlink_node_modules": attr.bool( - doc = """Turn symlinking of node_modules on + doc = """Turn symlinking of node_modules on. + +**Use of this feature is not recommended, as Bazel is removing `managed_directories`. +See https://github.com/bazelbuild/bazel/issues/15463 When False, the package manager will run in the external repository created by this rule. diff --git a/npm_deps.bzl b/npm_deps.bzl index 08b7444650..1f6342561c 100644 --- a/npm_deps.bzl +++ b/npm_deps.bzl @@ -470,7 +470,6 @@ filegroup( patch_args = ["-p0"], patch_tool = "patch", post_install_patches = ["//internal/npm_install/test/patches_yarn_symlinked:semver+1.0.0.patch"], - symlink_node_modules = True, yarn_lock = "//internal/npm_install/test/patches_yarn_symlinked:yarn.lock", quiet = False, ) @@ -482,7 +481,6 @@ filegroup( patch_args = ["-p0"], patch_tool = "patch", post_install_patches = ["//internal/npm_install/test/patches_npm_symlinked:semver+1.0.0.patch"], - symlink_node_modules = True, quiet = False, ) @@ -546,8 +544,6 @@ filegroup( name = "cypress_deps", package_json = "//packages/cypress/test:package.json", yarn_lock = "//packages/cypress/test:yarn.lock", - # TODO: get cypress rule working with symlink_node_modules = False - symlink_node_modules = True, # TODO: get cypress rule working with exports_directories_only = True exports_directories_only = False, )