fix(py_venv): preserve PBS base for nested venvs#1201
Conversation
py_binary startup benchmark
sys.path quality
✅ No regression detected (PR is +0.4% vs HEAD main) |
tamird
force-pushed
the
fix-pbs-venv-cwd-prefix-v1
branch
2 times, most recently
from
c8db558 to
18266b8
Compare
|
I found a contract gap in my own review, so I moved this back to draft. Omitting |
|
Correction to my previous note: I overstated the |
|
Pushed |
|
Ready again: the refreshed upstream matrix is green, the contract review is clean, and the real Ty adapter targets pass with the strict parser kept outside this relocatable metadata path. |
|
@xangcastle could you review this when you have a chance? It is the same py_venv ownership boundary as PR 1147, and the full matrix is green. |
| home = config_values.get("home") | ||
|
|
||
| def test_base_prefix_not_install(): | ||
| assert sys.base_prefix != "/install", ( |
There was a problem hiding this comment.
Did any of these assertions change or only move? The diff is unclear
There was a problem hiding this comment.
They changed. The old test called verify_all(), then repeated the /install and stdlib checks. The action-cwd probe keeps the #1048 checks and adds cwd/PWD, base_exec_prefix and executable existence, relocatable/home metadata, _ctypes, child -S/-BS, exposed venv, and 3.9-3.13 controls. It drops the incidental verify_all() symlink-walk and sys.path checks because this fixture now targets startup from an arbitrary action cwd rather than general venv layout.
|
Add a snapshot test that shows the diff of the generated code |
|
Done. |
![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: 0de7d43642
ℹ️ 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".
| if omit_venv_home: | ||
| pyvenv_home = None |
There was a problem hiding this comment.
home so 3.11/3.12 venvs activate
For non-Windows CPython 3.11/3.12 runfiles runtimes with supports_build_time_venv, this sets pyvenv_home to None, so the generated pyvenv.cfg has no home line. In CPython 3.11/3.12 the startup code only retains the detected venv prefix while processing a home key; without it, sys.prefix remains the base install, the venv site-packages/.pth files are not loaded, and any py_binary/py_venv relying on deps from the assembled venv will fail to import them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
False positive. The action probe on this head asserts sys.prefix != sys.base_prefix for 3.11 and 3.12. I also ran the exposed 3.11 venv directly: sys.prefix was the generated .action_probe_311.venv, sys.base_prefix was the PBS interpreter, and sys.path included the generated venv site-packages directory. The runtime test executes both the launcher and exposed venv from an unrelated action cwd; omitting home does not disable venv activation in current CPython.
| # https://github.com/bazel-contrib/rules_python/blob/bac54949/python/private/py_runtime_info.bzl#L316-L337 | ||
| omit_venv_home = ( | ||
| py_toolchain.runfiles_interpreter and | ||
| not is_windows and |
There was a problem hiding this comment.
Windows is excluded deliberately because this workaround relies on the POSIX relocatable bin/python symlink path, and this fixture has no Windows repro. I am leaving Windows unchanged until there is a failing Windows case rather than broadening the extension without evidence.
|
Rebase |
tamird
force-pushed
the
fix-pbs-venv-cwd-prefix-v1
branch
from
0de7d43 to
61a492b
Compare
|
Done. |
|
squash, reduce unnecessary diffs |
tamird
force-pushed
the
fix-pbs-venv-cwd-prefix-v1
branch
from
61a492b to
0217ea8
Compare
|
Done. |
tamird
force-pushed
the
fix-pbs-venv-cwd-prefix-v1
branch
from
0217ea8 to
fdc79ea
Compare
|
Correction: my earlier “Not required” reply was wrong. Exact downstream CI with the explicit line removed restored the /install encodings failure in four nested-venv tests. The gate reads the selected PyRuntimeInfo after toolchain plumbing, where the implicit default is not reliable. I restored the explicit producer-owned field in fdc79ea and updated the body. |
|
Second correction: the explicit field alone did not fix the exact downstream subset locally. The remaining failure is a nested stdlib venv whose home points at the outer relocatable venv bin directory, then falls back to /install. I moved this back to draft while I add that missing ownership case instead of asking you to review an incomplete fix. |
|
Create an e2e test reproducing the same issue as downstream |
tamird
force-pushed
the
fix-pbs-venv-cwd-prefix-v1
branch
from
fdc79ea to
6c8a2a7
Compare
tamird
changed the title
tamird
force-pushed
the
fix-pbs-venv-cwd-prefix-v1
branch
from
6c8a2a7 to
b990f23
Compare
|
Done. The action probe now creates a nested stdlib venv from the unrelated action cwd, starts its child interpreter, imports encodings, and rejects /install. Empty home fixes the three downstream nested-venv failures. The fourth downstream failure was our monorepo test's detached venv alias, not this metadata path; I narrowed that test back to its shell-trampoline contract. This reroll also removes the unproven implementation_name line. The exact four-test subset passes locally. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b990f23915
ℹ️ 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".
tamird
force-pushed
the
fix-pbs-venv-cwd-prefix-v1
branch
from
b990f23 to
af6f022
Compare
CPython 3.11 and 3.12 interpret a relative pyvenv.cfg home from the process working directory. rules_py 2 writes a runfiles-relative home, so an action tool can derive the PBS build-time /install prefix and fail before importing encodings. For in-build CPython 3.11 and 3.12 runtimes that support build-time venvs, write an empty home key instead. getpath sees the key and resolves the relocatable bin/python symlink to the real PBS base executable, but the empty value does not force cwd-relative prefix discovery. This keeps outer startup relocatable and lets stdlib-created child venvs inherit PBS rather than the outer venv bin directory. Keep system interpreters, Windows, unsupported runtimes, and other Python versions unchanged. Exercise PBS 3.9 through 3.13 from an unrelated action cwd, including child interpreters, the exposed venv, and a nested stdlib venv. Snapshot the empty 3.11 home beside the retained 3.13 home. Fixes issue 1048.
tamird
force-pushed
the
fix-pbs-venv-cwd-prefix-v1
branch
from
af6f022 to
a156bec
Compare
|
@jbedard Could you review the current exact remaining downstream set: #1201, #1205, #1206, and #1212? #1202 is merged. All four heads are green; #1205 is rerolled at 718f03a with the google/protobuf e2e, and #1212 is the separate dirname-free console-script fix exposed by the downstream manifest-only check. |
| py_runtime( | ||
| name = "system_py3_runtime", | ||
| interpreter_path = "/opt/fake-python/bin/python3", | ||
| # Braces make sure the generated home line is not formatted a second time. |
There was a problem hiding this comment.
This is the regression input for the brace-preservation fix in venv.bzl. interpreter_path is a host path string and may contain literal braces; before the change we formatted the home line once and then fed it through the outer pyvenv.cfg format, so {fake-python} was reinterpreted and either rewrote the path or failed analysis. Keeping braces here makes test_pyvenv_home fail on that bug; a plain path only covers the older home-directory behavior.
|
Review these changes |
2 participants
CPython 3.11 and 3.12 interpret a relative pyvenv.cfg home from the process working directory. rules_py 2 writes a runfiles-relative value. When a py_binary runs as a Bazel action tool, that directory is the action sandbox, so prefix discovery can fall back to the PBS build-time /install prefix before Python can import encodings.
py_venv owns both the generated pyvenv.cfg and the relocatable bin/python link. For non-Windows in-build CPython 3.11 and 3.12 runtimes that declare supports_build_time_venv, write home with an empty value. getpath sees the key and resolves bin/python to the real PBS base executable, while the empty value does not force cwd-relative prefix discovery. That keeps outer startup relocatable and lets stdlib-created child venvs inherit PBS rather than the outer venv bin directory. System interpreters, unsupported runtimes, Windows, and other Python versions retain their current home.
The e2e matrix runs PBS 3.9 through 3.13 from an unrelated action cwd. It exercises the launcher, child interpreters with and without site initialization, the exposed venv, and a nested stdlib venv. The nested child starts from that unrelated cwd, imports encodings, and rejects /install as its base prefix. Two checked-in pyvenv.cfg snapshots make the generated metadata delta reviewable: 3.11 keeps an empty home line, while 3.13 retains a normalized nonempty home line.
Fixes issue 1048.