Fix cross-drive script installation by konstin · Pull Request #11167 · astral-sh/uv

konstin · 2025-02-02T16:08:48Z

Fallback to copy if renaming a script doesn't work, similar to the site packages installation.

Test Plan

Failing:

docker volume rm -f gh11163_usr_local_bin
docker run -v gh11163_usr_local_bin:/usr/local/bin -e UV_LINK_MODE=copy -v $(pwd):/io -it --rm ghcr.io/astral-sh/uv:0.5-python3.11-bookworm-slim uv pip install --system ruff

Passing:

cargo build --target x86_64-unknown-linux-musl
docker volume rm -f gh11163_usr_local_bin
docker run -v gh11163_usr_local_bin:/usr/local/bin -e UV_LINK_MODE=copy -v $(pwd):/io -it --rm ghcr.io/astral-sh/uv:0.5-python3.11-bookworm-slim /io/target/x86_64-unknown-linux-musl/debug/uv pip install --system ruff

konstin · 2025-02-02T16:09:01Z

crates/uv-install-wheel/src/wheel.rs

+            // Here, two wrappers over rename are clashing: We want to retry for security software
+            // blocking the file, but we also need the copy fallback is the problem was trying to
+            // move a file cross-drive.
+            match uv_fs::rename_with_retry_sync(&path, &script_absolute) {
+                Ok(()) => (),
+                Err(err) => {
+                    debug!("Failed to rename, falling back to copy: {err}");
+                    fs_err::copy(&path, &script_absolute)?;
+                }
+            }


Should we do copy_with_retry_sync here, in the inner loop?

Also, does rename_with_retry_sync retry if the error is due to different drives? Hopefully not?

Do we have a copy_with_retry_sync?

