fix(builtin): fix an bug where a nodejs_binary would fail to resolve an npm package when the linker is disabled (#3492)

kormide · web-flow · commit 8a2dfc88efc8 · 2022-06-23T15:50:14.000-07:00
diff --git a/.bazelrc b/.bazelrc
@@ -5,8 +5,8 @@ import %workspace%/common.bazelrc
 # This lets us glob() up all the files inside the examples to make them inputs to tests
 # To update these lines, just run `yarn bazel:update-deleted-packages`
 # (Note, we cannot use common --deleted_packages because the bazel version command doesn't support it)
-build --deleted_packages=e2e/bazel_managed_deps,e2e/bazel_run_chdir,e2e/bazel_run_chdir/subfolder,e2e/concatjs_devserver,e2e/concatjs_devserver/genrule,e2e/concatjs_devserver/subpackage,e2e/coverage,e2e/fine_grained_symlinks,e2e/jasmine,e2e/node_loader_no_preserve_symlinks,e2e/node_loader_preserve_symlinks,e2e/nodejs_host,e2e/nodejs_image,e2e/nodejs_image/foolib,e2e/packages,e2e/symlinked_node_modules_npm,e2e/symlinked_node_modules_yarn,e2e/typescript,e2e/webapp,examples/angular,examples/angular/e2e,examples/angular/src,examples/angular/src/app,examples/angular/src/app/hello-world,examples/angular/src/app/home,examples/angular/src/app/todos,examples/angular/src/app/todos/reducers,examples/angular/src/assets,examples/angular/src/lib/shorten,examples/angular/src/shared/material,examples/angular/tools,examples/angular_bazel_architect,examples/angular_bazel_architect/projects/frontend-lib,examples/app,examples/app/styles,examples/app/test,examples/closure,examples/create-react-app,examples/cypress,examples/esbuild,examples/esbuild/src,examples/from_source,examples/jest,examples/jest/ts,examples/jest/ts/src,examples/jest/ts/test,examples/kotlin,examples/nestjs,examples/nestjs/src,examples/parcel,examples/protobufjs,examples/react_webpack,examples/toolchain,examples/vendored_node_and_yarn,examples/vendored_node_and_yarn/toolchains,examples/vue,examples/vue/src,examples/vue/src/components/HelloWorld,examples/web_testing,examples/webapp,examples/worker
-query --deleted_packages=e2e/bazel_managed_deps,e2e/bazel_run_chdir,e2e/bazel_run_chdir/subfolder,e2e/concatjs_devserver,e2e/concatjs_devserver/genrule,e2e/concatjs_devserver/subpackage,e2e/coverage,e2e/fine_grained_symlinks,e2e/jasmine,e2e/node_loader_no_preserve_symlinks,e2e/node_loader_preserve_symlinks,e2e/nodejs_host,e2e/nodejs_image,e2e/nodejs_image/foolib,e2e/packages,e2e/symlinked_node_modules_npm,e2e/symlinked_node_modules_yarn,e2e/typescript,e2e/webapp,examples/angular,examples/angular/e2e,examples/angular/src,examples/angular/src/app,examples/angular/src/app/hello-world,examples/angular/src/app/home,examples/angular/src/app/todos,examples/angular/src/app/todos/reducers,examples/angular/src/assets,examples/angular/src/lib/shorten,examples/angular/src/shared/material,examples/angular/tools,examples/angular_bazel_architect,examples/angular_bazel_architect/projects/frontend-lib,examples/app,examples/app/styles,examples/app/test,examples/closure,examples/create-react-app,examples/cypress,examples/esbuild,examples/esbuild/src,examples/from_source,examples/jest,examples/jest/ts,examples/jest/ts/src,examples/jest/ts/test,examples/kotlin,examples/nestjs,examples/nestjs/src,examples/parcel,examples/protobufjs,examples/react_webpack,examples/toolchain,examples/vendored_node_and_yarn,examples/vendored_node_and_yarn/toolchains,examples/vue,examples/vue/src,examples/vue/src/components/HelloWorld,examples/web_testing,examples/webapp,examples/worker
+build --deleted_packages=e2e/bazel_managed_deps,e2e/bazel_run_chdir,e2e/bazel_run_chdir/subfolder,e2e/concatjs_devserver,e2e/concatjs_devserver/genrule,e2e/concatjs_devserver/subpackage,e2e/coverage,e2e/fine_grained_symlinks,e2e/jasmine,e2e/linker_disabled_regression,e2e/linker_disabled_regression/subdir,e2e/node_loader_preserve_symlinks,e2e/nodejs_host,e2e/nodejs_image,e2e/nodejs_image/foolib,e2e/packages,e2e/symlinked_node_modules_npm,e2e/symlinked_node_modules_yarn,e2e/typescript,e2e/webapp,examples/angular,examples/angular/e2e,examples/angular/src,examples/angular/src/app,examples/angular/src/app/hello-world,examples/angular/src/app/home,examples/angular/src/app/todos,examples/angular/src/app/todos/reducers,examples/angular/src/assets,examples/angular/src/lib/shorten,examples/angular/src/shared/material,examples/angular/tools,examples/angular_bazel_architect,examples/angular_bazel_architect/projects/frontend-lib,examples/app,examples/app/styles,examples/app/test,examples/closure,examples/create-react-app,examples/cypress,examples/esbuild,examples/esbuild/src,examples/from_source,examples/jest,examples/jest/ts,examples/jest/ts/src,examples/jest/ts/test,examples/kotlin,examples/nestjs,examples/nestjs/src,examples/parcel,examples/protobufjs,examples/react_webpack,examples/toolchain,examples/vendored_node_and_yarn,examples/vendored_node_and_yarn/toolchains,examples/vue,examples/vue/src,examples/vue/src/components/HelloWorld,examples/web_testing,examples/webapp,examples/worker
+query --deleted_packages=e2e/bazel_managed_deps,e2e/bazel_run_chdir,e2e/bazel_run_chdir/subfolder,e2e/concatjs_devserver,e2e/concatjs_devserver/genrule,e2e/concatjs_devserver/subpackage,e2e/coverage,e2e/fine_grained_symlinks,e2e/jasmine,e2e/linker_disabled_regression,e2e/linker_disabled_regression/subdir,e2e/node_loader_preserve_symlinks,e2e/nodejs_host,e2e/nodejs_image,e2e/nodejs_image/foolib,e2e/packages,e2e/symlinked_node_modules_npm,e2e/symlinked_node_modules_yarn,e2e/typescript,e2e/webapp,examples/angular,examples/angular/e2e,examples/angular/src,examples/angular/src/app,examples/angular/src/app/hello-world,examples/angular/src/app/home,examples/angular/src/app/todos,examples/angular/src/app/todos/reducers,examples/angular/src/assets,examples/angular/src/lib/shorten,examples/angular/src/shared/material,examples/angular/tools,examples/angular_bazel_architect,examples/angular_bazel_architect/projects/frontend-lib,examples/app,examples/app/styles,examples/app/test,examples/closure,examples/create-react-app,examples/cypress,examples/esbuild,examples/esbuild/src,examples/from_source,examples/jest,examples/jest/ts,examples/jest/ts/src,examples/jest/ts/test,examples/kotlin,examples/nestjs,examples/nestjs/src,examples/parcel,examples/protobufjs,examples/react_webpack,examples/toolchain,examples/vendored_node_and_yarn,examples/vendored_node_and_yarn/toolchains,examples/vue,examples/vue/src,examples/vue/src/components/HelloWorld,examples/web_testing,examples/webapp,examples/worker
 
 # Mock versioning command to test the --stamp behavior
 build --workspace_status_command="echo BUILD_SCM_VERSION 1.2.3"
diff --git a/e2e/BUILD.bazel b/e2e/BUILD.bazel
@@ -178,3 +178,13 @@ e2e_integration_test(
     # TODO: figure out why this fails on Windows
     tags = ["no-bazelci-windows"],
 )
+
+e2e_integration_test(
+    name = "e2e_linker_disabled_regression",
+    bazel_commands = [
+        "build //...",
+        "run //subdir:binary",
+    ],
+    # TODO: figure out why this fails on Windows
+    tags = ["no-bazelci-windows"],
+)
diff --git a/e2e/linker_disabled_regression/.bazelignore b/e2e/linker_disabled_regression/.bazelignore
@@ -0,0 +1,2 @@
+node_modules
+subdir/node_modules
diff --git a/e2e/linker_disabled_regression/.bazelrc b/e2e/linker_disabled_regression/.bazelrc
@@ -0,0 +1,3 @@
+import %workspace%/../../common.bazelrc
+
+build --enable_runfiles
diff --git a/e2e/linker_disabled_regression/BUILD.bazel b/e2e/linker_disabled_regression/BUILD.bazel
@@ -0,0 +1,8 @@
+load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary")
+
+nodejs_binary(
+    name = "binary",
+    data = ["@npm//acorn"],
+    entry_point = "main.cjs",
+    templated_args = ["--nobazel_run_linker"],
+)
diff --git a/e2e/linker_disabled_regression/README.md b/e2e/linker_disabled_regression/README.md
@@ -0,0 +1,8 @@
+This e2e tests a regression that occurred under the following conditions:
+
+1. Two `yarn_install`s with in a subdirectory.
+2. `export_directories_only = False`
+3. The linker is disabled with `templated_args = ["--nobazel_run_linker"]`
+
+A binary that depends on an npm package declared in the subdirectory's package.json
+could not be resolved.
diff --git a/e2e/linker_disabled_regression/WORKSPACE b/e2e/linker_disabled_regression/WORKSPACE
@@ -0,0 +1,34 @@
+workspace(
+    name = "e2e_linker_disabled_regression",
+    managed_directories = {
+        "@npm": ["node_modules"],
+        "@subdir_npm": ["subdir/node_modules"],
+    },
+)
+
+local_repository(
+    name = "build_bazel_rules_nodejs",
+    path = "../..",
+)
+
+load("@build_bazel_rules_nodejs//:repositories.bzl", "build_bazel_rules_nodejs_dependencies")
+
+build_bazel_rules_nodejs_dependencies()
+
+load("@build_bazel_rules_nodejs//:index.bzl", "node_repositories", "yarn_install")
+
+node_repositories()
+
+yarn_install(
+    name = "npm",
+    exports_directories_only = False,
+    package_json = "//:package.json",
+    yarn_lock = "//:yarn.lock",
+)
+
+yarn_install(
+    name = "subdir_npm",
+    exports_directories_only = False,
+    package_json = "//subdir:package.json",
+    yarn_lock = "//subdir:yarn.lock",
+)
diff --git a/e2e/linker_disabled_regression/main.cjs b/e2e/linker_disabled_regression/main.cjs
@@ -0,0 +1 @@
+require("acorn");
diff --git a/e2e/linker_disabled_regression/package.json b/e2e/linker_disabled_regression/package.json
@@ -0,0 +1,7 @@
+{
+    "name": "root",
+    "private": true,
+    "dependencies": {
+        "acorn": "8.7.1"
+    }
+}
diff --git a/e2e/linker_disabled_regression/subdir/BUILD.bazel b/e2e/linker_disabled_regression/subdir/BUILD.bazel
@@ -0,0 +1,8 @@
+load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary")
+
+nodejs_binary(
+    name = "binary",
+    data = ["@subdir_npm//acorn"],
+    entry_point = "main.cjs",
+    templated_args = ["--nobazel_run_linker"],
+)
diff --git a/e2e/linker_disabled_regression/subdir/main.cjs b/e2e/linker_disabled_regression/subdir/main.cjs
@@ -0,0 +1 @@
+require("acorn");
diff --git a/e2e/linker_disabled_regression/subdir/package.json b/e2e/linker_disabled_regression/subdir/package.json
@@ -0,0 +1,7 @@
+{
+    "name": "subdir",
+    "private": true,
+    "dependencies": {
+        "acorn": "8.7.1"
+    }
+}
diff --git a/e2e/linker_disabled_regression/subdir/yarn.lock b/e2e/linker_disabled_regression/subdir/yarn.lock
@@ -0,0 +1,8 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
+# yarn lockfile v1
+
+
+acorn@8.7.1:
+  version "8.7.1"
+  resolved "https://registry.yarnpkg.com/acorn/-/acorn-8.7.1.tgz#0197122c843d1bf6d0a5e83220a788f278f63c30"
+  integrity sha512-Xx54uLJQZ19lKygFXOWsscKUbsBZW0CPykPhVQdhIeIwrbPmJzqeASDInc8nKBnp/JT6igTs82qPXz069H8I/A==
diff --git a/e2e/linker_disabled_regression/yarn.lock b/e2e/linker_disabled_regression/yarn.lock
@@ -0,0 +1,8 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
+# yarn lockfile v1
+
+
+acorn@8.7.1:
+  version "8.7.1"
+  resolved "https://registry.yarnpkg.com/acorn/-/acorn-8.7.1.tgz#0197122c843d1bf6d0a5e83220a788f278f63c30"
+  integrity sha512-Xx54uLJQZ19lKygFXOWsscKUbsBZW0CPykPhVQdhIeIwrbPmJzqeASDInc8nKBnp/JT6igTs82qPXz069H8I/A==
diff --git a/internal/node/node.bzl b/internal/node/node.bzl
@@ -197,7 +197,13 @@ def _nodejs_binary_impl(ctx, data = [], runfiles = [], expanded_args = []):
 
     if "" in node_modules_roots:
         node_modules_root = node_modules_roots[""] + "/node_modules"
+    elif len(node_modules_roots) == 1:
+        node_modules_root = node_modules_roots[node_modules_roots.keys()[0]] + "/node_modules"
     else:
+        if len(node_modules_roots) > 1:
+            # buildifier disable=print
+            print("Warning: nodejs_binary found more than one node_modules root: %s. Falling back to build_bazel_rules_nodejs/node_modules." % node_modules_roots.keys())
+
         # there are no fine grained deps but we still need a node_modules_root even if it is a non-existant one
         node_modules_root = "build_bazel_rules_nodejs/node_modules"
     _write_require_patch_script(ctx, data, node_modules_root)

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+node_modules`
	`2`	`+subdir/node_modules`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+import %workspace%/../../common.bazelrc`
	`2`	`+`
	`3`	`+build --enable_runfiles`