refactor(venv): rename venv.bzl to assemble_venv.bzl and split out wheel plan helpers#1317
refactor(venv): rename venv.bzl to assemble_venv.bzl and split out wheel plan helpers#1317xangcastle wants to merge 2 commits into
Conversation
Pure rename to preserve git history across the upcoming content refactor. No logic changes; only the bzl_library name, its srcs, and the load in py_venv.bzl follow the new filename.
✨ Aspect Workflows Tasks📅 Thu Jul 16 23:17:42 UTC 2026 ✅ 40 successful tasks
⏱ Last updated Thu Jul 16 23:21:41 UTC 2026 · 📊 GitHub API quota 1,770/15,000 (12% used, resets in 34m) |
py_binary startup benchmark
sys.path quality
Bazel analysis benchmark
Auxiliary metrics
|
xangcastle
changed the title
| venv, unless `expose_venv = True` routes them to a sibling py_venv) and | ||
| the standalone `py_venv` rule call `assemble_venv` to keep their layouts | ||
| bit-identical. | ||
| up a Python venv. Both ``py_binary`` / ``py_test`` (each with its own |
There was a problem hiding this comment.
because looks better when //py:defs.doc_extract parse the docstring
`foo` → interpreted text
``foo`` → literal/code
file names, attrs, function names and inline code should be literal/code
There was a problem hiding this comment.
Is that the case everywehre or only the defs.doc_extract? What about IDEs or BCR?
| # overrides with its own absolute value when invoked directly. | ||
| passed_env["VIRTUAL_ENV"] = venv_root(shared.venv.bin_python) | ||
|
|
||
| _expand_launcher(ctx, shared) |
There was a problem hiding this comment.
What's the reason for doing ones like this? IMO this just makes it more confusing... glancing at _py_venv_rule_impl now required scrolling up and down across this file while previously it all fit on one screen?
| load(":py_venv_exec.bzl", _py_venv_exec = "py_venv_exec") | ||
| load(":types.bzl", "VirtualenvInfo", "venv_root") | ||
|
|
||
| _VENV_ONLY_ATTRS = [ |
There was a problem hiding this comment.
Lets at least leave basic comments for things like this
…_venv_exec Refactors the content of assemble_venv.bzl (renamed in the previous commit) to consume compute_wheel_plan instead of calling resolve_wheel_collisions directly, moving the wheel lookup tables (tree_by_sp, known_layout) into the resolver plan. Extracts _declare_console_scripts and the .pth formatter into helpers. py_venv_exec_impl is split into focused helpers (_validate_main, _merge_environment, _set_contextual_env, _strip_contextual_from_inherited). py_venv_conflict, venv snapshots, and console-scripts tests pass on a forced re-run; bazel test //... is 250/250 green.
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
I found one runtime-behavior change and one avoidable hot-path cost in the venv refactor.
| if not ctx.attr.isolated: | ||
| flags = [f for f in flags if f != "-I"] | ||
| base = [f for f in base if f != "-I"] | ||
| return base + list(ctx.attr.interpreter_options) |
There was a problem hiding this comment.
This changes isolated = False behavior: previously -I was removed from the combined default and user flags, while the refactor appends a user-supplied -I after filtering and silently restores isolation (ignoring PYTHONPATH/user site). The standalone/exposed venv helper makes the same change. Please retain combined filtering, or call out and cover the behavior change with a non-isolated/exposed test that passes interpreter_options = ["-I"].
There was a problem hiding this comment.
The latest revision restores combined filtering for the standalone/exposed py_venv REPL, which fixes that half. py_venv_exec.bzl still filters only the base flags and then appends user interpreter_options, so a normal py_binary/py_test with isolated=False and interpreter_options=["-I"] still changes behavior. Please restore combined filtering there (and cover the binary/test path) before resolving.
| venv_name = venv_name, | ||
| ) | ||
|
|
||
| plan = compute_wheel_plan(ctx, _py_library.make_wheels_depset(ctx).to_list()) |
There was a problem hiding this comment.
compute_wheel_plan sorts every wheel path into wheel_fingerprints, but this field has no consumer in the repository. Calling it here adds an O(wheels log wheels) allocation/sort for every venv on an already hot analysis path. Can the unused fingerprint computation/field be dropped (or the lighter resolver/lookup path retained) until a real consumer needs it?
dc78b18 to
e403c87
Compare
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
The latest simplification restores combined -I filtering for the executable venv, but the normal binary/test launcher still appends a user -I after filtering and the unused per-venv wheel-fingerprint sort remains. One small rename cleanup: py/private/py_venv/py_venv.bzl:24 still points readers at the now-deleted venv.bzl::assemble_venv; update it to assemble_venv.bzl.
|
All test and benchmark steps passed on e403c87. The sole red check is the benchmark comment job: GitHub returned HTTP 503 while listing this PR’s comments. A maintainer will need to rerun the failed job; the substantive review threads are still open. |
3 participants
This renames
venv.bzltoassemble_venv.bzlto better reflect what the module actually contains, then refactors its internals to consume thecompute_wheel_planentry point so wheel lookup tables live alongside the collision resolver. Thepy_venv_execimplementation is split into smaller focused helpers, and the console-script template attribute is pointed at the correct template file.Changes are visible to end-users: no
Test plan