fix(npm): correct package_store_prefix_len for links in external-repo sub-packages (#2864)
When calculating the package store prefix truncation with external
modules (i.e. a repo name is included in the path), an extra slash after
the repo name was unaccounted for, resulting in incorrect symlinks (and
subsequently `ERR_MODULE_NOT_FOUND` error during builds).
Below is an example of the logic used to calculate the truncation
length:
```python
# ../[com_github_example_repo+]/[example_package]/[node_modules/.aspect_rules_js/]foo@1.2.3/node_modules/bar
# | repo name | | package name | | store prefix |
path = "../com_github_example_repo+/example_package/node_modules/.aspect_rules_js/foo@1.2.3/node_modules/bar"
truncate_length = len(repo_name) + len(package_name) + len(store_prefix) + 3 # = 72
truncated_path = path[truncate_length:] # = "s/foo@1.2.3/node_modules/bar"
```
The extra `s` in the beginning of `truncated_path` should not be there
and is solved by changing the `+ 3` to a `+ 4`.
---------
Co-authored-by: Fredrik Medley <fredrik@meroton.com>
Co-authored-by: Jason Bedard <jason+github@jbedard.ca>
diff --git a/e2e/bzlmod/BUILD.bazel b/e2e/bzlmod/BUILD.bazel
index 9cacb00..ba3ed46 100644
--- a/e2e/bzlmod/BUILD.bazel
+++ b/e2e/bzlmod/BUILD.bazel
@@ -134,6 +134,9 @@
# js_run_binary() running a binary in @other_repo
":run_other_module_binary_test",
+
+ # js_run_binary() running a binary linked in a sub-package of @other_repo
+ ":run_other_module_subpackage_binary_test",
],
)
@@ -148,3 +151,14 @@
silent_on_success = False,
tool = "@other_module//:lessc", # A js_binary() tool in a different repo
)
+
+js_run_binary(
+ name = "run_other_module_subpackage_binary_test",
+ srcs = ["my.less"],
+ outs = ["other-module-subpackage-my.css"],
+ args = [
+ "my.less",
+ "other-module-subpackage-my.css",
+ ],
+ tool = "@other_module//frontend:lessc", # A js_binary() tool linked in a sub-package of a different repo
+)
diff --git a/e2e/bzlmod/other_module/.bazelignore b/e2e/bzlmod/other_module/.bazelignore
index 3c3629e..aac7dfb 100644
--- a/e2e/bzlmod/other_module/.bazelignore
+++ b/e2e/bzlmod/other_module/.bazelignore
@@ -1 +1,2 @@
node_modules
+frontend/node_modules
diff --git a/e2e/bzlmod/other_module/MODULE.bazel b/e2e/bzlmod/other_module/MODULE.bazel
index 1e20ad9..c026237 100644
--- a/e2e/bzlmod/other_module/MODULE.bazel
+++ b/e2e/bzlmod/other_module/MODULE.bazel
@@ -9,7 +9,13 @@
pnpm_lock = "//:pnpm-lock.yaml",
verify_node_modules_ignored = "//:.bazelignore",
)
-use_repo(npm, "npm_other_module")
+npm.npm_translate_lock(
+ name = "npm_other_module_frontend",
+ npmrc = "//:.npmrc",
+ pnpm_lock = "//frontend:pnpm-lock.yaml",
+ verify_node_modules_ignored = "//:.bazelignore",
+)
+use_repo(npm, "npm_other_module", "npm_other_module_frontend")
# A non-root module may request a pnpm version for the default "pnpm" repo, but
# the version requested by the e2e test root module takes priority. The e2e test
diff --git a/e2e/bzlmod/other_module/frontend/BUILD.bazel b/e2e/bzlmod/other_module/frontend/BUILD.bazel
new file mode 100644
index 0000000..8c0af27
--- /dev/null
+++ b/e2e/bzlmod/other_module/frontend/BUILD.bazel
@@ -0,0 +1,11 @@
+load("@npm_other_module_frontend//:defs.bzl", "npm_link_all_packages")
+load("@npm_other_module_frontend//frontend:less/package_json.bzl", less_bin = "bin")
+
+# npm packages linked in a sub-package of an external module, see
+# https://github.com/aspect-build/rules_js/pull/2896
+npm_link_all_packages()
+
+less_bin.lessc_binary(
+ name = "lessc",
+ visibility = ["//visibility:public"],
+)
diff --git a/e2e/bzlmod/other_module/frontend/package.json b/e2e/bzlmod/other_module/frontend/package.json
new file mode 100644
index 0000000..11e1359
--- /dev/null
+++ b/e2e/bzlmod/other_module/frontend/package.json
@@ -0,0 +1,8 @@
+{
+ "pnpm": {
+ "onlyBuiltDependencies": []
+ },
+ "dependencies": {
+ "less": "4.1.3"
+ }
+}
diff --git a/e2e/bzlmod/other_module/frontend/pnpm-lock.yaml b/e2e/bzlmod/other_module/frontend/pnpm-lock.yaml
new file mode 100644
index 0000000..78c165c
--- /dev/null
+++ b/e2e/bzlmod/other_module/frontend/pnpm-lock.yaml
@@ -0,0 +1,160 @@
+lockfileVersion: '9.0'
+
+settings:
+ autoInstallPeers: true
+ excludeLinksFromLockfile: false
+
+importers:
+
+ .:
+ dependencies:
+ less:
+ specifier: 4.1.3
+ version: 4.1.3
+
+packages:
+
+ copy-anything@2.0.6:
+ resolution: {integrity: sha512-1j20GZTsvKNkc4BY3NpMOM8tt///wY3FpIzozTOFO2ffuZcV61nojHXVKIy3WM+7ADCy5FVhdZYHYDdgTU0yJw==}
+
+ errno@0.1.8:
+ resolution: {integrity: sha512-dJ6oBr5SQ1VSd9qkk7ByRgb/1SH4JZjCHSW/mr63/QcXO9zLVxvJ6Oy13nio03rxpSnVDDjFor75SjVeZWPW/A==}
+ hasBin: true
+
+ graceful-fs@4.2.11:
+ resolution: {integrity: sha512-RbJ5/jmFcNNCcDV5o9eTnBLJ/HszWV0P73bc+Ff4nS/rJj+YaS6IGyiOL0VoBYX+l1Wrl3k63h/KrH+nhJ0XvQ==}
+
+ iconv-lite@0.6.3:
+ resolution: {integrity: sha512-4fCk79wshMdzMp2rH06qWrJE4iolqLhCUH+OiuIgU++RB0+94NlDL81atO7GX55uUKueo0txHNtvEyI6D7WdMw==}
+ engines: {node: '>=0.10.0'}
+
+ image-size@0.5.5:
+ resolution: {integrity: sha512-6TDAlDPZxUFCv+fuOkIoXT/V/f3Qbq8e37p+YOiYrUv3v9cc3/6x78VdfPgFVaB9dZYeLUfKgHRebpkm/oP2VQ==}
+ engines: {node: '>=0.10.0'}
+ hasBin: true
+
+ is-what@3.14.1:
+ resolution: {integrity: sha512-sNxgpk9793nzSs7bA6JQJGeIuRBQhAaNGG77kzYQgMkrID+lS6SlK07K5LaptscDlSaIgH+GPFzf+d75FVxozA==}
+
+ less@4.1.3:
+ resolution: {integrity: sha512-w16Xk/Ta9Hhyei0Gpz9m7VS8F28nieJaL/VyShID7cYvP6IL5oHeL6p4TXSDJqZE/lNv0oJ2pGVjJsRkfwm5FA==}
+ engines: {node: '>=6'}
+ hasBin: true
+
+ make-dir@2.1.0:
+ resolution: {integrity: sha512-LS9X+dc8KLxXCb8dni79fLIIUA5VyZoyjSMCwTluaXA0o27cCK0bhXkpgw+sTXVpPy/lSO57ilRixqk0vDmtRA==}
+ engines: {node: '>=6'}
+
+ mime@1.6.0:
+ resolution: {integrity: sha512-x0Vn8spI+wuJ1O6S7gnbaQg8Pxh4NNHb7KSINmEWKiPE4RKOplvijn+NkmYmmRgP68mc70j2EbeTFRsrswaQeg==}
+ engines: {node: '>=4'}
+ hasBin: true
+
+ needle@3.5.0:
+ resolution: {integrity: sha512-jaQyPKKk2YokHrEg+vFDYxXIHTCBgiZwSHOoVx/8V3GIBS8/VN6NdVRmg8q1ERtPkMvmOvebsgga4sAj5hls/w==}
+ engines: {node: '>= 4.4.x'}
+ hasBin: true
+
+ parse-node-version@1.0.1:
+ resolution: {integrity: sha512-3YHlOa/JgH6Mnpr05jP9eDG254US9ek25LyIxZlDItp2iJtwyaXQb57lBYLdT3MowkUFYEV2XXNAYIPlESvJlA==}
+ engines: {node: '>= 0.10'}
+
+ pify@4.0.1:
+ resolution: {integrity: sha512-uB80kBFb/tfd68bVleG9T5GGsGPjJrLAUpR5PZIrhBnIaRTQRjqdJSsIKkOP6OAIFbj7GOrcudc5pNjZ+geV2g==}
+ engines: {node: '>=6'}
+
+ prr@1.0.1:
+ resolution: {integrity: sha512-yPw4Sng1gWghHQWj0B3ZggWUm4qVbPwPFcRG8KyxiU7J2OHFSoEHKS+EZ3fv5l1t9CyCiop6l/ZYeWbrgoQejw==}
+
+ safer-buffer@2.1.2:
+ resolution: {integrity: sha512-YZo3K82SD7Riyi0E1EQPojLz7kpepnSQI9IyPbHHg1XXXevb5dJI7tpyN2ADxGcQbHG7vcyRHk0cbwqcQriUtg==}
+
+ sax@1.6.1:
+ resolution: {integrity: sha512-42tBVwLWnaQvW5zc4HbZrTuWccECCZfBi92FDuwtqxasH+JbPB3/FOKb1m222K42R4WxuxzzMsTswfzgtSu64Q==}
+ engines: {node: '>=11.0.0'}
+
+ semver@5.7.2:
+ resolution: {integrity: sha512-cBznnQ9KjJqU67B52RMC65CMarK2600WFnbkcaiwWq3xy/5haFJlshgnpjovMVJ+Hff49d8GEn0b87C5pDQ10g==}
+ hasBin: true
+
+ source-map@0.6.1:
+ resolution: {integrity: sha512-UjgapumWlbMhkBgzT7Ykc5YXUT46F0iKu8SGXq0bcwP5dz/h0Plj6enJqjz1Zbq2l5WaqYnrVbwWOWMyF3F47g==}
+ engines: {node: '>=0.10.0'}
+
+ tslib@2.8.1:
+ resolution: {integrity: sha512-oJFu94HQb+KVduSUQL7wnpmqnfmLsOA/nAh6b6EH0wCEoK0/mPeXU6c3wKDV83MkOuHPRHtSXKKU99IBazS/2w==}
+
+snapshots:
+
+ copy-anything@2.0.6:
+ dependencies:
+ is-what: 3.14.1
+
+ errno@0.1.8:
+ dependencies:
+ prr: 1.0.1
+ optional: true
+
+ graceful-fs@4.2.11:
+ optional: true
+
+ iconv-lite@0.6.3:
+ dependencies:
+ safer-buffer: 2.1.2
+ optional: true
+
+ image-size@0.5.5:
+ optional: true
+
+ is-what@3.14.1: {}
+
+ less@4.1.3:
+ dependencies:
+ copy-anything: 2.0.6
+ parse-node-version: 1.0.1
+ tslib: 2.8.1
+ optionalDependencies:
+ errno: 0.1.8
+ graceful-fs: 4.2.11
+ image-size: 0.5.5
+ make-dir: 2.1.0
+ mime: 1.6.0
+ needle: 3.5.0
+ source-map: 0.6.1
+
+ make-dir@2.1.0:
+ dependencies:
+ pify: 4.0.1
+ semver: 5.7.2
+ optional: true
+
+ mime@1.6.0:
+ optional: true
+
+ needle@3.5.0:
+ dependencies:
+ iconv-lite: 0.6.3
+ sax: 1.6.1
+ optional: true
+
+ parse-node-version@1.0.1: {}
+
+ pify@4.0.1:
+ optional: true
+
+ prr@1.0.1:
+ optional: true
+
+ safer-buffer@2.1.2:
+ optional: true
+
+ sax@1.6.1:
+ optional: true
+
+ semver@5.7.2:
+ optional: true
+
+ source-map@0.6.1:
+ optional: true
+
+ tslib@2.8.1: {}
diff --git a/e2e/bzlmod/other_module/frontend/pnpm-workspace.yaml b/e2e/bzlmod/other_module/frontend/pnpm-workspace.yaml
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/e2e/bzlmod/other_module/frontend/pnpm-workspace.yaml
diff --git a/npm/private/npm_package_store.bzl b/npm/private/npm_package_store.bzl
index c33ac08..dab5c13 100644
--- a/npm/private/npm_package_store.bzl
+++ b/npm/private/npm_package_store.bzl
@@ -12,8 +12,6 @@
_SUPPORTS_SYMLINK_TARGET_TYPE = bazel_features.rules.symlink_action_has_target_type
-_PACKAGE_STORE_PREFIX_LEN = len("node_modules/{}/".format(utils.package_store_root))
-
_EXTRACT_EXECUTION_REQUIREMENTS = {
"supports-path-mapping": "1",
}
@@ -191,11 +189,8 @@
if not version:
fail("No package version specified to link to. Package version must either be specified explicitly via 'version' attribute or come from the 'src' 'JsInfo|NpmPackageInfo', typically a 'js_library|npm_package' target")
- package_store_prefix_len = _PACKAGE_STORE_PREFIX_LEN
- if ctx.label.package:
- package_store_prefix_len += len(ctx.label.package)
- if ctx.label.repo_name:
- package_store_prefix_len += len(ctx.label.repo_name) + 3 # +3 for ../
+ package_store_prefix = utils.package_store_prefix(ctx.label.repo_name, ctx.label.package)
+ package_store_prefix_len = len(package_store_prefix)
package_key = "{}@{}".format(package, version)
package_store_name = utils.package_store_name(package_key)
@@ -225,14 +220,7 @@
npm_pkg_info = ctx.attr.src[NpmPackageInfo]
# output the package as a TreeArtifact to its package store location
- if ctx.label.repo_name and ctx.label.package:
- expected_short_path = "../{}/{}/{}".format(ctx.label.repo_name, ctx.label.package, package_store_directory_path)
- elif ctx.label.repo_name:
- expected_short_path = "../{}/{}".format(ctx.label.repo_name, package_store_directory_path)
- elif ctx.label.package:
- expected_short_path = "{}/{}".format(ctx.label.package, package_store_directory_path)
- else:
- expected_short_path = package_store_directory_path
+ expected_short_path = "{}{}/node_modules/{}".format(package_store_prefix, package_store_name, package)
src = npm_pkg_info.src
if src.short_path == expected_short_path:
diff --git a/npm/private/test/utils_tests.bzl b/npm/private/test/utils_tests.bzl
index aba47c4..e794213 100644
--- a/npm/private/test/utils_tests.bzl
+++ b/npm/private/test/utils_tests.bzl
@@ -219,6 +219,21 @@
)
return unittest.end(env)
+# buildifier: disable=function-docstring
+def test_package_store_prefix(ctx):
+ env = unittest.begin(ctx)
+
+ asserts.equals(env, "node_modules/.aspect_rules_js/", utils.package_store_prefix("", ""))
+ asserts.equals(env, "frontend/node_modules/.aspect_rules_js/", utils.package_store_prefix("", "frontend"))
+ asserts.equals(env, "../other_module+/node_modules/.aspect_rules_js/", utils.package_store_prefix("other_module+", ""))
+ asserts.equals(env, "../other_module+/frontend/node_modules/.aspect_rules_js/", utils.package_store_prefix("other_module+", "frontend"))
+
+ # the prefix is stripped from a dependency's package store short_path to form the relative symlink target
+ short_path = "../other_module+/frontend/node_modules/.aspect_rules_js/less@4.1.3/node_modules/less"
+ asserts.equals(env, "less@4.1.3/node_modules/less", short_path[len(utils.package_store_prefix("other_module+", "frontend")):])
+
+ return unittest.end(env)
+
t2_test = unittest.make(test_package_store_and_target_name)
t3_test = unittest.make(test_friendly_name)
t6_test = unittest.make(test_parse_package_name)
@@ -226,6 +241,7 @@
t8_test = unittest.make(test_npm_registry_url)
t9_test = unittest.make(test_package_store_name_link_versions)
t10_test = unittest.make(test_hex_to_base64)
+t11_test = unittest.make(test_package_store_prefix)
def utils_tests(name):
unittest.suite(
@@ -237,4 +253,5 @@
t8_test,
t9_test,
t10_test,
+ t11_test,
)
diff --git a/npm/private/utils.bzl b/npm/private/utils.bzl
index 461712c..0f0c4ba 100644
--- a/npm/private/utils.bzl
+++ b/npm/private/utils.bzl
@@ -382,6 +382,18 @@
return "".join(output)
+_PACKAGE_STORE_ROOT = ".aspect_rules_js"
+_PACKAGE_STORE_PREFIX = "node_modules/{}/".format(_PACKAGE_STORE_ROOT)
+
+def _package_store_prefix(repo_name, package):
+ """Path of the package store of a repository package, relative to the main repository."""
+ prefix = _PACKAGE_STORE_PREFIX
+ if package:
+ prefix = "{}/{}".format(package, prefix)
+ if repo_name:
+ prefix = "../{}/{}".format(repo_name, prefix)
+ return prefix
+
utils = struct(
sorted_map = _sorted_map,
friendly_name = _friendly_name,
@@ -389,9 +401,10 @@
importer_to_link = _importer_to_link,
package_repo_name = _package_repo_name,
package_store_name = _package_store_name,
+ package_store_prefix = _package_store_prefix,
make_directory_symlink = _make_directory_symlink,
# Symlinked node_modules structure package store path under node_modules
- package_store_root = ".aspect_rules_js",
+ package_store_root = _PACKAGE_STORE_ROOT,
# Suffix for npm_import links repository
links_repo_suffix = "__links",
# Output group name for the package directory of a linked npm package
