Skip to content

Commit b2a16ff

Browse files
konstinLoïc LESCOAT
authored and
Loïc LESCOAT
committed
Fix cross-drive script installation (astral-sh#11167)
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 ```
1 parent f1392d7 commit b2a16ff

File tree

3 files changed

+82
-17
lines changed

3 files changed

+82
-17
lines changed

crates/uv-fs/src/lib.rs

+12-7
Original file line numberDiff line numberDiff line change
@@ -246,10 +246,14 @@ pub async fn rename_with_retry(
246246
}
247247
}
248248

249-
/// Rename a file, retrying (on Windows) if it fails due to transient operating system errors, in a synchronous context.
250-
pub fn rename_with_retry_sync(
249+
/// Rename or copy a file, retrying (on Windows) if it fails due to transient operating system
250+
/// errors, in a synchronous context.
251+
#[cfg_attr(not(windows), allow(unused_variables))]
252+
pub fn with_retry_sync(
251253
from: impl AsRef<Path>,
252254
to: impl AsRef<Path>,
255+
operation_name: &str,
256+
operation: impl Fn() -> Result<(), std::io::Error>,
253257
) -> Result<(), std::io::Error> {
254258
#[cfg(windows)]
255259
{
@@ -261,15 +265,15 @@ pub fn rename_with_retry_sync(
261265
// See: <https://github.com/astral-sh/uv/issues/1491> & <https://github.com/astral-sh/uv/issues/9531>
262266
let from = from.as_ref();
263267
let to = to.as_ref();
264-
let rename = || fs_err::rename(from, to);
265268

266-
rename
269+
operation
267270
.retry(backoff_file_move())
268271
.sleep(std::thread::sleep)
269272
.when(|err| err.kind() == std::io::ErrorKind::PermissionDenied)
270273
.notify(|err, _dur| {
271274
warn!(
272-
"Retrying rename from {} to {} due to transient error: {}",
275+
"Retrying {} from {} to {} due to transient error: {}",
276+
operation_name,
273277
from.display(),
274278
to.display(),
275279
err
@@ -280,7 +284,8 @@ pub fn rename_with_retry_sync(
280284
std::io::Error::new(
281285
std::io::ErrorKind::Other,
282286
format!(
283-
"Failed to rename {} to {}: {}",
287+
"Failed {} {} to {}: {}",
288+
operation_name,
284289
from.display(),
285290
to.display(),
286291
err
@@ -290,7 +295,7 @@ pub fn rename_with_retry_sync(
290295
}
291296
#[cfg(not(windows))]
292297
{
293-
fs_err::rename(from, to)
298+
operation()
294299
}
295300
}
296301

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

-1
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,6 @@ pub fn install_wheel(
111111
// 2.b Move each subtree of distribution-1.0.data/ onto its destination path. Each subdirectory of distribution-1.0.data/ is a key into a dict of destination directories, such as distribution-1.0.data/(purelib|platlib|headers|scripts|data). The initially supported paths are taken from distutils.command.install.
112112
let data_dir = site_packages.join(format!("{dist_info_prefix}.data"));
113113
if data_dir.is_dir() {
114-
trace!(?name, "Installing data");
115114
install_data(
116115
layout,
117116
relocatable,

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

+70-9
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,11 @@ use fs_err::{DirEntry, File};
99
use mailparse::parse_headers;
1010
use rustc_hash::FxHashMap;
1111
use sha2::{Digest, Sha256};
12-
use tracing::{instrument, warn};
12+
use tracing::{debug, instrument, trace, warn};
1313
use walkdir::WalkDir;
1414

1515
use uv_cache_info::CacheInfo;
16-
use uv_fs::{persist_with_retry_sync, relative_to, rename_with_retry_sync, Simplified};
16+
use uv_fs::{persist_with_retry_sync, relative_to, Simplified};
1717
use uv_normalize::PackageName;
1818
use uv_pypi_types::DirectUrl;
1919
use uv_shell::escape_posix_for_single_quotes;
@@ -312,6 +312,7 @@ pub(crate) fn move_folder_recorded(
312312
site_packages: &Path,
313313
record: &mut [RecordEntry],
314314
) -> Result<(), Error> {
315+
let mut rename_or_copy = RenameOrCopy::default();
315316
fs::create_dir_all(dest_dir)?;
316317
for entry in WalkDir::new(src_dir) {
317318
let entry = entry?;
@@ -330,7 +331,7 @@ pub(crate) fn move_folder_recorded(
330331
if entry.file_type().is_dir() {
331332
fs::create_dir_all(&target)?;
332333
} else {
333-
fs::rename(src, &target)?;
334+
rename_or_copy.rename_or_copy(src, &target)?;
334335
let entry = record
335336
.iter_mut()
336337
.find(|entry| Path::new(&entry.path) == relative_to_site_packages)
@@ -349,13 +350,14 @@ pub(crate) fn move_folder_recorded(
349350

350351
/// Installs a single script (not an entrypoint).
351352
///
352-
/// Has to deal with both binaries files (just move) and scripts (rewrite the shebang if applicable).
353+
/// Binary files are moved with a copy fallback, while we rewrite scripts' shebangs if applicable.
353354
fn install_script(
354355
layout: &Layout,
355356
relocatable: bool,
356357
site_packages: &Path,
357358
record: &mut [RecordEntry],
358359
file: &DirEntry,
360+
#[allow(unused)] rename_or_copy: &mut RenameOrCopy,
359361
) -> Result<(), Error> {
360362
let file_type = file.file_type()?;
361363

@@ -460,12 +462,13 @@ fn install_script(
460462
use std::os::unix::fs::PermissionsExt;
461463

462464
let permissions = fs::metadata(&path)?.permissions();
463-
464465
if permissions.mode() & 0o111 == 0o111 {
465466
// If the permissions are already executable, we don't need to change them.
466-
rename_with_retry_sync(&path, &script_absolute)?;
467+
// We fall back to copy when the file is on another drive.
468+
rename_or_copy.rename_or_copy(&path, &script_absolute)?;
467469
} else {
468-
// If we have to modify the permissions, copy the file, since we might not own it.
470+
// If we have to modify the permissions, copy the file, since we might not own it,
471+
// and we may not be allowed to change permissions on an unowned moved file.
469472
warn!(
470473
"Copying script from {} to {} (permissions: {:o})",
471474
path.simplified_display(),
@@ -484,7 +487,21 @@ fn install_script(
484487

485488
#[cfg(not(unix))]
486489
{
487-
rename_with_retry_sync(&path, &script_absolute)?;
490+
// Here, two wrappers over rename are clashing: We want to retry for security software
491+
// blocking the file, but we also need the copy fallback is the problem was trying to
492+
// move a file cross-drive.
493+
match uv_fs::with_retry_sync(&path, &script_absolute, "renaming", || {
494+
fs_err::rename(&path, &script_absolute)
495+
}) {
496+
Ok(()) => (),
497+
Err(err) => {
498+
debug!("Failed to rename, falling back to copy: {err}");
499+
uv_fs::with_retry_sync(&path, &script_absolute, "copying", || {
500+
fs_err::copy(&path, &script_absolute)?;
501+
Ok(())
502+
})?;
503+
}
504+
}
488505
}
489506

490507
None
@@ -533,10 +550,13 @@ pub(crate) fn install_data(
533550

534551
match path.file_name().and_then(|name| name.to_str()) {
535552
Some("data") => {
553+
trace!(?dist_name, "Installing data/data");
536554
// Move the content of the folder to the root of the venv
537555
move_folder_recorded(&path, &layout.scheme.data, site_packages, record)?;
538556
}
539557
Some("scripts") => {
558+
trace!(?dist_name, "Installing data/scripts");
559+
let mut rename_or_copy = RenameOrCopy::default();
540560
let mut initialized = false;
541561
for file in fs::read_dir(path)? {
542562
let file = file?;
@@ -563,17 +583,27 @@ pub(crate) fn install_data(
563583
initialized = true;
564584
}
565585

566-
install_script(layout, relocatable, site_packages, record, &file)?;
586+
install_script(
587+
layout,
588+
relocatable,
589+
site_packages,
590+
record,
591+
&file,
592+
&mut rename_or_copy,
593+
)?;
567594
}
568595
}
569596
Some("headers") => {
597+
trace!(?dist_name, "Installing data/headers");
570598
let target_path = layout.scheme.include.join(dist_name.as_str());
571599
move_folder_recorded(&path, &target_path, site_packages, record)?;
572600
}
573601
Some("purelib") => {
602+
trace!(?dist_name, "Installing data/purelib");
574603
move_folder_recorded(&path, &layout.scheme.purelib, site_packages, record)?;
575604
}
576605
Some("platlib") => {
606+
trace!(?dist_name, "Installing data/platlib");
577607
move_folder_recorded(&path, &layout.scheme.platlib, site_packages, record)?;
578608
}
579609
_ => {
@@ -801,6 +831,37 @@ pub(crate) fn parse_scripts(
801831
scripts_from_ini(extras, python_minor, ini)
802832
}
803833

834+
/// Rename a file with a fallback to copy that switches over on the first failure.
835+
#[derive(Default, Copy, Clone)]
836+
enum RenameOrCopy {
837+
#[default]
838+
Rename,
839+
Copy,
840+
}
841+
842+
impl RenameOrCopy {
843+
/// Try to rename, and on failure, copy.
844+
///
845+
/// Usually, source and target are on the same device, so we can rename, but if that fails, we
846+
/// have to copy. If renaming failed once, we switch to copy permanently.
847+
fn rename_or_copy(&mut self, from: impl AsRef<Path>, to: impl AsRef<Path>) -> io::Result<()> {
848+
match self {
849+
Self::Rename => match fs_err::rename(from.as_ref(), to.as_ref()) {
850+
Ok(()) => {}
851+
Err(err) => {
852+
*self = RenameOrCopy::Copy;
853+
debug!("Failed to rename, falling back to copy: {err}");
854+
fs_err::copy(from.as_ref(), to.as_ref())?;
855+
}
856+
},
857+
Self::Copy => {
858+
fs_err::copy(from.as_ref(), to.as_ref())?;
859+
}
860+
}
861+
Ok(())
862+
}
863+
}
864+
804865
#[cfg(test)]
805866
mod test {
806867
use std::io::Cursor;

0 commit comments

Comments
 (0)