diff --git a/Cargo.lock b/Cargo.lock index 8f922b04a910..3f0a67ab12c0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2229,7 +2229,7 @@ dependencies = [ [[package]] name = "pubgrub" version = "0.2.1" -source = "git+https://github.com/zanieb/pubgrub?rev=a1d584a5e506b8f0a600269373190c4a35b298d5#a1d584a5e506b8f0a600269373190c4a35b298d5" +source = "git+https://github.com/zanieb/pubgrub?rev=8fff95d6a233a9fa4ee5ab5429033f0f0d3ddb20#8fff95d6a233a9fa4ee5ab5429033f0f0d3ddb20" dependencies = [ "indexmap 2.1.0", "log", diff --git a/Cargo.toml b/Cargo.toml index 68b062e1b1d3..b75616122242 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -51,7 +51,7 @@ once_cell = { version = "1.18.0" } petgraph = { version = "0.6.4" } platform-info = { version = "2.0.2" } plist = { version = "1.6.0" } -pubgrub = { git = "https://github.com/zanieb/pubgrub", rev = "a1d584a5e506b8f0a600269373190c4a35b298d5" } +pubgrub = { git = "https://github.com/zanieb/pubgrub", rev = "8fff95d6a233a9fa4ee5ab5429033f0f0d3ddb20" } pyo3 = { version = "0.20.0" } pyo3-log = { version = "0.9.0"} pyproject-toml = { version = "0.8.0" } diff --git a/crates/puffin-cli/src/commands/pip_compile.rs b/crates/puffin-cli/src/commands/pip_compile.rs index 8d0aab845ae2..00756cfca216 100644 --- a/crates/puffin-cli/src/commands/pip_compile.rs +++ b/crates/puffin-cli/src/commands/pip_compile.rs @@ -152,7 +152,7 @@ pub(crate) async fn pip_compile( let resolver = Resolver::new(manifest, options, &markers, &tags, &client, &build_dispatch) .with_reporter(ResolverReporter::from(printer)); let resolution = match resolver.resolve().await { - Err(puffin_resolver::ResolveError::PubGrub(err)) => { + Err(puffin_resolver::ResolveError::NoSolution(err)) => { #[allow(clippy::print_stderr)] { let report = miette::Report::msg(format!("{err}")) diff --git a/crates/puffin-cli/src/commands/pip_install.rs b/crates/puffin-cli/src/commands/pip_install.rs index 27016a3520b6..0c24e928c981 100644 --- a/crates/puffin-cli/src/commands/pip_install.rs +++ b/crates/puffin-cli/src/commands/pip_install.rs @@ -237,14 +237,14 @@ async fn resolve( let resolver = Resolver::new(manifest, options, markers, &tags, &client, &build_dispatch) .with_reporter(ResolverReporter::from(printer)); let resolution = match resolver.resolve().await { - Err(puffin_resolver::ResolveError::PubGrub(err)) => { + Err(puffin_resolver::ResolveError::NoSolution(err)) => { #[allow(clippy::print_stderr)] { let report = miette::Report::msg(format!("{err}")) .context("No solution found when resolving dependencies:"); eprint!("{report:?}"); } - return Err(puffin_resolver::ResolveError::PubGrub(err).into()); + return Err(puffin_resolver::ResolveError::NoSolution(err).into()); } result => result, }?; diff --git a/crates/puffin-resolver/src/error.rs b/crates/puffin-resolver/src/error.rs index 39996c3e19f5..d215b805cc94 100644 --- a/crates/puffin-resolver/src/error.rs +++ b/crates/puffin-resolver/src/error.rs @@ -1,7 +1,9 @@ use std::fmt::Formatter; use pubgrub::range::Range; -use pubgrub::report::{DefaultStringReporter, Reporter}; +use pubgrub::report::{DefaultStringReporter, DerivationTree, Reporter}; +use pypi_types::IndexUrl; +use rustc_hash::FxHashMap; use thiserror::Error; use url::Url; @@ -9,8 +11,10 @@ use distribution_types::{BuiltDist, PathBuiltDist, PathSourceDist, SourceDist}; use pep508_rs::Requirement; use puffin_distribution::DistributionDatabaseError; use puffin_normalize::PackageName; +use puffin_traits::OnceMap; use crate::pubgrub::{PubGrubPackage, PubGrubVersion}; +use crate::version_map::VersionMap; use crate::PubGrubReportFormatter; #[derive(Error, Debug)] @@ -30,9 +34,6 @@ pub enum ResolveError { #[error(transparent)] Join(#[from] tokio::task::JoinError), - #[error(transparent)] - PubGrub(#[from] RichPubGrubError), - #[error("Package metadata name `{metadata}` does not match given name `{given}`")] NameMismatch { given: PackageName, @@ -65,6 +66,37 @@ pub enum ResolveError { #[error("Failed to build: {0}")] Build(Box, #[source] DistributionDatabaseError), + + #[error(transparent)] + NoSolution(#[from] NoSolutionError), + + #[error("Retrieving dependencies of {package} {version} failed")] + ErrorRetrievingDependencies { + /// Package whose dependencies we want. + package: Box, + /// Version of the package for which we want the dependencies. + version: Box, + /// Error raised by the implementer of [DependencyProvider](crate::solver::DependencyProvider). + source: Box, + }, + + #[error("{package} {version} depends on itself")] + SelfDependency { + /// Package whose dependencies we want. + package: Box, + /// Version of the package for which we want the dependencies. + version: Box, + }, + + #[error("Decision making failed")] + ErrorChoosingPackageVersion(Box), + + #[error("We should cancel")] + ErrorInShouldCancel(Box), + + /// Something unexpected happened. + #[error("{0}")] + Failure(String), } impl From> for ResolveError { @@ -73,28 +105,83 @@ impl From> for ResolveError { } } -/// A wrapper around [`pubgrub::error::PubGrubError`] that displays a resolution failure report. +impl From>> for ResolveError { + fn from(value: pubgrub::error::PubGrubError>) -> Self { + match value { + pubgrub::error::PubGrubError::ErrorChoosingPackageVersion(inner) => { + ResolveError::ErrorChoosingPackageVersion(inner) + } + pubgrub::error::PubGrubError::ErrorInShouldCancel(inner) => { + ResolveError::ErrorInShouldCancel(inner) + } + pubgrub::error::PubGrubError::ErrorRetrievingDependencies { + package, + version, + source, + } => ResolveError::ErrorRetrievingDependencies { + package: Box::new(package), + version: Box::new(version), + source, + }, + pubgrub::error::PubGrubError::Failure(inner) => ResolveError::Failure(inner), + pubgrub::error::PubGrubError::NoSolution(derivation_tree) => { + ResolveError::NoSolution(NoSolutionError { + derivation_tree, + available_versions: FxHashMap::default(), + }) + } + pubgrub::error::PubGrubError::SelfDependency { package, version } => { + ResolveError::SelfDependency { + package: Box::new(package), + version: Box::new(version), + } + } + } + } +} + +/// A wrapper around [`pubgrub::error::PubGrubError::NoSolution`] that displays a resolution failure report. #[derive(Debug)] -pub struct RichPubGrubError { - source: pubgrub::error::PubGrubError>, +pub struct NoSolutionError { + derivation_tree: DerivationTree>, + available_versions: FxHashMap>, } -impl std::error::Error for RichPubGrubError {} +impl std::error::Error for NoSolutionError {} -impl std::fmt::Display for RichPubGrubError { +impl std::fmt::Display for NoSolutionError { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - if let pubgrub::error::PubGrubError::NoSolution(derivation_tree) = &self.source { - let formatter = PubGrubReportFormatter; - let report = DefaultStringReporter::report_with_formatter(derivation_tree, &formatter); - write!(f, "{report}") - } else { - write!(f, "{}", self.source) - } + let formatter = PubGrubReportFormatter { + available_versions: &self.available_versions, + }; + let report = + DefaultStringReporter::report_with_formatter(&self.derivation_tree, &formatter); + write!(f, "{report}") } } -impl From>> for ResolveError { - fn from(value: pubgrub::error::PubGrubError>) -> Self { - ResolveError::PubGrub(RichPubGrubError { source: value }) +impl NoSolutionError { + /// Update the available versions attached to the error using the given package version index. + /// + /// Only packages used in the error's deriviation tree will be retrieved. + pub(crate) fn update_available_versions( + mut self, + package_versions: &OnceMap, + ) -> Self { + for package in self.derivation_tree.packages() { + if let PubGrubPackage::Package(name, ..) = package { + if let Some(entry) = package_versions.get(name) { + let (_, version_map) = entry.value(); + self.available_versions.insert( + package.clone(), + version_map + .iter() + .map(|(version, _)| version.clone()) + .collect(), + ); + } + } + } + self } } diff --git a/crates/puffin-resolver/src/pubgrub/report.rs b/crates/puffin-resolver/src/pubgrub/report.rs index 9908e42f1e01..f75ce3ff8842 100644 --- a/crates/puffin-resolver/src/pubgrub/report.rs +++ b/crates/puffin-resolver/src/pubgrub/report.rs @@ -2,13 +2,32 @@ use pubgrub::range::Range; use pubgrub::report::{External, ReportFormatter}; use pubgrub::term::Term; use pubgrub::type_aliases::Map; +use rustc_hash::FxHashMap; use super::{PubGrubPackage, PubGrubVersion}; -#[derive(Debug, Default)] -pub struct PubGrubReportFormatter; +#[derive(Debug)] +pub struct PubGrubReportFormatter<'a> { + /// The versions that were available for each package + pub available_versions: &'a FxHashMap>, +} + +impl PubGrubReportFormatter<'_> { + fn simplify_set( + &self, + set: &Range, + package: &PubGrubPackage, + ) -> Range { + set.simplify( + self.available_versions + .get(package) + .unwrap_or(&vec![]) + .iter(), + ) + } +} -impl ReportFormatter> for PubGrubReportFormatter { +impl ReportFormatter> for PubGrubReportFormatter<'_> { type Output = String; fn format_external( @@ -23,6 +42,7 @@ impl ReportFormatter> for PubGrubReportFor if set == &Range::full() { format!("there is no available version for {package}") } else { + let set = self.simplify_set(set, package); format!("there is no version of {package} available matching {set}") } } @@ -30,6 +50,7 @@ impl ReportFormatter> for PubGrubReportFor if set == &Range::full() { format!("dependencies of {package} are unavailable") } else { + let set = self.simplify_set(set, package); format!("dependencies of {package} at version {set} are unavailable") } } @@ -41,6 +62,7 @@ impl ReportFormatter> for PubGrubReportFor if set == &Range::full() { format!("dependencies of {package} are unusable: {reason}") } else { + let set = self.simplify_set(set, package); format!("dependencies of {package}{set} are unusable: {reason}",) } } @@ -48,6 +70,7 @@ impl ReportFormatter> for PubGrubReportFor if set == &Range::full() { format!("dependencies of {package} are unusable") } else { + let set = self.simplify_set(set, package); format!("dependencies of {package}{set} are unusable") } } @@ -56,19 +79,23 @@ impl ReportFormatter> for PubGrubReportFor if package_set == &Range::full() && dependency_set == &Range::full() { format!("{package} depends on {dependency}") } else if package_set == &Range::full() { + let dependency_set = self.simplify_set(dependency_set, package); format!("{package} depends on {dependency}{dependency_set}") } else if dependency_set == &Range::full() { if matches!(package, PubGrubPackage::Root(_)) { // Exclude the dummy version for root packages format!("{package} depends on {dependency}") } else { + let package_set = self.simplify_set(package_set, package); format!("{package}{package_set} depends on {dependency}") } } else { + let dependency_set = self.simplify_set(dependency_set, package); if matches!(package, PubGrubPackage::Root(_)) { // Exclude the dummy version for root packages format!("{package} depends on {dependency}{dependency_set}") } else { + let package_set = self.simplify_set(package_set, package); format!("{package}{package_set} depends on {dependency}{dependency_set}") } } @@ -82,9 +109,21 @@ impl ReportFormatter> for PubGrubReportFor match terms_vec.as_slice() { [] | [(PubGrubPackage::Root(_), _)] => "version solving failed".into(), [(package @ PubGrubPackage::Package(..), Term::Positive(range))] => { + let range = range.simplify( + self.available_versions + .get(package) + .unwrap_or(&vec![]) + .iter(), + ); format!("{package}{range} is forbidden") } [(package @ PubGrubPackage::Package(..), Term::Negative(range))] => { + let range = range.simplify( + self.available_versions + .get(package) + .unwrap_or(&vec![]) + .iter(), + ); format!("{package}{range} is mandatory") } [(p1, Term::Positive(r1)), (p2, Term::Negative(r2))] => self.format_external( diff --git a/crates/puffin-resolver/src/resolver.rs b/crates/puffin-resolver/src/resolver.rs index 1740ef02eaee..5cb68404a8de 100644 --- a/crates/puffin-resolver/src/resolver.rs +++ b/crates/puffin-resolver/src/resolver.rs @@ -247,7 +247,14 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> { return Err(ResolveError::StreamTermination); } resolution = resolve_fut => { - resolution? + resolution.map_err(|err| { + // Add version information to improve unsat error messages + if let ResolveError::NoSolution(err) = err { + ResolveError::NoSolution(err.update_available_versions(&self.index.packages)) + } else { + err + } + })? } };