Skip to content

Commit

Permalink
Improve error messaging when a dependency is not found (#1241)
Browse files Browse the repository at this point in the history
Previously, whenever we encountered a missing package we would throw an
error without information about why the package was requested. This
meant that if a transitive dependency required a missing package, the
user would have no idea why it was even selected. Here, we track
`NotFound` and `NoIndex` errors as `NoVersions` incompatibilities with
an attached reason. Improves our test coverage for `--no-index` without
`--find-links`.

The
[snapshots](https://github.com/astral-sh/puffin/pull/1241/files#diff-3eea1658f165476252f1f061d0aa9f915aabdceafac21611cdf45019447f60ec)
show a nice improvement.

I think this will also enable backtracking to another version if some
version of transitive dependency has a missing dependency. I'll write a
scenario for that next.

Requires astral-sh/pubgrub#22
  • Loading branch information
zanieb authored Feb 5, 2024
1 parent be9125b commit d090acf
Show file tree
Hide file tree
Showing 13 changed files with 351 additions and 74 deletions.
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ owo-colors = { version = "4.0.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 = "86447f2a391c0aa25c56acb3a8b6ce10aac305b6" }
pubgrub = { git = "https://github.com/zanieb/pubgrub", rev = "1b150cdbd1e6f93b1f465de9d08f499660d7f708" }
pyo3 = { version = "0.20.2" }
pyo3-log = { version = "0.9.0"}
pyproject-toml = { version = "0.8.1" }
Expand Down
4 changes: 4 additions & 0 deletions crates/puffin-client/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ impl Error {
*self.kind
}

pub fn kind(&self) -> &ErrorKind {
&self.kind
}

pub(crate) fn from_json_err(err: serde_json::Error, url: Url) -> Self {
ErrorKind::BadJson { source: err, url }.into()
}
Expand Down
22 changes: 12 additions & 10 deletions crates/puffin-resolver/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use puffin_normalize::PackageName;
use crate::candidate_selector::CandidateSelector;
use crate::pubgrub::{PubGrubPackage, PubGrubPython, PubGrubReportFormatter};
use crate::python_requirement::PythonRequirement;
use crate::version_map::VersionMap;
use crate::resolver::VersionsResponse;

#[derive(Debug, thiserror::Error)]
pub enum ResolveError {
Expand Down Expand Up @@ -168,7 +168,7 @@ impl NoSolutionError {
mut self,
python_requirement: &PythonRequirement,
visited: &DashSet<PackageName>,
package_versions: &OnceMap<PackageName, VersionMap>,
package_versions: &OnceMap<PackageName, VersionsResponse>,
) -> Self {
let mut available_versions = IndexMap::default();
for package in self.derivation_tree.packages() {
Expand All @@ -192,14 +192,16 @@ impl NoSolutionError {
// these packages, but it's non-deterministic, and omitting them ensures that
// we represent the state of the resolver at the time of failure.
if visited.contains(name) {
if let Some(version_map) = package_versions.get(name) {
available_versions.insert(
package.clone(),
version_map
.iter()
.map(|(version, _)| version.clone())
.collect(),
);
if let Some(response) = package_versions.get(name) {
if let VersionsResponse::Found(ref version_map) = *response {
available_versions.insert(
package.clone(),
version_map
.iter()
.map(|(version, _)| version.clone())
.collect(),
);
}
}
}
}
Expand Down
15 changes: 13 additions & 2 deletions crates/puffin-resolver/src/pubgrub/report.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ impl ReportFormatter<PubGrubPackage, Range<Version>> for PubGrubReportFormatter<
External::NotRoot(package, version) => {
format!("we are solving dependencies of {package} {version}")
}
External::NoVersions(package, set) => {
External::NoVersions(package, set, reason) => {
if matches!(package, PubGrubPackage::Python(_)) {
if let Some(python) = self.python_requirement {
if python.target() == python.installed() {
Expand Down Expand Up @@ -75,6 +75,17 @@ impl ReportFormatter<PubGrubPackage, Range<Version>> for PubGrubReportFormatter<
);
}
let set = self.simplify_set(set, package);

// Check for a reason
if let Some(reason) = reason {
let formatted = if set.as_ref() == &Range::full() {
format!("{package} {reason}")
} else {
format!("{package}{set} {reason}")
};
return formatted;
}

if set.as_ref() == &Range::full() {
format!("there are no versions of {package}")
} else if set.as_singleton().is_some() {
Expand Down Expand Up @@ -353,7 +364,7 @@ impl PubGrubReportFormatter<'_> {
let mut hints = IndexSet::default();
match derivation_tree {
DerivationTree::External(external) => match external {
External::NoVersions(package, set) => {
External::NoVersions(package, set, _) => {
if set.bounds().any(Version::any_prerelease) {
// A pre-release marker appeared in the version requirements.
if !allowed_prerelease(package, selector) {
Expand Down
34 changes: 20 additions & 14 deletions crates/puffin-resolver/src/resolution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use petgraph::Direction;
use pubgrub::range::Range;
use pubgrub::solver::{Kind, State};
use pubgrub::type_aliases::SelectedDependencies;

use rustc_hash::FxHashMap;
use url::Url;

Expand All @@ -20,7 +21,8 @@ use pypi_types::{Hashes, Metadata21};

use crate::pins::FilePins;
use crate::pubgrub::{PubGrubDistribution, PubGrubPackage, PubGrubPriority};
use crate::version_map::VersionMap;
use crate::resolver::VersionsResponse;

use crate::ResolveError;

/// A complete resolution graph in which every node represents a pinned package and every edge
Expand All @@ -42,7 +44,7 @@ impl ResolutionGraph {
pub(crate) fn from_state(
selection: &SelectedDependencies<PubGrubPackage, Version>,
pins: &FilePins,
packages: &OnceMap<PackageName, VersionMap>,
packages: &OnceMap<PackageName, VersionsResponse>,
distributions: &OnceMap<PackageId, Metadata21>,
redirects: &DashMap<Url, Url>,
state: &State<PubGrubPackage, Range<Version>, PubGrubPriority>,
Expand All @@ -68,12 +70,14 @@ impl ResolutionGraph {
.clone();

// Add its hashes to the index.
if let Some(version_map) = packages.get(package_name) {
hashes.insert(package_name.clone(), {
let mut hashes = version_map.hashes(version);
hashes.sort_unstable();
hashes
});
if let Some(versions_response) = packages.get(package_name) {
if let VersionsResponse::Found(ref version_map) = *versions_response {
hashes.insert(package_name.clone(), {
let mut hashes = version_map.hashes(version);
hashes.sort_unstable();
hashes
});
}
}

// Add the distribution to the graph.
Expand All @@ -93,12 +97,14 @@ impl ResolutionGraph {
};

// Add its hashes to the index.
if let Some(version_map) = packages.get(package_name) {
hashes.insert(package_name.clone(), {
let mut hashes = version_map.hashes(version);
hashes.sort_unstable();
hashes
});
if let Some(versions_response) = packages.get(package_name) {
if let VersionsResponse::Found(ref version_map) = *versions_response {
hashes.insert(package_name.clone(), {
let mut hashes = version_map.hashes(version);
hashes.sort_unstable();
hashes
});
}
}

// Add the distribution to the graph.
Expand Down
4 changes: 2 additions & 2 deletions crates/puffin-resolver/src/resolver/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ use once_map::OnceMap;
use puffin_normalize::PackageName;
use pypi_types::Metadata21;

use crate::version_map::VersionMap;
use super::provider::VersionsResponse;

/// In-memory index of package metadata.
#[derive(Default)]
pub struct InMemoryIndex {
/// A map from package name to the metadata for that package and the index where the metadata
/// came from.
pub(crate) packages: OnceMap<PackageName, VersionMap>,
pub(crate) packages: OnceMap<PackageName, VersionsResponse>,

/// A map from package ID to metadata for that distribution.
pub(crate) distributions: OnceMap<PackageId, Metadata21>,
Expand Down
Loading

0 comments on commit d090acf

Please sign in to comment.