Fix symlink preservation in virtual environment creation#14933
Fix symlink preservation in virtual environment creation#14933zanieb merged 5 commits intoastral-sh:mainfrom
Conversation
ad57ed5 to
fe54e96
Compare
|
Can you explain why
was happening? |
When recreating the venv, the symlink will be broken, just like your comment #14670 (comment), and there is no clear warning to the user. Is this one of the design purposes of uv itself? I think the current MR is not good enough, so I redesigned it. When the user runs uv venv again, it detects the presence of symbolic links in the virtual environment. We can prompt: Key information: Warning: The virtual environment at '[path]' contains symbolic links. Preserving symlinks can affect portability and stability if the linked paths change outside the environment. Options: (K)eep symlinks: Keep all symbolic links unchanged. (R)eplace with directories: For example: WARNING: This will replace symlinks with EMPTY directories. Any data expected to be at the symlink location within the venv will be lost or redirected to this new empty directory. Proceed only if you understand the consequences. (A)bort: Abort the operation, let the user handle it themselves. If you agree with this idea, I can continue to make modifications. |
No this is not by design, but I'm confused about the root cause of this abnormal behavior and I think it'd be helpful if your pull request explained that so a reviewer does not need to figure it out. I don't think we should prompt here, we should always follow the symlink. |
|
Thanks for the feedback! I've updated the PR description to explain the root cause. The issue was that Path::metadata() automatically follows symlinks, so the code treated symlinked venv paths as regular directories and would delete the symlink itself when clearing existing environments. |
| let target = fs::read_link(location)?; | ||
| let resolved_target = if target.is_absolute() { | ||
| target | ||
| } else { | ||
| location.parent().unwrap_or(Path::new(".")).join(target) | ||
| }; |
There was a problem hiding this comment.
Should we just canonicalize here? Won't this fail for nested symlinks?
| match location.metadata() { | ||
| match actual_location.metadata() { |
There was a problem hiding this comment.
Could we retain the name location throughout here to reduce the diff?
| remove_virtualenv(location)?; | ||
| fs::create_dir_all(location)?; | ||
| remove_virtualenv(&actual_location)?; | ||
| fs::create_dir_all(&actual_location)?; |
There was a problem hiding this comment.
Isn't the crux of the issue just that we need to canonicalize the location here, when we remove and create it again? Isn't everything else working fine as-is?
There was a problem hiding this comment.
you’re right, this wouldn’t handle nested symlinks properly. I’ve updated the code to use canonicalize. Thanks for catching that! 274cbb5
I've tested the nested symlinks situation and it can handle it.
➜ test-env alias uv-dev='/Users/wingmunfung/workspace/uv/target/debug/uv'
➜ test-env ln -s dummy foo
➜ test-env ln -s foo .venv
➜ test-env ls -lah
total 0
drwxr-xr-x 4 wingmunfung staff 128B Jul 30 10:39 .
drwxr-xr-x 48 wingmunfung staff 1.5K Jul 29 17:08 ..
lrwxr-xr-x 1 wingmunfung staff 3B Jul 30 10:39 .venv -> foo
lrwxr-xr-x 1 wingmunfung staff 5B Jul 30 10:39 foo -> dummy
➜ test-env uv-dev venv
Using CPython 3.13.2
Creating virtual environment at: .venv
error: Failed to create virtual environment
Caused by: failed to create directory `.venv`: File exists (os error 17)
➜ test-env mkdir dummy
➜ test-env uv-dev venv
Using CPython 3.13.2
Creating virtual environment at: .venv
Activate with: source .venv/bin/activate
➜ test-env ls -lah
total 0
drwxr-xr-x 5 wingmunfung staff 160B Jul 30 10:39 .
drwxr-xr-x 48 wingmunfung staff 1.5K Jul 29 17:08 ..
lrwxr-xr-x 1 wingmunfung staff 3B Jul 30 10:39 .venv -> foo
drwxr-xr-x 7 wingmunfung staff 224B Jul 30 10:39 dummy
lrwxr-xr-x 1 wingmunfung staff 5B Jul 30 10:39 foo -> dummy
➜ test-env uv-dev venv
Using CPython 3.13.2
Creating virtual environment at: .venv
✔ A virtual environment already exists at `.venv`. Do you want to replace it? · yes
Activate with: source .venv/bin/activate
➜ test-env ls -lah
total 0
drwxr-xr-x 5 wingmunfung staff 160B Jul 30 10:39 .
drwxr-xr-x 48 wingmunfung staff 1.5K Jul 29 17:08 ..
lrwxr-xr-x 1 wingmunfung staff 3B Jul 30 10:39 .venv -> foo
drwxr-xr-x@ 7 wingmunfung staff 224B Jul 30 10:39 dummy
lrwxr-xr-x 1 wingmunfung staff 5B Jul 30 10:39 foo -> dummy6e895f2 to
274cbb5
Compare
Fixes issue where `uv venv` would preserve symlinks on the first run but replace them with regular directories. The issue occurred because `Path::metadata()` automatically follows symlinks, causing the removal logic to delete the symlink itself rather than just clearing the target directory contents. Solution: Use `canonicalize()` to resolve symlinks only when removing and recreating virtual environments, ensuring operations target the actual directory while preserving the symlink structure.
274cbb5 to
2a4ff5e
Compare
|
Thanks for making those changes! Can you write a test case that's |
adcd9fd to
1e616f0
Compare
1e616f0 to
c31606e
Compare
I‘ve added three test cases for symlink preservation:
|
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.8.4` -> `0.8.5` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>astral-sh/uv (astral-sh/uv)</summary> ### [`v0.8.5`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#085) [Compare Source](astral-sh/uv@0.8.4...0.8.5) ##### Enhancements - Enable `uv run` with a GitHub Gist ([#​15058](astral-sh/uv#15058)) - Improve HTTP response caching log messages ([#​15067](astral-sh/uv#15067)) - Show wheel tag hints in install plan ([#​15066](astral-sh/uv#15066)) - Support installing additional executables in `uv tool install` ([#​14014](astral-sh/uv#14014)) ##### Preview features - Enable extra build dependencies to 'match runtime' versions ([#​15036](astral-sh/uv#15036)) - Remove duplicate `extra-build-dependencies` warnings for `uv pip` ([#​15088](astral-sh/uv#15088)) - Use "option" instead of "setting" in `pylock` warning ([#​15089](astral-sh/uv#15089)) - Respect extra build requires when reading from wheel cache ([#​15030](astral-sh/uv#15030)) - Preserve lowered extra build dependencies ([#​15038](astral-sh/uv#15038)) ##### Bug fixes - Add Python versions to markers implied from wheels ([#​14913](astral-sh/uv#14913)) - Ensure consistent indentation when adding dependencies ([#​14991](astral-sh/uv#14991)) - Fix handling of `python-preference = system` when managed interpreters are on the PATH ([#​15059](astral-sh/uv#15059)) - Fix symlink preservation in virtual environment creation ([#​14933](astral-sh/uv#14933)) - Gracefully handle entrypoint permission errors ([#​15026](astral-sh/uv#15026)) - Include wheel hashes from local Simple indexes ([#​14993](astral-sh/uv#14993)) - Prefer system Python installations over managed ones when `--system` is used ([#​15061](astral-sh/uv#15061)) - Remove retry wrapper when matching on error kind ([#​14996](astral-sh/uv#14996)) - Revert `h2` upgrade ([#​15079](astral-sh/uv#15079)) ##### Documentation - Improve visibility of copy and line separator in dark mode ([#​14987](astral-sh/uv#14987)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS41Mi4yIiwidXBkYXRlZEluVmVyIjoiNDEuNTIuMiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
Summary
Fixes inconsistent symlink handling in
uv venvcommand (#14670).Problem
uv/crates/uv-virtualenv/src/virtualenv.rs
Line 81 in 00efde0
The original code used
Path::metadata()which automatically follows symlinks, causing the system to treat symlinked virtual environment paths as regular directories. When a user runs uv venv on an existing symlinked virtual environment(.venv -> foo), the code incorrectly treats the symlink as a regular directory becauselocation.metadata()automatically follows the symlink and returns metadata for the target directoryfoo/. This causes the removal logic to delete the symlink itself and permanently breaking the symlink relationship and replacing it with a standard directory structure.Solution
environments
structure
Test Plan