Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error message for dependencies with no versions available #342

Merged
merged 2 commits into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions crates/puffin-cli/src/commands/pip_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,8 @@ pub(crate) async fn pip_compile(
.with_reporter(ResolverReporter::from(printer));
let resolution = match resolver.resolve().await {
Err(puffin_resolver::ResolveError::PubGrub(pubgrub::error::PubGrubError::NoSolution(
mut derivation_tree,
derivation_tree,
))) => {
derivation_tree.collapse_no_versions();
#[allow(clippy::print_stderr)]
{
let report =
Expand Down
49 changes: 49 additions & 0 deletions crates/puffin-cli/tests/pip_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1479,3 +1479,52 @@ dependencies = ["django==5.0b1", "django==5.0a1"]

Ok(())
}

/// Compile requirements in a `pyproject.toml` file that cannot be resolved due to
/// a requirement with a version that is not available online.
#[test]
fn compile_unsolvable_requirements_version_not_available() -> Result<()> {
let temp_dir = assert_fs::TempDir::new()?;
let cache_dir = assert_fs::TempDir::new()?;
let venv = temp_dir.child(".venv");

Command::new(get_cargo_bin(BIN_NAME))
.arg("venv")
.arg(venv.as_os_str())
.arg("--cache-dir")
.arg(cache_dir.path())
.current_dir(&temp_dir)
.assert()
.success();
venv.assert(predicates::path::is_dir());

let pyproject_toml = temp_dir.child("pyproject.toml");
pyproject_toml.touch()?;
pyproject_toml.write_str(
r#"[build-system]
requires = ["setuptools", "wheel"]

[project]
name = "my-project"
dependencies = ["django==300.1.4"]
"#,
)?;

insta::with_settings!({
filters => vec![
(r"\d(ms|s)", "[TIME]"),
(r"# .* pip-compile", "# [BIN_PATH] pip-compile"),
(r"--cache-dir .*", "--cache-dir [CACHE_DIR]"),
]
}, {
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.arg("pip-compile")
.arg("pyproject.toml")
.arg("--cache-dir")
.arg(cache_dir.path())
.env("VIRTUAL_ENV", venv.as_os_str())
.current_dir(&temp_dir));
});

Ok(())
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
---
source: crates/puffin-cli/tests/pip_compile.rs
info:
program: puffin
args:
- pip-compile
- pyproject.toml
- "--cache-dir"
- /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmp2JLrkd
env:
VIRTUAL_ENV: /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmp0ZV4ob/.venv
---
success: false
exit_code: 1
----- stdout -----

----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because there is no version of django available matching ==300.1.4 and
my-project depends on django==300.1.4, my-project cannot be satisfied.

Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,16 @@ info:
- pip-compile
- requirements.in
- "--cache-dir"
- /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpleIayX
- /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpYAeXfn
env:
VIRTUAL_ENV: /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmp24qRXe/.venv
VIRTUAL_ENV: /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpf7hodD/.venv
---
success: false
exit_code: 1
----- stdout -----

----- stderr -----
× No solution found when resolving dependencies:
╰─▶ root depends on werkzeug==3.0.0
╰─▶ Because there is no version of werkzeug available matching ==3.0.0 and
root depends on werkzeug==3.0.0, root cannot be satisfied.

Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,17 @@ info:
- pip-compile
- requirements.in
- "--cache-dir"
- /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpfXoEZG
- /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpiccUSp
env:
VIRTUAL_ENV: /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpJkukgJ/.venv
VIRTUAL_ENV: /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpCLX1o7/.venv
---
success: false
exit_code: 1
----- stdout -----

----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because flask ==3.0.0 depends on werkzeug >=3.0.0 and root ==0a0.dev0
depends on flask ==3.0.0, root ==0a0.dev0 is forbidden.
╰─▶ Because there is no version of werkzeug available matching >=3.0.0 and
flask==3.0.0 depends on werkzeug>=3.0.0, flask ==3.0.0 is forbidden.
And because root depends on flask==3.0.0, root cannot be satisfied.

109 changes: 64 additions & 45 deletions crates/puffin-resolver/src/pubgrub/report.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
use std::fmt;

use pubgrub::package::Package;
use pubgrub::range::Range;
use pubgrub::report::{DerivationTree, Derived, External, Reporter};
use pubgrub::term::Term;
use pubgrub::type_aliases::Map;
use pubgrub::version_set::VersionSet;

use super::{PubGrubPackage, PubGrubVersion};

Expand All @@ -31,7 +29,7 @@ impl ResolutionFailureReporter {
}
}

fn build_recursive<P: Package, VS: VersionSet>(&mut self, derived: &Derived<P, VS>) {
fn build_recursive(&mut self, derived: &Derived<PubGrubPackage, Range<PubGrubVersion>>) {
self.build_recursive_helper(derived);
if let Some(id) = derived.shared_id {
if self.shared_with_ref.get(&id).is_none() {
Expand All @@ -41,7 +39,7 @@ impl ResolutionFailureReporter {
};
}

fn build_recursive_helper<P: Package, VS: VersionSet>(&mut self, current: &Derived<P, VS>) {
fn build_recursive_helper(&mut self, current: &Derived<PubGrubPackage, Range<PubGrubVersion>>) {
match (&*current.cause1, &*current.cause2) {
(DerivationTree::External(external1), DerivationTree::External(external2)) => {
// Simplest case, we just combine two external incompatibilities.
Expand Down Expand Up @@ -118,11 +116,11 @@ impl ResolutionFailureReporter {
///
/// The result will depend on the fact that the derived incompatibility
/// has already been explained or not.
fn report_one_each<P: Package, VS: VersionSet>(
fn report_one_each(
&mut self,
derived: &Derived<P, VS>,
external: &External<P, VS>,
current_terms: &Map<P, Term<VS>>,
derived: &Derived<PubGrubPackage, Range<PubGrubVersion>>,
external: &External<PubGrubPackage, Range<PubGrubVersion>>,
current_terms: &Map<PubGrubPackage, Term<Range<PubGrubVersion>>>,
) {
match self.line_ref_of(derived.shared_id) {
Some(ref_id) => self.lines.push(Self::explain_ref_and_external(
Expand All @@ -136,11 +134,11 @@ impl ResolutionFailureReporter {
}

/// Report one derived (without a line ref yet) and one external.
fn report_recurse_one_each<P: Package, VS: VersionSet>(
fn report_recurse_one_each(
&mut self,
derived: &Derived<P, VS>,
external: &External<P, VS>,
current_terms: &Map<P, Term<VS>>,
derived: &Derived<PubGrubPackage, Range<PubGrubVersion>>,
external: &External<PubGrubPackage, Range<PubGrubVersion>>,
current_terms: &Map<PubGrubPackage, Term<Range<PubGrubVersion>>>,
) {
match (&*derived.cause1, &*derived.cause2) {
// If the derived cause has itself one external prior cause,
Expand Down Expand Up @@ -174,27 +172,27 @@ impl ResolutionFailureReporter {
// String explanations #####################################################

/// Simplest case, we just combine two external incompatibilities.
fn explain_both_external<P: Package, VS: VersionSet>(
external1: &External<P, VS>,
external2: &External<P, VS>,
current_terms: &Map<P, Term<VS>>,
fn explain_both_external(
external1: &External<PubGrubPackage, Range<PubGrubVersion>>,
external2: &External<PubGrubPackage, Range<PubGrubVersion>>,
current_terms: &Map<PubGrubPackage, Term<Range<PubGrubVersion>>>,
) -> String {
// TODO: order should be chosen to make it more logical.
format!(
"Because {} and {}, {}.",
external1,
external2,
PuffinExternal::from_pubgrub(external1.clone()),
PuffinExternal::from_pubgrub(external2.clone()),
Self::string_terms(current_terms)
)
}

/// Both causes have already been explained so we use their refs.
fn explain_both_ref<P: Package, VS: VersionSet>(
fn explain_both_ref(
ref_id1: usize,
derived1: &Derived<P, VS>,
derived1: &Derived<PubGrubPackage, Range<PubGrubVersion>>,
ref_id2: usize,
derived2: &Derived<P, VS>,
current_terms: &Map<P, Term<VS>>,
derived2: &Derived<PubGrubPackage, Range<PubGrubVersion>>,
current_terms: &Map<PubGrubPackage, Term<Range<PubGrubVersion>>>,
) -> String {
// TODO: order should be chosen to make it more logical.
format!(
Expand All @@ -210,39 +208,39 @@ impl ResolutionFailureReporter {
/// One cause is derived (already explained so one-line),
/// the other is a one-line external cause,
/// and finally we conclude with the current incompatibility.
fn explain_ref_and_external<P: Package, VS: VersionSet>(
fn explain_ref_and_external(
ref_id: usize,
derived: &Derived<P, VS>,
external: &External<P, VS>,
current_terms: &Map<P, Term<VS>>,
derived: &Derived<PubGrubPackage, Range<PubGrubVersion>>,
external: &External<PubGrubPackage, Range<PubGrubVersion>>,
current_terms: &Map<PubGrubPackage, Term<Range<PubGrubVersion>>>,
) -> String {
// TODO: order should be chosen to make it more logical.
format!(
"Because {} ({}) and {}, {}.",
Self::string_terms(&derived.terms),
ref_id,
external,
PuffinExternal::from_pubgrub(external.clone()),
Self::string_terms(current_terms)
)
}

/// Add an external cause to the chain of explanations.
fn and_explain_external<P: Package, VS: VersionSet>(
external: &External<P, VS>,
current_terms: &Map<P, Term<VS>>,
fn and_explain_external(
external: &External<PubGrubPackage, Range<PubGrubVersion>>,
current_terms: &Map<PubGrubPackage, Term<Range<PubGrubVersion>>>,
) -> String {
format!(
"And because {}, {}.",
external,
PuffinExternal::from_pubgrub(external.clone()),
Self::string_terms(current_terms)
)
}

/// Add an already explained incompat to the chain of explanations.
fn and_explain_ref<P: Package, VS: VersionSet>(
fn and_explain_ref(
ref_id: usize,
derived: &Derived<P, VS>,
current_terms: &Map<P, Term<VS>>,
derived: &Derived<PubGrubPackage, Range<PubGrubVersion>>,
current_terms: &Map<PubGrubPackage, Term<Range<PubGrubVersion>>>,
) -> String {
format!(
"And because {} ({}), {}.",
Expand All @@ -253,10 +251,10 @@ impl ResolutionFailureReporter {
}

/// Add an already explained incompat to the chain of explanations.
fn and_explain_prior_and_external<P: Package, VS: VersionSet>(
prior_external: &External<P, VS>,
external: &External<P, VS>,
current_terms: &Map<P, Term<VS>>,
fn and_explain_prior_and_external(
prior_external: &External<PubGrubPackage, Range<PubGrubVersion>>,
external: &External<PubGrubPackage, Range<PubGrubVersion>>,
current_terms: &Map<PubGrubPackage, Term<Range<PubGrubVersion>>>,
) -> String {
format!(
"And because {} and {}, {}.",
Expand All @@ -267,18 +265,36 @@ impl ResolutionFailureReporter {
}

/// Try to print terms of an incompatibility in a human-readable way.
pub fn string_terms<P: Package, VS: VersionSet>(terms: &Map<P, Term<VS>>) -> String {
pub fn string_terms(terms: &Map<PubGrubPackage, Term<Range<PubGrubVersion>>>) -> String {
let terms_vec: Vec<_> = terms.iter().collect();
match terms_vec.as_slice() {
[] => "version solving failed".into(),
// TODO: special case when that unique package is root.
[(package, Term::Positive(range))] => format!("{package} {range} is forbidden"),
[(package, Term::Negative(range))] => format!("{package} {range} is mandatory"),
[(package @ PubGrubPackage::Root(_), _)] => {
format!("{package} cannot be satisfied")
}
[(package @ PubGrubPackage::Package(..), Term::Positive(range))] => {
format!("{package} {range} is forbidden")
}
[(package @ PubGrubPackage::Package(..), Term::Negative(range))] => {
format!("{package} {range} is mandatory")
}
[(p1, Term::Positive(r1)), (p2, Term::Negative(r2))] => {
External::FromDependencyOf(p1, r1.clone(), p2, r2.clone()).to_string()
PuffinExternal::FromDependencyOf(
(*p1).clone(),
r1.clone(),
(*p2).clone(),
r2.clone(),
)
.to_string()
}
[(p1, Term::Negative(r1)), (p2, Term::Positive(r2))] => {
External::FromDependencyOf(p2, r2.clone(), p1, r1.clone()).to_string()
PuffinExternal::FromDependencyOf(
(*p2).clone(),
r2.clone(),
(*p1).clone(),
r1.clone(),
)
.to_string()
}
slice => {
let str_terms: Vec<_> = slice.iter().map(|(p, t)| format!("{p} {t}")).collect();
Expand Down Expand Up @@ -366,7 +382,10 @@ impl fmt::Display for PuffinExternal {
if set == &Range::full() {
write!(f, "there is no available version for {package}")
} else {
write!(f, "there is no version of {package} in {set}")
write!(
f,
"there is no version of {package} available matching {set}"
)
}
}
Self::UnavailableDependencies(package, set) => {
Expand Down