Optimize package store symlink creation #2401

dzbarsky · 2025-10-18T16:53:47Z

We waste a fair bit of analysis time computing what these symlink targets should be, but we can do this much more directly.

Before:

After:

Changes are visible to end-users: no

Test plan

existing tests

aspect-workflows · 2025-10-18T16:56:15Z

Bazel 7 (Test)

All tests were cache hits

281 tests (100.0%) were fully cached saving 41s.

Bazel 8 (Test)

All tests were cache hits

242 tests (100.0%) were fully cached saving 30s.

Bazel 7 (Test)

e2e/bzlmod

All tests were cache hits

5 tests (100.0%) were fully cached saving 588ms.

Bazel 7 (Test)

e2e/git_dep_metadata

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Bazel 7 (Test)

e2e/gyp_no_install_script

All tests were cache hits

2 tests (100.0%) were fully cached saving 196ms.

Bazel 7 (Test)

e2e/js_image_oci

All tests were cache hits

1 test (100.0%) was fully cached saving 6s.

Bazel 7 (Test)

e2e/npm_link_package

All tests were cache hits

2 tests (100.0%) were fully cached saving 298ms.

Bazel 7 (Test)

e2e/npm_link_package-esm

All tests were cache hits

2 tests (100.0%) were fully cached saving 200ms.

Bazel 7 (Test)

e2e/npm_link_package-rerooted

All tests were cache hits

2 tests (100.0%) were fully cached saving 196ms.

Bazel 7 (Test)

e2e/npm_translate_lock

All tests were cache hits

3 tests (100.0%) were fully cached saving 634ms.

Bazel 7 (Test)

e2e/npm_translate_lock_disable_hooks

All tests were cache hits

3 tests (100.0%) were fully cached saving 227ms.

Bazel 7 (Test)

e2e/npm_translate_lock_empty

All tests were cache hits

2 tests (100.0%) were fully cached saving 110ms.

Bazel 7 (Test)

e2e/npm_translate_lock_exclude_package_contents

All tests were cache hits

1 test (100.0%) was fully cached saving 34ms.

Bazel 7 (Test)

e2e/npm_translate_lock_link_workspace

⚠️ Buildkite build #11047 failed.

//foo:run_main failed to build

main failed: error executing JsRunBinary command (from target //foo:run_main) bazel-out/k8-opt-exec-ST-d57f47055a04/bin/foo/main_/main
 
Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
 
node:internal/modules/cjs/loader:1210
  throw err;
  ^
 
Error: Cannot find module '@aspect-test/b/package.json'
Require stack:
- /mnt/ephemeral/output/rules_js/e2e_npm_translate_lock_link_workspace/sandbox/linux-sandbox/9/execroot/_main/bazel-out/k8-fastbuild/bin/external/aspect_rules_js~~npm~foo/node_modules/.aspect_rules_js/@[email protected]/node_modules/@aspect-test/a/index.js
- /mnt/ephemeral/output/rules_js/e2e_npm_translate_lock_link_workspace/sandbox/linux-sandbox/9/execroot/_main/bazel-out/k8-fastbuild/bin/external/aspect_rules_js~~npm~foo/node_modules/.aspect_rules_js/@[email protected]/node_modules/@aspect-test/a/bin.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1207:15)
    at Module._load (node:internal/modules/cjs/loader:1038:27)
    at Module.require (node:internal/modules/cjs/loader:1289:19)
    at require (node:internal/modules/helpers:182:18)
    at Object.<anonymous> (/mnt/ephemeral/output/rules_js/e2e_npm_translate_lock_link_workspace/sandbox/linux-sandbox/9/execroot/_main/bazel-out/k8-fastbuild/bin/external/aspect_rules_js~~npm~foo/node_modules/.aspect_rules_js/@[email protected]/node_modules/@aspect-test/a/index.js:2:22)
    at Module._compile (node:internal/modules/cjs/loader:1521:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1623:10)
    at Module.load (node:internal/modules/cjs/loader:1266:32)
    at Module._load (node:internal/modules/cjs/loader:1091:12)
    at Module.require (node:internal/modules/cjs/loader:1289:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/mnt/ephemeral/output/rules_js/e2e_npm_translate_lock_link_workspace/sandbox/linux-sandbox/9/execroot/_main/bazel-out/k8-fastbuild/bin/external/aspect_rules_js~~npm~foo/node_modules/.aspect_rules_js/@[email protected]/node_modules/@aspect-test/a/index.js',
    '/mnt/ephemeral/output/rules_js/e2e_npm_translate_lock_link_workspace/sandbox/linux-sandbox/9/execroot/_main/bazel-out/k8-fastbuild/bin/external/aspect_rules_js~~npm~foo/node_modules/.aspect_rules_js/@[email protected]/node_modules/@aspect-test/a/bin.js'
  ]

