From c4472ebbb969d24cb804e2929ba87ff4ac938cda Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 10 Apr 2024 01:13:33 -0400 Subject: [PATCH] Enforce and backtrack on invalid versions in source metadata (#2954) ## Summary If we build a source distribution from the registry, and the version doesn't match that of the filename, we should error, just as we do for mismatched package names. However, we should also backtrack here, which we didn't previously. Closes https://github.com/astral-sh/uv/issues/2953. ## Test Plan Verified that `cargo run pip install docutils --verbose --no-cache --reinstall` installs `docutils==0.21` instead of the invalid `docutils==0.21.post1`. In the logs, I see: ``` WARN Unable to extract metadata for docutils: Package metadata version `0.21` does not match given version `0.21.post1` ``` --- crates/distribution-types/src/buildable.rs | 10 ++++ crates/uv-distribution/src/error.rs | 3 ++ crates/uv-distribution/src/source/mod.rs | 59 ++++++++++----------- crates/uv-resolver/src/pubgrub/report.rs | 33 ++++++++++++ crates/uv-resolver/src/resolver/mod.rs | 24 +++++++++ crates/uv-resolver/src/resolver/provider.rs | 8 +++ 6 files changed, 105 insertions(+), 32 deletions(-) diff --git a/crates/distribution-types/src/buildable.rs b/crates/distribution-types/src/buildable.rs index 1b105d299..42e7fe789 100644 --- a/crates/distribution-types/src/buildable.rs +++ b/crates/distribution-types/src/buildable.rs @@ -1,6 +1,7 @@ use std::borrow::Cow; use std::path::Path; +use pep440_rs::Version; use url::Url; use uv_normalize::PackageName; @@ -28,6 +29,15 @@ impl BuildableSource<'_> { } } + /// Return the [`Version`] of the source, if available. + pub fn version(&self) -> Option<&Version> { + match self { + Self::Dist(SourceDist::Registry(dist)) => Some(&dist.filename.version), + Self::Dist(_) => None, + Self::Url(_) => None, + } + } + /// Return the [`BuildableSource`] as a [`SourceDist`], if it is a distribution. pub fn as_dist(&self) -> Option<&SourceDist> { match self { diff --git a/crates/uv-distribution/src/error.rs b/crates/uv-distribution/src/error.rs index fe61b95da..42bf4f48b 100644 --- a/crates/uv-distribution/src/error.rs +++ b/crates/uv-distribution/src/error.rs @@ -3,6 +3,7 @@ use tokio::task::JoinError; use zip::result::ZipError; use distribution_filename::WheelFilenameError; +use pep440_rs::Version; use uv_client::BetterReqwestError; use uv_normalize::PackageName; @@ -47,6 +48,8 @@ pub enum Error { given: PackageName, metadata: PackageName, }, + #[error("Package metadata version `{metadata}` does not match given version `{given}`")] + VersionMismatch { given: Version, metadata: Version }, #[error("Failed to parse metadata from built wheel")] Metadata(#[from] pypi_types::MetadataError), #[error("Failed to read `dist-info` metadata from built wheel")] diff --git a/crates/uv-distribution/src/source/mod.rs b/crates/uv-distribution/src/source/mod.rs index 34b52c105..62ccd8c74 100644 --- a/crates/uv-distribution/src/source/mod.rs +++ b/crates/uv-distribution/src/source/mod.rs @@ -1109,14 +1109,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { let metadata = read_wheel_metadata(&filename, cache_shard.join(&disk_filename))?; // Validate the metadata. - if let Some(name) = source.name() { - if metadata.name != *name { - return Err(Error::NameMismatch { - metadata: metadata.name, - given: name.clone(), - }); - } - } + validate(source, &metadata)?; debug!("Finished building: {source}"); Ok((disk_filename, filename, metadata)) @@ -1138,14 +1131,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { debug!("Found static `PKG-INFO` for: {source}"); // Validate the metadata. - if let Some(name) = source.name() { - if metadata.name != *name { - return Err(Error::NameMismatch { - metadata: metadata.name, - given: name.clone(), - }); - } - } + validate(source, &metadata)?; return Ok(Some(metadata)); } @@ -1161,14 +1147,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { debug!("Found static `pyproject.toml` for: {source}"); // Validate the metadata. - if let Some(name) = source.name() { - if metadata.name != *name { - return Err(Error::NameMismatch { - metadata: metadata.name, - given: name.clone(), - }); - } - } + validate(source, &metadata)?; return Ok(Some(metadata)); } @@ -1208,14 +1187,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { let metadata = Metadata23::parse_metadata(&content)?; // Validate the metadata. - if let Some(name) = source.name() { - if metadata.name != *name { - return Err(Error::NameMismatch { - metadata: metadata.name, - given: name.clone(), - }); - } - } + validate(source, &metadata)?; Ok(Some(metadata)) } @@ -1278,6 +1250,29 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { } } +/// Validate that the source distribution matches the built metadata. +fn validate(source: &BuildableSource<'_>, metadata: &Metadata23) -> Result<(), Error> { + if let Some(name) = source.name() { + if metadata.name != *name { + return Err(Error::NameMismatch { + metadata: metadata.name.clone(), + given: name.clone(), + }); + } + } + + if let Some(version) = source.version() { + if metadata.version != *version { + return Err(Error::VersionMismatch { + metadata: metadata.version.clone(), + given: version.clone(), + }); + } + } + + Ok(()) +} + /// Read an existing HTTP-cached [`Revision`], if it exists. pub(crate) fn read_http_revision(cache_entry: &CacheEntry) -> Result, Error> { match fs_err::File::open(cache_entry.path()) { diff --git a/crates/uv-resolver/src/pubgrub/report.rs b/crates/uv-resolver/src/pubgrub/report.rs index da63129ef..f4c7aea9e 100644 --- a/crates/uv-resolver/src/pubgrub/report.rs +++ b/crates/uv-resolver/src/pubgrub/report.rs @@ -446,6 +446,15 @@ impl PubGrubReportFormatter<'_> { reason: reason.clone(), }); } + IncompletePackage::InconsistentMetadata(reason) => { + hints.insert( + PubGrubHint::InconsistentVersionMetadata { + package: package.clone(), + version: version.clone(), + reason: reason.clone(), + }, + ); + } IncompletePackage::InvalidStructure(reason) => { hints.insert( PubGrubHint::InvalidVersionStructure { @@ -530,6 +539,15 @@ pub(crate) enum PubGrubHint { #[derivative(PartialEq = "ignore", Hash = "ignore")] reason: String, }, + /// Metadata for a package version was inconsistent (e.g., the package name did not match that + /// of the file). + InconsistentVersionMetadata { + package: PubGrubPackage, + #[derivative(PartialEq = "ignore", Hash = "ignore")] + version: Version, + #[derivative(PartialEq = "ignore", Hash = "ignore")] + reason: String, + }, /// The structure of a package version was invalid (e.g., multiple `.dist-info` directories). InvalidVersionStructure { package: PubGrubPackage, @@ -629,6 +647,21 @@ impl std::fmt::Display for PubGrubHint { textwrap::indent(reason, " ") ) } + PubGrubHint::InconsistentVersionMetadata { + package, + version, + reason, + } => { + write!( + f, + "{}{} Metadata for {}=={} was inconsistent:\n{}", + "hint".bold().cyan(), + ":".bold(), + package.bold(), + version.bold(), + textwrap::indent(reason, " ") + ) + } } } } diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index 2f2795c03..5e3723f4c 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -93,6 +93,8 @@ pub(crate) enum IncompletePackage { Offline, /// The wheel metadata was found, but could not be parsed. InvalidMetadata(String), + /// The wheel metadata was found, but the metadata was inconsistent. + InconsistentMetadata(String), /// The wheel has an invalid structure. InvalidStructure(String), } @@ -646,6 +648,13 @@ impl< ); return Ok(None); } + MetadataResponse::InconsistentMetadata(err) => { + self.unavailable_packages.insert( + package_name.clone(), + UnavailablePackage::InvalidMetadata(err.to_string()), + ); + return Ok(None); + } MetadataResponse::InvalidStructure(err) => { self.unavailable_packages.insert( package_name.clone(), @@ -945,6 +954,7 @@ impl< )); } MetadataResponse::InvalidMetadata(err) => { + warn!("Unable to extract metadata for {package_name}: {err}"); self.incomplete_packages .entry(package_name.clone()) .or_default() @@ -956,7 +966,21 @@ impl< "the package metadata could not be parsed".to_string(), )); } + MetadataResponse::InconsistentMetadata(err) => { + warn!("Unable to extract metadata for {package_name}: {err}"); + self.incomplete_packages + .entry(package_name.clone()) + .or_default() + .insert( + version.clone(), + IncompletePackage::InconsistentMetadata(err.to_string()), + ); + return Ok(Dependencies::Unavailable( + "the package metadata was inconsistent".to_string(), + )); + } MetadataResponse::InvalidStructure(err) => { + warn!("Unable to extract metadata for {package_name}: {err}"); self.incomplete_packages .entry(package_name.clone()) .or_default() diff --git a/crates/uv-resolver/src/resolver/provider.rs b/crates/uv-resolver/src/resolver/provider.rs index 10c89a001..c778e780e 100644 --- a/crates/uv-resolver/src/resolver/provider.rs +++ b/crates/uv-resolver/src/resolver/provider.rs @@ -38,6 +38,8 @@ pub enum MetadataResponse { Found(Metadata23), /// The wheel metadata was found, but could not be parsed. InvalidMetadata(Box), + /// The wheel metadata was found, but the metadata was inconsistent. + InconsistentMetadata(Box), /// The wheel has an invalid structure. InvalidStructure(Box), /// The wheel metadata was not found in the cache and the network is not available. @@ -185,6 +187,12 @@ impl<'a, Context: BuildContext + Send + Sync> ResolverProvider } kind => Err(uv_client::Error::from(kind).into()), }, + uv_distribution::Error::VersionMismatch { .. } => { + Ok(MetadataResponse::InconsistentMetadata(Box::new(err))) + } + uv_distribution::Error::NameMismatch { .. } => { + Ok(MetadataResponse::InconsistentMetadata(Box::new(err))) + } uv_distribution::Error::Metadata(err) => { Ok(MetadataResponse::InvalidMetadata(Box::new(err))) }