Skip to content

Address PR #942 review comments + buildifier fix#960

Draft
thesayyn wants to merge 23 commits intonew_layeringfrom
claude/review-pr-942-gZbpc
Draft

Address PR #942 review comments + buildifier fix#960
thesayyn wants to merge 23 commits intonew_layeringfrom
claude/review-pr-942-gZbpc

Conversation

@thesayyn
Copy link
Copy Markdown
Member

@thesayyn thesayyn commented May 1, 2026

Addresses review comments on #942 and the buildifier CI failure. Targets new_layering so the changes flow into the existing PR.

Changes

  • normalize_label (per @jbedard): replaced the hardcoded aspect_rules_py++uv+whl_install__ prefix match with an _extract_whl_install_pkg helper anchored on the stable whl_install__ substring, so the same code path works under both Bazel 8 (+) and Bazel 9 (~) module-extension separators.
  • _py_image_layer_impl (per @akafael):
    • Collapsed seen_labels + all_pkgs list + pkg_by_label rebuild into a single dedup-by-label dict pass; all_pkgs = pkg_by_label.values().
    • Dropped the dep_tars double-bookkeeping inside the loops — snapshot dep_tars = list(all_tars) once before the source layer is appended.
  • py/private/BUILD.bazel (per @jbedard): dropped the @rules_python//python:defs.bzl load; the validator now uses rules_py's own py_binary rule.
  • py/tests/internal-deps/adder/BUILD.bazel (per @jbedard): reverted visibility back to the prior explicit list. Flagging that this may break e2e/cases/oci/{py_image_layer,py_venv_image_layer} consumers — happy to widen the list if CI complains.
  • Buildifier: blank line at the interpreter-layer block, multi-line _declare_group_tar calls split per arg.

Test plan

  • CI green on Bazel 8 and Bazel 9 (the bazel-9 separator fix should unblock the multi-member group path under Bazel 9).
  • //cases/interpreter-features-836:test and //cases/oci/py_venv_image_layer:py_amd64_image_command_test — the venv command test was Bazel-9-only and may resolve via the separator fix; the interpreter-features test is independent and worth checking buildkite logs for.

Generated by Claude Code

jbedard and others added 13 commits April 24, 2026 02:48
### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases
### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases
… py_binary (#949)

### Changes are visible to end-users: no

### Test plan

- Covered by existing tests
### Changes are visible to end-users: no

### Test plan

- New test cases added
### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases
### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases
### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases
Added note about using pytest_main and wrapper macro for py_test.

---

### Changes are visible to end-users: yes/no

<!-- If no, please delete this section. -->

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases
### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases
Add some tests now so #944 highlights changes

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

Co-authored-by: Greg Magolan <greg@aspect.build>
Add a UV module extension/toolchain for fetching UV via bazel.

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

The `uv` module extension's new `toolchain()` tag downloads a hermetic
uv and publishes `@uv` (host alias) plus `@uv//:all` (per-platform
toolchains).

### Test plan

- Covered by existing test cases
- New test cases added
- Manual testing; `bazel run @UV -- --help`
This provider has been dead code since b6de8cb
The `LIB_MODE` was added more recently in
76548d2

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 1, 2026

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ hofbi
❌ claude
You have signed the CLA already but the status is still pending? Let us recheck it.

@jbedard
Copy link
Copy Markdown
Member

jbedard commented May 1, 2026

Please dont spam like this lol 🙏

thesayyn and others added 5 commits May 1, 2026 21:27
- normalize_label: handle Bazel 8 (`+`) and Bazel 9 (`~`) module-extension
  separators by anchoring on `whl_install__` instead of the full prefix.
- _py_image_layer_impl: collapse double-bookkeeping of all_tars/dep_tars
  into a single snapshot before the source layer is appended; simplify
  pip dedup to a single dict pass (per @akafael).
- py/private/BUILD.bazel: drop @rules_python load and use rules_py's own
  py_binary rule for the validator binary (per @jbedard).
- adder visibility: revert to the prior explicit list (per @jbedard).
- buildifier: split multi-line _declare_group_tar calls; blank-line fix.
@thesayyn thesayyn force-pushed the claude/review-pr-942-gZbpc branch from 0fa9da8 to 46ea075 Compare May 1, 2026 21:27
@aspect-workflows
Copy link
Copy Markdown

aspect-workflows Bot commented May 1, 2026

Bazel 8 (Test)

All tests were cache hits

123 tests (100.0%) were fully cached saving 60s.


Bazel 9 (Test)

All tests were cache hits

122 tests (100.0%) were fully cached saving 1m 5s.


Bazel 8 (Test)

e2e

1 test target passed

Targets
//cases/interpreter-features-836:test [k8-fastbuild]5s

Total test execution time was 5s. 66 tests (98.5%) were fully cached saving 1m 10s.


Bazel 9 (Test)

e2e

⚠️ Buildkite build #3378 failed.

Failed tests (2)
//cases/interpreter-features-836:test [k8-fastbuild]🔗
//cases/oci/py_venv_image_layer:py_amd64_image_command_test [k8-fastbuild]🔗

💡 To reproduce the test failures, run

bazel test //cases/interpreter-features-836:test //cases/oci/py_venv_image_layer:py_amd64_image_command_test

Bazel 8 (Test)

examples/uv_pip_compile

All tests were cache hits

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


Buildifier

claude added 5 commits May 1, 2026 21:58
The e2e cases (cases/oci/py_image_layer, py_venv_image_layer) live in a
separate Bazel module via local_path_override. They depend on
@aspect_rules_py//py/tests/internal-deps/adder, which requires
cross-module visibility — only //visibility:public satisfies that.

Reverting the explicit list breaks e2e analysis with:
  target '@@aspect_rules_py+//py/tests/internal-deps/adder:adder' is not
  visible from target '//cases/oci/py_image_layer:my_app_bin'

This was the original change in #942; restoring it with a comment
explaining why public is required for this fixture.
main commits f1fec29 (refactor: do not duplicate site_packages into
DefaultInfo+runfiles of py_binary) and e735612 (fix: check main by
resolved file basename) shifted the py_binary launcher script content
by 6 bytes. Refresh the three py_image_layer goldens that pin its size.

Venv launcher goldens (py_venv_image_layer) likely need similar updates
but the buildkite log didn't surface their specific diffs; will iterate
on the next CI run.
main commit 6dc87d5 (chore: upgrade llvm module to 0.7.1) changed how
`--stripopt=--strip-all` strips the venv-staged python binary on
linux amd64. Refresh the snapshot. Arm64 likely needs similar but
the buildkite log only surfaced amd64's diff; iterating.
The new py_image_layer puts the binary at ./app.runfiles/_main/<short_path>
by default. The test had the binary at /cases/interpreter-features-836/
check_no_turtle and the venv at .../check_no_turtle.runfiles/_main/...
which never matched the actual layout (the file-existence tests were
trivially passing on bogus non-existent paths).

Mirror what cases/oci/py_venv_image_layer does: remap to root=/app via
strip_prefix, run the binary as /app, and check venv files at
/app.runfiles/_main/cases/interpreter-features-836/.no-turtle-test/...
which is where py_image_layer actually lays them out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants