-
-
Notifications
You must be signed in to change notification settings - Fork 51
feat(py_venv): activate-less interpreter #629
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This patch reworks how the interpreter shim operates to eliminate the data dependency on the `activate` script having been evaluated and move the parameters which previously came via the shell environment. Because the behavior of `activate` isn't actually specified we previously assumed that it was safe to just extend `activate`. This isn't true at all it turns out, as under normal operation the `./bin/python` link and the behavior of the interpreter initialization as implemented in `Modules/getpath.py` provide special behavior when the interpreter's observed filename (`argv[0]`) is a symlink as it is in a normal non-relocatable virtualenv and will search for a `pyvenv.cfg` and implicitly activate the venv as needed in that case. Our `./bin/python` being an actual binary tool interrupts this which means we're on our own to make `getpath` and `site` do the needful. This should be as simple as setting the `home=` key in the `pyvenv.cfg`, but there isn't a principled way other than going through the runfiles library to locate where the interpreter lands in the runfiles tree relative to the virtualenv. Ideally we'd set the `home=` key to a relative path and mostly move on. Instead due in part to bzlmod munging we have to do the repo mapping dance which can't be done statically. So we need to do `rlocation` somewhere. The solution this patch comes to is moving that `rlocation` logic out of the `activate` script and into the launcher itself so that the launcher can search for the needed `$PYTHONHOME` and desired interpreter without the user having previously loaded the required parameters into the env via `activate`. Fixes #594 at the cost of making the shim binary unusable outside the context of `rules_py`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with 2 comments/questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw you succeed against the test suite last night. Everything I've noted down is for Rust style rather than Aspect logic, so it is up to you whether you change or not.
…e-avec-couture' into arrdem/interpreter-sans-activate
|
@arrdem, just tested out 1.6.4-rc1 which includes this patch, and running
This is just the most basic initial test I could think of, hopefully it's a valid one. I also tried running
I also tried configuring VSCode to use a rules_py-generated virtualenv and it failed with: Am I doing something wrong? |
This patch reworks how the interpreter shim operates to eliminate the data dependency on the
activate
script having been evaluated and move the parameters which previously came via the shell environment.Because the behavior of
activate
isn't actually specified, we previously assumed that it was safe to just extendactivate
. This isn't true at all it turns out. Under normal operation the interpreter initialization as implemented inModules/getpath.py
provides special behavior when the interpreter's observed filename (argv[0]
) is a symlink as it is in a normal non-relocatable virtualenv. In this case the interpreter and will search for apyvenv.cfg
relative to the "interpreter" and implicitly activate the venv as needed.Our
./bin/python
being an actual binary interrupts this which means we're on our own to makegetpath
andsite
do the needful.This should be as simple as setting the
home=
key in thepyvenv.cfg
, but there isn't a principled way other than going through the runfiles library to locate where the interpreter lands in the runfiles tree relative to the virtualenv. Ideally we'd set thehome=
key to a relative path and mostly move on. Instead due in part to bzlmod munging we have to do the repo mapping dance which can't be done statically. So we need to dorlocation
somewhere.The solution this patch comes to is moving that
rlocation
logic out of theactivate
script and into the launcher itself so that the launcher can search for the needed$PYTHONHOME
and desired interpreter without the user having previously loaded the required parameters into the env viaactivate
.Fixes #594.
Fixes #631.
Changes are visible to end-users: yes
The
py_venv_binary
and friends no longer depend on their virtualenvs beingactivated
via the shell. Their interpreters can safely be invoked directly as under an IDE or in a Spark environment.The
py_venv_binary
now no longer includes NEITHER the user's site-packages NOR the interpreter's site-packages directory unless theenable_system_site_packages
orenable_user_site_packages
attributes are explicitly set. This prevents accidental non-hermetic imports.Test plan
TODO