chore: warn on usage of install_bazel_dependencies

This function is no longer needed since #1149 was finished. However, @angular/bazel relies on it, as might other third-party packages we know nothing about. The conservative rollout is to warn on usage for now, then remove the feature in 3.0 This change also removes our own usage of the function except for angular_view_engine. We also stop writing it in new workspaces created with @bazel/create
bazel-contrib · May 28, 2020 · c18a98d · c18a98d
1 parent 8e7c574
commit c18a98d
Show file tree

Hide file tree

Showing 25 changed files with 55 additions and 88 deletions.
diff --git a/WORKSPACE b/WORKSPACE
@@ -78,10 +78,12 @@ npm_install(
     package_lock_json = "//packages/angular:package-lock.json",
 )
 
-# Install all Bazel dependencies needed for npm packages that supply Bazel rules
+# Install all Bazel dependencies needed for integration test
+# tools/npm_packages/bazel_workspaces
+# (tested on CI and in the scripts/test_all.sh)
 load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
 
-install_bazel_dependencies()
+install_bazel_dependencies(suppress_warning = True)
 
 #
 # Install @bazel/typescript dependencies

diff --git a/docs/repositories.md b/docs/repositories.md
@@ -52,7 +52,7 @@ Commonly used ones are:
 - If you had a dependency on the `foo` package, you can reference `@npm//foo` to get all the files. We mirror the npm dependency graph, so if `foo` declares a dependency on another package `dep`, Bazel will include that dependency when `foo` is used.
 - If the `foo` package has an executable program `bar`, then `@npm//foo/bin:bar` is a `nodejs_binary` that you can call with `bazel run` or can pass as the `executable` to your own rules.
 - Sometimes you need a UMD bundle, but a package doesn't ship one. For example, the `ts_devserver` rule depends on third-party libraries having a named UMD entry point. The `@npm//foo:foo__umd` target will automatically run Browserify to convert the package's `main` entry into UMD.
-- A helper to install npm packages into their own Bazel repository: `@npm//:install_bazel_dependencies.bzl` provides a `install_bazel_dependencies` function. Some npm packages ship custom bazel rules, for example, the `@bazel/typescript` package provides rules which you should load from `@npm_bazel_typescript//:index.bzl`. The `install_bazel_dependencies` function installs such npm packages into their equivalent Bazel repository. (Note, we expect this could be removed in the future, as `load("@npm//@bazel/typescript:index.bzl")` would be a more natural way to load these rules.)
+- DEPRECATED: A helper to install npm packages into their own Bazel repository: `@npm//:install_bazel_dependencies.bzl` provides a `install_bazel_dependencies` function. Some npm packages ship custom bazel rules, for example, the `@angular/bazel` package provides rules which you should load from `@npm_angular_bazel//:index.bzl`. However this causes the build to always fetch npm packages even when not needed, so we plan to remove this in a future release.
 
 > One convenient (maybe also confusing) way to understand what BUILD files are generated is to look at our integration test at https://github.com/bazelbuild/rules_nodejs/tree/master/internal/npm_install/test/golden - this directory looks similar to the content of an `@npm` repository.
 

diff --git a/e2e/bazel_managed_deps/WORKSPACE b/e2e/bazel_managed_deps/WORKSPACE
@@ -41,7 +41,3 @@ filegroup(
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )
-
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
diff --git a/e2e/jasmine/WORKSPACE b/e2e/jasmine/WORKSPACE
@@ -32,7 +32,3 @@ yarn_install(
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )
-
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
diff --git a/e2e/ts_devserver/WORKSPACE b/e2e/ts_devserver/WORKSPACE
@@ -33,10 +33,6 @@ yarn_install(
     yarn_lock = "//:yarn.lock",
 )
 
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
-
 load("@npm//@bazel/typescript:index.bzl", "ts_setup_workspace")
 
 ts_setup_workspace()

diff --git a/e2e/typescript/WORKSPACE b/e2e/typescript/WORKSPACE
@@ -33,10 +33,6 @@ yarn_install(
     yarn_lock = "//:yarn.lock",
 )
 
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
-
 load("@npm//@bazel/typescript:index.bzl", "ts_setup_workspace")
 
 ts_setup_workspace()
diff --git a/e2e/webapp/WORKSPACE b/e2e/webapp/WORKSPACE
@@ -32,7 +32,3 @@ yarn_install(
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )
-
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
diff --git a/examples/angular/WORKSPACE b/examples/angular/WORKSPACE
@@ -41,11 +41,6 @@ yarn_install(
     yarn_lock = "//:yarn.lock",
 )
 
