Add a dedicated error message to hint users towards enabling pre-releases (#697)

This PR adds a dedicated error message for resolutions that fail, but
might've succeeded if pre-releases were allowed. Specifically, if we see
a failed resolution, and failed to find a version for a package that
included a pre-release marker, we add a hint nudging the user to
explicitly enable all pre-releases.

We'd prefer a solution like
https://github.com/astral-sh/puffin/pull/666, but believe that it will
break some assumptions in PubGrub, so this is the lighter-weight
solution.

Closes https://github.com/astral-sh/puffin/issues/659.
This commit is contained in:
Charlie Marsh 2023-12-28 22:44:35 -04:00 committed by GitHub
parent 3f8dc9f5bb
commit 2cfa4a3574
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 197 additions and 27 deletions

View file

@ -13,7 +13,7 @@ use crate::resolution_mode::ResolutionStrategy;
use crate::version_map::{ResolvableFile, VersionMap};
use crate::{Manifest, ResolutionOptions};
#[derive(Debug)]
#[derive(Debug, Clone)]
pub(crate) struct CandidateSelector {
resolution_strategy: ResolutionStrategy,
prerelease_strategy: PreReleaseStrategy,
@ -35,10 +35,22 @@ impl CandidateSelector {
preferences: Preferences::from(manifest.preferences.as_slice()),
}
}
#[inline]
#[allow(dead_code)]
pub(crate) fn resolution_strategy(&self) -> &ResolutionStrategy {
&self.resolution_strategy
}
#[inline]
#[allow(dead_code)]
pub(crate) fn prerelease_strategy(&self) -> &PreReleaseStrategy {
&self.prerelease_strategy
}
}
/// A set of pinned packages that should be preserved during resolution, if possible.
#[derive(Debug)]
#[derive(Debug, Clone)]
struct Preferences(FxHashMap<PackageName, PubGrubVersion>);
impl Preferences {

View file

@ -14,7 +14,8 @@ use puffin_normalize::PackageName;
use puffin_traits::OnceMap;
use pypi_types::BaseUrl;
use crate::pubgrub::{PubGrubPackage, PubGrubReportFormatter, PubGrubVersion};
use crate::candidate_selector::CandidateSelector;
use crate::pubgrub::{PubGrubHints, PubGrubPackage, PubGrubReportFormatter, PubGrubVersion};
use crate::version_map::VersionMap;
#[derive(Error, Debug)]
@ -107,6 +108,7 @@ impl From<pubgrub::error::PubGrubError<PubGrubPackage, Range<PubGrubVersion>, In
ResolveError::NoSolution(NoSolutionError {
derivation_tree,
available_versions: FxHashMap::default(),
selector: None,
})
}
pubgrub::error::PubGrubError::SelfDependency { package, version } => {
@ -124,18 +126,29 @@ impl From<pubgrub::error::PubGrubError<PubGrubPackage, Range<PubGrubVersion>, In
pub struct NoSolutionError {
derivation_tree: DerivationTree<PubGrubPackage, Range<PubGrubVersion>>,
available_versions: FxHashMap<PubGrubPackage, Vec<PubGrubVersion>>,
selector: Option<CandidateSelector>,
}
impl std::error::Error for NoSolutionError {}
impl std::fmt::Display for NoSolutionError {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
// Write the derivation report.
let formatter = PubGrubReportFormatter {
available_versions: &self.available_versions,
};
let report =
DefaultStringReporter::report_with_formatter(&self.derivation_tree, &formatter);
write!(f, "{report}")
write!(f, "{report}")?;
// Include any additional hints.
if let Some(selector) = &self.selector {
for hint in PubGrubHints::from_derivation_tree(&self.derivation_tree, selector).iter() {
write!(f, "\n\n{hint}")?;
}
}
Ok(())
}
}
@ -143,15 +156,17 @@ impl NoSolutionError {
/// Update the available versions attached to the error using the given package version index.
///
/// Only packages used in the error's derivation tree will be retrieved.
pub(crate) fn update_available_versions(
#[must_use]
pub(crate) fn with_available_versions(
mut self,
package_versions: &OnceMap<PackageName, (IndexUrl, BaseUrl, VersionMap)>,
) -> Self {
let mut available_versions = FxHashMap::default();
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(
available_versions.insert(
package.clone(),
version_map
.iter()
@ -161,6 +176,14 @@ impl NoSolutionError {
}
}
}
self.available_versions = available_versions;
self
}
/// Update the candidate selector attached to the error.
#[must_use]
pub(crate) fn with_selector(mut self, selector: CandidateSelector) -> Self {
self.selector = Some(selector);
self
}
}

View file

@ -27,7 +27,7 @@ pub enum PreReleaseMode {
/// Like [`PreReleaseMode`], but with any additional information required to select a candidate,
/// like the set of direct dependencies.
#[derive(Debug)]
#[derive(Debug, Clone)]
pub(crate) enum PreReleaseStrategy {
/// Disallow all pre-release versions.
Disallow,

View file

@ -2,7 +2,7 @@ pub(crate) use crate::pubgrub::dependencies::PubGrubDependencies;
pub(crate) use crate::pubgrub::distribution::PubGrubDistribution;
pub(crate) use crate::pubgrub::package::PubGrubPackage;
pub(crate) use crate::pubgrub::priority::{PubGrubPriorities, PubGrubPriority};
pub(crate) use crate::pubgrub::report::PubGrubReportFormatter;
pub(crate) use crate::pubgrub::report::{PubGrubHints, PubGrubReportFormatter};
pub(crate) use crate::pubgrub::version::{PubGrubVersion, MIN_VERSION};
mod dependencies;

View file

@ -1,8 +1,12 @@
use crate::candidate_selector::CandidateSelector;
use crate::prerelease_mode::PreReleaseStrategy;
use colored::Colorize;
use derivative::Derivative;
use pubgrub::range::Range;
use pubgrub::report::{External, ReportFormatter};
use pubgrub::report::{DerivationTree, External, ReportFormatter};
use pubgrub::term::Term;
use pubgrub::type_aliases::Map;
use rustc_hash::FxHashMap;
use rustc_hash::{FxHashMap, FxHashSet};
use super::{PubGrubPackage, PubGrubVersion};
@ -12,21 +16,6 @@ pub(crate) struct PubGrubReportFormatter<'a> {
pub(crate) available_versions: &'a FxHashMap<PubGrubPackage, Vec<PubGrubVersion>>,
}
impl PubGrubReportFormatter<'_> {
fn simplify_set(
&self,
set: &Range<PubGrubVersion>,
package: &PubGrubPackage,
) -> Range<PubGrubVersion> {
set.simplify(
self.available_versions
.get(package)
.unwrap_or(&vec![])
.iter(),
)
}
}
impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFormatter<'_> {
type Output = String;
@ -139,3 +128,122 @@ impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFor
}
}
}
impl PubGrubReportFormatter<'_> {
fn simplify_set(
&self,
set: &Range<PubGrubVersion>,
package: &PubGrubPackage,
) -> Range<PubGrubVersion> {
set.simplify(
self.available_versions
.get(package)
.unwrap_or(&vec![])
.iter(),
)
}
}
/// A set of hints to help users resolve errors by providing additional context or modifying
/// their requirements.
#[derive(Debug, Default)]
pub(crate) struct PubGrubHints(FxHashSet<PubGrubHint>);
impl PubGrubHints {
/// Create a set of hints from a derivation tree.
pub(crate) fn from_derivation_tree(
derivation_tree: &DerivationTree<PubGrubPackage, Range<PubGrubVersion>>,
selector: &CandidateSelector,
) -> Self {
let mut hints = Self::default();
match derivation_tree {
DerivationTree::External(external) => match external {
External::NoVersions(package, set) => {
// Determine whether a pre-release marker appeared in the version requirements.
if set.bounds().any(PubGrubVersion::any_prerelease) {
// Determine whether pre-releases were allowed for this package.
let allowed_prerelease = match selector.prerelease_strategy() {
PreReleaseStrategy::Disallow => false,
PreReleaseStrategy::Allow => true,
PreReleaseStrategy::IfNecessary => false,
PreReleaseStrategy::Explicit(packages) => {
if let PubGrubPackage::Package(package, ..) = package {
packages.contains(package)
} else {
false
}
}
PreReleaseStrategy::IfNecessaryOrExplicit(packages) => {
if let PubGrubPackage::Package(package, ..) = package {
packages.contains(package)
} else {
false
}
}
};
if !allowed_prerelease {
hints.insert(PubGrubHint::NoVersionsWithPreRelease {
package: package.clone(),
range: set.clone(),
});
}
}
}
External::NotRoot(..) => {}
External::UnavailableDependencies(..) => {}
External::UnusableDependencies(..) => {}
External::FromDependencyOf(..) => {}
},
DerivationTree::Derived(derived) => {
hints.extend(Self::from_derivation_tree(&derived.cause1, selector));
hints.extend(Self::from_derivation_tree(&derived.cause2, selector));
}
}
hints
}
/// Iterate over the hints in the set.
pub(crate) fn iter(&self) -> impl Iterator<Item = &PubGrubHint> {
self.0.iter()
}
/// Insert a hint into the set.
fn insert(&mut self, hint: PubGrubHint) -> bool {
self.0.insert(hint)
}
/// Extend the set with another set of hints.
fn extend(&mut self, hints: Self) {
self.0.extend(hints.0);
}
}
#[derive(Derivative, Debug, Clone)]
#[derivative(Hash, PartialEq, Eq)]
pub(crate) enum PubGrubHint {
/// A package was requested with a pre-release marker, but pre-releases weren't enabled for
/// that package.
NoVersionsWithPreRelease {
package: PubGrubPackage,
#[derivative(PartialEq = "ignore", Hash = "ignore")]
range: Range<PubGrubVersion>,
},
}
impl std::fmt::Display for PubGrubHint {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
PubGrubHint::NoVersionsWithPreRelease { package, range } => {
write!(
f,
"{}{} {} was requested with a pre-release marker (e.g., {}), but pre-releases weren't enabled (try: `--prerelease=allow`)",
"hint".bold().cyan(),
":".bold(),
format!("{package}").bold(),
format!("{range}").bold(),
)
}
}
}
}

