Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propagate URL errors in verbatim parsing #3720

Merged
merged 1 commit into from
May 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions crates/pep508-rs/src/unnamed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,12 @@ fn preprocess_unnamed_url(
#[cfg(feature = "non-pep508-extensions")]
if let Some(working_dir) = working_dir {
let url = VerbatimUrl::parse_path(path.as_ref(), working_dir)
.map_err(|err| Pep508Error {
message: Pep508ErrorSource::<VerbatimUrl>::UrlError(err),
start,
len,
input: cursor.to_string(),
})?
.with_given(url.to_string());
return Ok((url, extras));
}
Expand Down Expand Up @@ -270,6 +276,12 @@ fn preprocess_unnamed_url(
_ => {
if let Some(working_dir) = working_dir {
let url = VerbatimUrl::parse_path(expanded.as_ref(), working_dir)
.map_err(|err| Pep508Error {
message: Pep508ErrorSource::<VerbatimUrl>::UrlError(err),
start,
len,
input: cursor.to_string(),
})?
.with_given(url.to_string());
return Ok((url, extras));
}
Expand All @@ -288,8 +300,14 @@ fn preprocess_unnamed_url(
} else {
// Ex) `../editable/`
if let Some(working_dir) = working_dir {
let url =
VerbatimUrl::parse_path(expanded.as_ref(), working_dir).with_given(url.to_string());
let url = VerbatimUrl::parse_path(expanded.as_ref(), working_dir)
.map_err(|err| Pep508Error {
message: Pep508ErrorSource::<VerbatimUrl>::UrlError(err),
start,
len,
input: cursor.to_string(),
})?
.with_given(url.to_string());
return Ok((url, extras));
}

Expand Down
48 changes: 28 additions & 20 deletions crates/pep508-rs/src/verbatim_url.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,25 +38,26 @@ impl VerbatimUrl {
/// Create a [`VerbatimUrl`] from a file path.
///
/// Assumes that the path is absolute.
pub fn from_path(path: impl AsRef<Path>) -> Self {
pub fn from_path(path: impl AsRef<Path>) -> Result<Self, VerbatimUrlError> {
let path = path.as_ref();

// Normalize the path.
let path = normalize_path(path).expect("path is absolute");
let path = normalize_path(path)
.map_err(|err| VerbatimUrlError::Normalization(path.to_path_buf(), err))?;

// Extract the fragment, if it exists.
let (path, fragment) = split_fragment(&path);

// Convert to a URL.
let mut url = Url::from_file_path(path.clone())
.unwrap_or_else(|_| panic!("path is absolute: {}", path.display()));
.map_err(|_| VerbatimUrlError::UrlConversion(path.to_path_buf()))?;

// Set the fragment, if it exists.
if let Some(fragment) = fragment {
url.set_fragment(Some(fragment));
}

Self { url, given: None }
Ok(Self { url, given: None })
}

/// Parse a URL from a string, expanding any environment variables.
Expand All @@ -67,7 +68,10 @@ impl VerbatimUrl {

/// Parse a URL from an absolute or relative path.
#[cfg(feature = "non-pep508-extensions")] // PEP 508 arguably only allows absolute file URLs.
pub fn parse_path(path: impl AsRef<Path>, working_dir: impl AsRef<Path>) -> Self {
pub fn parse_path(
path: impl AsRef<Path>,
working_dir: impl AsRef<Path>,
) -> Result<Self, VerbatimUrlError> {
let path = path.as_ref();

// Convert the path to an absolute path, if necessary.
Expand All @@ -78,26 +82,22 @@ impl VerbatimUrl {
};

// Normalize the path.
let path = normalize_path(&path).expect("path is absolute");
let path = normalize_path(&path)
.map_err(|err| VerbatimUrlError::Normalization(path.to_path_buf(), err))?;

// Extract the fragment, if it exists.
let (path, fragment) = split_fragment(&path);

// Convert to a URL.
let mut url = Url::from_file_path(path.clone()).unwrap_or_else(|_| {
panic!(
"path is absolute: {}, {}",
path.display(),
working_dir.as_ref().display()
)
});
let mut url = Url::from_file_path(path.clone())
.map_err(|_| VerbatimUrlError::UrlConversion(path.to_path_buf()))?;

// Set the fragment, if it exists.
if let Some(fragment) = fragment {
url.set_fragment(Some(fragment));
}

Self { url, given: None }
Ok(Self { url, given: None })
}

/// Parse a URL from an absolute path.
Expand All @@ -108,12 +108,12 @@ impl VerbatimUrl {
let path = if path.is_absolute() {
path.to_path_buf()
} else {
return Err(VerbatimUrlError::RelativePath(path.to_path_buf()));
return Err(VerbatimUrlError::WorkingDirectory(path.to_path_buf()));
};

// Normalize the path.
let Ok(path) = normalize_path(&path) else {
return Err(VerbatimUrlError::RelativePath(path));
return Err(VerbatimUrlError::WorkingDirectory(path));
};

// Extract the fragment, if it exists.
Expand Down Expand Up @@ -226,7 +226,7 @@ impl Pep508Url for VerbatimUrl {

#[cfg(feature = "non-pep508-extensions")]
if let Some(working_dir) = working_dir {
return Ok(VerbatimUrl::parse_path(path.as_ref(), working_dir)
return Ok(VerbatimUrl::parse_path(path.as_ref(), working_dir)?
.with_given(url.to_string()));
}

Expand All @@ -245,7 +245,7 @@ impl Pep508Url for VerbatimUrl {
_ => {
#[cfg(feature = "non-pep508-extensions")]
if let Some(working_dir) = working_dir {
return Ok(VerbatimUrl::parse_path(expanded.as_ref(), working_dir)
return Ok(VerbatimUrl::parse_path(expanded.as_ref(), working_dir)?
.with_given(url.to_string()));
}

Expand All @@ -257,7 +257,7 @@ impl Pep508Url for VerbatimUrl {
// Ex) `../editable/`
#[cfg(feature = "non-pep508-extensions")]
if let Some(working_dir) = working_dir {
return Ok(VerbatimUrl::parse_path(expanded.as_ref(), working_dir)
return Ok(VerbatimUrl::parse_path(expanded.as_ref(), working_dir)?
.with_given(url.to_string()));
}

Expand All @@ -275,7 +275,15 @@ pub enum VerbatimUrlError {

/// Received a relative path, but no working directory was provided.
#[error("relative path without a working directory: {0}")]
RelativePath(PathBuf),
WorkingDirectory(PathBuf),

/// Received a path that could not be converted to a URL.
#[error("path could not be converted to a URL: {0}")]
UrlConversion(PathBuf),

/// Received a path that could not be normalized.
#[error("path could not be normalized: {0}")]
Normalization(PathBuf, #[source] std::io::Error),
}

/// Expand all available environment variables.
Expand Down
35 changes: 26 additions & 9 deletions crates/requirements-txt/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -297,10 +297,15 @@ impl EditableRequirement {
VerbatimUrl::parse_path(expanded.as_ref(), working_dir.as_ref())
};

let url = url.map_err(|err| RequirementsTxtParserError::VerbatimUrl {
source: err,
url: expanded.to_string(),
})?;

// Create a `PathBuf`.
let path = url
.to_file_path()
.map_err(|()| RequirementsTxtParserError::InvalidEditablePath(expanded.to_string()))?;
.map_err(|()| RequirementsTxtParserError::UrlConversion(expanded.to_string()))?;

// Add the verbatim representation of the URL to the `VerbatimUrl`.
let url = url.with_given(requirement.to_string());
Expand Down Expand Up @@ -1034,7 +1039,11 @@ pub enum RequirementsTxtParserError {
start: usize,
end: usize,
},
InvalidEditablePath(String),
VerbatimUrl {
source: pep508_rs::VerbatimUrlError,
url: String,
},
UrlConversion(String),
UnsupportedUrl(String),
MissingRequirementPrefix(String),
NoBinary {
Expand Down Expand Up @@ -1091,7 +1100,7 @@ impl RequirementsTxtParserError {
fn with_offset(self, offset: usize) -> Self {
match self {
Self::IO(err) => Self::IO(err),
Self::InvalidEditablePath(given) => Self::InvalidEditablePath(given),
Self::UrlConversion(given) => Self::UrlConversion(given),
Self::Url {
source,
url,
Expand All @@ -1103,6 +1112,7 @@ impl RequirementsTxtParserError {
start: start + offset,
end: end + offset,
},
Self::VerbatimUrl { source, url } => Self::VerbatimUrl { source, url },
Self::UnsupportedUrl(url) => Self::UnsupportedUrl(url),
Self::MissingRequirementPrefix(given) => Self::MissingRequirementPrefix(given),
Self::NoBinary {
Expand Down Expand Up @@ -1171,12 +1181,15 @@ impl Display for RequirementsTxtParserError {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
match self {
Self::IO(err) => err.fmt(f),
Self::InvalidEditablePath(given) => {
write!(f, "Invalid editable path: {given}")
}
Self::Url { url, start, .. } => {
write!(f, "Invalid URL at position {start}: `{url}`")
}
Self::VerbatimUrl { source, url } => {
write!(f, "Invalid URL: `{url}`: {source}")
}
Self::UrlConversion(given) => {
write!(f, "Unable to convert URL to path: {given}")
}
Self::UnsupportedUrl(url) => {
write!(f, "Unsupported URL (expected a `file://` scheme): `{url}`")
}
Expand Down Expand Up @@ -1231,7 +1244,8 @@ impl std::error::Error for RequirementsTxtParserError {
match &self {
Self::IO(err) => err.source(),
Self::Url { source, .. } => Some(source),
Self::InvalidEditablePath(_) => None,
Self::VerbatimUrl { source, .. } => Some(source),
Self::UrlConversion(_) => None,
Self::UnsupportedUrl(_) => None,
Self::MissingRequirementPrefix(_) => None,
Self::NoBinary { source, .. } => Some(source),
Expand Down Expand Up @@ -1260,10 +1274,13 @@ impl Display for RequirementsTxtFileError {
self.file.user_display(),
)
}
RequirementsTxtParserError::InvalidEditablePath(given) => {
RequirementsTxtParserError::VerbatimUrl { url, .. } => {
write!(f, "Invalid URL in `{}`: `{url}`", self.file.user_display())
}
RequirementsTxtParserError::UrlConversion(given) => {
write!(
f,
"Invalid editable path in `{}`: {given}",
"Unable to convert URL to path `{}`: {given}",
self.file.user_display()
)
}
Expand Down
14 changes: 11 additions & 3 deletions crates/uv-client/src/flat_index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,20 @@ use crate::{Connectivity, Error, ErrorKind, RegistryClient};
#[derive(Debug, thiserror::Error)]
pub enum FlatIndexError {
#[error("Failed to read `--find-links` directory: {0}")]
FindLinksDirectory(PathBuf, #[source] std::io::Error),
FindLinksDirectory(PathBuf, #[source] FindLinksDirectoryError),

#[error("Failed to read `--find-links` URL: {0}")]
FindLinksUrl(Url, #[source] Error),
}

#[derive(Debug, thiserror::Error)]
pub enum FindLinksDirectoryError {
#[error(transparent)]
Io(#[from] std::io::Error),
#[error(transparent)]
VerbatimUrl(#[from] pep508_rs::VerbatimUrlError),
}

#[derive(Debug, Default, Clone)]
pub struct FlatIndexEntries {
/// The list of `--find-links` entries.
Expand Down Expand Up @@ -202,10 +210,10 @@ impl<'a> FlatIndexClient<'a> {
}

/// Read a flat remote index from a `--find-links` directory.
fn read_from_directory(path: &PathBuf) -> Result<FlatIndexEntries, std::io::Error> {
fn read_from_directory(path: &PathBuf) -> Result<FlatIndexEntries, FindLinksDirectoryError> {
// Absolute paths are required for the URL conversion.
let path = fs_err::canonicalize(path)?;
let index_url = IndexUrl::Path(VerbatimUrl::from_path(&path));
let index_url = IndexUrl::Path(VerbatimUrl::from_path(&path)?);

let mut dists = Vec::new();
for entry in fs_err::read_dir(path)? {
Expand Down
2 changes: 2 additions & 0 deletions crates/uv-distribution/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ pub enum Error {
// Network error
#[error("Failed to parse URL: {0}")]
Url(String, #[source] url::ParseError),
#[error("Expected an absolute path, but received: {}", _0.user_display())]
RelativePath(PathBuf),
#[error(transparent)]
JoinRelativeUrl(#[from] pypi_types::JoinRelativeError),
#[error("Git operation failed")]
Expand Down
4 changes: 2 additions & 2 deletions crates/uv-distribution/src/index/registry_wheel_index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,10 @@ impl<'a> RegistryWheelIndex<'a> {
.filter_map(|flat_index| match flat_index {
FlatIndexLocation::Path(path) => {
let path = fs_err::canonicalize(path).ok()?;
Some(IndexUrl::Path(VerbatimUrl::from_path(path)))
Some(IndexUrl::Path(VerbatimUrl::from_path(path).ok()?))
}
FlatIndexLocation::Url(url) => {
Some(IndexUrl::Url(VerbatimUrl::unknown(url.clone())))
Some(IndexUrl::Url(VerbatimUrl::from_url(url.clone())))
}
})
.collect();
Expand Down
6 changes: 4 additions & 2 deletions crates/uv-distribution/src/source/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
Url::parse(url).map_err(|err| Error::Url(url.clone(), err))?
}
FileLocation::Path(path) => {
let url = Url::from_file_path(path).expect("path is absolute");
let url = Url::from_file_path(path)
.map_err(|()| Error::RelativePath(path.clone()))?;
return self
.archive(
source,
Expand Down Expand Up @@ -262,7 +263,8 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
Url::parse(url).map_err(|err| Error::Url(url.clone(), err))?
}
FileLocation::Path(path) => {
let url = Url::from_file_path(path).expect("path is absolute");
let url = Url::from_file_path(path)
.map_err(|()| Error::RelativePath(path.clone()))?;
return self
.archive_metadata(
source,
Expand Down
4 changes: 3 additions & 1 deletion crates/uv-requirements/src/pyproject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ pub enum LoweringError {
InvalidEntry,
#[error(transparent)]
InvalidUrl(#[from] url::ParseError),
#[error(transparent)]
InvalidVerbatimUrl(#[from] pep508_rs::VerbatimUrlError),
#[error("Can't combine URLs from both `project.dependencies` and `tool.uv.sources`")]
ConflictingUrls,
#[error("Could not normalize path: `{0}`")]
Expand Down Expand Up @@ -551,7 +553,7 @@ fn path_source(
project_dir: &Path,
editable: bool,
) -> Result<RequirementSource, LoweringError> {
let url = VerbatimUrl::parse_path(&path, project_dir).with_given(path.clone());
let url = VerbatimUrl::parse_path(&path, project_dir)?.with_given(path.clone());
let path_buf = PathBuf::from(&path);
let path_buf = path_buf
.absolutize_from(project_dir)
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-requirements/src/specification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ impl RequirementsSpecification {
project: None,
requirements: vec![UnresolvedRequirementSpecification {
requirement: UnresolvedRequirement::Unnamed(UnnamedRequirement {
url: VerbatimUrl::from_path(path),
url: VerbatimUrl::from_path(path)?,
extras: vec![],
marker: None,
origin: None,
Expand Down
Loading