... 20 chars truncated

Failed tests (1)

//foo:test [k8-fastbuild]🔗

💡 To reproduce the build failures, run

bazel build //foo:run_main

💡 To reproduce the test failures, run

bazel test //foo:test

Bazel 7 (Test)

e2e/npm_translate_lock_multi

All tests were cache hits

2 tests (100.0%) were fully cached saving 164ms.

Bazel 7 (Test)

e2e/npm_translate_lock_partial_clone

All tests were cache hits

1 test (100.0%) was fully cached saving 61ms.

Bazel 7 (Test)

e2e/npm_translate_lock_replace_packages

All tests were cache hits

4 tests (100.0%) were fully cached saving 472ms.

Bazel 7 (Test)

e2e/npm_translate_lock_subdir_patch

All tests were cache hits

1 test (100.0%) was fully cached saving 83ms.

Bazel 7 (Test)

e2e/npm_translate_package_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Bazel 7 (Test)

e2e/npm_translate_yarn_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Bazel 7 (Test)

e2e/package_json_module

All tests were cache hits

1 test (100.0%) was fully cached saving 239ms.

Bazel 7 (Test)

e2e/patch_from_repo

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Bazel 7 (Test)

e2e/pnpm_lockfiles

All tests were cache hits

35 tests (100.0%) were fully cached saving 4s.

Bazel 7 (Test)

e2e/pnpm_repo_install

All tests were cache hits

1 test (100.0%) was fully cached saving 838ms.

Bazel 7 (Test)

e2e/pnpm_version

All tests were cache hits

1 test (100.0%) was fully cached saving 56ms.

Bazel 7 (Test)

e2e/pnpm_workspace

All tests were cache hits

15 tests (100.0%) were fully cached saving 2s.

Bazel 7 (Test)

e2e/pnpm_workspace_deps

All tests were cache hits

3 tests (100.0%) were fully cached saving 366ms.

Bazel 7 (Test)

e2e/pnpm_workspace_rerooted

All tests were cache hits

15 tests (100.0%) were fully cached saving 3s.

Bazel 7 (Test)

e2e/repo_mapping

All tests were cache hits

3 tests (100.0%) were fully cached saving 419ms.

Bazel 7 (Test)

e2e/runfiles

All tests were cache hits

1 test (100.0%) was fully cached saving 101ms.

Bazel 7 (Test)

e2e/stamped_package_json

All tests were cache hits

1 test (100.0%) was fully cached saving 44ms.

Bazel 7 (Test)

e2e/vendored_node

All tests were cache hits

1 test (100.0%) was fully cached saving 107ms.

Bazel 7 (Test)

e2e/vendored_tarfile

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Bazel 7 (Test)

e2e/verify_patches

All tests were cache hits

2 tests (100.0%) were fully cached saving 109ms.

Bazel 7 (Test)

e2e/worker

All tests were cache hits

1 test (100.0%) was fully cached saving 109ms.

Buildifier Format

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2025-10-18T16:57:08Z

npm/private/npm_package_store.bzl

