From 8b8f34ac21d4fe884882ac707594bdb9532c2f38 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 26 Jul 2024 08:57:33 -0400 Subject: [PATCH] Avoid canonicalizing executables on Windows (#5446) ## Summary If you have an executable path on a network share path (like `\\some-host\some-share\...\python.exe`), canonicalizing it adds the `\\?` prefix, but dunce cannot safely strip it. This PR changes the Windows logic to avoid canonicalizing altogether. We don't really expect symlinks on Windows, so it seems unimportant to resolve them. Closes: https://github.com/astral-sh/uv/issues/5440. --- crates/uv-cache/src/timestamp.rs | 2 +- crates/uv-fs/src/path.rs | 130 +++---------------------- crates/uv-python/src/interpreter.rs | 17 ++-- crates/uv-virtualenv/src/virtualenv.rs | 2 +- 4 files changed, 26 insertions(+), 125 deletions(-) diff --git a/crates/uv-cache/src/timestamp.rs b/crates/uv-cache/src/timestamp.rs index e5ffc708c..aa5405366 100644 --- a/crates/uv-cache/src/timestamp.rs +++ b/crates/uv-cache/src/timestamp.rs @@ -15,7 +15,7 @@ pub struct Timestamp(std::time::SystemTime); impl Timestamp { /// Return the [`Timestamp`] for the given path. pub fn from_path(path: impl AsRef) -> std::io::Result { - let metadata = path.as_ref().metadata()?; + let metadata = fs_err::metadata(path.as_ref())?; Ok(Self::from_metadata(&metadata)) } diff --git a/crates/uv-fs/src/path.rs b/crates/uv-fs/src/path.rs index fe683cd91..f0e0a10c3 100644 --- a/crates/uv-fs/src/path.rs +++ b/crates/uv-fs/src/path.rs @@ -1,7 +1,6 @@ use either::Either; use std::borrow::Cow; use std::path::{Component, Path, PathBuf}; -use std::{io, iter}; use once_cell::sync::Lazy; use path_slash::PathExt; @@ -251,131 +250,30 @@ pub fn absolutize_path(path: &Path) -> Result, std::io::Error> { path.absolutize_from(CWD.simplified()) } -/// Like `fs_err::canonicalize`, but with permissive failures on Windows. -/// -/// On Windows, we can't canonicalize the resolved path to Pythons that are installed via the -/// Windows Store. For example, if you install Python via the Windows Store, then run `python` -/// and print the `sys.executable` path, you'll get a path like: -/// -/// ```text -/// C:\Users\crmar\AppData\Local\Microsoft\WindowsApps\PythonSoftwareFoundation.Python.3.11_qbs5n2kfra8p0\python.exe -/// ``` -/// -/// Attempting to canonicalize this path will fail with `ErrorKind::Uncategorized`. +/// Like `fs_err::canonicalize`, but avoids attempting to resolve symlinks on Windows. pub fn canonicalize_executable(path: impl AsRef) -> std::io::Result { let path = path.as_ref(); - if is_windows_store_python(path) { + debug_assert!( + path.is_absolute(), + "path must be absolute: {}", + path.display() + ); + if cfg!(windows) { Ok(path.to_path_buf()) } else { fs_err::canonicalize(path) } } -/// Returns `true` if this is a Python executable or shim installed via the Windows Store, based on -/// the path. -/// -/// This method does _not_ introspect the filesystem to determine if the shim is a redirect to the -/// Windows Store installer. In other words, it assumes that the path represents a Python -/// executable, not a redirect. -fn is_windows_store_python(path: &Path) -> bool { - /// Returns `true` if this is a Python executable shim installed via the Windows Store, like: - /// - /// ```text - /// C:\Users\crmar\AppData\Local\Microsoft\WindowsApps\python3.exe - /// ``` - fn is_windows_store_python_shim(path: &Path) -> bool { - let mut components = path.components().rev(); - - // Ex) `python.exe`, or `python3.exe`, or `python3.12.exe` - if !components - .next() - .and_then(|component| component.as_os_str().to_str()) - .is_some_and(|component| component.starts_with("python")) - { - return false; - } - - // Ex) `WindowsApps` - if !components - .next() - .is_some_and(|component| component.as_os_str() == "WindowsApps") - { - return false; - } - - // Ex) `Microsoft` - if !components - .next() - .is_some_and(|component| component.as_os_str() == "Microsoft") - { - return false; - } - - true - } - - /// Returns `true` if this is a Python executable installed via the Windows Store, like: - /// - /// ```text - /// C:\Users\crmar\AppData\Local\Microsoft\WindowsApps\PythonSoftwareFoundation.Python.3.11_qbs5n2kfra8p0\python.exe - /// ``` - fn is_windows_store_python_executable(path: &Path) -> bool { - let mut components = path.components().rev(); - - // Ex) `python.exe` - if !components - .next() - .and_then(|component| component.as_os_str().to_str()) - .is_some_and(|component| component.starts_with("python")) - { - return false; - } - - // Ex) `PythonSoftwareFoundation.Python.3.11_qbs5n2kfra8p0` - if !components - .next() - .and_then(|component| component.as_os_str().to_str()) - .is_some_and(|component| component.starts_with("PythonSoftwareFoundation.Python.3.")) - { - return false; - } - - // Ex) `WindowsApps` - if !components - .next() - .is_some_and(|component| component.as_os_str() == "WindowsApps") - { - return false; - } - - // Ex) `Microsoft` - if !components - .next() - .is_some_and(|component| component.as_os_str() == "Microsoft") - { - return false; - } - - true - } - - if !cfg!(windows) { - return false; - } - - if !path.is_absolute() { - return false; - } - - is_windows_store_python_shim(path) || is_windows_store_python_executable(path) -} - /// Compute a path describing `path` relative to `base`. /// /// `lib/python/site-packages/foo/__init__.py` and `lib/python/site-packages` -> `foo/__init__.py` /// `lib/marker.txt` and `lib/python/site-packages` -> `../../marker.txt` /// `bin/foo_launcher` and `lib/python/site-packages` -> `../../../bin/foo_launcher` -pub fn relative_to(path: impl AsRef, base: impl AsRef) -> Result { +pub fn relative_to( + path: impl AsRef, + base: impl AsRef, +) -> Result { // Find the longest common prefix, and also return the path stripped from that prefix let (stripped, common_prefix) = base .as_ref() @@ -388,8 +286,8 @@ pub fn relative_to(path: impl AsRef, base: impl AsRef) -> Result, base: impl AsRef) -> Result(); + let up = std::iter::repeat("..").take(levels_up).collect::(); Ok(up.join(stripped)) } diff --git a/crates/uv-python/src/interpreter.rs b/crates/uv-python/src/interpreter.rs index 71a9ff2d0..693e7d721 100644 --- a/crates/uv-python/src/interpreter.rs +++ b/crates/uv-python/src/interpreter.rs @@ -709,24 +709,27 @@ impl InterpreterInfo { /// unless the Python executable changes, so we use the executable's last modified /// time as a cache key. pub(crate) fn query_cached(executable: &Path, cache: &Cache) -> Result { + let absolute = uv_fs::absolutize_path(executable)?; + let cache_entry = cache.entry( CacheBucket::Interpreter, "", - // We use the absolute path for the cache entry to avoid cache collisions for relative paths - // but we do not want to query the executable with symbolic links resolved - format!("{}.msgpack", digest(&uv_fs::absolutize_path(executable)?)), + // We use the absolute path for the cache entry to avoid cache collisions for relative + // paths. But we don't to query the executable with symbolic links resolved. + format!("{}.msgpack", digest(&absolute)), ); // We check the timestamp of the canonicalized executable to check if an underlying - // interpreter has been modified - let modified = - Timestamp::from_path(uv_fs::canonicalize_executable(executable).map_err(|err| { + // interpreter has been modified. + let modified = uv_fs::canonicalize_executable(&absolute) + .and_then(Timestamp::from_path) + .map_err(|err| { if err.kind() == io::ErrorKind::NotFound { Error::NotFound(executable.to_path_buf()) } else { err.into() } - })?)?; + })?; // Read from the cache. if cache diff --git a/crates/uv-virtualenv/src/virtualenv.rs b/crates/uv-virtualenv/src/virtualenv.rs index a1203f902..09348e8e5 100644 --- a/crates/uv-virtualenv/src/virtualenv.rs +++ b/crates/uv-virtualenv/src/virtualenv.rs @@ -76,7 +76,7 @@ pub(crate) fn create( interpreter.sys_base_prefix().join("python.exe") } } else { - uv_fs::canonicalize_executable(interpreter.sys_executable())? + interpreter.sys_executable().to_path_buf() } } else { unimplemented!("Only Windows and Unix are supported")