From 19ac9af167a2964fdc9c9c32fda5095a48ec6d4d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 9 Aug 2024 21:43:36 -0400 Subject: [PATCH] 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. --- crates/distribution-types/src/lib.rs | 16 ++++++++-------- crates/pep508-rs/src/verbatim_url.rs | 28 ++++++++++++++++++---------- crates/pypi-types/src/parsed_url.rs | 4 ++-- crates/pypi-types/src/requirement.rs | 4 ++-- crates/uv-installer/src/plan.rs | 10 +++++----- crates/uv/tests/sync.rs | 17 ++++++----------- 6 files changed, 41 insertions(+), 38 deletions(-) diff --git a/crates/distribution-types/src/lib.rs b/crates/distribution-types/src/lib.rs index 2c62947ad57f..476cd6ad7a80 100644 --- 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 { // 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 { // 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 index 5975194c3fdb..31d794ebaa37 100644 --- 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) -> Result { 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 index 091575065895..f39968ede1ba 100644 --- 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 index dc7003fe053f..5d994b3d3787 100644 --- 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 index ab2f6b8cfc1e..d8b823d8dee5 100644 --- 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 index 06fe5272232b..6ea879b39f79 100644 --- 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(())