Use consistent canonicalization for URLs (#5980)

Right now, the URL gets out-of-sync with the install path, since the install path is canonicalized. This leads to a subtle error on Windows (in CI) in which we don't preserve caching across resolution and installation.
astral-sh · Aug 10, 2024 · 19ac9af · 19ac9af
1 parent cd0171a
commit 19ac9af
Show file tree

Hide file tree

Showing 6 changed files with 41 additions and 38 deletions.
diff --git a/crates/distribution-types/src/lib.rs b/crates/distribution-types/src/lib.rs
@@ -216,7 +216,7 @@ pub struct DirectUrlBuiltDist {
 #[derive(Debug, Clone, Hash)]
 pub struct PathBuiltDist {
     pub filename: WheelFilename,
-    /// The resolved, absolute path to the wheel which we use for installing.
+    /// The absolute, canonicalized path to the wheel which we use for installing.
     pub install_path: PathBuf,
     /// The absolute path or path relative to the workspace root pointing to the wheel
     /// which we use for locking. Unlike `given` on the verbatim URL all environment variables
@@ -277,7 +277,7 @@ pub struct GitSourceDist {
 #[derive(Debug, Clone, Hash)]
 pub struct PathSourceDist {
     pub name: PackageName,
-    /// The resolved, absolute path to the distribution which we use for installing.
+    /// The absolute, canonicalized path to the distribution which we use for installing.
     pub install_path: PathBuf,
     /// The absolute path or path relative to the workspace root pointing to the distribution
     /// which we use for locking. Unlike `given` on the verbatim URL all environment variables
@@ -293,7 +293,7 @@ pub struct PathSourceDist {
 #[derive(Debug, Clone, Hash)]
 pub struct DirectorySourceDist {
     pub name: PackageName,
-    /// The resolved, absolute path to the distribution which we use for installing.
+    /// The absolute, canonicalized path to the distribution which we use for installing.
     pub install_path: PathBuf,
     /// The absolute path or path relative to the workspace root pointing to the distribution
     /// which we use for locking. Unlike `given` on the verbatim URL all environment variables
@@ -354,7 +354,7 @@ impl Dist {
         ext: DistExtension,
     ) -> Result<Dist, Error> {
         // Store the canonicalized path, which also serves to validate that it exists.
-        let canonicalized_path = match install_path.canonicalize() {
+        let install_path = match install_path.canonicalize() {
             Ok(path) => path,
             Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
                 return Err(Error::NotFound(url.to_url()));
@@ -376,14 +376,14 @@ impl Dist {
                 }
                 Ok(Self::Built(BuiltDist::Path(PathBuiltDist {
                     filename,
-                    install_path: canonicalized_path.clone(),
+                    install_path,
                     lock_path: lock_path.to_path_buf(),
                     url,
                 })))
             }
             DistExtension::Source(ext) => Ok(Self::Source(SourceDist::Path(PathSourceDist {
                 name,
-                install_path: canonicalized_path.clone(),
+                install_path,
                 lock_path: lock_path.to_path_buf(),
                 ext,
                 url,
@@ -400,7 +400,7 @@ impl Dist {
         editable: bool,
     ) -> Result<Dist, Error> {
         // Store the canonicalized path, which also serves to validate that it exists.
-        let canonicalized_path = match install_path.canonicalize() {
+        let install_path = match install_path.canonicalize() {
             Ok(path) => path,
             Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
                 return Err(Error::NotFound(url.to_url()));
@@ -411,7 +411,7 @@ impl Dist {
         // Determine whether the path represents an archive or a directory.
         Ok(Self::Source(SourceDist::Directory(DirectorySourceDist {
             name,
-            install_path: canonicalized_path.clone(),
+            install_path,
             lock_path: lock_path.to_path_buf(),
             editable,
             url,

diff --git a/crates/pep508-rs/src/verbatim_url.rs b/crates/pep508-rs/src/verbatim_url.rs
@@ -7,7 +7,7 @@ use std::sync::LazyLock;
 use thiserror::Error;
 use url::{ParseError, Url};
 
-use uv_fs::{normalize_absolute_path, normalize_url_path};
+use uv_fs::{normalize_absolute_path, normalize_url_path, Simplified};
 
 use crate::Pep508Url;
 
@@ -36,9 +36,12 @@ impl VerbatimUrl {
     pub fn from_path(path: impl AsRef<Path>) -> Result<Self, VerbatimUrlError> {
         let path = path.as_ref();
 
-        // Normalize the path.
-        let path = normalize_absolute_path(path)
-            .map_err(|err| VerbatimUrlError::Normalization(path.to_path_buf(), err))?;
+        // Normalize the path (and canonicalize it, if possible).
+        let path = match path.simple_canonicalize() {
+            Ok(path) => path,
+            Err(_) => normalize_absolute_path(path)
+                .map_err(|err| VerbatimUrlError::Normalization(path.to_path_buf(), err))?,
+        };
 
         // Extract the fragment, if it exists.
         let (path, fragment) = split_fragment(&path);
@@ -77,9 +80,12 @@ impl VerbatimUrl {
             base_dir.as_ref().join(path)
         };
 
-        // Normalize the path.
-        let path = normalize_absolute_path(&path)
-            .map_err(|err| VerbatimUrlError::Normalization(path.clone(), err))?;
+        // Normalize the path (and canonicalize it, if possible).
+        let path = match path.simple_canonicalize() {
+            Ok(path) => path,
+            Err(_) => normalize_absolute_path(&path)
+                .map_err(|err| VerbatimUrlError::Normalization(path.clone(), err))?,
+        };
 
         // Extract the fragment, if it exists.
         let (path, fragment) = split_fragment(&path);
@@ -107,9 +113,11 @@ impl VerbatimUrl {
             return Err(VerbatimUrlError::WorkingDirectory(path.to_path_buf()));
         };
 
-        // Normalize the path.
-        let Ok(path) = normalize_absolute_path(&path) else {
-            return Err(VerbatimUrlError::WorkingDirectory(path));
+        // Normalize the path (and canonicalize it, if possible).
+        let path = match path.simple_canonicalize() {
+            Ok(path) => path,
+            Err(_) => normalize_absolute_path(&path)
+                .map_err(|_| VerbatimUrlError::WorkingDirectory(path))?,
         };
 
         // Extract the fragment, if it exists.

diff --git a/crates/pypi-types/src/parsed_url.rs b/crates/pypi-types/src/parsed_url.rs
@@ -185,7 +185,7 @@ impl ParsedUrl {
 #[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)]
 pub struct ParsedPathUrl {
     pub url: Url,
-    /// The resolved, absolute path to the distribution which we use for installing.
+    /// The absolute, canonicalized path to the distribution which we use for installing.
     pub install_path: PathBuf,
     /// The absolute path or path relative to the workspace root pointing to the distribution
     /// which we use for locking. Unlike `given` on the verbatim URL all environment variables
@@ -219,7 +219,7 @@ impl ParsedPathUrl {
 #[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)]
 pub struct ParsedDirectoryUrl {
     pub url: Url,
-    /// The resolved, absolute path to the distribution which we use for installing.
+    /// The absolute, canonicalized path to the distribution which we use for installing.
     pub install_path: PathBuf,
     /// The absolute path or path relative to the workspace root pointing to the distribution
     /// which we use for locking. Unlike `given` on the verbatim URL all environment variables

diff --git a/crates/pypi-types/src/requirement.rs b/crates/pypi-types/src/requirement.rs
@@ -313,7 +313,7 @@ pub enum RequirementSource {
     /// be a binary distribution (a `.whl` file) or a source distribution archive (a `.zip` or
     /// `.tar.gz` file).
     Path {
-        /// The resolved, absolute path to the distribution which we use for installing.
+        /// The absolute, canonicalized path to the distribution which we use for installing.
         install_path: PathBuf,
         /// The absolute path or path relative to the workspace root pointing to the distribution
         /// which we use for locking. Unlike `given` on the verbatim URL all environment variables
@@ -328,7 +328,7 @@ pub enum RequirementSource {
     /// A local source tree (a directory with a pyproject.toml in, or a legacy
     /// source distribution with only a setup.py but non pyproject.toml in it).
     Directory {
-        /// The resolved, absolute path to the distribution which we use for installing.
+        /// The absolute, canonicalized path to the distribution which we use for installing.
         install_path: PathBuf,
         /// The absolute path or path relative to the workspace root pointing to the distribution
         /// which we use for locking. Unlike `given` on the verbatim URL all environment variables

diff --git a/crates/uv-installer/src/plan.rs b/crates/uv-installer/src/plan.rs
@@ -269,7 +269,7 @@ impl<'a> Planner<'a> {
                     lock_path,
                 } => {
                     // Store the canonicalized path, which also serves to validate that it exists.
-                    let path = match install_path.canonicalize() {
+                    let install_path = match install_path.canonicalize() {
                         Ok(path) => path,
                         Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
                             return Err(Error::NotFound(url.to_url()).into());
@@ -280,7 +280,7 @@ impl<'a> Planner<'a> {
                     let sdist = DirectorySourceDist {
                         name: requirement.name.clone(),
                         url: url.clone(),
-                        install_path: path,
+                        install_path,
                         lock_path: lock_path.clone(),
                         editable: *editable,
                     };
@@ -306,7 +306,7 @@ impl<'a> Planner<'a> {
                     lock_path,
                 } => {
                     // Store the canonicalized path, which also serves to validate that it exists.
-                    let path = match install_path.canonicalize() {
+                    let install_path = match install_path.canonicalize() {
                         Ok(path) => path,
                         Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
                             return Err(Error::NotFound(url.to_url()).into());
@@ -330,7 +330,7 @@ impl<'a> Planner<'a> {
                             let wheel = PathBuiltDist {
                                 filename,
                                 url: url.clone(),
-                                install_path: install_path.clone(),
+                                install_path,
                                 lock_path: lock_path.clone(),
                             };
 
@@ -382,7 +382,7 @@ impl<'a> Planner<'a> {
                             let sdist = PathSourceDist {
                                 name: requirement.name.clone(),
                                 url: url.clone(),
-                                install_path: path,
+                                install_path,
                                 lock_path: lock_path.clone(),
                                 ext: *ext,
                             };

diff --git a/crates/uv/tests/sync.rs b/crates/uv/tests/sync.rs
@@ -682,12 +682,7 @@ fn sync_build_isolation_package() -> Result<()> {
 /// Test that relative wheel paths are correctly preserved.
 #[test]
 fn sync_relative_wheel() -> Result<()> {
-    // TODO(charlie): On Windows, this test currently prepares two packages (vs. one of Unix). This
-    // is due to an error in path canonicalization, and GitHub Actions on Windows have a symlink
-    // in the path.
-    //
-    // See: https://github.com/astral-sh/uv/issues/5979
-    let context = TestContext::new("3.12").with_filtered_counts();
+    let context = TestContext::new("3.12");
 
     let requirements = r#"[project]
     name = "relative_wheel"
@@ -727,9 +722,9 @@ fn sync_relative_wheel() -> Result<()> {
     ----- stderr -----
     warning: `uv sync` is experimental and may change without warning
     warning: `uv.sources` is experimental and may change without warning
-    Resolved [N] packages in [TIME]
-    Prepared [N] packages in [TIME]
-    Installed [N] packages in [TIME]
+    Resolved 2 packages in [TIME]
+    Prepared 1 package in [TIME]
+    Installed 2 packages in [TIME]
      + ok==1.0.0 (from file://[TEMP_DIR]/wheels/ok-1.0.0-py3-none-any.whl)
      + relative-wheel==0.1.0 (from file://[TEMP_DIR]/)
     "###);
@@ -778,8 +773,8 @@ fn sync_relative_wheel() -> Result<()> {
     ----- stderr -----
     warning: `uv sync` is experimental and may change without warning
     warning: `uv.sources` is experimental and may change without warning
-    Resolved [N] packages in [TIME]
-    Audited [N] packages in [TIME]
+    Resolved 2 packages in [TIME]
+    Audited 2 packages in [TIME]
     "###);
 
     Ok(())