feat(py_image_layer): support multiple binaries#1353
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daa872cc78
ℹ️ 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".
|
Explain why this is better then #1349? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17572a8163
ℹ️ 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".
17572a8 to
749ce46
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 749ce46b3e
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c8793cc39
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98a9b6c1c0
ℹ️ 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".
|
#1349 compares normalized labels/group names and emits only the primary dependency and interpreter layers, so a secondary launcher from another lock or Python configuration can silently receive the wrong runtime; it also shifts existing positional arguments. #1353 unions concrete wheel, first-party, and interpreter artifacts, validates the complete expanded runfile layout, preserves positional and select-based callers, supports |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34073df11e
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fe44706de
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88118212fd
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 039b10b8d4
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8830e17f3e
ℹ️ 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".
|
Also please tidy up this PR, diff is huge, and its touching code that it probably shouldn't have. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Agreed. I’m reducing this to the multi-binary ownership/API change and one execution-level fixture. I’ll move the generalized mtree normalization/collision machinery and its configured-tree/wheel/interpreter harness out of this PR, keep the existing scalar action behavior, and retain only the concrete closure union plus launcher/alias handling required for two runtimes. I’ll also add the strip_prefix regression before requesting another review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d29fb51c8
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd83dc4c1e
ℹ️ 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".
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Re-reviewed cd83dc4c1e7e084abf0bebac07a237450920bb79 after the previous changes-requested head was updated.
The scalar external-launcher repository-mapping issue is fixed: I independently executed this commit's exact helpers and verified that both Bazel 8 and Bazel 9 external repository names now stage the mapping at ./app.runfiles/_repo_mapping.
Requesting changes for the remaining, independently reproduced cross-layer mtree parsing issue in the existing discussion: #1353 (comment).
Running this commit's actual mapping-branch match/split against real mtree rows confirms that grouped destinations containing content=, contents=, or link= are indexed under the destination substring instead of the trailing source metadata. The bug also affects ordinary valid filenames such as step=123.txt: split(content_field, parts, "=") truncates the real execution path at the filename's =. A normal-path control indexes correctly; all four affected cases miss the real symlink target and produce a dangling cross-layer link.
Please restrict metadata matching to fields after the destination, split only at the first =, and add grouped, cross-layer regression coverage for marker-bearing and ordinary =-containing paths. All 47 current-head checks are green; the requested changes are for this reproduced runtime-correctness gap, not CI.
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Re-reviewed c0e878e411b1b196f65f747d514a3c2b34179041 immediately after the previous changes-requested head was updated.
The cross-layer mtree parsing issue from the previous review is fixed: independently executing the updated parser confirms that content=, contents=, and link= in destination paths and ordinary =-containing source filenames now retain the correct symlink target. The earlier scalar repository-mapping fix also remains intact.
Requesting changes for the remaining independently reproduced external-launcher strip-prefix defect in the existing discussion: #1353 (comment).
Executing this commit's exact _apply_strip_prefix, _source_destination, _file_to_mtree_entry, and _source_file_to_mtree demonstrates that a single external executable without launcher_dir, with a configured directory-level strip prefix, is staged as ./app rather than ./app/external_launcher:
Bazel 8 short_path: ../aspect_rules_py+/py/tests/internal-deps/adder/external_launcher
Bazel 8 strip_prefix: ../aspect_rules_py+/py/tests/internal-deps/adder
Bazel 9 short_path: ../aspect_rules_py~/py/tests/internal-deps/adder/external_launcher
Bazel 9 strip_prefix: ../aspect_rules_py~/py/tests/internal-deps/adder
expected: ./app/external_launcher
actual: ./app
The equivalent main-repository directory prefix, an explicit launcher_dir, the intentional no-prefix external case, and both repository-mapping controls pass. The external-path fallback runs before the explicitly configured directory prefix, silently discarding the launcher's basename and breaking the documented image entrypoint.
Please honor the configured directory strip_prefix before the generic external-launcher fallback while preserving exact mapping and no-prefix behavior, and add an extracted-image/runtime regression with a single external binary and a directory-level strip prefix. Required current-head CI is still running; the requested change is for the reproduced source-level runtime defect, not CI.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b73ae523f9
ℹ️ 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".
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Re-reviewed b73ae523f9676710924b7a910784cfe65fbb4e5a at the author's explicit request after independently checking all 47 passing head-specific checks, all 49 review discussions, the full cumulative change, and the fixed external-launcher/mapping regressions.
Two independently reproduced source-level blockers remain:
-
The new inline review comment demonstrates the active source-row AWK defect using this commit's actual mtree generator, the complete committed AWK, two input streams, and a real filesystem symlink. Clean hot/slow controls pass, while valid marker-bearing source aliases produce a dangling host path or silently stop being symlinks.
-
The distinct, already-reported launcher/runfiles defect remains current: #1353 (comment). Executing the exact helpers with
executable_dsts = {"pkg/app": "./app/bin/app"}andstrip_prefix = "pkg"placespkg/main.pyat./app/main.pyinstead of./app.runfiles/_main/pkg/main.py; nested data is misplaced in the same way. The relocated native launcher therefore cannot resolve its main module fromRUNFILES_DIR. I am referencing the existing discussion instead of adding a duplicate inline.
The previously reported external directory prefix, scalar repository mapping, and cross-layer target-metadata issues are fixed. The new changes request is based on the two reproduced remaining runtime failures, not on CI; all 47 head-specific checks are green.
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Re-reviewed 7adce7e039f5fa201be37d718daf5c93e69f914b because the author updated the previously changes-requested PR.
The relocated-launcher runfiles issue from the previous review is fixed. I independently executed this commit's actual Starlark helpers and verified that local main runfiles now remain at ./app.runfiles/_main/pkg/main.py, while both Bazel 8 and Bazel 9 external-prefix and repository-mapping controls continue to pass.
Requesting changes because the separate source-row symlink defect remains current and unchanged in the existing discussion: #1353 (comment).
The complete modify_mtree.awk blob is byte-for-byte identical to the last reviewed commit. Re-running this exact immutable script against real mtree rows generated by _file_to_mtree_entry, two input streams, and the existing /usr/bin/python3 -> python3.12 symlink again gives:
normal hot alias: type=link link=../pkg/python3.12
normal slow alias: type=link link=../pkg/python3.12
grouped/link=alias: type=link link=/usr/bin/python3 (dangling)
grouped/content=alias: type=file content=/usr/bin/python3 (flattened)
Please parse source-row metadata only after the destination, preserve everything after the first =, and anchor the END replacements to metadata fields as well; add extracted-image hot- and slow-symlink regressions with marker-bearing source filenames. The current grouped regression only exercises a marker-bearing target.
The current Bazel 8 end-to-end check is also failing and other checks remain in progress, but the changes request is independently grounded in the reproduced runtime bug above; no CI failure is assumed to have the same root cause.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7adce7e039
ℹ️ 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".
Keep explicit scalar prefixes effective for sibling runfiles without changing relocated, external, or nonmatching layouts. Parse and replace only mtree metadata fields so marker text and equals signs in destinations cannot corrupt symlink rewriting.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8869593833
ℹ️ 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".
Provide each source and non-prebuilt first-party tar with mapping-only destinations from the other emitted source tiers, without duplicating payload bytes. Detect target-file symlinks in rule groups and exercise both source-to-group and group-to-source links through real extraction.
|
@codex review — tamirdex |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Re-reviewed bed76baa4c04a5214fc9bd525cac2ac411256676 against the true fcff60cf merge base and all 26 cumulative files.
The preceding source-row AWK, scalar sibling-runfiles, and ordinary grouped source-to-source symlink findings are fixed. Requesting changes for the new interpreter-layer correctness issue in the inline review.
I independently executed each revision’s exact Starlark mappers and complete committed AWK against a real existing Bazel target-file symlink. With a separate interpreter layer, the merge base emits a working type=file contents=... alias; this head emits a dangling type=link link=bazel-out/... because the prebuilt interpreter is absent from the shared destination mapping. Adding the actual interpreter-mapper row as a mapping-only input produces the correct link=../../python_repo/bin/python3.12 without duplicating the interpreter payload.
The existing extraction fixture checks links only among default, rule-group, and first-party source tiers, not a link targeting interpreter_group. Please include interpreter-layer files and their actual mapper in the cross-layer references and cover the supported public-toolchain target_file case. The 47 head-specific checks are passing; this changes request is based on the reproduced runtime defect, not CI.
A rule-group target-file symlink can point into a separately emitted interpreter layer. Include the interpreter destinations in cross-tier mapping rows so extraction preserves a resolvable link while the prebuilt interpreter tar remains the sole payload owner.
|
@codex review — tamirdex |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Re-reviewed a82c9dcee5f5ce023cb8cc1001097c5eb25a70b7 against the true merge base and all 26 cumulative files in response to the latest review request.
The previously reported interpreter-layer target-file/symlink defect is fixed. The new regression test extracts the actual image layers, executes the symlinked interpreter, and verifies that the interpreter payload is not duplicated. I found no remaining actionable source-level issue.
Holding approval because the required test-all check is still failing: the Bazel 9 examples/py_pex_binary matrix job received 500 Internal Server Error while downloading the Python standalone SHA256SUMS, and the required aggregator propagated that matrix failure. Both the interpreter extension and the PEX example are unchanged from the merge base. This is an externally caused required-check failure, not a new requested code change.
Once the required check succeeds, the previous changes-requested source finding is resolved.
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Re-reviewed the full 26-file change at a82c9dcee5f5ce023cb8cc1001097c5eb25a70b7 against the true merge base. The previously requested cross-layer interpreter-symlink, runfiles, and mtree issues are fixed; the extraction regression executes the interpreter and verifies its payload is emitted exactly once.
The remaining Bazel 9 PEX failure is not a code regression: the job failed fetching the unchanged Python standalone SHA256SUMS because GitHub returned HTTP 500; the same example and interpreter code pass on the merge base. test-all only propagates that infrastructure failure.
Approving the source changes. Required checks must still pass before merge.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2352e3991
ℹ️ 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".
4 participants
Build a single OCI image from multiple Python binaries with distinct
wheels, interpreter versions, first-party sources, and runfiles.
Normalize the existing scalar binary argument into one binaries list.
Reuse binary-owned merged wheel layers for singleton images; combine
multiple binaries' wheel groups once and reject conflicting runfiles and
launcher names.
Place multiple launchers in /app/bin and share their /app.runfiles tree
without losing each launcher's configured dependencies or repository
mappings.