-# Install all bazel dependencies of our npm packages
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
-
 # Load @bazel/protractor dependencies
 load("@npm//@bazel/protractor:package.bzl", "npm_bazel_protractor_dependencies")
 

diff --git a/examples/angular_bazel_architect/WORKSPACE b/examples/angular_bazel_architect/WORKSPACE
@@ -32,8 +32,3 @@ yarn_install(
     symlink_node_modules = False,
     yarn_lock = "//:yarn.lock",
 )
-
-# Install any Bazel rules which were extracted earlier by the yarn_install rule.
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
diff --git a/examples/angular_view_engine/WORKSPACE b/examples/angular_view_engine/WORKSPACE
@@ -41,10 +41,12 @@ yarn_install(
     yarn_lock = "//:yarn.lock",
 )
 
-# Install all bazel dependencies of our npm packages
+# Install the @angular/bazel package into @npm_angular_bazel
+# Note, this will probably break in a future rules_nodejs release.
+# It causes all builds to fetch npm packages even if not needed (eg. only building go code)
 load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
 
-install_bazel_dependencies()
+install_bazel_dependencies(suppress_warning = True)
 
 # Load @bazel/protractor dependencies
 load("@npm//@bazel/protractor:package.bzl", "npm_bazel_protractor_dependencies")

diff --git a/examples/app/WORKSPACE b/examples/app/WORKSPACE
@@ -33,10 +33,6 @@ yarn_install(
     yarn_lock = "//:yarn.lock",
 )
 
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
-
 load("@npm//@bazel/typescript:index.bzl", "ts_setup_workspace")
 
 ts_setup_workspace()

diff --git a/examples/jest/WORKSPACE b/examples/jest/WORKSPACE
@@ -36,7 +36,3 @@ yarn_install(
     quiet = False,
     yarn_lock = "//:yarn.lock",
 )
-
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
diff --git a/examples/kotlin/WORKSPACE b/examples/kotlin/WORKSPACE
@@ -21,11 +21,6 @@ npm_install(
     package_lock_json = "//:package-lock.json",
 )
 
-# Install any Bazel rules which were extracted earlier by the yarn_install rule.
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
-
 # Install external Kotlin/Java dependencies
 http_archive(
     name = "rules_jvm_external",

diff --git a/examples/nestjs/WORKSPACE b/examples/nestjs/WORKSPACE
@@ -33,10 +33,6 @@ yarn_install(
     yarn_lock = "//:yarn.lock",
 )
 
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
-
 http_archive(
     name = "io_bazel_rules_docker",
     sha256 = "3efbd23e195727a67f87b2a04fb4388cc7a11a0c0c2cf33eec225fb8ffbb27ea",

diff --git a/examples/parcel/WORKSPACE b/examples/parcel/WORKSPACE
@@ -32,7 +32,3 @@ npm_install(
     package_json = "//:package.json",
     package_lock_json = "//:package-lock.json",
 )
-
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
diff --git a/examples/protocol_buffers/WORKSPACE b/examples/protocol_buffers/WORKSPACE
@@ -49,10 +49,6 @@ yarn_install(
     yarn_lock = "//:yarn.lock",
 )
 
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
-
 load("@npm//@bazel/protractor:package.bzl", "npm_bazel_protractor_dependencies")
 
 npm_bazel_protractor_dependencies()

diff --git a/examples/react_webpack/WORKSPACE b/examples/react_webpack/WORKSPACE
@@ -19,7 +19,3 @@ yarn_install(
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )
-
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
diff --git a/examples/vendored_node/WORKSPACE b/examples/vendored_node/WORKSPACE
@@ -49,7 +49,3 @@ npm_install(
     package_json = "//:package.json",
     package_lock_json = "//:package-lock.json",
 )
-
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
diff --git a/examples/vendored_node_and_yarn/WORKSPACE b/examples/vendored_node_and_yarn/WORKSPACE
@@ -57,7 +57,3 @@ yarn_install(
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )
-
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
diff --git a/examples/web_testing/WORKSPACE b/examples/web_testing/WORKSPACE
@@ -33,10 +33,6 @@ yarn_install(
     yarn_lock = "//:yarn.lock",
 )
 
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
-
 load("@npm//@bazel/karma:package.bzl", "npm_bazel_karma_dependencies")
 
 npm_bazel_karma_dependencies()

diff --git a/examples/webapp/WORKSPACE b/examples/webapp/WORKSPACE
@@ -33,10 +33,6 @@ yarn_install(
     yarn_lock = "//:yarn.lock",
 )
 
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-
-install_bazel_dependencies()
-
 load("@npm//@bazel/protractor:package.bzl", "npm_bazel_protractor_dependencies")
 
 npm_bazel_protractor_dependencies()

diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts
@@ -360,8 +360,23 @@ function generateInstallBazelDependencies(workspaces: string[]) {
     bzlFile += `load(\":install_${workspace}.bzl\", \"install_${workspace}\")
 `;
   });
-  bzlFile += `def install_bazel_dependencies():
+  bzlFile += `def install_bazel_dependencies(suppress_warning = False):
     """Installs all workspaces listed in bazelWorkspaces of all npm packages"""
+    if not suppress_warning:
+        print("""
+NOTICE: install_bazel_dependencies is no longer needed,
+since @bazel/* npm packages can be load()ed without copying to another repository.
+See https://github.com/bazelbuild/rules_nodejs/issues/1877
+
+install_bazel_dependencies is harmful because it causes npm_install/yarn_install to run even
+if the requested output artifacts for the build don't require nodejs, making multi-language monorepo
+use cases slower.
+
+You should be able to remove install_bazel_workspaces from your WORKSPACE file unless you depend
+on a package that exposes a separate repository, like @angular/bazel exposes @npm_angular_bazel//:index.bzl
+
+You can suppress this message by passing "suppress_warning = True" to install_bazel_dependencies()
+""")
 `;
   workspaces.forEach(workspace => {
     bzlFile += `    install_${workspace}()

diff --git a/internal/npm_install/index.js b/internal/npm_install/index.js
@@ -223,8 +223,23 @@ function generateInstallBazelDependencies(workspaces) {
         bzlFile += `load(\":install_${workspace}.bzl\", \"install_${workspace}\")
 `;
     });
