Python
/
uv
mirror of https://github.com/astral-sh/uv
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Charlie Marsh 2024-08-09 21:43:36 -04:00 committed by GitHub
parent cd0171a2ed
commit 19ac9af167
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 41 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
crates/distribution-types/src/lib.rs
View File

@ -216,7 +216,7 @@ pub struct DirectUrlBuiltDist {
#[derive(Debug, Clone, Hash)] #[derive(Debug, Clone, Hash)]
pub struct PathBuiltDist { pub struct PathBuiltDist {
pub filename: WheelFilename, 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, pub install_path: PathBuf,
/// The absolute path or path relative to the workspace root pointing to the wheel /// 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 /// 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)] #[derive(Debug, Clone, Hash)]
pub struct PathSourceDist { pub struct PathSourceDist {
pub name: PackageName, 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, pub install_path: PathBuf,
/// The absolute path or path relative to the workspace root pointing to the distribution /// 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 /// 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)] #[derive(Debug, Clone, Hash)]
pub struct DirectorySourceDist { pub struct DirectorySourceDist {
pub name: PackageName, 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, pub install_path: PathBuf,
/// The absolute path or path relative to the workspace root pointing to the distribution /// 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 /// which we use for locking. Unlike `given` on the verbatim URL all environment variables
@ -354,7 +354,7 @@ impl Dist {
ext: DistExtension, ext: DistExtension,
) -> Result<Dist, Error> { ) -> Result<Dist, Error> {
// Store the canonicalized path, which also serves to validate that it exists. // 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, Ok(path) => path,
Err(err) if err.kind() == std::io::ErrorKind::NotFound => { Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
return Err(Error::NotFound(url.to_url())); return Err(Error::NotFound(url.to_url()));
@ -376,14 +376,14 @@ impl Dist {
} }
Ok(Self::Built(BuiltDist::Path(PathBuiltDist { Ok(Self::Built(BuiltDist::Path(PathBuiltDist {
filename, filename,
install_path: canonicalized_path.clone(), install_path,
lock_path: lock_path.to_path_buf(), lock_path: lock_path.to_path_buf(),
url, url,
}))) })))
} }
DistExtension::Source(ext) => Ok(Self::Source(SourceDist::Path(PathSourceDist { DistExtension::Source(ext) => Ok(Self::Source(SourceDist::Path(PathSourceDist {
name, name,
install_path: canonicalized_path.clone(), install_path,
lock_path: lock_path.to_path_buf(), lock_path: lock_path.to_path_buf(),
ext, ext,
url, url,
@ -400,7 +400,7 @@ impl Dist {
editable: bool, editable: bool,
) -> Result<Dist, Error> { ) -> Result<Dist, Error> {
// Store the canonicalized path, which also serves to validate that it exists. // 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, Ok(path) => path,
Err(err) if err.kind() == std::io::ErrorKind::NotFound => { Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
return Err(Error::NotFound(url.to_url())); return Err(Error::NotFound(url.to_url()));
@ -411,7 +411,7 @@ impl Dist {
// Determine whether the path represents an archive or a directory. // Determine whether the path represents an archive or a directory.
Ok(Self::Source(SourceDist::Directory(DirectorySourceDist { Ok(Self::Source(SourceDist::Directory(DirectorySourceDist {
name, name,
install_path: canonicalized_path.clone(), install_path,
lock_path: lock_path.to_path_buf(), lock_path: lock_path.to_path_buf(),
editable, editable,
url, url,

28
crates/pep508-rs/src/verbatim_url.rs
View File

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

4
crates/pypi-types/src/parsed_url.rs
View File

@ -185,7 +185,7 @@ impl ParsedUrl {
#[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)] #[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)]
pub struct ParsedPathUrl { pub struct ParsedPathUrl {
pub url: Url, 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, pub install_path: PathBuf,
/// The absolute path or path relative to the workspace root pointing to the distribution /// 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 /// 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)] #[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)]
pub struct ParsedDirectoryUrl { pub struct ParsedDirectoryUrl {
pub url: Url, 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, pub install_path: PathBuf,
/// The absolute path or path relative to the workspace root pointing to the distribution /// 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 /// which we use for locking. Unlike `given` on the verbatim URL all environment variables

4
crates/pypi-types/src/requirement.rs
View File

@ -313,7 +313,7 @@ pub enum RequirementSource {
/// be a binary distribution (a `.whl` file) or a source distribution archive (a `.zip` or /// be a binary distribution (a `.whl` file) or a source distribution archive (a `.zip` or
/// `.tar.gz` file). /// `.tar.gz` file).
Path { 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, install_path: PathBuf,
/// The absolute path or path relative to the workspace root pointing to the distribution /// 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 /// 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 /// 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). /// source distribution with only a setup.py but non pyproject.toml in it).
Directory { 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, install_path: PathBuf,
/// The absolute path or path relative to the workspace root pointing to the distribution /// 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 /// which we use for locking. Unlike `given` on the verbatim URL all environment variables

10
crates/uv-installer/src/plan.rs
View File

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

17
crates/uv/tests/sync.rs
View File

@ -682,12 +682,7 @@ fn sync_build_isolation_package() -> Result<()> {
/// Test that relative wheel paths are correctly preserved. /// Test that relative wheel paths are correctly preserved.
#[test] #[test]
fn sync_relative_wheel() -> Result<()> { fn sync_relative_wheel() -> Result<()> {
// TODO(charlie): On Windows, this test currently prepares two packages (vs. one of Unix). This let context = TestContext::new("3.12");
// 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 requirements = r#"[project] let requirements = r#"[project]
name = "relative_wheel" name = "relative_wheel"
@ -727,9 +722,9 @@ fn sync_relative_wheel() -> Result<()> {
----- stderr ----- ----- stderr -----
warning: `uv sync` is experimental and may change without warning warning: `uv sync` is experimental and may change without warning
warning: `uv.sources` is experimental and may change without warning warning: `uv.sources` is experimental and may change without warning
Resolved [N] packages in [TIME] Resolved 2 packages in [TIME]
Prepared [N] packages in [TIME] Prepared 1 package in [TIME]
Installed [N] packages in [TIME] Installed 2 packages in [TIME]
+ ok==1.0.0 (from file://[TEMP_DIR]/wheels/ok-1.0.0-py3-none-any.whl) + 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]/) + relative-wheel==0.1.0 (from file://[TEMP_DIR]/)
"###); "###);
@ -778,8 +773,8 @@ fn sync_relative_wheel() -> Result<()> {
----- stderr ----- ----- stderr -----
warning: `uv sync` is experimental and may change without warning warning: `uv sync` is experimental and may change without warning
warning: `uv.sources` is experimental and may change without warning warning: `uv.sources` is experimental and may change without warning
Resolved [N] packages in [TIME] Resolved 2 packages in [TIME]
Audited [N] packages in [TIME] Audited 2 packages in [TIME]
"###); "###);
Ok(()) Ok(())