This is mostly guess work, my idea was the security checkers would make the `rename_with_retry_sync loop fail and then the copy would succeed first try, but we can make this more defensive if we have a good way to.

I was suggesting we create it? I think the more important question here though is whether we can catch the "different drive" error, or whether we need this catch-all Err(err) branch. Do you know?

Ok, I looked into it myself, and CrossesDevices is unstable. Please add a TODO here to catch that error when it stabilizes. Then decide whether you want to add a copy_with_retry_sync method (I would suggest doing so, but it's up to you) and go ahead with the merge.

Ah now I get what you were getting at.

I now implemented the more defensive version where we retry in both rename or copy. The callback version is kinda ugly but it's only used in this one place.

I didn't realize there was a dedicated error code for cross drive, i had initially assumed that this is part of some other platform-specific error code. Once it stabilized we can upgrade this code path.

If you have suggestions for a prettier version of with_retry_sync i'll follow up.

crates/uv-install-wheel/src/wheel.rs

charliermarsh · 2025-02-11T13:55:08Z

crates/uv-install-wheel/src/wheel.rs

+        match self {
+            Self::Rename => match fs_err::rename(from.as_ref(), to.as_ref()) {
+                Ok(()) => {}
+                Err(err) => {


Same question here: can we make this branch exclusive to "different drive"? Is there an error kind for it?

Fallback to copy if renaming a script doesn't work, similar to the site packages installation. **Test Plan** ``` cargo build --target x86_64-unknown-linux-musl docker volume rm -f gh11163_usr_local_bin docker run -v gh11163_usr_local_bin:/usr/local/bin -e UV_LINK_MODE=copy -v $(pwd):/io -it --rm ghcr.io/astral-sh/uv:0.5-python3.11-bookworm-slim /io/target/x86_64-unknown-linux-musl/debug/uv pip install --system ruff docker volume rm -f gh11163_usr_local_bin docker run -v gh11163_usr_local_bin:/usr/local/bin -e UV_LINK_MODE=copy -v $(pwd):/io -it --rm ghcr.io/astral-sh/uv:0.5-python3.11-bookworm-slim uv pip install --system ruff ```

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.5.30` -> `0.5.31` | 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.5.31`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0531) [Compare Source](astral-sh/uv@0.5.30...0.5.31) ##### Enhancements - Add `uv sync --script` ([#11361](astral-sh/uv#11361)) - Allow PEP 508 requirements in tool requests ([#11337](astral-sh/uv#11337)) - Allow source distributions to produce wheels with `+local` suffixes ([#11429](astral-sh/uv#11429)) - Bring parity to `uvx` and `uv tool install` requests ([#11345](astral-sh/uv#11345)) - Use a stable directory for local, remote, and stdin script virtual environments ([#11347](astral-sh/uv#11347), [#11364](astral-sh/uv#11364)) - Detect infinite recursion in `uv run` ([#11386](astral-sh/uv#11386)) ##### Python The managed Python distributions have been updated, including: - CPython 3.14.0a5, which includes a new [tail calling interpreter](https://docs.python.org/3.14/whatsnew/3.14.html#whatsnew314-tail-call) for a significant performance improvement - The bundled OpenSSL version was updated from 3.0.15 to 3.0.16 which fixes a [security advisory](https://openssl-library.org/news/secadv/20241016.txt) See the [`python-build-standalone` release notes](https://github.com/astral-sh/python-build-standalone/releases/tag/20250212) for more details. ##### Bug fixes - Fix cross-drive script installation ([#11167](astral-sh/uv#11167)) - Add indexes in priority order ([#11451](astral-sh/uv#11451)) - Allow `--python <dir>` requests to match existing environments if `sys.executable` is the same file ([#11290](astral-sh/uv#11290)) - Avoid comparing to system site packages in `--dry-run` mode ([#11427](astral-sh/uv#11427)) - Prefer running executables in the environment with `<name>` over `<name>/__main__.py` ([#11431](astral-sh/uv#11431)) - Retry local clones without hardlinks if they fail ([#11421](astral-sh/uv#11421)) ##### Documentation - Update alternative-indexes.md to use `UV_INDEX` instead of `UV_EXTRA_INDEX_URL` ([#11381](astral-sh/uv#11381)) - Update scripts guide to include using package indexes ([#11443](astral-sh/uv#11443)) </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. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Fallback to copy if renaming a script doesn't work, similar to the site packages installation. Fixes astral-sh#11163 **Test Plan** Failing: ``` docker volume rm -f gh11163_usr_local_bin docker run -v gh11163_usr_local_bin:/usr/local/bin -e UV_LINK_MODE=copy -v $(pwd):/io -it --rm ghcr.io/astral-sh/uv:0.5-python3.11-bookworm-slim uv pip install --system ruff ``` Passing: ``` cargo build --target x86_64-unknown-linux-musl docker volume rm -f gh11163_usr_local_bin docker run -v gh11163_usr_local_bin:/usr/local/bin -e UV_LINK_MODE=copy -v $(pwd):/io -it --rm ghcr.io/astral-sh/uv:0.5-python3.11-bookworm-slim /io/target/x86_64-unknown-linux-musl/debug/uv pip install --system ruff ```

konstin added the bug Something isn't working label Feb 2, 2025

konstin commented Feb 2, 2025

View reviewed changes

charliermarsh reviewed Feb 4, 2025

View reviewed changes

crates/uv-install-wheel/src/wheel.rs Show resolved Hide resolved

charliermarsh reviewed Feb 4, 2025

View reviewed changes

crates/uv-install-wheel/src/wheel.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Feb 4, 2025

View reviewed changes

crates/uv-install-wheel/src/wheel.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Feb 4, 2025

View reviewed changes

crates/uv-install-wheel/src/wheel.rs Show resolved Hide resolved

konstin force-pushed the konsti/fix-data-move branch from afb681e to 7480131 Compare February 4, 2025 12:10

charliermarsh approved these changes Feb 11, 2025

View reviewed changes

charliermarsh reviewed Feb 11, 2025

View reviewed changes

konstin added 3 commits February 12, 2025 18:37

Review

b67b9cf

Use copy with retry too

f5e33ec

konstin force-pushed the konsti/fix-data-move branch from b564fbd to f5e33ec Compare February 12, 2025 17:52

konstin merged commit 070120e into main Feb 12, 2025
73 checks passed

konstin deleted the konsti/fix-data-move branch February 12, 2025 18:00

BrewTestBot mentioned this pull request Feb 12, 2025

uv 0.5.31 Homebrew/homebrew-core#207491

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cross-drive script installation#11167

Fix cross-drive script installation#11167
konstin merged 3 commits intomainfrom
konsti/fix-data-move

konstin commented Feb 2, 2025

Uh oh!

konstin Feb 2, 2025

Uh oh!

charliermarsh Feb 11, 2025

Uh oh!

konstin Feb 11, 2025

Uh oh!

charliermarsh Feb 11, 2025

Uh oh!

charliermarsh Feb 12, 2025

Uh oh!

konstin Feb 12, 2025

Uh oh!

konstin Feb 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

charliermarsh Feb 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

konstin commented Feb 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants