Skip to content

Commit

Permalink
Use separate path types for directories and files (#4285)
Browse files Browse the repository at this point in the history
## Summary

This is what I consider to be the "real" fix for #8072. We now treat
directory and path URLs as separate `ParsedUrl` types and
`RequirementSource` types. This removes a lot of `.is_dir()` forking
within the `ParsedUrl::Path` arms and makes some states impossible
(e.g., you can't have a `.whl` path that is editable). It _also_ fixes
the `direct_url.json` for direct URLs that refer to files. Previously,
we wrote out to these as if they were installed as directories, which is
just wrong.
  • Loading branch information
charliermarsh committed Jun 12, 2024
1 parent c448301 commit d8f1de6
Show file tree
Hide file tree
Showing 28 changed files with 523 additions and 166 deletions.
4 changes: 2 additions & 2 deletions crates/distribution-types/src/cached.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use anyhow::{anyhow, Result};

use distribution_filename::WheelFilename;
use pep508_rs::VerbatimUrl;
use pypi_types::{HashDigest, ParsedPathUrl};
use pypi_types::{HashDigest, ParsedDirectoryUrl};
use uv_normalize::PackageName;

use crate::{
Expand Down Expand Up @@ -120,7 +120,7 @@ impl CachedDist {
.url
.to_file_path()
.map_err(|()| anyhow!("Invalid path in file URL"))?;
Ok(Some(ParsedUrl::Path(ParsedPathUrl {
Ok(Some(ParsedUrl::Directory(ParsedDirectoryUrl {
url: dist.url.raw().clone(),
install_path: path.clone(),
lock_path: path,
Expand Down
10 changes: 0 additions & 10 deletions crates/distribution-types/src/error.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use url::Url;

use pep508_rs::VerbatimUrl;
use uv_normalize::PackageName;

#[derive(thiserror::Error, Debug)]
Expand All @@ -14,21 +13,12 @@ pub enum Error {
#[error(transparent)]
WheelFilename(#[from] distribution_filename::WheelFilenameError),

#[error("Unable to extract file path from URL: {0}")]
MissingFilePath(Url),

#[error("Could not extract path segments from URL: {0}")]
MissingPathSegments(Url),

#[error("Distribution not found at: {0}")]
NotFound(Url),

#[error("Unsupported scheme `{0}` on URL: {1} ({2})")]
UnsupportedScheme(String, String, String),

#[error("Requested package name `{0}` does not match `{1}` in the distribution filename: {2}")]
PackageNameMismatch(PackageName, PackageName, String),

#[error("Only directories can be installed as editable, not filenames: `{0}`")]
EditableFile(VerbatimUrl),
}
62 changes: 37 additions & 25 deletions crates/distribution-types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,6 @@ impl Dist {
url: VerbatimUrl,
install_path: &Path,
lock_path: &Path,
editable: bool,
) -> Result<Dist, Error> {
// Store the canonicalized path, which also serves to validate that it exists.
let canonicalized_path = match install_path.canonicalize() {
Expand All @@ -340,16 +339,8 @@ impl Dist {
Err(err) => return Err(err.into()),
};

// Determine whether the path represents an archive or a directory.
if canonicalized_path.is_dir() {
Ok(Self::Source(SourceDist::Directory(DirectorySourceDist {
name,
install_path: canonicalized_path.clone(),
lock_path: lock_path.to_path_buf(),
editable,
url,
})))
} else if canonicalized_path
// Determine whether the path represents a built or source distribution.
if canonicalized_path
.extension()
.is_some_and(|ext| ext.eq_ignore_ascii_case("whl"))
{
Expand All @@ -362,30 +353,48 @@ impl Dist {
url.verbatim().to_string(),
));
}

if editable {
return Err(Error::EditableFile(url));
}

Ok(Self::Built(BuiltDist::Path(PathBuiltDist {
filename,
path: canonicalized_path,
url,
})))
} else {
if editable {
return Err(Error::EditableFile(url));
}

Ok(Self::Source(SourceDist::Path(PathSourceDist {
name,
install_path: canonicalized_path.clone(),
lock_path: canonicalized_path,
lock_path: lock_path.to_path_buf(),
url,
})))
}
}

/// A local source tree from a `file://` URL.
pub fn from_directory_url(
name: PackageName,
url: VerbatimUrl,
install_path: &Path,
lock_path: &Path,
editable: bool,
) -> Result<Dist, Error> {
// Store the canonicalized path, which also serves to validate that it exists.
let canonicalized_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()),
};

// Determine whether the path represents an archive or a directory.
Ok(Self::Source(SourceDist::Directory(DirectorySourceDist {
name,
install_path: canonicalized_path.clone(),
lock_path: lock_path.to_path_buf(),
editable,
url,
})))
}

/// A remote source distribution from a `git+https://` or `git+ssh://` url.
pub fn from_git_url(
name: PackageName,
Expand All @@ -407,12 +416,15 @@ impl Dist {
ParsedUrl::Archive(archive) => {
Self::from_http_url(name, url.verbatim, archive.url, archive.subdirectory)
}
ParsedUrl::Path(file) => Self::from_file_url(
ParsedUrl::Path(file) => {
Self::from_file_url(name, url.verbatim, &file.install_path, &file.lock_path)
}
ParsedUrl::Directory(directory) => Self::from_directory_url(
name,
url.verbatim,
&file.install_path,
&file.lock_path,
file.editable,
&directory.install_path,
&directory.lock_path,
directory.editable,
),
ParsedUrl::Git(git) => {
Self::from_git_url(name, url.verbatim, git.url, git.subdirectory)
Expand Down
4 changes: 1 addition & 3 deletions crates/distribution-types/src/resolution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@ impl From<&ResolvedDist> for Requirement {
install_path: wheel.path.clone(),
lock_path: wheel.path.clone(),
url: wheel.url.clone(),
editable: false,
},
Dist::Source(SourceDist::Registry(sdist)) => RequirementSource::Registry {
specifier: pep440_rs::VersionSpecifiers::from(
Expand Down Expand Up @@ -172,9 +171,8 @@ impl From<&ResolvedDist> for Requirement {
install_path: sdist.install_path.clone(),
lock_path: sdist.lock_path.clone(),
url: sdist.url.clone(),
editable: false,
},
Dist::Source(SourceDist::Directory(sdist)) => RequirementSource::Path {
Dist::Source(SourceDist::Directory(sdist)) => RequirementSource::Directory {
install_path: sdist.install_path.clone(),
lock_path: sdist.lock_path.clone(),
url: sdist.url.clone(),
Expand Down
Loading

0 comments on commit d8f1de6

Please sign in to comment.