feat(rules): emit RunfilesGroupInfo from py_binary, py_test, and py_runtime - #3962
feat(rules): emit RunfilesGroupInfo from py_binary, py_test, and py_runtime#3962FrankPortman wants to merge 4 commits into
RunfilesGroupInfo from py_binary, py_test, and py_runtime#3962Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
5757fd5 to
432966d
Compare
There was a problem hiding this comment.
In general I quite like the solution, thank you for submitting the PR. I think @rickeylev is doing some experiments with the venv building so I'll defer to him for final approval. The only thing is that I would love to keep the @rules_runfiles_group//runfiles_group:enabled an internal target and force the users to go through the main flag in rules_python. THis makes it easier to reason about the API surface.
For my understanding:
- If this is disabled, the overhead would be minimal right? It's basically just an extra
ifcheck. - This is disabled by default?
- Where does the
rules_runfiles_groupdependency comes from?
One last thing, if we could link to upstream docs on how one can create this layering rule for container image, that would be a big +1 from me.
| When enabled (via `--@rules_runfiles_group//runfiles_group:enabled` or | ||
| {obj}`--@rules_python//python/config_settings:runfiles_groups`), `py_binary` |
There was a problem hiding this comment.
I think we should advertise only one - the //python/config_settings one.
…untime Implements bazel-contrib#3757: experimental support for the RunfilesGroupInfo provider from rules_runfiles_group 0.1.0. Emission is gated by two flags: the ecosystem-wide --@rules_runfiles_group//runfiles_group:enabled switch, and --//python/config_settings:runfiles_groups, whose default (auto) follows the ecosystem switch and whose explicit values override it for rules_python only. Off by default, zero cost when off. When enabled, binaries split their runfiles into named groups so packaging rules can build layered container images (shared interpreter layer, one layer per package): * rules_python#runtime: interpreter + stdlib (rank FOUNDATION, do_not_merge, kind foundation). py_runtime also emits this for its own runfiles. * rules_python#pypi/<name>: one group per PyPI package, detected via the dist-info METADATA file so group names stay stable even though whl repo names aren't (rank SHARED_DEPS, kind third_party). * One Label-named group per other py_library. * rules_python#venv: the binary-specific venv symlinks and support files. * rules_python#app: the binary's own code, executable, bootstrap and build data; named as the provider's executable_group. * One Label-named group per rule target in data attributes. Only binaries (and py_runtime) emit the public provider, because only they can satisfy its contract that the union of all groups equals DefaultInfo.default_runfiles exactly; analysis tests verify that. A py_library's sources reach the binary through PyInfo rather than its own runfiles, so libraries propagate their entries bottom-up through the private PyRunfilesGroupsInfo provider instead: entries reference dependencies' depsets rather than copying them, so per-target cost is constant. A consequence worth maintainer review: a foreign ruleset's binary with py_library deps gets no groups from them. Binaries that override pyc_collection away from the configuration default, or use the deprecated implicit __init__.py creation, fall back to a coarse runtime/venv/app grouping so the invariant still holds. Fixes bazel-contrib#3757
The checked-in lockfile validated by bzlmod_lockfile_test needs the BCR registry entries for rules_runfiles_group@0.1.0. Regenerated with the command from the test's README under Bazel 9.1.0.
The test bzl loaded @rules_runfiles_group directly, but that repo is only defined where the feature is available (bzlmod, Bazel 9+). Route the loads through the runfiles_groups_shim like the rule code does. The flag label from the shim is canonicalized via Label() so rules_testing's transition can resolve it.
dab6888 to
fcba61a
Compare
The generated py_library targets gained the _runfiles_groups_flag implicit attribute, so add it to the expected dep set. The upstream flag attribute is filtered out like toolchain types are: the shim points it at @rules_runfiles_group only on Bazel 9+, and at //python:none (already in the set) elsewhere.
|
Thanks for the review! Re the flag, I deliberately built an interaction with the upstream flag but this definitely up for discussion. The upstream
My thought was that it would be nice to not have to opt individual rulesets in, if we're comfortable doing a top-down Your questions:
I'll add pointers to the |
2 participants
Implements #3757 against rules_runfiles_group 0.1.0 (the new entries-depset API).
When enabled, binaries emit these groups:
rules_python#runtime: interpreter + stdlibrules_python#pypi/<name>: one per PyPI package, stable across whl repo namesrules_python#<label>: oneLabelnamed group perpy_libraryrules_python#venv: the binary-specific venv symlinks and support filesrules_python#app: the binary's own code, executable, bootstrap, and build data (executable_group = True)data#<label>: rule targets from data attributes--srcs_as_runfiles) that I feel like is somehow implicated. Libraries propagate entries through a private provider.py_librarymay not get RGI behavior for free without its own shim. We can change this if we are willing to accept a change in semantics aroundunion == default_runfiles(I think).--@rules_runfiles_group//runfiles_group:enabled flag;--//python/config_settings:runfiles_groupsdefaults to following it and can override either way.pyc_collectionaway from the config default, or using legacy implicit__init__.py, falls back to coarse runtime/venv/app grouping (dep entries are computed against the config, so the union would otherwise break).I had Claude patch the Bazel 9 vs Bazel 8 autoloads stuff just to get CI working, since I figure y'all will have a preference for how that is handled, and I can just adapt accordingly.
Fixes #3757
To implement this PR, I first looked at the reference implementation and browsed the
rules_pythonenough to form an opinion on what a sketch might look like. I then steered Claude Fable pretty heavily in plan-mode, given a sketch/pseudocode of how I think it should look. Finally, I used Claude Fable to put finishing touches on comments, documentation, help me think of new test cases, and then stress test some edge cases.I believe I understand this code well enough to respond to feedback and "own" the changes, conceptually, but I want to be transparent about LLM assistance.