-    bzlFile += `def install_bazel_dependencies():
+    bzlFile += `def install_bazel_dependencies(suppress_warning = False):
     """Installs all workspaces listed in bazelWorkspaces of all npm packages"""
+    if not suppress_warning:
+        print("""
+NOTICE: install_bazel_dependencies is no longer needed,
+since @bazel/* npm packages can be load()ed without copying to another repository.
+See https://github.com/bazelbuild/rules_nodejs/issues/1877
+
+install_bazel_dependencies is harmful because it causes npm_install/yarn_install to run even
+if the requested output artifacts for the build don't require nodejs, making multi-language monorepo
+use cases slower.
+
+You should be able to remove install_bazel_workspaces from your WORKSPACE file unless you depend
+on a package that exposes a separate repository, like @angular/bazel exposes @npm_angular_bazel//:index.bzl
+
+You can suppress this message by passing "suppress_warning = True" to install_bazel_dependencies()
+""")
 `;
     workspaces.forEach(workspace => {
         bzlFile += `    install_${workspace}()

diff --git a/internal/npm_install/test/golden/install_bazel_dependencies.bzl.golden b/internal/npm_install/test/golden/install_bazel_dependencies.bzl.golden
@@ -1,2 +1,14 @@
-def install_bazel_dependencies():
+def install_bazel_dependencies(suppress_warning = False):
     """Installs all workspaces listed in bazelWorkspaces of all npm packages"""
+    if not suppress_warning:
+        print("""
+NOTICE: install_bazel_dependencies is no longer needed,
+since @bazel/* npm packages can be load()ed without copying to another repository.
+See https://github.com/bazelbuild/rules_nodejs/issues/1877
+install_bazel_dependencies is harmful because it causes npm_install/yarn_install to run even
+if the requested output artifacts for the build don't require nodejs, making multi-language monorepo
+use cases slower.
+You should be able to remove install_bazel_workspaces from your WORKSPACE file unless you depend
+on a package that exposes a separate repository, like @angular/bazel exposes @npm_angular_bazel//:index.bzl
+You can suppress this message by passing "suppress_warning = True" to install_bazel_dependencies()
+""")
diff --git a/packages/create/index.js b/packages/create/index.js
@@ -161,11 +161,7 @@ http_archive(
     urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/1.6.0/rules_nodejs-1.6.0.tar.gz"],
 )
 
-${pkgMgr === 'yarn' ? yarnInstallCmd : npmInstallCmd}
-
-# Install any Bazel rules which were extracted earlier by the ${pkgMgr}_install rule.
-load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")
-install_bazel_dependencies()`;
+${pkgMgr === 'yarn' ? yarnInstallCmd : npmInstallCmd}`;
   if (args['typescript']) {
     workspaceContent += `