feat: set exports_directories_only default to True in yarn_install & …

…npm_install repository rules
bazel-contrib · Jan 12, 2022 · ee0e507 · ee0e507
1 parent 7aafe15
commit ee0e507
Show file tree

Hide file tree

Showing 61 changed files with 5,523 additions and 10,642 deletions.
diff --git a/docs/Built-ins.md b/docs/Built-ins.md
@@ -567,10 +567,11 @@ Defaults to `{}`
 
 (*Boolean*): Export only top-level package directory artifacts from node_modules.
 
-Turning this on will decrease the time it takes for Bazel to setup runfiles and sandboxing when
+Set to `False` to export all individual files from node_modules.
+
+When enabled, this decreases the time it takes for Bazel to setup runfiles and sandboxing when
 there are a large number of npm dependencies as inputs to an action.
 
-This breaks compatibility for labels that reference files within npm packages such as `@npm//:node_modules/prettier/bin-prettier.js`.
 To reference files within npm packages, you can use the `directory_file_path` rule and/or `DirectoryFilePathInfo` provider.
 Note, some rules still need upgrading to support consuming `DirectoryFilePathInfo` where needed.
 
@@ -584,44 +585,24 @@ NB: `protractor_web_test` and `protractor_web_test_suite` do not support directo
 
 For the `nodejs_binary` & `nodejs_test` `entry_point` attribute (which often needs to reference a file within
 an npm package) you can set the entry_point to a dict with a single entry, where the key corresponds to the directory
-label and the value corresponds to the path within that directory to the entry point.
-
-For example,
+label and the value corresponds to the path within that directory to the entry point, e.g.
 
 ```
 nodejs_binary(
     name = "prettier",
     data = ["@npm//prettier"],
-    entry_point = "@npm//:node_modules/prettier/bin-prettier.js",
-)
-```
-
-becomes,
-
-```
-nodejs_binary(
-    name = "prettier",
-    data = ["@npm//prettier"],
-    entry_point = { "@npm//:node_modules/prettier": "bin-prettier.js" },
+    entry_point = { "@npm//prettier:directory": "bin-prettier.js" },
 )
 ```
 
 For labels that are passed to `$(rootpath)`, `$(execpath)`, or `$(location)` you can simply break these apart into
-the directory label that gets passed to the expander & path part to follows it.
-
-For example,
+the directory label that gets passed to the expander & path part to follows it, e.g.
 
 ```
-$(rootpath @npm//:node_modules/prettier/bin-prettier.js")
+$(rootpath @npm///prettier:directory)/bin-prettier.js
 ```
 
-becomes,
-
-```
-$(rootpath @npm//:node_modules/prettier)/bin-prettier.js
-```
-
-Defaults to `False`
+Defaults to `True`
 
 <h4 id="npm_install-generate_local_modules_build_files">generate_local_modules_build_files</h4>
 
@@ -1256,10 +1237,11 @@ Defaults to `{}`
 
 (*Boolean*): Export only top-level package directory artifacts from node_modules.
 
-Turning this on will decrease the time it takes for Bazel to setup runfiles and sandboxing when
+Set to `False` to export all individual files from node_modules.
+
+When enabled, this decreases the time it takes for Bazel to setup runfiles and sandboxing when
 there are a large number of npm dependencies as inputs to an action.
 
-This breaks compatibility for labels that reference files within npm packages such as `@npm//:node_modules/prettier/bin-prettier.js`.
 To reference files within npm packages, you can use the `directory_file_path` rule and/or `DirectoryFilePathInfo` provider.
 Note, some rules still need upgrading to support consuming `DirectoryFilePathInfo` where needed.
 
@@ -1273,44 +1255,24 @@ NB: `protractor_web_test` and `protractor_web_test_suite` do not support directo
 
 For the `nodejs_binary` & `nodejs_test` `entry_point` attribute (which often needs to reference a file within
 an npm package) you can set the entry_point to a dict with a single entry, where the key corresponds to the directory
-label and the value corresponds to the path within that directory to the entry point.
-
-For example,
+label and the value corresponds to the path within that directory to the entry point, e.g.
 
 ```
 nodejs_binary(
     name = "prettier",
     data = ["@npm//prettier"],
-    entry_point = "@npm//:node_modules/prettier/bin-prettier.js",
-)
-```
-
-becomes,
-
-```
-nodejs_binary(
-    name = "prettier",
-    data = ["@npm//prettier"],
-    entry_point = { "@npm//:node_modules/prettier": "bin-prettier.js" },
+    entry_point = { "@npm//prettier:directory": "bin-prettier.js" },
 )
 ```
 
 For labels that are passed to `$(rootpath)`, `$(execpath)`, or `$(location)` you can simply break these apart into
