refactor: group fp package linking alongside third-party #2427

jbedard · 2025-11-06T08:17:46Z

Generate factory methods instead of inline code within npm_link_all_packages to better align first-party package linking with third-party.

Changes are visible to end-users: no

Test plan

Covered by existing test cases

aspect-workflows · 2025-11-06T08:21:03Z

Test

3 test targets passed

Targets

//npm/private/test:write_npm_translate_lock_0_test [k8-fastbuild] 174ms
//npm/private:_test_gendocs_0_0 [k8-fastbuild]                    105ms
//npm/private:_test_gendocs_0_1 [k8-fastbuild]                    44ms

Total test execution time was 323ms. 291 tests (99.0%) were fully cached saving 48s.

Test

e2e/bzlmod

All tests were cache hits

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

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 184ms.

Test

e2e/js_image_oci

All tests were cache hits

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

Test

e2e/npm_link_package

All tests were cache hits

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

Test

e2e/npm_link_package-esm

All tests were cache hits

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

Test

e2e/npm_link_package-rerooted

All tests were cache hits

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

Test

e2e/npm_translate_lock

All tests were cache hits

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

Test

e2e/npm_translate_lock_disable_hooks

All tests were cache hits

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

Test

e2e/npm_translate_lock_empty

All tests were cache hits

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

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

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

Test

e2e/npm_translate_lock_subdir_patch

All tests were cache hits

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

Test

e2e/npm_translate_package_lock

Buildkite build #10009 is running...

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 313ms.

Test

e2e/patch_from_repo

All tests were cache hits

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

Test

e2e/pnpm_lockfiles

5 test targets passed

Targets

//v101:repos_0_test [k8-fastbuild]                                89ms
//v54:repos_0_test [k8-fastbuild]                                 112ms
//v60:repos_0_test [k8-fastbuild]                                 136ms
//v61:repos_0_test [k8-fastbuild]                                 152ms
//v90:repos_0_test [k8-fastbuild]                                 151ms

Total test execution time was 640ms. 62 tests (92.5%) were fully cached saving 7s.

Test

e2e/pnpm_repo_install

All tests were cache hits

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

Test

e2e/pnpm_workspace

1 test target passed

Targets

//:repos_test [k8-fastbuild]                                      137ms

Total test execution time was 137ms. 14 tests (93.3%) were fully cached saving 2s.

Test

e2e/pnpm_workspace_deps

All tests were cache hits

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

Test

e2e/pnpm_workspace_rerooted

1 test target passed

Targets

//:repos_test [k8-fastbuild]                                      158ms

Total test execution time was 158ms. 14 tests (93.3%) were fully cached saving 4s.

Test

e2e/repo_mapping

All tests were cache hits

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

Test

e2e/rules_foo

All tests were cache hits

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

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

dzbarsky · 2025-11-06T08:24:37Z

e2e/pnpm_lockfiles/v101/snapshots/defs.bzl

            link_68("{}/uvu".format(name), False, name, "uvu")
            link_targets.append(":{}/uvu".format(name))
+            _fp_link_0(name)
+            link_targets.append(":{}/@scoped/c".format(name))


ideally we'd generate a bit tighter code here and didn't keep checking for @scoped not in scope_targets

That code has simply moved and is not new. But now it is alongside the third-party code (which does the exact same thing) so I hope we can cleanup both together now, coming soon...

dzbarsky · 2025-11-06T08:26:50Z

e2e/pnpm_lockfiles/v101/snapshots/defs.bzl

+# Generated npm_link_package_store for linking of first-party "@scoped/c" package
+# buildifier: disable=function-docstring
+def _fp_link_0(name):
+    _npm_link_package_store(


probably want to package these up into a macro in the rules_js repo and invoke that with the extra args it needs, to generate less code?

You mean similar to what _LINK_JS_PACKAGE_LINK_IMPORTED_STORE_TMPL in npm_import.bzl is doing?

dzbarsky · 2025-11-06T08:30:52Z

npm/private/npm_translate_lock_generate.bzl

+        for link_package in all_fp_link_packages.keys():
+            if link_package not in links_pkg_bzl:
+                links_pkg_bzl[link_package] = []
+            links_pkg_bzl[link_package].append("""            _fp_link_{i}(name)""".format(i = i))


can we arrange to do this first so link_targets is empty before this, and then the below will essentially reduce to scope_targets["{package_scope}"] = list(link_targets)?

Hmm... now that this PR is moving all first+third-party into a single if bazel_package == "x" block I think you're right... it can probably change to a single assignment (per scope) instead of all this appending.

That will probably be in a followup...

dzbarsky

feel free to do the codegen cleanups in a followup

refactor: group fp package linking alongside third-party

a64ebad

jbedard requested review from Mivr and dzbarsky November 6, 2025 08:17

dzbarsky reviewed Nov 6, 2025

View reviewed changes

dzbarsky approved these changes Nov 6, 2025

View reviewed changes

jbedard merged commit 7378e38 into aspect-build:main Nov 6, 2025
102 checks passed

jbedard deleted the locals-reorder2 branch November 6, 2025 16:38

Uh oh!

refactor: group fp package linking alongside third-party #2427

refactor: group fp package linking alongside third-party #2427

Uh oh!

Conversation

jbedard commented Nov 6, 2025

Changes are visible to end-users: no

Test plan

Uh oh!

aspect-workflows bot commented Nov 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzbarsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aspect-workflows bot commented Nov 6, 2025 •

edited

Loading