From ceb7a7ec5b51cb24b79e524d9c3eb1b6a5f751f4 Mon Sep 17 00:00:00 2001 From: Luca BRUNO Date: Fri, 20 Sep 2024 19:37:41 +0200 Subject: [PATCH 1/2] Improve same-interpreter detection logic --- Cargo.lock | 1 + crates/uv/Cargo.toml | 1 + crates/uv/src/commands/tool/install.rs | 18 +++++++++++++++--- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3164f0b72b29..c3ecec78d735 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4482,6 +4482,7 @@ dependencies = [ "regex", "reqwest", "rustc-hash", + "same-file", "serde", "serde_json", "similar", diff --git a/crates/uv/Cargo.toml b/crates/uv/Cargo.toml index 32af21d9bf01..c711da74d76b 100644 --- a/crates/uv/Cargo.toml +++ b/crates/uv/Cargo.toml @@ -66,6 +66,7 @@ owo-colors = { workspace = true } rayon = { workspace = true } regex = { workspace = true } rustc-hash = { workspace = true } +same-file = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } tempfile = { workspace = true } diff --git a/crates/uv/src/commands/tool/install.rs b/crates/uv/src/commands/tool/install.rs index 590958162300..ba32346729cb 100644 --- a/crates/uv/src/commands/tool/install.rs +++ b/crates/uv/src/commands/tool/install.rs @@ -276,13 +276,25 @@ pub(crate) async fn install( installed_tools .get_environment(&from.name, &cache)? .filter(|environment| { + let same_interpreter: bool; + // NOTE(lucab): this compares `base_prefix` paths as a proxy for // detecting interpreters mismatches. Directly comparing interpreters // (by paths or binaries on-disk) would result in several false // positives on Windows due to file-copying and shims. - let old_base_prefix = environment.interpreter().sys_base_prefix(); - let selected_base_prefix = interpreter.sys_base_prefix(); - if old_base_prefix == selected_base_prefix { + #[cfg(target_os="windows")] + { + let old_base_prefix = environment.interpreter().sys_base_prefix(); + let selected_base_prefix = interpreter.sys_base_prefix(); + same_interpreter = old_base_prefix == selected_base_prefix; + } + #[cfg(not(target_os="windows"))] + { + same_interpreter = environment.interpreter().sys_executable() == interpreter.sys_executable() + || same_file::is_same_file(environment.interpreter().sys_executable(), interpreter.sys_executable()).unwrap_or(false); + } + + if same_interpreter { trace!( "Existing interpreter matches the requested interpreter for `{}`: {}", from.name, From 8ed90ba89242cc659ccf4b8d80e641e14585121c Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Fri, 20 Sep 2024 12:54:17 -0500 Subject: [PATCH 2/2] Review --- crates/uv/src/commands/tool/install.rs | 28 ++++++++++++-------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/crates/uv/src/commands/tool/install.rs b/crates/uv/src/commands/tool/install.rs index ba32346729cb..ef30de6eb612 100644 --- a/crates/uv/src/commands/tool/install.rs +++ b/crates/uv/src/commands/tool/install.rs @@ -276,23 +276,21 @@ pub(crate) async fn install( installed_tools .get_environment(&from.name, &cache)? .filter(|environment| { - let same_interpreter: bool; - - // NOTE(lucab): this compares `base_prefix` paths as a proxy for - // detecting interpreters mismatches. Directly comparing interpreters - // (by paths or binaries on-disk) would result in several false - // positives on Windows due to file-copying and shims. - #[cfg(target_os="windows")] - { + // TODO(zanieb): Consider using `sysconfig.get_path("stdlib")` instead, which + // should be generally robust. + // TODO(zanieb): Move this into a utility on `Interpreter` since it's non-trivial. + let same_interpreter = if cfg!(windows) { + // On Windows, we can't canonicalize an interpreter based on its executable path + // because the executables are separate shim files (not links). Instead, we + // compare the `sys.base_prefix`. let old_base_prefix = environment.interpreter().sys_base_prefix(); let selected_base_prefix = interpreter.sys_base_prefix(); - same_interpreter = old_base_prefix == selected_base_prefix; - } - #[cfg(not(target_os="windows"))] - { - same_interpreter = environment.interpreter().sys_executable() == interpreter.sys_executable() - || same_file::is_same_file(environment.interpreter().sys_executable(), interpreter.sys_executable()).unwrap_or(false); - } + old_base_prefix == selected_base_prefix + } else { + // On Unix, we can see if the canonicalized executable is the same file. + environment.interpreter().sys_executable() == interpreter.sys_executable() + || same_file::is_same_file(environment.interpreter().sys_executable(), interpreter.sys_executable()).unwrap_or(false) + }; if same_interpreter { trace!(