-the directory label that gets passed to the expander & path part to follows it.
-
-For example,
+the directory label that gets passed to the expander & path part to follows it, e.g.
 
 ```
-$(rootpath @npm//:node_modules/prettier/bin-prettier.js")
+$(rootpath @npm///prettier:directory)/bin-prettier.js
 ```
 
-becomes,
-
-```
-$(rootpath @npm//:node_modules/prettier)/bin-prettier.js
-```
-
-Defaults to `False`
+Defaults to `True`
 
 <h4 id="yarn_install-frozen_lockfile">frozen_lockfile</h4>
 

diff --git a/e2e/bazel_managed_deps/WORKSPACE b/e2e/bazel_managed_deps/WORKSPACE
@@ -30,6 +30,7 @@ load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")
 
 yarn_install(
     name = "npm",
+    exports_directories_only = False,
     manual_build_file_contents = """
 filegroup(
   name = "bin_files",

diff --git a/e2e/concatjs_devserver/WORKSPACE b/e2e/concatjs_devserver/WORKSPACE
@@ -30,6 +30,7 @@ load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")
 
 yarn_install(
     name = "npm",
+    exports_directories_only = False,
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )

diff --git a/e2e/coverage/WORKSPACE b/e2e/coverage/WORKSPACE
@@ -37,6 +37,7 @@ load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")
 
 yarn_install(
     name = "npm",
+    exports_directories_only = False,
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )
diff --git a/e2e/jasmine/WORKSPACE b/e2e/jasmine/WORKSPACE
@@ -30,6 +30,7 @@ load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")
 
 yarn_install(
     name = "npm",
+    exports_directories_only = False,
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )
diff --git a/e2e/packages/WORKSPACE b/e2e/packages/WORKSPACE
@@ -23,6 +23,7 @@ npm_install(
     name = "e2e_packages_npm_install",
     args = ["--production"],
     data = ["//:postinstall.js"],
+    exports_directories_only = False,
     package_json = "//:npm1/package.json",
     package_lock_json = "//:npm1/package-lock.json",
 )
@@ -31,6 +32,7 @@ npm_install(
     name = "e2e_packages_npm_install_duplicate_for_determinism_testing",
     args = ["--production"],
     data = ["//:postinstall.js"],
+    exports_directories_only = False,
     package_json = "//:npm2/package.json",
     package_lock_json = "//:npm2/package-lock.json",
 )
@@ -39,6 +41,7 @@ yarn_install(
     name = "e2e_packages_yarn_install",
     args = ["--prod"],
     data = ["//:postinstall.js"],
+    exports_directories_only = False,
     package_json = "//:yarn1/package.json",
     yarn_lock = "//:yarn1/yarn.lock",
 )
@@ -47,6 +50,7 @@ yarn_install(
     name = "e2e_packages_yarn_install_duplicate_for_determinism_testing",
     args = ["--prod"],
     data = ["//:postinstall.js"],
+    exports_directories_only = False,
     package_json = "//:yarn2/package.json",
     yarn_lock = "//:yarn2/yarn.lock",
 )
diff --git a/e2e/typescript/WORKSPACE b/e2e/typescript/WORKSPACE
@@ -37,6 +37,7 @@ load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")
 
 yarn_install(
     name = "npm",
+    exports_directories_only = False,
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )
diff --git a/examples/angular/WORKSPACE b/examples/angular/WORKSPACE
@@ -53,6 +53,8 @@ load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")
 # Setup the Node.js toolchain & install our npm dependencies into @npm
 yarn_install(
     name = "npm",
+    # ts_library & ng_module do not work with exports_directories_only
+    exports_directories_only = False,
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )

diff --git a/examples/from_source/WORKSPACE b/examples/from_source/WORKSPACE
@@ -40,7 +40,6 @@ node_repositories(
 
 yarn_install(
     name = "npm",
-    exports_directories_only = True,
     package_json = "//:package.json",
     strict_visibility = True,
     symlink_node_modules = False,

diff --git a/examples/jest/WORKSPACE b/examples/jest/WORKSPACE
@@ -30,7 +30,6 @@ load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")
 
 yarn_install(
     name = "npm",
-    exports_directories_only = True,
     package_json = "//:package.json",
     quiet = False,
     yarn_lock = "//:yarn.lock",

diff --git a/examples/nestjs/WORKSPACE b/examples/nestjs/WORKSPACE
@@ -30,6 +30,7 @@ load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")
 
 yarn_install(
     name = "npm",
+    exports_directories_only = False,
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )

diff --git a/examples/parcel/WORKSPACE b/examples/parcel/WORKSPACE
@@ -55,7 +55,6 @@ load("@build_bazel_rules_nodejs//:index.bzl", "npm_install")
 
 npm_install(
     name = "npm",
-    exports_directories_only = True,
     package_json = "//:package.json",
     package_lock_json = "//:package-lock.json",
 )
diff --git a/examples/protobufjs/WORKSPACE b/examples/protobufjs/WORKSPACE
@@ -46,6 +46,7 @@ load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")
 
 yarn_install(
     name = "npm",
+    exports_directories_only = False,
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )
diff --git a/examples/vendored_node/WORKSPACE b/examples/vendored_node/WORKSPACE
@@ -44,6 +44,7 @@ npm_install(
     data = [
         "@vendored_node_15_0_1//:node-v15.0.1-linux-x64/bin/node",
     ],
+    exports_directories_only = False,
     package_json = "//:package.json",
     package_lock_json = "//:package-lock.json",
 )
diff --git a/examples/vendored_node_and_yarn/WORKSPACE b/examples/vendored_node_and_yarn/WORKSPACE
@@ -57,6 +57,7 @@ yarn_install(
         "@vendored_node_10_12_0//:node-v10.12.0-linux-x64/bin/node",
         "@vendored_yarn_1_10_0//:yarn-v1.10.0/bin/yarn.js",
     ],
+    exports_directories_only = False,
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )
diff --git a/examples/vue/WORKSPACE b/examples/vue/WORKSPACE
@@ -23,7 +23,6 @@ load("@build_bazel_rules_nodejs//:index.bzl", "npm_install")
 
 npm_install(
     name = "npm",
-    exports_directories_only = True,
     package_json = "//:package.json",
     package_lock_json = "//:package-lock.json",
 )
diff --git a/examples/web_testing/WORKSPACE b/examples/web_testing/WORKSPACE
@@ -30,6 +30,7 @@ load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")
 
 yarn_install(
     name = "npm",
+    exports_directories_only = False,
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )

diff --git a/examples/webapp/WORKSPACE b/examples/webapp/WORKSPACE
@@ -30,6 +30,8 @@ load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")
 
 yarn_install(
     name = "npm_deps",
+    # Cannot opt-in yet because protractor rule looks for
+    # @npm//:node_modules/protractor/bin/protractor
     exports_directories_only = False,
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",

diff --git a/internal/linker/link_node_modules.bzl b/internal/linker/link_node_modules.bzl
@@ -42,30 +42,35 @@ def add_arg(args, arg):
 
 def _link_mapping(label, mappings, k, v):
     # Check that two package name mapping do not map to two different source paths
-    package_name = k.split(":")[0]
+    k_segs = k.split(":")
+    package_name = k_segs[0]
+    package_path = k_segs[1] if len(k_segs) > 1 else ""
     link_path = v
 
     for iter_key, iter_values in mappings.items():
         # Map key is of format "package_name:package_path"
         # Map values are of format [deprecated, link_path]
-        iter_package_name = iter_key.split(":")[0]
+        iter_segs = iter_key.split(":")
+        iter_package_name = iter_segs[0]
+        iter_package_path = iter_segs[1] if len(iter_segs) > 1 else ""
         iter_source_path = iter_values
-        if package_name == iter_package_name:
+        if package_name == iter_package_name and package_path == iter_package_path:
             # If we're linking to the output tree be tolerant of linking to different
             # output trees since we can have "static" links that come from cfg="exec" binaries.
             # In the future when we static link directly into runfiles without the linker
             # we can remove this logic.
             link_path_segments = link_path.split("/")
             iter_source_path_segments = iter_source_path.split("/")
-            bin_links = link_path_segments[0] == "bazel-out" and iter_source_path_segments[0] == "bazel-out" and link_path_segments[2] == "bin" and iter_source_path_segments[2] == "bin"
+            bin_links = len(link_path_segments) >= 3 and len(iter_source_path_segments) >= 3 and link_path_segments[0] == "bazel-out" and iter_source_path_segments[0] == "bazel-out" and link_path_segments[2] == "bin" and iter_source_path_segments[2] == "bin"
             if bin_links:
-                compare_link_path = "/".join(link_path_segments[3:])
-                compare_iter_source_path = "/".join(iter_source_path_segments[3:])
+                compare_link_path = "/".join(link_path_segments[3:]) if len(link_path_segments) > 3 else ""
+                compare_iter_source_path = "/".join(iter_source_path_segments[3:]) if len(iter_source_path_segments) > 3 else ""
             else:
                 compare_link_path = link_path
                 compare_iter_source_path = iter_source_path
             if compare_link_path != compare_iter_source_path:
-                fail("conflicting mapping at '%s': '%s' and '%s' map to conflicting %s and %s" % (label, k, iter_key, compare_link_path, compare_iter_source_path))
+                msg = "conflicting mapping at '%s': '%s' and '%s' map to conflicting %s and %s" % (label, k, iter_key, compare_link_path, compare_iter_source_path)
+                fail(msg)
 
     return True
 

diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh
@@ -284,40 +284,30 @@ if [[ "${RUNFILES}" ]] && [[ "${RUNFILES}" != "${RUNFILES_ROOT}" ]]; then
   export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${RUNFILES},${RUNFILES}/${BAZEL_WORKSPACE}/node_modules"
 fi
 if [[ -n "${BAZEL_NODE_MODULES_ROOTS:-}" ]]; then
-  # BAZEL_NODE_MODULES_ROOTS is in the format "<path>:<workspace>,<path>:<workspace>".
-  # For example,
-  #   "internal/linker/test:npm_internal_linker_test,:npm"
+  # BAZEL_NODE_MODULES_ROOTS is in the format "<path>,<path>,..."
   if [[ -n "${VERBOSE_LOGS:-}" ]]; then
     echo "BAZEL_NODE_MODULES_ROOTS=${BAZEL_NODE_MODULES_ROOTS}" >&2
   fi
   OLDIFS="${IFS}"
   IFS=","
   roots=(${BAZEL_NODE_MODULES_ROOTS})
   IFS="${OLDIFS}"
-  for root in "${roots[@]}"; do
-    if [[ "${root}" ]]; then
-      OLDIFS="${IFS}"
-      IFS=":"
-      root_pair=(${root})
-      IFS="${OLDIFS}"
-      root_path="${root_pair[0]}"
-      root_workspace="${root_pair[1]:-}"
-      if [[ "${root_path}" ]]; then
-        # Guard non-root execroot & bazel-out node_modules as well.
+  for root_path in "${roots[@]}"; do
+    if [[ "${root_path}" ]]; then
+      # Guard non-root execroot & bazel-out node_modules as well.
+      # For example,
+      #   .../execroot/build_bazel_rules_nodejs/internal/linker/test/node_modules
+      #   .../execroot/build_bazel_rules_nodejs/bazel-out/*/bin/internal/linker/test/node_modules
+      export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${EXECROOT}/${root_path}/node_modules,${EXECROOT}/bazel-out/*/bin/${root_path}/node_modules"
+      if [[ "${RUNFILES_ROOT}" ]]; then
+        # If in runfiles guard the node_modules location in runfiles as well.
         # For example,
-        #   .../execroot/build_bazel_rules_nodejs/internal/linker/test/node_modules
-        #   .../execroot/build_bazel_rules_nodejs/bazel-out/*/bin/internal/linker/test/node_modules
-        export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${EXECROOT}/${root_path}/node_modules,${EXECROOT}/bazel-out/*/bin/${root_path}/node_modules"
-        if [[ "${RUNFILES_ROOT}" ]]; then
-          # If in runfiles guard the node_modules location in runfiles as well.
-          # For example,
-          #   .../execroot/build_bazel_rules_nodejs/bazel-out/darwin-fastbuild/bin/internal/linker/test/multi_linker/test.sh.runfiles/build_bazel_rules_nodejs/internal/linker/test/node_modules
-          export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${RUNFILES_ROOT}/${BAZEL_WORKSPACE}/${root_path}/node_modules"
-        fi
-        if [[ "${RUNFILES}" ]] && [[ "${RUNFILES}" != "${RUNFILES_ROOT}" ]]; then
-          # Same as above but for RUNFILES is not equal to RUNFILES_ROOT
-          export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${RUNFILES}/${BAZEL_WORKSPACE}/${root_path}/node_modules"
-        fi
+        #   .../execroot/build_bazel_rules_nodejs/bazel-out/darwin-fastbuild/bin/internal/linker/test/multi_linker/test.sh.runfiles/build_bazel_rules_nodejs/internal/linker/test/node_modules
+        export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${RUNFILES_ROOT}/${BAZEL_WORKSPACE}/${root_path}/node_modules"
+      fi
+      if [[ "${RUNFILES}" ]] && [[ "${RUNFILES}" != "${RUNFILES_ROOT}" ]]; then
+        # Same as above but for RUNFILES is not equal to RUNFILES_ROOT
+        export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${RUNFILES}/${BAZEL_WORKSPACE}/${root_path}/node_modules"
       fi
     fi
   done