From 7ae06b3b46b96be579713a392d8161ac639bc9ab Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 9 Apr 2024 23:12:10 -0400 Subject: [PATCH] Surface invalid metadata as hints in error reports (#2850) ## Summary Closes #2847. --- Cargo.lock | 1 + crates/uv-resolver/Cargo.toml | 1 + crates/uv-resolver/src/error.rs | 31 +++++- crates/uv-resolver/src/pubgrub/report.rs | 136 ++++++++++++++++++++++- crates/uv-resolver/src/resolver/mod.rs | 75 +++++++++---- crates/uv/tests/pip_compile.rs | 9 ++ crates/uv/tests/pip_sync.rs | 3 + 7 files changed, 229 insertions(+), 27 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2912918d4..22ef029e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4786,6 +4786,7 @@ dependencies = [ "requirements-txt", "rkyv", "rustc-hash", + "textwrap", "thiserror", "tokio", "tokio-stream", diff --git a/crates/uv-resolver/Cargo.toml b/crates/uv-resolver/Cargo.toml index 1c8c5dbb3..55269d8c6 100644 --- a/crates/uv-resolver/Cargo.toml +++ b/crates/uv-resolver/Cargo.toml @@ -48,6 +48,7 @@ petgraph = { workspace = true } pubgrub = { workspace = true } rkyv = { workspace = true } rustc-hash = { workspace = true } +textwrap = { workspace = true } thiserror = { workspace = true } tokio = { workspace = true, features = ["macros"] } tokio-stream = { workspace = true } diff --git a/crates/uv-resolver/src/error.rs b/crates/uv-resolver/src/error.rs index 96c46fcb1..623136dff 100644 --- a/crates/uv-resolver/src/error.rs +++ b/crates/uv-resolver/src/error.rs @@ -1,4 +1,4 @@ -use std::collections::BTreeSet; +use std::collections::{BTreeMap, BTreeSet}; use std::fmt::Formatter; use std::ops::Deref; @@ -20,7 +20,7 @@ use crate::candidate_selector::CandidateSelector; use crate::dependency_provider::UvDependencyProvider; use crate::pubgrub::{PubGrubPackage, PubGrubPython, PubGrubReportFormatter}; use crate::python_requirement::PythonRequirement; -use crate::resolver::{UnavailablePackage, VersionsResponse}; +use crate::resolver::{IncompletePackage, UnavailablePackage, VersionsResponse}; #[derive(Debug, thiserror::Error)] pub enum ResolveError { @@ -125,6 +125,7 @@ impl From> for ResolveError { python_requirement: None, index_locations: None, unavailable_packages: FxHashMap::default(), + incomplete_packages: FxHashMap::default(), }) } pubgrub::error::PubGrubError::SelfDependency { package, version } => { @@ -146,6 +147,7 @@ pub struct NoSolutionError { python_requirement: Option, index_locations: Option, unavailable_packages: FxHashMap, + incomplete_packages: FxHashMap>, } impl std::error::Error for NoSolutionError {} @@ -167,6 +169,7 @@ impl std::fmt::Display for NoSolutionError { &self.selector, &self.index_locations, &self.unavailable_packages, + &self.incomplete_packages, ) { write!(f, "\n\n{hint}")?; } @@ -261,6 +264,30 @@ impl NoSolutionError { self } + /// Update the incomplete packages attached to the error. + #[must_use] + pub(crate) fn with_incomplete_packages( + mut self, + incomplete_packages: &DashMap>, + ) -> Self { + let mut new = FxHashMap::default(); + for package in self.derivation_tree.packages() { + if let PubGrubPackage::Package(name, ..) = package { + if let Some(entry) = incomplete_packages.get(name) { + let versions = entry.value(); + for entry in versions { + let (version, reason) = entry.pair(); + new.entry(name.clone()) + .or_insert_with(BTreeMap::default) + .insert(version.clone(), reason.clone()); + } + } + } + } + self.incomplete_packages = new; + self + } + /// Update the Python requirements attached to the error. #[must_use] pub(crate) fn with_python_requirement( diff --git a/crates/uv-resolver/src/pubgrub/report.rs b/crates/uv-resolver/src/pubgrub/report.rs index dfb93997a..da63129ef 100644 --- a/crates/uv-resolver/src/pubgrub/report.rs +++ b/crates/uv-resolver/src/pubgrub/report.rs @@ -1,6 +1,6 @@ use std::borrow::Cow; use std::cmp::Ordering; -use std::collections::BTreeSet; +use std::collections::{BTreeMap, BTreeSet}; use std::ops::Bound; use derivative::Derivative; @@ -17,7 +17,7 @@ use uv_normalize::PackageName; use crate::candidate_selector::CandidateSelector; use crate::python_requirement::PythonRequirement; -use crate::resolver::UnavailablePackage; +use crate::resolver::{IncompletePackage, UnavailablePackage}; use super::PubGrubPackage; @@ -342,6 +342,7 @@ impl PubGrubReportFormatter<'_> { selector: &Option, index_locations: &Option, unavailable_packages: &FxHashMap, + incomplete_packages: &FxHashMap>, ) -> IndexSet { /// Returns `true` if pre-releases were allowed for a package. fn allowed_prerelease(package: &PubGrubPackage, selector: &CandidateSelector) -> bool { @@ -354,7 +355,7 @@ impl PubGrubReportFormatter<'_> { let mut hints = IndexSet::default(); match derivation_tree { DerivationTree::External(external) => match external { - External::NoVersions(package, set, _) => { + External::Unavailable(package, set, _) | External::NoVersions(package, set, _) => { // Check for no versions due to pre-release options if let Some(selector) = selector { let any_prerelease = set.iter().any(|(start, end)| { @@ -404,6 +405,7 @@ impl PubGrubReportFormatter<'_> { index_locations.flat_index().peekable().peek().is_none(); if let PubGrubPackage::Package(name, ..) = package { + // Add hints due to the package being entirely unavailable. match unavailable_packages.get(name) { Some(UnavailablePackage::NoIndex) => { if no_find_links { @@ -413,13 +415,55 @@ impl PubGrubReportFormatter<'_> { Some(UnavailablePackage::Offline) => { hints.insert(PubGrubHint::Offline); } - _ => {} + Some(UnavailablePackage::InvalidMetadata(reason)) => { + hints.insert(PubGrubHint::InvalidPackageMetadata { + package: package.clone(), + reason: reason.clone(), + }); + } + Some(UnavailablePackage::InvalidStructure(reason)) => { + hints.insert(PubGrubHint::InvalidPackageStructure { + package: package.clone(), + reason: reason.clone(), + }); + } + Some(UnavailablePackage::NotFound) => {} + None => {} + } + + // Add hints due to the package being unavailable at specific versions. + if let Some(versions) = incomplete_packages.get(name) { + for (version, incomplete) in versions.iter().rev() { + if set.contains(version) { + match incomplete { + IncompletePackage::Offline => { + hints.insert(PubGrubHint::Offline); + } + IncompletePackage::InvalidMetadata(reason) => { + hints.insert(PubGrubHint::InvalidVersionMetadata { + package: package.clone(), + version: version.clone(), + reason: reason.clone(), + }); + } + IncompletePackage::InvalidStructure(reason) => { + hints.insert( + PubGrubHint::InvalidVersionStructure { + package: package.clone(), + version: version.clone(), + reason: reason.clone(), + }, + ); + } + } + break; + } + } } } } } External::NotRoot(..) => {} - External::Unavailable(..) => {} External::FromDependencyOf(..) => {} }, DerivationTree::Derived(derived) => { @@ -428,12 +472,14 @@ impl PubGrubReportFormatter<'_> { selector, index_locations, unavailable_packages, + incomplete_packages, )); hints.extend(self.hints( &derived.cause2, selector, index_locations, unavailable_packages, + incomplete_packages, )); } } @@ -462,8 +508,36 @@ pub(crate) enum PubGrubHint { /// Requirements were unavailable due to lookups in the index being disabled and no extra /// index was provided via `--find-links` NoIndex, - /// A package was not found in the registry, but + /// A package was not found in the registry, but network access was disabled. Offline, + /// Metadata for a package could not be parsed. + InvalidPackageMetadata { + package: PubGrubPackage, + #[derivative(PartialEq = "ignore", Hash = "ignore")] + reason: String, + }, + /// The structure of a package was invalid (e.g., multiple `.dist-info` directories). + InvalidPackageStructure { + package: PubGrubPackage, + #[derivative(PartialEq = "ignore", Hash = "ignore")] + reason: String, + }, + /// Metadata for a package version could not be parsed. + InvalidVersionMetadata { + 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, + #[derivative(PartialEq = "ignore", Hash = "ignore")] + version: Version, + #[derivative(PartialEq = "ignore", Hash = "ignore")] + reason: String, + }, } impl std::fmt::Display for PubGrubHint { @@ -505,6 +579,56 @@ impl std::fmt::Display for PubGrubHint { ":".bold(), ) } + Self::InvalidPackageMetadata { package, reason } => { + write!( + f, + "{}{} Metadata for {} could not be parsed:\n{}", + "hint".bold().cyan(), + ":".bold(), + package.bold(), + textwrap::indent(reason, " ") + ) + } + Self::InvalidPackageStructure { package, reason } => { + write!( + f, + "{}{} The structure of {} was invalid:\n{}", + "hint".bold().cyan(), + ":".bold(), + package.bold(), + textwrap::indent(reason, " ") + ) + } + Self::InvalidVersionMetadata { + package, + version, + reason, + } => { + write!( + f, + "{}{} Metadata for {}=={} could not be parsed:\n{}", + "hint".bold().cyan(), + ":".bold(), + package.bold(), + version.bold(), + textwrap::indent(reason, " ") + ) + } + Self::InvalidVersionStructure { + package, + version, + reason, + } => { + write!( + f, + "{}{} The structure of {}=={} was invalid:\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 11ec5a9b0..2f2795c03 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -71,19 +71,30 @@ pub(crate) enum UnavailableVersion { IncompatibleDist(IncompatibleDist), } -/// The package is unavailable and cannot be used +/// The package is unavailable and cannot be used. #[derive(Debug, Clone)] pub(crate) enum UnavailablePackage { - /// Index lookups were disabled (i.e., `--no-index`) and the package was not found in a flat index (i.e. from `--find-links`) + /// Index lookups were disabled (i.e., `--no-index`) and the package was not found in a flat index (i.e. from `--find-links`). NoIndex, /// Network requests were disabled (i.e., `--offline`), and the package was not found in the cache. Offline, - /// The package was not found in the registry + /// The package was not found in the registry. NotFound, + /// The package metadata was found, but could not be parsed. + InvalidMetadata(String), + /// The package has an invalid structure. + InvalidStructure(String), +} + +/// The package is unavailable at specific versions. +#[derive(Debug, Clone)] +pub(crate) enum IncompletePackage { + /// Network requests were disabled (i.e., `--offline`), and the wheel metadata was not found in the cache. + Offline, /// The wheel metadata was found, but could not be parsed. - InvalidMetadata, + InvalidMetadata(String), /// The wheel has an invalid structure. - InvalidStructure, + InvalidStructure(String), } enum ResolverVersion { @@ -113,8 +124,10 @@ pub struct Resolver< selector: CandidateSelector, index: &'a InMemoryIndex, installed_packages: &'a InstalledPackages, - /// Incompatibilities for packages that are entirely unavailable + /// Incompatibilities for packages that are entirely unavailable. unavailable_packages: DashMap, + /// Incompatibilities for packages that are unavailable at specific versions. + incomplete_packages: DashMap>, /// The set of all registry-based packages visited during resolution. visited: DashSet, reporter: Option>, @@ -185,6 +198,7 @@ impl< Ok(Self { index, unavailable_packages: DashMap::default(), + incomplete_packages: DashMap::default(), visited: DashSet::default(), selector: CandidateSelector::for_resolution(options, &manifest, markers), dependency_mode: options.dependency_mode, @@ -247,7 +261,8 @@ impl< .with_selector(self.selector.clone()) .with_python_requirement(&self.python_requirement) .with_index_locations(self.provider.index_locations()) - .with_unavailable_packages(&self.unavailable_packages), + .with_unavailable_packages(&self.unavailable_packages) + .with_incomplete_packages(&self.incomplete_packages), ) } else { err @@ -352,10 +367,10 @@ impl< UnavailablePackage::NotFound => { "was not found in the package registry" } - UnavailablePackage::InvalidMetadata => { + UnavailablePackage::InvalidMetadata(_) => { "was found, but the metadata could not be parsed" } - UnavailablePackage::InvalidStructure => { + UnavailablePackage::InvalidStructure(_) => { "was found, but has an invalid format" } }) @@ -624,14 +639,18 @@ impl< .insert(package_name.clone(), UnavailablePackage::Offline); return Ok(None); } - MetadataResponse::InvalidMetadata(_) => { - self.unavailable_packages - .insert(package_name.clone(), UnavailablePackage::InvalidMetadata); + MetadataResponse::InvalidMetadata(err) => { + self.unavailable_packages.insert( + package_name.clone(), + UnavailablePackage::InvalidMetadata(err.to_string()), + ); return Ok(None); } - MetadataResponse::InvalidStructure(_) => { - self.unavailable_packages - .insert(package_name.clone(), UnavailablePackage::InvalidStructure); + MetadataResponse::InvalidStructure(err) => { + self.unavailable_packages.insert( + package_name.clone(), + UnavailablePackage::InvalidStructure(err.to_string()), + ); return Ok(None); } }; @@ -913,20 +932,38 @@ impl< .await .ok_or(ResolveError::Unregistered)?; - let metadata = match *response { - MetadataResponse::Found(ref metadata) => metadata, + let metadata = match &*response { + MetadataResponse::Found(metadata) => metadata, MetadataResponse::Offline => { + self.incomplete_packages + .entry(package_name.clone()) + .or_default() + .insert(version.clone(), IncompletePackage::Offline); return Ok(Dependencies::Unavailable( "network connectivity is disabled, but the metadata wasn't found in the cache" .to_string(), )); } - MetadataResponse::InvalidMetadata(_) => { + MetadataResponse::InvalidMetadata(err) => { + self.incomplete_packages + .entry(package_name.clone()) + .or_default() + .insert( + version.clone(), + IncompletePackage::InvalidMetadata(err.to_string()), + ); return Ok(Dependencies::Unavailable( "the package metadata could not be parsed".to_string(), )); } - MetadataResponse::InvalidStructure(_) => { + MetadataResponse::InvalidStructure(err) => { + self.incomplete_packages + .entry(package_name.clone()) + .or_default() + .insert( + version.clone(), + IncompletePackage::InvalidStructure(err.to_string()), + ); return Ok(Dependencies::Unavailable( "the package has an invalid format".to_string(), )); diff --git a/crates/uv/tests/pip_compile.rs b/crates/uv/tests/pip_compile.rs index 292d4507b..4bff90244 100644 --- a/crates/uv/tests/pip_compile.rs +++ b/crates/uv/tests/pip_compile.rs @@ -4555,6 +4555,12 @@ fn invalid_metadata_requires_python() -> Result<()> { ----- stderr ----- × No solution found when resolving dependencies: ╰─▶ Because validation==2.0.0 is unusable because the package metadata could not be parsed and you require validation==2.0.0, we can conclude that the requirements are unsatisfiable. + + hint: Metadata for validation==2.0.0 could not be parsed: + Failed to parse version: Unexpected end of version specifier, expected operator: + 12 + ^^ + "### ); @@ -4581,6 +4587,9 @@ fn invalid_metadata_multiple_dist_info() -> Result<()> { ----- stderr ----- × No solution found when resolving dependencies: ╰─▶ Because validation==3.0.0 is unusable because the package has an invalid format and you require validation==3.0.0, we can conclude that the requirements are unsatisfiable. + + hint: The structure of validation==3.0.0 was invalid: + Multiple .dist-info directories found: validation-2.0.0, validation-3.0.0 "### ); diff --git a/crates/uv/tests/pip_sync.rs b/crates/uv/tests/pip_sync.rs index b387eb9ac..202d3a9c6 100644 --- a/crates/uv/tests/pip_sync.rs +++ b/crates/uv/tests/pip_sync.rs @@ -1106,6 +1106,9 @@ fn mismatched_name() -> Result<()> { ----- stderr ----- × No solution found when resolving dependencies: ╰─▶ Because foo was found, but has an invalid format and you require foo, we can conclude that the requirements are unsatisfiable. + + hint: The structure of foo was invalid: + The .dist-info directory tomli-2.0.1 does not start with the normalized package name: foo "### );