Improve error message for dependencies with no versions available (#342)

Partially addresses https://github.com/astral-sh/puffin/issues/310 Addresses case at https://github.com/astral-sh/puffin/issues/309#issuecomment-1793541558 Follow-up to #300 ensuring `PuffinExternal` is used consistently when formatting messages Example at https://github.com/astral-sh/puffin/pull/342/files#diff-5c74a74ef34ef1d6e7453de8d2d19134813156e8b6a657e6b5ed71fda5a3a870
2025-07-07 13:25:00 +00:00 · 2023-11-06 14:04:29 -06:00 · 2023-11-06 14:04:29 -06:00 · b0720ea5b2
commit b0720ea5b2
parent 1748cfb522
6 changed files with 144 additions and 54 deletions
--- a/crates/puffin-cli/src/commands/pip_compile.rs
+++ b/crates/puffin-cli/src/commands/pip_compile.rs
@ -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 =
--- a/crates/puffin-cli/tests/pip_compile.rs
+++ b/crates/puffin-cli/tests/pip_compile.rs
@ -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(())
+}
--- a/crates/puffin-cli/tests/snapshots/pip_compile__compile_unsolvable_requirements_version_not_available.snap
+++ b/crates/puffin-cli/tests/snapshots/pip_compile__compile_unsolvable_requirements_version_not_available.snap
@ -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.
+
--- a/crates/puffin-cli/tests/snapshots/pip_compile__conflicting_direct_url_dependency.snap
+++ b/crates/puffin-cli/tests/snapshots/pip_compile__conflicting_direct_url_dependency.snap
@ -6,9 +6,9 @@ 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
@ -16,5 +16,6 @@ exit_code: 1

 ----- 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.

--- a/crates/puffin-cli/tests/snapshots/pip_compile__conflicting_transitive_url_dependency.snap
+++ b/crates/puffin-cli/tests/snapshots/pip_compile__conflicting_transitive_url_dependency.snap
@ -6,9 +6,9 @@ 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
@ -16,6 +16,7 @@ exit_code: 1

 ----- 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.

--- a/crates/puffin-resolver/src/pubgrub/report.rs
+++ b/crates/puffin-resolver/src/pubgrub/report.rs
@ -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};

@ -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() {
@ -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.
@ -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(
@ -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,
@ -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!(
@ -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 {} ({}), {}.",
@ -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 {}, {}.",
@ -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();
@ -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) => {