View file

@ -18,7 +18,7 @@ pub enum ResolutionMode {
/// Like [`ResolutionMode`], but with any additional information required to select a candidate,
/// like the set of direct dependencies.
#[derive(Debug)]
#[derive(Debug, Clone)]
pub(crate) enum ResolutionStrategy {
/// Resolve the highest compatible version of each package.
Highest,

View file

@ -290,7 +290,7 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
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))
ResolveError::NoSolution(err.with_available_versions(&self.index.packages).with_selector(self.selector.clone()))
} else {
err
}

View file

@ -636,6 +636,33 @@ async fn pylint_allow_explicit_prerelease_with_marker() -> Result<()> {
Ok(())
}
/// Resolve `msgraph-sdk==1.0.0`, which depends on `msgraph-core>=1.0.0a2`. The resolver should
/// fail with a pre-release-centric hint.
#[tokio::test]
async fn msgraph_sdk() -> Result<()> {
colored::control::set_override(false);
let manifest = Manifest::simple(vec![Requirement::from_str("msgraph-sdk==1.0.0").unwrap()]);
let options = ResolutionOptions::new(
ResolutionMode::default(),
PreReleaseMode::default(),
Some(*EXCLUDE_NEWER),
);
let err = resolve(manifest, options, &MARKERS_311, &TAGS_311)
.await
.unwrap_err();
insta::assert_display_snapshot!(err, @r###"
Because there is no version of msgraph-core available matching >=1.0.0a2 and msgraph-sdk==1.0.0 depends on msgraph-core>=1.0.0a2, msgraph-sdk==1.0.0 is forbidden.
And because root depends on msgraph-sdk==1.0.0, version solving failed.
hint: msgraph-core was requested with a pre-release marker (e.g., >=1.0.0a2), but pre-releases weren't enabled (try: `--prerelease=allow`)
"###);
Ok(())
}
static MARKERS_311: Lazy<MarkerEnvironment> = Lazy::new(|| {
MarkerEnvironment {
implementation_name: "cpython".to_string(),