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

Test

All tests were cache hits

278 tests (100.0%) were fully cached saving 37s.

Test

e2e/bzlmod

All tests were cache hits

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

Test

e2e/git_dep_metadata

All tests were cache hits

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

Test

e2e/gyp_no_install_script

All tests were cache hits

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

Test

e2e/js_image_oci

All tests were cache hits

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

Test

e2e/npm_link_package

⚠️ Buildkite build #9665 failed.

Failed tests (2)

//:aspect_test_a_bin_test [k8-fastbuild]🔗
//src:test [k8-fastbuild]🔗

💡 To reproduce the test failures, run

bazel test //:aspect_test_a_bin_test //src:test

Test

e2e/npm_link_package-esm

⚠️ Buildkite build #9665 failed.

Failed tests (2)

//:aspect_test_a_bin_test [k8-fastbuild]🔗
//src:test [k8-fastbuild]🔗

💡 To reproduce the test failures, run

bazel test //:aspect_test_a_bin_test //src:test

Test

e2e/npm_link_package-rerooted

⚠️ Buildkite build #9665 failed.

Failed tests (2)

//root/src:test [k8-fastbuild]🔗
//root:aspect_test_a_bin_test [k8-fastbuild]🔗

💡 To reproduce the test failures, run

bazel test //root:aspect_test_a_bin_test //root/src:test

Test

e2e/npm_translate_lock

All tests were cache hits

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

Test

e2e/npm_translate_lock_disable_hooks

All tests were cache hits

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

Test

e2e/npm_translate_lock_empty

All tests were cache hits

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

Test

e2e/npm_translate_lock_exclude_package_contents

All tests were cache hits

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

Test

e2e/npm_translate_lock_multi

All tests were cache hits

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

Test

e2e/npm_translate_lock_partial_clone

All tests were cache hits

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

Test

e2e/npm_translate_lock_replace_packages

All tests were cache hits

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

Test

e2e/npm_translate_lock_subdir_patch

All tests were cache hits

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

Test

e2e/npm_translate_package_lock

All tests were cache hits

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

Test

e2e/npm_translate_yarn_lock

All tests were cache hits

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

Test

e2e/package_json_module

All tests were cache hits

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

Test

e2e/patch_from_repo

All tests were cache hits

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

Test

e2e/pnpm_lockfiles

All tests were cache hits

67 tests (100.0%) were fully cached saving 9s.

Test

e2e/pnpm_repo_install

All tests were cache hits

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

Test

e2e/pnpm_workspace

All tests were cache hits

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

Test

e2e/pnpm_workspace_deps

All tests were cache hits

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

Test

e2e/pnpm_workspace_rerooted

All tests were cache hits

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

Test

e2e/repo_mapping

All tests were cache hits

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

Test

e2e/rules_foo

⚠️ Buildkite build #9665 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:1078
  throw err;
  ^
 
Error: Cannot find module '@aspect-test/b/package.json'
Require stack:
- /mnt/ephemeral/output/rules_js/e2e_rules_foo/sandbox/linux-sandbox/21/execroot/_main/bazel-out/k8-fastbuild/bin/external/foo/node_modules/.aspect_rules_js/@aspect-test+a@5.0.2/node_modules/@aspect-test/a/index.js
- /mnt/ephemeral/output/rules_js/e2e_rules_foo/sandbox/linux-sandbox/21/execroot/_main/bazel-out/k8-fastbuild/bin/external/foo/node_modules/.aspect_rules_js/@aspect-test+a@5.0.2/node_modules/@aspect-test/a/bin.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1075:15)
    at Module._load (node:internal/modules/cjs/loader:920:27)
    at Module.require (node:internal/modules/cjs/loader:1141:19)
    at require (node:internal/modules/cjs/helpers:110:18)
    at Object.<anonymous> (/mnt/ephemeral/output/rules_js/e2e_rules_foo/sandbox/linux-sandbox/21/execroot/_main/bazel-out/k8-fastbuild/bin/external/foo/node_modules/.aspect_rules_js/@aspect-test+a@5.0.2/node_modules/@aspect-test/a/index.js:2:22)
    at Module._compile (node:internal/modules/cjs/loader:1254:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
    at Module.load (node:internal/modules/cjs/loader:1117:32)
    at Module._load (node:internal/modules/cjs/loader:958:12)
    at Module.require (node:internal/modules/cjs/loader:1141:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/mnt/ephemeral/output/rules_js/e2e_rules_foo/sandbox/linux-sandbox/21/execroot/_main/bazel-out/k8-fastbuild/bin/external/foo/node_modules/.aspect_rules_js/@aspect-test+a@5.0.2/node_modules/@aspect-test/a/index.js',
    '/mnt/ephemeral/output/rules_js/e2e_rules_foo/sandbox/linux-sandbox/21/execroot/_main/bazel-out/k8-fastbuild/bin/external/foo/node_modules/.aspect_rules_js/@aspect-test+a@5.0.2/node_modules/@aspect-test/a/bin.js'
  ]
}
 
Node.js v18.14.2

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

Test

e2e/runfiles

All tests were cache hits

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

Test

e2e/stamped_package_json

All tests were cache hits

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

Test

e2e/vendored_node

All tests were cache hits

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

Test

e2e/vendored_tarfile

All tests were cache hits

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

Test

e2e/verify_patches

All tests were cache hits

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

Test

e2e/worker

All tests were cache hits

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

Test

e2e/workspace

All tests were cache hits

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

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

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_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_workspace

e2e/pnpm_workspace_deps

e2e/pnpm_workspace_rerooted

e2e/repo_mapping

e2e/rules_foo

e2e/runfiles

e2e/stamped_package_json

e2e/vendored_node

e2e/vendored_tarfile

e2e/verify_patches

e2e/worker

e2e/workspace

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