Skip to content

Commit 490fb55

Browse files
zaniebzanieb
authored
Use available versions to simplify unsat error reports (#547)
Uses pubgrub-rs/pubgrub#156 to consolidate version ranges in error reports using the actual available versions for each package. Alternative to astral-sh/pubgrub#8 which implements this behavior as a method in the `Reporter` — here it's implemented in our custom report formatter (#521) instead which requires no upstream changes. Requires astral-sh/pubgrub#11 to only retrieve the versions for packages that will be used in the report. This is a work in progress. Some things to do: - ~We may want to allow lazy retrieval of the version maps from the formatter~ - [x] We should probably create a separate error type for no solution instead of mixing them with other resolve errors - ~We can probably do something smarter than creating vectors to hold the versions~ - [x] This degrades error messages when a single version is not available, we'll need to special case that - [x] It seems safer to coerce the error type in `resolve` instead of `solve` if feasible
1 parent a8512d7 commit 490fb55

File tree

7 files changed

+161
-28
lines changed

7 files changed

+161
-28
lines changed

‎Cargo.lock

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ once_cell = { version = "1.18.0" }
5151
petgraph = { version = "0.6.4" }
5252
platform-info = { version = "2.0.2" }
5353
plist = { version = "1.6.0" }
54-
pubgrub = { git = "https://github.com/zanieb/pubgrub", rev = "a1d584a5e506b8f0a600269373190c4a35b298d5" }
54+
pubgrub = { git = "https://github.com/zanieb/pubgrub", rev = "8fff95d6a233a9fa4ee5ab5429033f0f0d3ddb20" }
5555
pyo3 = { version = "0.20.0" }
5656
pyo3-log = { version = "0.9.0"}
5757
pyproject-toml = { version = "0.8.0" }

‎crates/puffin-cli/src/commands/pip_compile.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ pub(crate) async fn pip_compile(
152152
let resolver = Resolver::new(manifest, options, &markers, &tags, &client, &build_dispatch)
153153
.with_reporter(ResolverReporter::from(printer));
154154
let resolution = match resolver.resolve().await {
155-
Err(puffin_resolver::ResolveError::PubGrub(err)) => {
155+
Err(puffin_resolver::ResolveError::NoSolution(err)) => {
156156
#[allow(clippy::print_stderr)]
157157
{
158158
let report = miette::Report::msg(format!("{err}"))

‎crates/puffin-cli/src/commands/pip_install.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -237,14 +237,14 @@ async fn resolve(
237237
let resolver = Resolver::new(manifest, options, markers, &tags, &client, &build_dispatch)
238238
.with_reporter(ResolverReporter::from(printer));
239239
let resolution = match resolver.resolve().await {
240-
Err(puffin_resolver::ResolveError::PubGrub(err)) => {
240+
Err(puffin_resolver::ResolveError::NoSolution(err)) => {
241241
#[allow(clippy::print_stderr)]
242242
{
243243
let report = miette::Report::msg(format!("{err}"))
244244
.context("No solution found when resolving dependencies:");
245245
eprint!("{report:?}");
246246
}
247-
return Err(puffin_resolver::ResolveError::PubGrub(err).into());
247+
return Err(puffin_resolver::ResolveError::NoSolution(err).into());
248248
}
249249
result => result,
250250
}?;

‎crates/puffin-resolver/src/error.rs

Lines changed: 106 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,20 @@
11
use std::fmt::Formatter;
22

33
use pubgrub::range::Range;
4-
use pubgrub::report::{DefaultStringReporter, Reporter};
4+
use pubgrub::report::{DefaultStringReporter, DerivationTree, Reporter};
5+
use pypi_types::IndexUrl;
6+
use rustc_hash::FxHashMap;
57
use thiserror::Error;
68
use url::Url;
79

810
use distribution_types::{BuiltDist, PathBuiltDist, PathSourceDist, SourceDist};
911
use pep508_rs::Requirement;
1012
use puffin_distribution::DistributionDatabaseError;
1113
use puffin_normalize::PackageName;
14+
use puffin_traits::OnceMap;
1215

1316
use crate::pubgrub::{PubGrubPackage, PubGrubVersion};
17+
use crate::version_map::VersionMap;
1418
use crate::PubGrubReportFormatter;
1519

1620
#[derive(Error, Debug)]
@@ -30,9 +34,6 @@ pub enum ResolveError {
3034
#[error(transparent)]
3135
Join(#[from] tokio::task::JoinError),
3236

33-
#[error(transparent)]
34-
PubGrub(#[from] RichPubGrubError),
35-
3637
#[error("Package metadata name `{metadata}` does not match given name `{given}`")]
3738
NameMismatch {
3839
given: PackageName,
@@ -65,6 +66,37 @@ pub enum ResolveError {
6566

6667
#[error("Failed to build: {0}")]
6768
Build(Box<PathSourceDist>, #[source] DistributionDatabaseError),
69+
70+
#[error(transparent)]
71+
NoSolution(#[from] NoSolutionError),
72+
73+
#[error("Retrieving dependencies of {package} {version} failed")]
74+
ErrorRetrievingDependencies {
75+
/// Package whose dependencies we want.
76+
package: Box<PubGrubPackage>,
77+
/// Version of the package for which we want the dependencies.
78+
version: Box<PubGrubVersion>,
79+
/// Error raised by the implementer of [DependencyProvider](crate::solver::DependencyProvider).
80+
source: Box<dyn std::error::Error + Send + Sync>,
81+
},
82+
83+
#[error("{package} {version} depends on itself")]
84+
SelfDependency {
85+
/// Package whose dependencies we want.
86+
package: Box<PubGrubPackage>,
87+
/// Version of the package for which we want the dependencies.
88+
version: Box<PubGrubVersion>,
89+
},
90+
91+
#[error("Decision making failed")]
92+
ErrorChoosingPackageVersion(Box<dyn std::error::Error + Send + Sync>),
93+
94+
#[error("We should cancel")]
95+
ErrorInShouldCancel(Box<dyn std::error::Error + Send + Sync>),
96+
97+
/// Something unexpected happened.
98+
#[error("{0}")]
99+
Failure(String),
68100
}
69101

70102
impl<T> From<futures::channel::mpsc::TrySendError<T>> for ResolveError {
@@ -73,28 +105,83 @@ impl<T> From<futures::channel::mpsc::TrySendError<T>> for ResolveError {
73105
}
74106
}
75107

76-
/// A wrapper around [`pubgrub::error::PubGrubError`] that displays a resolution failure report.
108+
impl From<pubgrub::error::PubGrubError<PubGrubPackage, Range<PubGrubVersion>>> for ResolveError {
109+
fn from(value: pubgrub::error::PubGrubError<PubGrubPackage, Range<PubGrubVersion>>) -> Self {
110+
match value {
111+
pubgrub::error::PubGrubError::ErrorChoosingPackageVersion(inner) => {
112+
ResolveError::ErrorChoosingPackageVersion(inner)
113+
}
114+
pubgrub::error::PubGrubError::ErrorInShouldCancel(inner) => {
115+
ResolveError::ErrorInShouldCancel(inner)
116+
}
117+
pubgrub::error::PubGrubError::ErrorRetrievingDependencies {
118+
package,
119+
version,
120+
source,
121+
} => ResolveError::ErrorRetrievingDependencies {
122+
package: Box::new(package),
123+
version: Box::new(version),
124+
source,
125+
},
126+
pubgrub::error::PubGrubError::Failure(inner) => ResolveError::Failure(inner),
127+
pubgrub::error::PubGrubError::NoSolution(derivation_tree) => {
128+
ResolveError::NoSolution(NoSolutionError {
129+
derivation_tree,
130+
available_versions: FxHashMap::default(),
131+
})
132+
}
133+
pubgrub::error::PubGrubError::SelfDependency { package, version } => {
134+
ResolveError::SelfDependency {
135+
package: Box::new(package),
136+
version: Box::new(version),
137+
}
138+
}
139+
}
140+
}
141+
}
142+
143+
/// A wrapper around [`pubgrub::error::PubGrubError::NoSolution`] that displays a resolution failure report.
77144
#[derive(Debug)]
78-
pub struct RichPubGrubError {
79-
source: pubgrub::error::PubGrubError<PubGrubPackage, Range<PubGrubVersion>>,
145+
pub struct NoSolutionError {
146+
derivation_tree: DerivationTree<PubGrubPackage, Range<PubGrubVersion>>,
147+
available_versions: FxHashMap<PubGrubPackage, Vec<PubGrubVersion>>,
80148
}
81149

82-
impl std::error::Error for RichPubGrubError {}
150+
impl std::error::Error for NoSolutionError {}
83151

84-
impl std::fmt::Display for RichPubGrubError {
152+
impl std::fmt::Display for NoSolutionError {
85153
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
86-
if let pubgrub::error::PubGrubError::NoSolution(derivation_tree) = &self.source {
87-
let formatter = PubGrubReportFormatter;
88-
let report = DefaultStringReporter::report_with_formatter(derivation_tree, &formatter);
89-
write!(f, "{report}")
90-
} else {
91-
write!(f, "{}", self.source)
92-
}
154+
let formatter = PubGrubReportFormatter {
155+
available_versions: &self.available_versions,
156+
};
157+
let report =
158+
DefaultStringReporter::report_with_formatter(&self.derivation_tree, &formatter);
159+
write!(f, "{report}")
93160
}
94161
}
95162

96-
impl From<pubgrub::error::PubGrubError<PubGrubPackage, Range<PubGrubVersion>>> for ResolveError {
97-
fn from(value: pubgrub::error::PubGrubError<PubGrubPackage, Range<PubGrubVersion>>) -> Self {
98-
ResolveError::PubGrub(RichPubGrubError { source: value })
163+
impl NoSolutionError {
164+
/// Update the available versions attached to the error using the given package version index.
165+
///
166+
/// Only packages used in the error's deriviation tree will be retrieved.
167+
pub(crate) fn update_available_versions(
168+
mut self,
169+
package_versions: &OnceMap<PackageName, (IndexUrl, VersionMap)>,
170+
) -> Self {
171+
for package in self.derivation_tree.packages() {
172+
if let PubGrubPackage::Package(name, ..) = package {
173+
if let Some(entry) = package_versions.get(name) {
174+
let (_, version_map) = entry.value();
175+
self.available_versions.insert(
176+
package.clone(),
177+
version_map
178+
.iter()
179+
.map(|(version, _)| version.clone())
180+
.collect(),
181+
);
182+
}
183+
}
184+
}
185+
self
99186
}
100187
}

‎crates/puffin-resolver/src/pubgrub/report.rs

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,32 @@ use pubgrub::range::Range;
22
use pubgrub::report::{External, ReportFormatter};
33
use pubgrub::term::Term;
44
use pubgrub::type_aliases::Map;
5+
use rustc_hash::FxHashMap;
56

67
use super::{PubGrubPackage, PubGrubVersion};
78

8-
#[derive(Debug, Default)]
9-
pub struct PubGrubReportFormatter;
9+
#[derive(Debug)]
10+
pub struct PubGrubReportFormatter<'a> {
11+
/// The versions that were available for each package
12+
pub available_versions: &'a FxHashMap<PubGrubPackage, Vec<PubGrubVersion>>,
13+
}
14+
15+
impl PubGrubReportFormatter<'_> {
16+
fn simplify_set(
17+
&self,
18+
set: &Range<PubGrubVersion>,
19+
package: &PubGrubPackage,
20+
) -> Range<PubGrubVersion> {
21+
set.simplify(
22+
self.available_versions
23+
.get(package)
24+
.unwrap_or(&vec![])
25+
.iter(),
26+
)
27+
}
28+
}
1029

11-
impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFormatter {
30+
impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFormatter<'_> {
1231
type Output = String;
1332

1433
fn format_external(
@@ -23,13 +42,15 @@ impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFor
2342
if set == &Range::full() {
2443
format!("there is no available version for {package}")
2544
} else {
45+
let set = self.simplify_set(set, package);
2646
format!("there is no version of {package} available matching {set}")
2747
}
2848
}
2949
External::UnavailableDependencies(package, set) => {
3050
if set == &Range::full() {
3151
format!("dependencies of {package} are unavailable")
3252
} else {
53+
let set = self.simplify_set(set, package);
3354
format!("dependencies of {package} at version {set} are unavailable")
3455
}
3556
}
@@ -41,13 +62,15 @@ impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFor
4162
if set == &Range::full() {
4263
format!("dependencies of {package} are unusable: {reason}")
4364
} else {
65+
let set = self.simplify_set(set, package);
4466
format!("dependencies of {package}{set} are unusable: {reason}",)
4567
}
4668
}
4769
} else {
4870
if set == &Range::full() {
4971
format!("dependencies of {package} are unusable")
5072
} else {
73+
let set = self.simplify_set(set, package);
5174
format!("dependencies of {package}{set} are unusable")
5275
}
5376
}
@@ -56,19 +79,23 @@ impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFor
5679
if package_set == &Range::full() && dependency_set == &Range::full() {
5780
format!("{package} depends on {dependency}")
5881
} else if package_set == &Range::full() {
82+
let dependency_set = self.simplify_set(dependency_set, package);
5983
format!("{package} depends on {dependency}{dependency_set}")
6084
} else if dependency_set == &Range::full() {
6185
if matches!(package, PubGrubPackage::Root(_)) {
6286
// Exclude the dummy version for root packages
6387
format!("{package} depends on {dependency}")
6488
} else {
89+
let package_set = self.simplify_set(package_set, package);
6590
format!("{package}{package_set} depends on {dependency}")
6691
}
6792
} else {
93+
let dependency_set = self.simplify_set(dependency_set, package);
6894
if matches!(package, PubGrubPackage::Root(_)) {
6995
// Exclude the dummy version for root packages
7096
format!("{package} depends on {dependency}{dependency_set}")
7197
} else {
98+
let package_set = self.simplify_set(package_set, package);
7299
format!("{package}{package_set} depends on {dependency}{dependency_set}")
73100
}
74101
}
@@ -82,9 +109,21 @@ impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFor
82109
match terms_vec.as_slice() {
83110
[] | [(PubGrubPackage::Root(_), _)] => "version solving failed".into(),
84111
[(package @ PubGrubPackage::Package(..), Term::Positive(range))] => {
112+
let range = range.simplify(
113+
self.available_versions
114+
.get(package)
115+
.unwrap_or(&vec![])
116+
.iter(),
117+
);
85118
format!("{package}{range} is forbidden")
86119
}
87120
[(package @ PubGrubPackage::Package(..), Term::Negative(range))] => {
121+
let range = range.simplify(
122+
self.available_versions
123+
.get(package)
124+
.unwrap_or(&vec![])
125+
.iter(),
126+
);
88127
format!("{package}{range} is mandatory")
89128
}
90129
[(p1, Term::Positive(r1)), (p2, Term::Negative(r2))] => self.format_external(

‎crates/puffin-resolver/src/resolver.rs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,14 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
247247
return Err(ResolveError::StreamTermination);
248248
}
249249
resolution = resolve_fut => {
250-
resolution?
250+
resolution.map_err(|err| {
251+
// Add version information to improve unsat error messages
252+
if let ResolveError::NoSolution(err) = err {
253+
ResolveError::NoSolution(err.update_available_versions(&self.index.packages))
254+
} else {
255+
err
256+
}
257+
})?
251258
}
252259
};
253260

0 commit comments

Comments
 (0)