From 0754418af00b45fbc045de24566a283a9ba1ba78 Mon Sep 17 00:00:00 2001 From: konstin Date: Tue, 13 May 2025 22:50:40 +0200 Subject: [PATCH 1/2] Warn when two packages write to the same module --- crates/uv-install-wheel/src/install.rs | 2 +- crates/uv-install-wheel/src/linker.rs | 104 ++++++++++++++---- crates/uv/tests/it/pip_install.rs | 139 +++++++++++++++++++++++++ 3 files changed, 225 insertions(+), 20 deletions(-) diff --git a/crates/uv-install-wheel/src/install.rs b/crates/uv-install-wheel/src/install.rs index 9235a3509be6f..a856cb6aca2f5 100644 --- a/crates/uv-install-wheel/src/install.rs +++ b/crates/uv-install-wheel/src/install.rs @@ -75,7 +75,7 @@ pub fn install_wheel( LibKind::Pure => &layout.scheme.purelib, LibKind::Plat => &layout.scheme.platlib, }; - let num_unpacked = link_mode.link_wheel_files(site_packages, &wheel, locks)?; + let num_unpacked = link_mode.link_wheel_files(site_packages, &wheel, locks, filename)?; trace!(?name, "Extracted {num_unpacked} files"); // Read the RECORD file. diff --git a/crates/uv-install-wheel/src/linker.rs b/crates/uv-install-wheel/src/linker.rs index cc08c17da5e07..168dfe3cc60ae 100644 --- a/crates/uv-install-wheel/src/linker.rs +++ b/crates/uv-install-wheel/src/linker.rs @@ -4,16 +4,53 @@ use fs_err::DirEntry; use reflink_copy as reflink; use rustc_hash::FxHashMap; use serde::{Deserialize, Serialize}; +use std::ffi::{OsStr, OsString}; use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; use std::time::SystemTime; use tempfile::tempdir_in; use tracing::{debug, instrument, trace}; -use uv_warnings::warn_user_once; +use uv_distribution_filename::WheelFilename; +use uv_fs::Simplified; +use uv_warnings::{warn_user, warn_user_once}; use walkdir::WalkDir; #[derive(Debug, Default)] -pub struct Locks(Mutex>>>); +pub struct Locks { + /// The parent directory of a file in a synchronized copy + copy_dir_locks: Mutex>>>, + /// Top level modules (excluding namespaces) we write to. + modules: Mutex>, +} + +impl Locks { + /// Warn when a module exists in multiple packages. + fn warn_module_conflict(&self, module: &OsStr, wheel_a: &WheelFilename) { + if let Some(wheel_b) = self + .modules + .lock() + .unwrap() + .insert(module.to_os_string(), wheel_a.clone()) + { + // Sort for consistent output, at least with two packages + let (wheel_a, wheel_b) = if wheel_b.name > wheel_a.name { + (&wheel_b, wheel_a) + } else { + (wheel_a, &wheel_b) + }; + warn_user!( + "The module `{}` is provided by more than one package, \ + which causes an install race condition and can result in a broken module. \ + Consider removing your dependency on either `{}` (v{}) or `{}` (v{}).", + module.simplified_display().green(), + wheel_a.name.cyan(), + wheel_a.version.cyan(), + wheel_b.name.cyan(), + wheel_b.version.cyan(), + ); + } + } +} #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(deny_unknown_fields, rename_all = "kebab-case")] @@ -48,12 +85,13 @@ impl LinkMode { site_packages: impl AsRef, wheel: impl AsRef, locks: &Locks, + filename: &WheelFilename, ) -> Result { match self { - Self::Clone => clone_wheel_files(site_packages, wheel, locks), - Self::Copy => copy_wheel_files(site_packages, wheel, locks), - Self::Hardlink => hardlink_wheel_files(site_packages, wheel, locks), - Self::Symlink => symlink_wheel_files(site_packages, wheel, locks), + Self::Clone => clone_wheel_files(site_packages, wheel, locks, filename), + Self::Copy => copy_wheel_files(site_packages, wheel, locks, filename), + Self::Hardlink => hardlink_wheel_files(site_packages, wheel, locks, filename), + Self::Symlink => symlink_wheel_files(site_packages, wheel, locks, filename), } } @@ -73,18 +111,25 @@ fn clone_wheel_files( site_packages: impl AsRef, wheel: impl AsRef, locks: &Locks, + filename: &WheelFilename, ) -> Result { + let wheel = wheel.as_ref(); let mut count = 0usize; let mut attempt = Attempt::default(); - for entry in fs::read_dir(wheel.as_ref())? { - clone_recursive( - site_packages.as_ref(), - wheel.as_ref(), - locks, - &entry?, - &mut attempt, - )?; + for entry in fs::read_dir(wheel)? { + let entry = entry?; + if entry.path().join("__init__.py").is_file() { + locks.warn_module_conflict( + entry + .path() + .strip_prefix(wheel) + .expect("wheel path starts with wheel root") + .as_os_str(), + filename, + ); + } + clone_recursive(site_packages.as_ref(), wheel, locks, &entry, &mut attempt)?; count += 1; } @@ -153,7 +198,10 @@ fn clone_recursive( ) -> Result<(), Error> { // Determine the existing and destination paths. let from = entry.path(); - let to = site_packages.join(from.strip_prefix(wheel).unwrap()); + let to = site_packages.join( + from.strip_prefix(wheel) + .expect("wheel path starts with wheel root"), + ); trace!("Cloning {} to {}", from.display(), to.display()); @@ -249,6 +297,7 @@ fn copy_wheel_files( site_packages: impl AsRef, wheel: impl AsRef, locks: &Locks, + filename: &WheelFilename, ) -> Result { let mut count = 0usize; @@ -256,10 +305,11 @@ fn copy_wheel_files( for entry in WalkDir::new(&wheel) { let entry = entry?; let path = entry.path(); - let relative = path.strip_prefix(&wheel).expect("walkdir starts with root"); let out_path = site_packages.as_ref().join(relative); + warn_module_conflict(locks, filename, relative); + if entry.file_type().is_dir() { fs::create_dir_all(&out_path)?; continue; @@ -278,6 +328,7 @@ fn hardlink_wheel_files( site_packages: impl AsRef, wheel: impl AsRef, locks: &Locks, + filename: &WheelFilename, ) -> Result { let mut attempt = Attempt::default(); let mut count = 0usize; @@ -286,10 +337,11 @@ fn hardlink_wheel_files( for entry in WalkDir::new(&wheel) { let entry = entry?; let path = entry.path(); - let relative = path.strip_prefix(&wheel).expect("walkdir starts with root"); let out_path = site_packages.as_ref().join(relative); + warn_module_conflict(locks, filename, relative); + if entry.file_type().is_dir() { fs::create_dir_all(&out_path)?; continue; @@ -376,6 +428,7 @@ fn symlink_wheel_files( site_packages: impl AsRef, wheel: impl AsRef, locks: &Locks, + filename: &WheelFilename, ) -> Result { let mut attempt = Attempt::default(); let mut count = 0usize; @@ -384,10 +437,11 @@ fn symlink_wheel_files( for entry in WalkDir::new(&wheel) { let entry = entry?; let path = entry.path(); - let relative = path.strip_prefix(&wheel).unwrap(); let out_path = site_packages.as_ref().join(relative); + warn_module_conflict(locks, filename, relative); + if entry.file_type().is_dir() { fs::create_dir_all(&out_path)?; continue; @@ -476,7 +530,7 @@ fn symlink_wheel_files( fn synchronized_copy(from: &Path, to: &Path, locks: &Locks) -> std::io::Result<()> { // Ensure we have a lock for the directory. let dir_lock = { - let mut locks_guard = locks.0.lock().unwrap(); + let mut locks_guard = locks.copy_dir_locks.lock().unwrap(); locks_guard .entry(to.parent().unwrap().to_path_buf()) .or_insert_with(|| Arc::new(Mutex::new(()))) @@ -492,6 +546,18 @@ fn synchronized_copy(from: &Path, to: &Path, locks: &Locks) -> std::io::Result<( Ok(()) } +/// Warn when a module exists in multiple packages. +fn warn_module_conflict(locks: &Locks, filename: &WheelFilename, relative: &Path) { + // Check for `__init__.py` to account for namespace packages. + // TODO(konsti): We need to warn for overlapping namespace packages, too. + if relative.components().count() == 2 + && relative.components().next_back().unwrap().as_os_str() == "__init__.py" + { + // Modules must be UTF-8, but we can skip the conversion using OsStr. + locks.warn_module_conflict(relative.components().next().unwrap().as_os_str(), filename); + } +} + #[cfg(unix)] fn create_symlink, Q: AsRef>(original: P, link: Q) -> std::io::Result<()> { fs_err::os::unix::fs::symlink(original, link) diff --git a/crates/uv/tests/it/pip_install.rs b/crates/uv/tests/it/pip_install.rs index 8126cade9d7ad..e0038f310caec 100644 --- a/crates/uv/tests/it/pip_install.rs +++ b/crates/uv/tests/it/pip_install.rs @@ -12455,3 +12455,142 @@ fn pip_install_build_dependencies_respect_locked_versions() -> Result<()> { Ok(()) } + +/// Check that we warn for overlapping packages but not for correctly overlapping namespace +/// packages. +#[test] +fn overlapping_packages_warning() -> Result<()> { + let context = TestContext::new("3.12"); + + context + .build() + .arg("--preview") + .arg(context.workspace_root.join("scripts/packages/built-by-uv")) + .arg("--out-dir") + .arg(context.temp_dir.path()) + .assert() + .success(); + + // Overlaps with `built-by-uv` + let also_build_by_uv = context.temp_dir.child("also-built-by-uv"); + also_build_by_uv.child("pyproject.toml").write_str( + r#" + [project] + name = "also-built-by-uv" + version = "0.1.0" + requires-python = ">=3.12" + + [tool.uv.build-backend] + module-name = "built_by_uv" + + [build-system] + requires = ["uv_build>=0.7,<10000"] + build-backend = "uv_build" + "#, + )?; + also_build_by_uv + .child("src") + .child("built_by_uv") + .child("__init__.py") + .touch()?; + context + .build() + .arg("--preview") + .arg(also_build_by_uv.path()) + .arg("--out-dir") + .arg(context.temp_dir.path()) + .assert() + .success(); + + // Check that overlapping packages show a warning + uv_snapshot!(context.filters(), context.pip_install() + .arg("--no-deps") + .arg(context.temp_dir.join("built_by_uv-0.1.0-py3-none-any.whl")) + .arg(context.temp_dir.join("also_built_by_uv-0.1.0-py3-none-any.whl")), @r" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 2 packages in [TIME] + Prepared 2 packages in [TIME] + warning: The module `built_by_uv` is provided by more than one package, which causes an install race condition and can result in a broken module. Consider removing your dependency on either `built-by-uv` (v0.1.0) or `also-built-by-uv` (v0.1.0). + Installed 2 packages in [TIME] + + also-built-by-uv==0.1.0 (from file://[TEMP_DIR]/also_built_by_uv-0.1.0-py3-none-any.whl) + + built-by-uv==0.1.0 (from file://[TEMP_DIR]/built_by_uv-0.1.0-py3-none-any.whl) + " + ); + + // Check that correctly overlapping namespace packages don't show a warning + uv_snapshot!(context.pip_install() + .arg("--no-deps") + .arg("poetry") + .arg("poetry-core"), @r" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 2 packages in [TIME] + Prepared 2 packages in [TIME] + Installed 2 packages in [TIME] + + poetry==1.8.2 + + poetry-core==1.9.0 + " + ); + + // Check that we can uninstall even if the venv is bogus. + uv_snapshot!(context.filters(), context.pip_uninstall() + .arg("built_by_uv"), @r" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Uninstalled 1 package in [TIME] + - built-by-uv==0.1.0 (from file://[TEMP_DIR]/built_by_uv-0.1.0-py3-none-any.whl) + " + ); + uv_snapshot!(context.filters(), context.pip_uninstall() + .arg("also_built_by_uv"), @r" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Uninstalled 1 package in [TIME] + - also-built-by-uv==0.1.0 (from file://[TEMP_DIR]/also_built_by_uv-0.1.0-py3-none-any.whl) + " + ); + + // Check that installing one of the packages on their own doesn't warn. + uv_snapshot!(context.filters(), context.pip_install() + .arg("--no-deps") + .arg(context.temp_dir.join("built_by_uv-0.1.0-py3-none-any.whl")), @r" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 1 package in [TIME] + Installed 1 package in [TIME] + + built-by-uv==0.1.0 (from file://[TEMP_DIR]/built_by_uv-0.1.0-py3-none-any.whl) + " + ); + // Currently, we don't warn if we install them one wheel at a time. + uv_snapshot!(context.filters(), context.pip_install() + .arg("--no-deps") + .arg(context.temp_dir.join("also_built_by_uv-0.1.0-py3-none-any.whl")), @r" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 1 package in [TIME] + Installed 1 package in [TIME] + + also-built-by-uv==0.1.0 (from file://[TEMP_DIR]/also_built_by_uv-0.1.0-py3-none-any.whl) + " + ); + + Ok(()) +} From f18ca4941572d7cf8d32513dcf13ce8cec0d8949 Mon Sep 17 00:00:00 2001 From: konstin Date: Thu, 24 Jul 2025 12:10:30 +0200 Subject: [PATCH 2/2] Use source trees directly --- crates/uv/tests/it/pip_install.rs | 39 +++++++++++-------------------- 1 file changed, 13 insertions(+), 26 deletions(-) diff --git a/crates/uv/tests/it/pip_install.rs b/crates/uv/tests/it/pip_install.rs index e0038f310caec..1c372cdec2223 100644 --- a/crates/uv/tests/it/pip_install.rs +++ b/crates/uv/tests/it/pip_install.rs @@ -12462,14 +12462,7 @@ fn pip_install_build_dependencies_respect_locked_versions() -> Result<()> { fn overlapping_packages_warning() -> Result<()> { let context = TestContext::new("3.12"); - context - .build() - .arg("--preview") - .arg(context.workspace_root.join("scripts/packages/built-by-uv")) - .arg("--out-dir") - .arg(context.temp_dir.path()) - .assert() - .success(); + let built_by_uv = context.workspace_root.join("scripts/packages/built-by-uv"); // Overlaps with `built-by-uv` let also_build_by_uv = context.temp_dir.child("also-built-by-uv"); @@ -12493,20 +12486,12 @@ fn overlapping_packages_warning() -> Result<()> { .child("built_by_uv") .child("__init__.py") .touch()?; - context - .build() - .arg("--preview") - .arg(also_build_by_uv.path()) - .arg("--out-dir") - .arg(context.temp_dir.path()) - .assert() - .success(); // Check that overlapping packages show a warning uv_snapshot!(context.filters(), context.pip_install() .arg("--no-deps") - .arg(context.temp_dir.join("built_by_uv-0.1.0-py3-none-any.whl")) - .arg(context.temp_dir.join("also_built_by_uv-0.1.0-py3-none-any.whl")), @r" + .arg(&built_by_uv) + .arg(also_build_by_uv.path()), @r" success: true exit_code: 0 ----- stdout ----- @@ -12516,8 +12501,8 @@ fn overlapping_packages_warning() -> Result<()> { Prepared 2 packages in [TIME] warning: The module `built_by_uv` is provided by more than one package, which causes an install race condition and can result in a broken module. Consider removing your dependency on either `built-by-uv` (v0.1.0) or `also-built-by-uv` (v0.1.0). Installed 2 packages in [TIME] - + also-built-by-uv==0.1.0 (from file://[TEMP_DIR]/also_built_by_uv-0.1.0-py3-none-any.whl) - + built-by-uv==0.1.0 (from file://[TEMP_DIR]/built_by_uv-0.1.0-py3-none-any.whl) + + also-built-by-uv==0.1.0 (from file://[TEMP_DIR]/also-built-by-uv) + + built-by-uv==0.1.0 (from file://[WORKSPACE]/scripts/packages/built-by-uv) " ); @@ -12548,7 +12533,7 @@ fn overlapping_packages_warning() -> Result<()> { ----- stderr ----- Uninstalled 1 package in [TIME] - - built-by-uv==0.1.0 (from file://[TEMP_DIR]/built_by_uv-0.1.0-py3-none-any.whl) + - built-by-uv==0.1.0 (from file://[WORKSPACE]/scripts/packages/built-by-uv) " ); uv_snapshot!(context.filters(), context.pip_uninstall() @@ -12559,36 +12544,38 @@ fn overlapping_packages_warning() -> Result<()> { ----- stderr ----- Uninstalled 1 package in [TIME] - - also-built-by-uv==0.1.0 (from file://[TEMP_DIR]/also_built_by_uv-0.1.0-py3-none-any.whl) + - also-built-by-uv==0.1.0 (from file://[TEMP_DIR]/also-built-by-uv) " ); // Check that installing one of the packages on their own doesn't warn. uv_snapshot!(context.filters(), context.pip_install() .arg("--no-deps") - .arg(context.temp_dir.join("built_by_uv-0.1.0-py3-none-any.whl")), @r" + .arg(built_by_uv), @r" success: true exit_code: 0 ----- stdout ----- ----- stderr ----- Resolved 1 package in [TIME] + Prepared 1 package in [TIME] Installed 1 package in [TIME] - + built-by-uv==0.1.0 (from file://[TEMP_DIR]/built_by_uv-0.1.0-py3-none-any.whl) + + built-by-uv==0.1.0 (from file://[WORKSPACE]/scripts/packages/built-by-uv) " ); // Currently, we don't warn if we install them one wheel at a time. uv_snapshot!(context.filters(), context.pip_install() .arg("--no-deps") - .arg(context.temp_dir.join("also_built_by_uv-0.1.0-py3-none-any.whl")), @r" + .arg(also_build_by_uv.path()), @r" success: true exit_code: 0 ----- stdout ----- ----- stderr ----- Resolved 1 package in [TIME] + Prepared 1 package in [TIME] Installed 1 package in [TIME] - + also-built-by-uv==0.1.0 (from file://[TEMP_DIR]/also_built_by_uv-0.1.0-py3-none-any.whl) + + also-built-by-uv==0.1.0 (from file://[TEMP_DIR]/also-built-by-uv) " );