Skip to content

Commit

Permalink
Don't canonicalize paths to user requirements (#6560)
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh authored Aug 24, 2024
1 parent 44e36a7 commit 611a900
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 59 deletions.
43 changes: 24 additions & 19 deletions crates/distribution-types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ use distribution_filename::{DistExtension, SourceDistExtension, WheelFilename};
use pep440_rs::Version;
use pep508_rs::{Pep508Url, VerbatimUrl};
use pypi_types::{ParsedUrl, VerbatimParsedUrl};
use uv_fs::normalize_absolute_path;
use uv_git::GitUrl;
use uv_normalize::PackageName;

Expand Down Expand Up @@ -234,7 +235,7 @@ pub struct DirectUrlBuiltDist {
#[derive(Debug, Clone, Hash)]
pub struct PathBuiltDist {
pub filename: WheelFilename,
/// The absolute, canonicalized path to the wheel which we use for installing.
/// The absolute 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
Expand Down Expand Up @@ -295,7 +296,7 @@ pub struct GitSourceDist {
#[derive(Debug, Clone, Hash)]
pub struct PathSourceDist {
pub name: PackageName,
/// The absolute, canonicalized path to the distribution which we use for installing.
/// The absolute 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
Expand All @@ -311,7 +312,7 @@ pub struct PathSourceDist {
#[derive(Debug, Clone, Hash)]
pub struct DirectorySourceDist {
pub name: PackageName,
/// The absolute, canonicalized path to the distribution which we use for installing.
/// The absolute 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
Expand Down Expand Up @@ -371,14 +372,16 @@ impl Dist {
lock_path: &Path,
ext: DistExtension,
) -> Result<Dist, Error> {
// Store the canonicalized path, which also serves to validate that it exists.
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()));
}
Err(err) => return Err(err.into()),
};
// Convert to an absolute path.
let install_path = std::path::absolute(install_path)?;

// Normalize the path.
let install_path = normalize_absolute_path(&install_path)?;

// Validate that the path exists.
if !install_path.exists() {
return Err(Error::NotFound(url.to_url()));
}

// Determine whether the path represents a built or source distribution.
match ext {
Expand Down Expand Up @@ -417,14 +420,16 @@ impl Dist {
lock_path: &Path,
editable: bool,
) -> Result<Dist, Error> {
// Store the canonicalized path, which also serves to validate that it exists.
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()));
}
Err(err) => return Err(err.into()),
};
// Convert to an absolute path.
let install_path = std::path::absolute(install_path)?;

// Normalize the path.
let install_path = normalize_absolute_path(&install_path)?;

// Validate that the path exists.
if !install_path.exists() {
return Err(Error::NotFound(url.to_url()));
}

// Determine whether the path represents an archive or a directory.
Ok(Self::Source(SourceDist::Directory(DirectorySourceDist {
Expand Down
28 changes: 9 additions & 19 deletions crates/pep508-rs/src/verbatim_url.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use std::sync::LazyLock;
use thiserror::Error;
use url::{ParseError, Url};

use uv_fs::{normalize_absolute_path, normalize_url_path, Simplified};
use uv_fs::{normalize_absolute_path, normalize_url_path};

use crate::Pep508Url;

Expand Down Expand Up @@ -46,12 +46,9 @@ impl VerbatimUrl {
pub fn from_path(path: impl AsRef<Path>) -> Result<Self, VerbatimUrlError> {
let path = path.as_ref();

// 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))?,
};
// Normalize the path.
let path = 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);
Expand Down Expand Up @@ -90,12 +87,8 @@ impl VerbatimUrl {
base_dir.as_ref().join(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(|err| VerbatimUrlError::Normalization(path.clone(), err))?,
};
let path = normalize_absolute_path(&path)
.map_err(|err| VerbatimUrlError::Normalization(path.clone(), err))?;

// Extract the fragment, if it exists.
let (path, fragment) = split_fragment(&path);
Expand Down Expand Up @@ -123,12 +116,9 @@ impl VerbatimUrl {
return Err(VerbatimUrlError::WorkingDirectory(path.to_path_buf()));
};

// 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))?,
};
// Normalize the path.
let path = normalize_absolute_path(&path)
.map_err(|err| VerbatimUrlError::Normalization(path.clone(), err))?;

// Extract the fragment, if it exists.
let (path, fragment) = split_fragment(&path);
Expand Down
4 changes: 2 additions & 2 deletions crates/pypi-types/src/parsed_url.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ impl ParsedUrl {
#[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)]
pub struct ParsedPathUrl {
pub url: Url,
/// The absolute, canonicalized path to the distribution which we use for installing.
/// The absolute 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
Expand Down Expand Up @@ -219,7 +219,7 @@ impl ParsedPathUrl {
#[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)]
pub struct ParsedDirectoryUrl {
pub url: Url,
/// The absolute, canonicalized path to the distribution which we use for installing.
/// The absolute 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
Expand Down
4 changes: 2 additions & 2 deletions crates/pypi-types/src/requirement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ pub enum RequirementSource {
/// be a binary distribution (a `.whl` file) or a source distribution archive (a `.zip` or
/// `.tar.gz` file).
Path {
/// The absolute, canonicalized path to the distribution which we use for installing.
/// The absolute 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
Expand All @@ -355,7 +355,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 absolute, canonicalized path to the distribution which we use for installing.
/// The absolute 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
Expand Down
38 changes: 21 additions & 17 deletions crates/uv-installer/src/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use uv_configuration::{BuildOptions, Reinstall};
use uv_distribution::{
BuiltWheelIndex, HttpArchivePointer, LocalArchivePointer, RegistryWheelIndex,
};
use uv_fs::Simplified;
use uv_fs::{normalize_absolute_path, Simplified};
use uv_git::GitUrl;
use uv_python::PythonEnvironment;
use uv_types::HashStrategy;
Expand Down Expand Up @@ -268,14 +268,16 @@ impl<'a> Planner<'a> {
install_path,
lock_path,
} => {
// Store the canonicalized path, which also serves to validate that it exists.
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());
}
Err(err) => return Err(err.into()),
};
// Convert to an absolute path.
let install_path = std::path::absolute(install_path)?;

// Normalize the path.
let install_path = normalize_absolute_path(&install_path)?;

// Validate that the path exists.
if !install_path.exists() {
return Err(Error::NotFound(url.to_url()).into());
}

let sdist = DirectorySourceDist {
name: requirement.name.clone(),
Expand Down Expand Up @@ -305,14 +307,16 @@ impl<'a> Planner<'a> {
install_path,
lock_path,
} => {
// Store the canonicalized path, which also serves to validate that it exists.
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());
}
Err(err) => return Err(err.into()),
};
// Convert to an absolute path.
let install_path = std::path::absolute(install_path)?;

// Normalize the path.
let install_path = normalize_absolute_path(&install_path)?;

// Validate that the path exists.
if !install_path.exists() {
return Err(Error::NotFound(url.to_url()).into());
}

match ext {
DistExtension::Wheel => {
Expand Down

0 comments on commit 611a900

Please sign in to comment.