Skip to content

refactor: group fp package store targets alongside third-party#2425

Merged
jbedard merged 1 commit intoaspect-build:mainfrom
jbedard:locals-reorder
Nov 6, 2025
Merged

refactor: group fp package store targets alongside third-party#2425
jbedard merged 1 commit intoaspect-build:mainfrom
jbedard:locals-reorder

Conversation

@jbedard
Copy link
Copy Markdown
Member

@jbedard jbedard commented Nov 6, 2025 •
edited
Loading

To align the first-party and third-party code more, in prep for more cleanup+refactoring.

There should be no functional changes other then the _npm_local_package_store invocations now all being done before the _npm_link_package_store and link_targets.append(...) which should have no side-effects.

Changes are visible to end-users: no

Test plan

  • Covered by existing test cases

@jbedard jbedard requested review from Mivr and dzbarsky November 6, 2025 07:18
@chatgpt-codex-connector
Copy link
Copy Markdown

chatgpt-codex-connector bot commented Nov 6, 2025

💡 Codex Review

rules_js/npm/private/npm_translate_lock_generate.bzl

Lines 459 to 469 in 7f05caf

# Generate a single _FP_DIRECT_TMPL block with all link packages
if len(all_fp_link_packages) > 0:
links_bzl.append(_FP_DIRECT_TMPL.format(
link_packages = list(all_fp_link_packages.keys()),
link_visibility = package_visibility,
pkg = fp_package,
package_directory_output_group = utils.package_directory_output_group,
root_package = root_package,
package_store_name = utils.package_store_name(fp_package, "0.0.0"),
package_store_root = utils.package_store_root,
))

P1 Badge Ensure links list is initialised before appending first‑party blocks

After the refactor links_bzl remains a dict unless at least one imported (third‑party) link package exists; it is converted to a list only inside the if len(links_bzl) > 0: block earlier in the function. The new first‑party logic unconditionally calls links_bzl.append(...) when all_fp_link_packages is non‑empty, so a workspace with only first‑party packages now crashes with AttributeError: 'dict' object has no attribute 'append' during npm_translate_lock_generate. Previously these statements were appended directly to npm_link_all_packages_bzl and worked regardless of imported packages. Consider converting links_bzl to a list before this section or initialising it as a list so first‑party link handling does not depend on third‑party packages being present.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@aspect-workflows
Copy link
Copy Markdown

aspect-workflows bot commented Nov 6, 2025 •
edited
Loading

Test

2 test targets passed

Targets 
//npm/private:_test_gendocs_0_0 [k8-fastbuild] 95ms
//npm/private:_test_gendocs_0_1 [k8-fastbuild] 79ms

Total test execution time was 174ms. 292 tests (99.3%) 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

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

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 8s.

Test

e2e/pnpm_repo_install

All tests were cache hits

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

Test

e2e/pnpm_workspace

All tests were cache hits

15 tests (100.0%) 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

All tests were cache hits

15 tests (100.0%) 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

@jbedard jbedard force-pushed the locals-reorder branch 3 times, most recently from 72c9e8b to 85bef64 Compare November 6, 2025 07:29
@jbedard jbedard enabled auto-merge (rebase) November 6, 2025 08:09
@jbedard jbedard disabled auto-merge November 6, 2025 08:12
@jbedard jbedard merged commit 701306b into aspect-build:main Nov 6, 2025
102 checks passed
@jbedard jbedard deleted the locals-reorder branch November 6, 2025 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mivr Mivr Mivr approved these changes

@dzbarsky dzbarsky Awaiting requested review from dzbarsky

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jbedard @Mivr