+                    target = dep_ref_def_package_store_directory.short_path[_PACKAGE_STORE_PREFIX_LEN:]
                    for dep_ref_dep_alias in dep_ref_dep_aliases:
                        # "node_modules/{package_store_root}/{package_store_name}/node_modules/{package}"
                        dep_ref_dep_symlink_path = "node_modules/{}/{}/node_modules/{}".format(utils.package_store_root, dep_package_store_name, dep_ref_dep_alias)
-                        files.append(utils.make_symlink(ctx, dep_ref_dep_symlink_path, dep_ref_def_package_store_directory.path))
+                        symlink = ctx.actions.declare_symlink(dep_ref_dep_symlink_path)
+                        ctx.actions.symlink(
+                            output = symlink,
+                            target_path = ("../../.." if "/" in dep_ref_dep_alias else "../..") + target,


Construct relative target path from wrong substring

The new symlink code slices dep_ref_def_package_store_directory.short_path using a fixed _PACKAGE_STORE_PREFIX_LEN that only accounts for node_modules/.aspect_rules_js. A package store directory’s short_path is actually prefixed by the rule’s package path (e.g. npm/private/node_modules/.aspect_rules_js/...), so slicing by a fixed offset removes part of the .aspect_rules_js prefix and often the leading slash. The resulting target_path becomes ../../..rules_js/... instead of ../../../.aspect_rules_js/..., producing broken symlinks for any npm package store defined in a subpackage rather than the workspace root. This will cause transitive dependencies to be unresolved. Consider computing the relative path with paths.relative_file or by trimming the substring starting after the first occurrence of node_modules/{package_store_root}.

Useful? React with 👍 / 👎.

dzbarsky requested a review from jbedard October 18, 2025 16:55

chatgpt-codex-connector bot reviewed Oct 18, 2025

View reviewed changes

dzbarsky force-pushed the zbarsky/symlink branch from 916a3d6 to 33b0187 Compare October 18, 2025 17:09

Optimize package store symlink creation

9d073e3

dzbarsky force-pushed the zbarsky/symlink branch from 33b0187 to 9d073e3 Compare October 18, 2025 17:14

dzbarsky changed the base branch from main to 3.x November 27, 2025 13:16

Merge branch '3.x' into zbarsky/symlink

a94a9ae

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Optimize package store symlink creation #2401

Optimize package store symlink creation #2401

Uh oh!

dzbarsky commented Oct 18, 2025

Uh oh!

aspect-workflows bot commented Oct 18, 2025 •

edited

Loading

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Oct 18, 2025

Uh oh!

dzbarsky Oct 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Optimize package store symlink creation #2401

Are you sure you want to change the base?

Optimize package store symlink creation #2401

Uh oh!

Conversation

dzbarsky commented Oct 18, 2025

Changes are visible to end-users: no

Test plan

Uh oh!

aspect-workflows bot commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

e2e/bzlmod

e2e/git_dep_metadata

e2e/gyp_no_install_script

e2e/js_image_oci

e2e/npm_link_package

e2e/npm_link_package-esm

e2e/npm_link_package-rerooted

e2e/npm_translate_lock

e2e/npm_translate_lock_disable_hooks

e2e/npm_translate_lock_empty

e2e/npm_translate_lock_exclude_package_contents

e2e/npm_translate_lock_link_workspace

e2e/npm_translate_lock_multi

e2e/npm_translate_lock_partial_clone

e2e/npm_translate_lock_replace_packages

e2e/npm_translate_lock_subdir_patch

e2e/npm_translate_package_lock

e2e/npm_translate_yarn_lock

e2e/package_json_module

e2e/patch_from_repo

e2e/pnpm_lockfiles

e2e/pnpm_repo_install

e2e/pnpm_version

e2e/pnpm_workspace

e2e/pnpm_workspace_deps

e2e/pnpm_workspace_rerooted

e2e/repo_mapping

e2e/runfiles

e2e/stamped_package_json

e2e/vendored_node

e2e/vendored_tarfile

e2e/verify_patches

e2e/worker

Buildifier Format

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

aspect-workflows bot commented Oct 18, 2025 •

edited

Loading