Skip to content

Commit

Permalink
Treat missing package name error as an unsupported requirement (#1025)
Browse files Browse the repository at this point in the history
## Summary

Based on user feedback. Calling it a "parse error" is misleading, since
this is really something we don't support, but that users can work
around.
  • Loading branch information
charliermarsh authored Jan 22, 2024
1 parent 4026710 commit 540442b
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 18 deletions.
11 changes: 6 additions & 5 deletions crates/pep508-rs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ pub enum Pep508ErrorSource {
/// A URL parsing error.
#[error(transparent)]
UrlError(#[from] verbatim_url::VerbatimUrlError),
/// The version requirement is not supported.
#[error("{0}")]
UnsupportedRequirement(String),
}

impl Display for Pep508Error {
Expand Down Expand Up @@ -842,11 +845,9 @@ fn parse(cursor: &mut Cursor) -> Result<Requirement, Pep508Error> {
// a package name. pip supports this in `requirements.txt`, but it doesn't adhere to
// the PEP 508 grammar.
let mut clone = cursor.clone().at(start);
return if let Ok(url) = parse_url(&mut clone) {
return if parse_url(&mut clone).is_ok() {
Err(Pep508Error {
message: Pep508ErrorSource::String(format!(
"URL requirement is missing a package name; expected: `package_name @ {url}`",
)),
message: Pep508ErrorSource::UnsupportedRequirement("URL requirement must be preceded by a package name. Add the name of the package before the URL (e.g., `package_name @ https://...`).".to_string()),
start,
len: clone.pos() - start,
input: clone.to_string(),
Expand Down Expand Up @@ -1305,7 +1306,7 @@ mod tests {
assert_err(
r#"git+https://github.com/pallets/flask.git"#,
indoc! {"
URL requirement is missing a package name; expected: `package_name @ git+https://github.com/pallets/flask.git`
URL requirement must be preceded by a package name. Add the name of the package before the URL (e.g., `package_name @ https://...`).
git+https://github.com/pallets/flask.git
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^"
},
Expand Down
12 changes: 7 additions & 5 deletions crates/pep508-rs/src/verbatim_url.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,20 @@ use std::path::{Component, Path, PathBuf};

use once_cell::sync::Lazy;
use regex::Regex;
use serde::{Deserialize, Serialize};
use url::Url;

/// A wrapper around [`Url`] that preserves the original string.
#[derive(Debug, Clone, Eq, derivative::Derivative)]
#[derivative(PartialEq, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
pub struct VerbatimUrl {
/// The parsed URL.
#[serde(
serialize_with = "Url::serialize_internal",
deserialize_with = "Url::deserialize_internal"
#[cfg_attr(
feature = "serde",
serde(
serialize_with = "Url::serialize_internal",
deserialize_with = "Url::deserialize_internal"
)
)]
url: Url,
/// The URL as it was provided by the user.
Expand Down
38 changes: 38 additions & 0 deletions crates/puffin/tests/pip_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3495,3 +3495,41 @@ fn missing_editable_requirement() -> Result<()> {

Ok(())
}

/// Attempt to resolve a URL requirement without a package name.
#[test]
fn missing_package_name() -> Result<()> {
let temp_dir = TempDir::new()?;
let cache_dir = TempDir::new()?;
let venv = create_venv_py312(&temp_dir, &cache_dir);

let requirements_in = temp_dir.child("requirements.in");
requirements_in.write_str("https://files.pythonhosted.org/packages/36/42/015c23096649b908c809c69388a805a571a3bea44362fe87e33fc3afa01f/flask-3.0.0-py3-none-any.whl")?;

insta::with_settings!({
filters => INSTA_FILTERS.to_vec()
}, {
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.arg("pip")
.arg("compile")
.arg("requirements.in")
.arg("--cache-dir")
.arg(cache_dir.path())
.arg("--exclude-newer")
.arg(EXCLUDE_NEWER)
.env("VIRTUAL_ENV", venv.as_os_str())
.current_dir(&temp_dir), @r###"
success: false
exit_code: 2
----- stdout -----
----- stderr -----
error: Unsupported requirement in requirements.in position 0 to 135
Caused by: URL requirement must be preceded by a package name. Add the name of the package before the URL (e.g., `package_name @ https://...`).
https://files.pythonhosted.org/packages/36/42/015c23096649b908c809c69388a805a571a3bea44362fe87e33fc3afa01f/flask-3.0.0-py3-none-any.whl
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"###);
});

Ok(())
}
47 changes: 39 additions & 8 deletions crates/requirements-txt/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ use tracing::warn;
use unscanny::{Pattern, Scanner};
use url::Url;

use pep508_rs::{Pep508Error, Requirement, VerbatimUrl};
use pep508_rs::{Pep508Error, Pep508ErrorSource, Requirement, VerbatimUrl};

/// We emit one of those for each requirements.txt entry
enum RequirementsTxtStatement {
Expand Down Expand Up @@ -468,13 +468,26 @@ fn parse_requirement_and_hashes(
break (end, false);
}
};
let requirement = Requirement::from_str(&content[start..end]).map_err(|err| {
RequirementsTxtParserError::Pep508 {
source: err,
start,
end,
}
})?;
let requirement =
Requirement::from_str(&content[start..end]).map_err(|err| match err.message {
Pep508ErrorSource::String(_) => RequirementsTxtParserError::Pep508 {
source: err,
start,
end,
},
Pep508ErrorSource::UrlError(_) => RequirementsTxtParserError::Pep508 {
source: err,
start,
end,
},
Pep508ErrorSource::UnsupportedRequirement(_) => {
RequirementsTxtParserError::UnsupportedRequirement {
source: err,
start,
end,
}
}
})?;
let hashes = if has_hashes {
let hashes = parse_hashes(s)?;
eat_trailing_line(s)?;
Expand Down Expand Up @@ -547,6 +560,11 @@ pub enum RequirementsTxtParserError {
message: String,
location: usize,
},
UnsupportedRequirement {
source: Pep508Error,
start: usize,
end: usize,
},
Pep508 {
source: Pep508Error,
start: usize,
Expand All @@ -572,6 +590,9 @@ impl Display for RequirementsTxtParserError {
RequirementsTxtParserError::Parser { message, location } => {
write!(f, "{message} at position {location}")
}
RequirementsTxtParserError::UnsupportedRequirement { start, end, .. } => {
write!(f, "Unsupported requirement in position {start} to {end}")
}
RequirementsTxtParserError::Pep508 { start, end, .. } => {
write!(f, "Couldn't parse requirement in position {start} to {end}")
}
Expand All @@ -591,6 +612,7 @@ impl std::error::Error for RequirementsTxtParserError {
RequirementsTxtParserError::IO(err) => err.source(),
RequirementsTxtParserError::InvalidEditablePath(_) => None,
RequirementsTxtParserError::UnsupportedUrl(_) => None,
RequirementsTxtParserError::UnsupportedRequirement { source, .. } => Some(source),
RequirementsTxtParserError::Pep508 { source, .. } => Some(source),
RequirementsTxtParserError::Subfile { source, .. } => Some(source.as_ref()),
RequirementsTxtParserError::Parser { .. } => None,
Expand Down Expand Up @@ -626,6 +648,15 @@ impl Display for RequirementsTxtFileError {
location
)
}
RequirementsTxtParserError::UnsupportedRequirement { start, end, .. } => {
write!(
f,
"Unsupported requirement in {} position {} to {}",
self.file.display(),
start,
end,
)
}
RequirementsTxtParserError::Pep508 { start, end, .. } => {
write!(
f,
Expand Down

0 comments on commit 540442b

Please sign in to comment.