Skip to content
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

feat: detect venv relocation #11168

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jonaslb
Copy link

@jonaslb jonaslb commented Feb 2, 2025

Summary

Virtual environments are not relocatable (by default) in particular due to hardcoded paths in shebangs of scripts. While it is perhaps not very common to move a virtual environment by itself, it is not unthinkable that a user might reorganize his projects and hence effectively also the virtual environments. This can lead to weird errors, for example if a call to pytest "falls through" to the system installation instead of the (broken) entrypoint in the virtual environment.

This change adds a field uv-venv-path to pyvenv.cfg. The field is only added when the venv is not relocatable. When uv looks for usable venvs when e.g. syncing, it verifies that this path matches the actual path of the venv. If it does not match, the venv will be deleted and recreated.

Fixes #10895

Test Plan

  • There are a few simple tests to verify the contents of a written pyvenv.cfg for non-relocatable (default, uv-venv-path present) and relocatable venvs (uv-venv-path absent).
  • Added an assert to sync_invalid_environment to check that the venv is recreated when uv-venv-path does not match the actual path.
  • Also added an assert in the case that the uv-venv-path is absent from pyvenv.cfg (this will recreate the venv).

@jonaslb jonaslb force-pushed the detect-venv-relocation branch from 7e98a0a to 45330f8 Compare February 2, 2025 17:38
@jonaslb jonaslb marked this pull request as draft February 2, 2025 17:50
@jonaslb
Copy link
Author

jonaslb commented Feb 2, 2025

It appears that I've missed a few other test cases that have also been affected by the changes. I will review and update.

@jonaslb
Copy link
Author

jonaslb commented Feb 2, 2025

There were some tests that were failing due to having hardcoded pyvenv.cfg line numbers that I've fixed. There is a remaining failure related to symlink venvs introduced in #11083 that I need to look a little bit more into.

@jonaslb jonaslb force-pushed the detect-venv-relocation branch from 6d195f2 to df37c45 Compare February 2, 2025 18:45
@zanieb
Copy link
Member

zanieb commented Feb 3, 2025

I think we were also considering making the --relocatable flag the default?

See

@charliermarsh / @konstin thoughts?

@jonaslb
Copy link
Author

jonaslb commented Feb 3, 2025

Ah, you already saw this :) Was just about to ask for feedback. The remaining CI failure (in benchmarks) is due to not fully resolving relative paths "that go up". I think this is fixable by using std::fs::canonicalize (edit: or something like Cargo's normalize_path) (instead of std::path::absolute) in a few places, but I don't have more time right now, so it'll have to be later, unless this turns out not to have interest :)

I also wonder what you think (@zanieb) of the change I made to the symlink test. I think the behaviour (with my changes) is correct, but it might not be "in the spirit of the test" now. Perhaps it would be more correct to change it to use a relocatable venv.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect if project directory was moved for venv reconstruction
2 participants