From 0ab97ba874cf3c28a0e8187b6ae4d48e732cbfa4 Mon Sep 17 00:00:00 2001 From: thesayyn Date: Wed, 30 Mar 2022 22:32:34 +0300 Subject: [PATCH 1/2] fix: exports_directories_only implies --bazel_run_from_execroot --- internal/npm_install/generate_build_file.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts index 2246796b9e..6d04497a07 100644 --- a/internal/npm_install/generate_build_file.ts +++ b/internal/npm_install/generate_build_file.ts @@ -1344,14 +1344,16 @@ export function printIndexBzl(pkg: Dep) { const entryPoint = config.exports_directories_only ? `{ "@${config.workspace}//:node_modules/${pkg._dir}": "${path}" }` : `"@${config.workspace}//:node_modules/${pkg._dir}/${path}"`; + const args = config.exports_directories_only ? ["--bazel_run_from_execroot"]:[] result = `${result} # Generated helper macro to call ${name} def ${name.replace(/-/g, '_')}(**kwargs): output_dir = kwargs.pop("output_dir", False) if "outs" in kwargs or output_dir: + args = kwargs.pop("args", []) + [${args.map(p => `"${p}"`).join(', ')}] npm_package_bin(tool = "@${config.workspace}//${pkg._dir}/bin:${ - name}", output_dir = output_dir, **kwargs) + name}", output_dir = output_dir, args = args, **kwargs) else: nodejs_binary( entry_point = ${entryPoint}, From a0ddf20f99f0d08fbdab0d88cca9707212acab01 Mon Sep 17 00:00:00 2001 From: thesayyn Date: Thu, 31 Mar 2022 17:06:20 +0300 Subject: [PATCH 2/2] fix(builtin): correctly translate tree artifact entry point when from a exports_directories_only npm module --- internal/node/launcher.sh | 2 +- internal/node/node.bzl | 19 +++++++++++++----- .../test/instanceof_test_execroot/BUILD.bazel | 19 ++++++++++++++++++ .../test/instanceof_test_execroot/index.bzl | 0 .../instanceof_test_execroot/lib/BUILD.bazel | 16 +++++++++++++++ .../instanceof_test_execroot/lib/index.js | 20 +++++++++++++++++++ .../instanceof_test_execroot/lib/package.json | 1 + internal/npm_install/generate_build_file.ts | 4 +--- npm_deps.bzl | 4 ++++ package.json | 1 + tools/npm_packages/pkg_with_bin/main.js | 3 +++ tools/npm_packages/pkg_with_bin/package.json | 10 ++++++++++ yarn.lock | 3 +++ 13 files changed, 93 insertions(+), 9 deletions(-) create mode 100644 internal/node/test/instanceof_test_execroot/BUILD.bazel create mode 100644 internal/node/test/instanceof_test_execroot/index.bzl create mode 100644 internal/node/test/instanceof_test_execroot/lib/BUILD.bazel create mode 100644 internal/node/test/instanceof_test_execroot/lib/index.js create mode 100644 internal/node/test/instanceof_test_execroot/lib/package.json create mode 100644 tools/npm_packages/pkg_with_bin/main.js create mode 100644 tools/npm_packages/pkg_with_bin/package.json diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh index 79a3502d1d..591c35fb76 100644 --- a/internal/node/launcher.sh +++ b/internal/node/launcher.sh @@ -337,7 +337,7 @@ else # Entry point is the user-supplied script MAIN="${PWD}/"TEMPLATED_entry_point_execroot_path # TODO: after we link-all-bins we should not need this extra lookup - if [[ ! -f "$MAIN" ]]; then + if [[ ! -e "$MAIN" ]]; then if [ "$FROM_EXECROOT" = true ]; then MAIN="$EXECROOT/"TEMPLATED_entry_point_execroot_path else diff --git a/internal/node/node.bzl b/internal/node/node.bzl index fe76d61c4f..c27980b592 100644 --- a/internal/node/node.bzl +++ b/internal/node/node.bzl @@ -139,12 +139,21 @@ def _to_manifest_path(ctx, file): return ctx.workspace_name + "/" + file.short_path def _to_execroot_path(ctx, file): + # Check for special case of external//node_modules or + # bazel-out//bin/external//node_modules. + # TODO: This assumes we are linking this to the root node_modules which is not always the case + # since the linker was updated to support linking to sub-directories since this special case was + # added parts = file.path.split("/") - if parts[0] == "external": - if parts[2] == "node_modules": - # external/npm/node_modules -> node_modules/foo - # the linker will make sure we can resolve node_modules from npm - return "/".join(parts[2:]) + if file.is_directory and not file.is_source: + # Strip the output root to handle the case of a TreeArtifact exported by js_library with + # exports directories only. Since a TreeArtifact is generated by an action, it resides + # inside bazel-out directory. + parts = parts[3:] + if len(parts) > 3 and parts[0] == "external" and parts[2] == "node_modules": + # Transform external/npm/node_modules/foo to node_modules/foo. + # The linker will make sure we can resolve node_modules from npm. + return "/".join(parts[2:]) return file.path diff --git a/internal/node/test/instanceof_test_execroot/BUILD.bazel b/internal/node/test/instanceof_test_execroot/BUILD.bazel new file mode 100644 index 0000000000..b23912e26d --- /dev/null +++ b/internal/node/test/instanceof_test_execroot/BUILD.bazel @@ -0,0 +1,19 @@ +load("@npm//pkg_with_bin:index.bzl", "instanceof_execroot_test") +load( + "@npm_directory_artifacts//pkg_with_bin:index.bzl", + instanceof_execroot_test_exports_directories = "instanceof_execroot_test", +) + +instanceof_execroot_test( + name = "test", + data = [ + "//internal/node/test/instanceof_test_execroot/lib", + ], +) + +instanceof_execroot_test_exports_directories( + name = "test_exports_directories_only", + data = [ + "//internal/node/test/instanceof_test_execroot/lib", + ], +) diff --git a/internal/node/test/instanceof_test_execroot/index.bzl b/internal/node/test/instanceof_test_execroot/index.bzl new file mode 100644 index 0000000000..e69de29bb2 diff --git a/internal/node/test/instanceof_test_execroot/lib/BUILD.bazel b/internal/node/test/instanceof_test_execroot/lib/BUILD.bazel new file mode 100644 index 0000000000..518e76efbc --- /dev/null +++ b/internal/node/test/instanceof_test_execroot/lib/BUILD.bazel @@ -0,0 +1,16 @@ +load("//:index.bzl", "js_library") + +package(default_visibility = ["//internal/node/test/instanceof_test_execroot:__subpackages__"]) + +js_library( + name = "lib", + # required by tools/npm_packages/pkg_with_bin as third-party dep + package_name = "instanceof_test_execroot_lib", + srcs = [ + "index.js", + "package.json", + ], + deps = [ + "@npm//node_resolve_main", + ], +) diff --git a/internal/node/test/instanceof_test_execroot/lib/index.js b/internal/node/test/instanceof_test_execroot/lib/index.js new file mode 100644 index 0000000000..94ce9184d4 --- /dev/null +++ b/internal/node/test/instanceof_test_execroot/lib/index.js @@ -0,0 +1,20 @@ +// This file will fail when one dependency is resolved from two distinct node_modules +// mostly likely to be one inside runfiles and other from bazel-out +// See: https://github.com/bazelbuild/rules_nodejs/pull/3380 for more. +const assert = require("assert"); + +const resolved_by_lib = require.resolve("node_resolve_main"); +const resolved_by_pkg_with_bin = globalThis["node_resolve_main_resolved_path_by_pkg_with_bin"] +assert.equal( + resolved_by_lib, + resolved_by_pkg_with_bin, + ` +Expected to resolve package "node_resolve_main" from the same node_modules but + +tools/npm_packages/pkg_with_bin resolved it from ${resolved_by_pkg_with_bin} + +internal/node/test/instanceof_test_execroot/lib resolved it from ${resolved_by_lib} + +See: https://github.com/bazelbuild/rules_nodejs/pull/3380 for context. +` +); \ No newline at end of file diff --git a/internal/node/test/instanceof_test_execroot/lib/package.json b/internal/node/test/instanceof_test_execroot/lib/package.json new file mode 100644 index 0000000000..84582c4845 --- /dev/null +++ b/internal/node/test/instanceof_test_execroot/lib/package.json @@ -0,0 +1 @@ +{"name": "instanceof_test_execroot_lib"} \ No newline at end of file diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts index 6d04497a07..2246796b9e 100644 --- a/internal/npm_install/generate_build_file.ts +++ b/internal/npm_install/generate_build_file.ts @@ -1344,16 +1344,14 @@ export function printIndexBzl(pkg: Dep) { const entryPoint = config.exports_directories_only ? `{ "@${config.workspace}//:node_modules/${pkg._dir}": "${path}" }` : `"@${config.workspace}//:node_modules/${pkg._dir}/${path}"`; - const args = config.exports_directories_only ? ["--bazel_run_from_execroot"]:[] result = `${result} # Generated helper macro to call ${name} def ${name.replace(/-/g, '_')}(**kwargs): output_dir = kwargs.pop("output_dir", False) if "outs" in kwargs or output_dir: - args = kwargs.pop("args", []) + [${args.map(p => `"${p}"`).join(', ')}] npm_package_bin(tool = "@${config.workspace}//${pkg._dir}/bin:${ - name}", output_dir = output_dir, args = args, **kwargs) + name}", output_dir = output_dir, **kwargs) else: nodejs_binary( entry_point = ${entryPoint}, diff --git a/npm_deps.bzl b/npm_deps.bzl index ca247d66e8..2e7f8a8a97 100644 --- a/npm_deps.bzl +++ b/npm_deps.bzl @@ -42,6 +42,8 @@ def npm_deps(): "//:tools/npm_packages/node_resolve_nested_main/nested/package.json", "//:tools/npm_packages/testy/index.js", "//:tools/npm_packages/testy/package.json", + "//:tools/npm_packages/pkg_with_bin/main.js", + "//:tools/npm_packages/pkg_with_bin/package.json", ], environment = { "SOME_USER_ENV": "yarn is great!", @@ -107,6 +109,8 @@ js_library( "//:tools/npm_packages/node_resolve_nested_main/nested/package.json", "//:tools/npm_packages/testy/index.js", "//:tools/npm_packages/testy/package.json", + "//:tools/npm_packages/pkg_with_bin/main.js", + "//:tools/npm_packages/pkg_with_bin/package.json", ], environment = { "SOME_USER_ENV": "yarn is great!", diff --git a/package.json b/package.json index e247ea8e7a..38e21f8eaf 100644 --- a/package.json +++ b/package.json @@ -77,6 +77,7 @@ "node_resolve_main": "file:./tools/npm_packages/node_resolve_main", "node_resolve_main_2": "file:./tools/npm_packages/node_resolve_main_2", "node_resolve_nested_main": "file:./tools/npm_packages/node_resolve_nested_main", + "pkg_with_bin": "file:./tools/npm_packages/pkg_with_bin", "npm": "6.13.7", "patch-package": "^6.2.2", "protobufjs": "6.8.8", diff --git a/tools/npm_packages/pkg_with_bin/main.js b/tools/npm_packages/pkg_with_bin/main.js new file mode 100644 index 0000000000..7eb78154a8 --- /dev/null +++ b/tools/npm_packages/pkg_with_bin/main.js @@ -0,0 +1,3 @@ +globalThis["node_resolve_main_resolved_path_by_pkg_with_bin"] = require.resolve("node_resolve_main"); +require("instanceof_test_execroot_lib"); +// See: internal/node/test/instanceof_test_execroot \ No newline at end of file diff --git a/tools/npm_packages/pkg_with_bin/package.json b/tools/npm_packages/pkg_with_bin/package.json new file mode 100644 index 0000000000..0d1830eac1 --- /dev/null +++ b/tools/npm_packages/pkg_with_bin/package.json @@ -0,0 +1,10 @@ +{ + "version": "0.0.1", + "description": "See: internal/node/test/instanceof_test_execroot", + "bin": { + "instanceof_execroot": "main.js" + }, + "peerDependencies": { + "node_resolve_main": "*" + } +} \ No newline at end of file diff --git a/yarn.lock b/yarn.lock index c13aa850df..f8de4224b2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8745,6 +8745,9 @@ pkg-dir@^4.2.0: dependencies: find-up "^4.0.0" +"pkg_with_bin@file:./tools/npm_packages/pkg_with_bin": + version "0.0.1" + please-upgrade-node@^3.1.1: version "3.2.0" resolved "https://registry.yarnpkg.com/please-upgrade-node/-/please-upgrade-node-3.2.0.tgz#aeddd3f994c933e4ad98b99d9a556efa0e2fe942"