From 0d9d4f9fcae985f97c41f20945322b4001e0eee0 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Thu, 16 Nov 2023 14:02:06 -0600 Subject: [PATCH] Add an `UnusableDependencies` incompatibility kind and use for conflicting versions (#424) Addresses https://github.com/astral-sh/puffin/issues/309#issuecomment-1792648969 Similar to #338 this throws an error when merging versions results in an empty set. Instead of propagating that error, we capture it and return a new dependency type of `Unusable`. Unusable dependencies are a new incompatibility kind which includes an arbitrary "reason" string that we present to the user. Adding a new incompatibility kind requires changes to the vendored pubgrub crate. We could use this same incompatibility kind for conflicting urls as in #284 which should allow the solver to backtrack to another valid version instead of failing (see #425). Unlike #383 this does not require changes to PubGrub's package mapping model. I think in the long run we'll want PubGrub to accept multiple versions per package to solve this specific issue, but we're interested in it being merged upstream first. This pull request is just using the issue as a simple case to explore adding a new incompatibility type. We may or may not be able convince them to add this new incompatibility type upstream. As discussed in https://github.com/pubgrub-rs/pubgrub/issues/152, we may want a more general incompatibility kind instead which can be used for arbitrary problems. An upstream pull request has been opened for discussion at https://github.com/pubgrub-rs/pubgrub/pull/153. Related to: - https://github.com/pubgrub-rs/pubgrub/issues/152 - #338 - #383 --------- Co-authored-by: konsti --- Cargo.lock | 2 +- Cargo.toml | 2 +- ...pile__compile_unsolvable_requirements.snap | 7 +++--- crates/puffin-resolver/src/error.rs | 3 +++ .../src/pubgrub/dependencies.rs | 17 +++++++++---- crates/puffin-resolver/src/pubgrub/report.rs | 24 +++++++++++++++++++ crates/puffin-resolver/src/resolver.rs | 16 ++++++++++++- 7 files changed, 61 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3ab2087eb..b20b5f7b6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2287,7 +2287,7 @@ dependencies = [ [[package]] name = "pubgrub" version = "0.2.1" -source = "git+https://github.com/zanieb/pubgrub?rev=46f1214fe6b7886709a35d8d2f2c0e1b56433b26#46f1214fe6b7886709a35d8d2f2c0e1b56433b26" +source = "git+https://github.com/zanieb/pubgrub?rev=efe34571a876831dacac1cbba3ce5bc358f2a6e7#efe34571a876831dacac1cbba3ce5bc358f2a6e7" dependencies = [ "indexmap 2.1.0", "log", diff --git a/Cargo.toml b/Cargo.toml index 2787fb9cb..20c6f93d3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -48,7 +48,7 @@ once_cell = { version = "1.18.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 = "46f1214fe6b7886709a35d8d2f2c0e1b56433b26" } +pubgrub = { git = "https://github.com/zanieb/pubgrub", rev = "efe34571a876831dacac1cbba3ce5bc358f2a6e7" } pyproject-toml = { version = "0.8.0" } rand = { version = "0.8.5" } rayon = { version = "1.8.0" } diff --git a/crates/puffin-cli/tests/snapshots/pip_compile__compile_unsolvable_requirements.snap b/crates/puffin-cli/tests/snapshots/pip_compile__compile_unsolvable_requirements.snap index 61a6e7f1e..db0d74a94 100644 --- a/crates/puffin-cli/tests/snapshots/pip_compile__compile_unsolvable_requirements.snap +++ b/crates/puffin-cli/tests/snapshots/pip_compile__compile_unsolvable_requirements.snap @@ -6,9 +6,9 @@ info: - pip-compile - pyproject.toml - "--cache-dir" - - /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpN531dN + - /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpJ9Dkj3 env: - VIRTUAL_ENV: /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmp99w9dK/.venv + VIRTUAL_ENV: /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpBOyFGn/.venv --- success: false exit_code: 1 @@ -16,5 +16,6 @@ exit_code: 1 ----- stderr ----- × No solution found when resolving dependencies: - ╰─▶ my-project depends on django∅ + ╰─▶ my-project dependencies are unusable: Conflicting versions for `django`: + `django==5.0b1` does not intersect with `django==5.0a1` diff --git a/crates/puffin-resolver/src/error.rs b/crates/puffin-resolver/src/error.rs index 48089c963..59f0c6d75 100644 --- a/crates/puffin-resolver/src/error.rs +++ b/crates/puffin-resolver/src/error.rs @@ -44,6 +44,9 @@ pub enum ResolveError { #[error("Conflicting URLs for package `{0}`: {1} and {2}")] ConflictingUrls(PackageName, String, String), + #[error("Conflicting versions for `{0}`: {1}")] + ConflictingVersions(String, String), + #[error("Package `{0}` attempted to resolve via URL: {1}. URL dependencies must be expressed as direct requirements or constraints. Consider adding `{0} @ {1}` to your dependencies or constraints file.")] DisallowedUrl(PackageName, Url), diff --git a/crates/puffin-resolver/src/pubgrub/dependencies.rs b/crates/puffin-resolver/src/pubgrub/dependencies.rs index f59247384..e9fe2f110 100644 --- a/crates/puffin-resolver/src/pubgrub/dependencies.rs +++ b/crates/puffin-resolver/src/pubgrub/dependencies.rs @@ -59,7 +59,7 @@ impl PubGrubDependencies { if let Some(entry) = dependencies.get_key_value(&package) { // Merge the versions. - let version = merge_versions(entry.1, &version); + let version = merge_versions(&package, entry.1, &version)?; // Merge the package. if let Some(package) = merge_package(entry.0, &package)? { @@ -107,7 +107,7 @@ impl PubGrubDependencies { if let Some(entry) = dependencies.get_key_value(&package) { // Merge the versions. - let version = merge_versions(entry.1, &version); + let version = merge_versions(&package, entry.1, &version)?; // Merge the package. if let Some(package) = merge_package(entry.0, &package)? { @@ -178,10 +178,19 @@ fn to_pubgrub( /// Merge two [`PubGrubVersion`] ranges. fn merge_versions( + package: &PubGrubPackage, left: &Range, right: &Range, -) -> Range { - left.intersection(right) +) -> Result, ResolveError> { + let result = left.intersection(right); + if result.is_empty() { + Err(ResolveError::ConflictingVersions( + package.to_string(), + format!("`{package}{left}` does not intersect with `{package}{right}`"), + )) + } else { + Ok(result) + } } /// Merge two [`PubGrubPackage`] instances. diff --git a/crates/puffin-resolver/src/pubgrub/report.rs b/crates/puffin-resolver/src/pubgrub/report.rs index 3565205a2..14f648916 100644 --- a/crates/puffin-resolver/src/pubgrub/report.rs +++ b/crates/puffin-resolver/src/pubgrub/report.rs @@ -345,6 +345,8 @@ enum PuffinExternal { NoVersions(PubGrubPackage, Range), /// Dependencies of the package are unavailable for versions in that set. UnavailableDependencies(PubGrubPackage, Range), + /// Dependencies of the package are unusable for versions in that set. + UnusableDependencies(PubGrubPackage, Range, Option), /// Incompatibility coming from the dependencies of a given package. FromDependencyOf( PubGrubPackage, @@ -362,6 +364,9 @@ impl PuffinExternal { External::UnavailableDependencies(p, vs) => { PuffinExternal::UnavailableDependencies(p, vs) } + External::UnusableDependencies(p, vs, reason) => { + PuffinExternal::UnusableDependencies(p, vs, reason) + } External::FromDependencyOf(p, vs, p_dep, vs_dep) => { PuffinExternal::FromDependencyOf(p, vs, p_dep, vs_dep) } @@ -395,6 +400,25 @@ impl fmt::Display for PuffinExternal { ) } } + Self::UnusableDependencies(package, set, reason) => { + if let Some(reason) = reason { + if matches!(package, PubGrubPackage::Root(_)) { + write!(f, "{package} dependencies are unusable: {reason}") + } else { + if set == &Range::full() { + write!(f, "dependencies of {package} are unusable: {reason}") + } else { + write!(f, "dependencies of {package}{set} are unusable: {reason}",) + } + } + } else { + if set == &Range::full() { + write!(f, "dependencies of {package} are unusable") + } else { + write!(f, "dependencies of {package}{set} are unusable") + } + } + } Self::FromDependencyOf(package, package_set, dependency, dependency_set) => { if package_set == &Range::full() && dependency_set == &Range::full() { write!(f, "{package} depends on {dependency}") diff --git a/crates/puffin-resolver/src/resolver.rs b/crates/puffin-resolver/src/resolver.rs index a30400d3c..9ce74acef 100644 --- a/crates/puffin-resolver/src/resolver.rs +++ b/crates/puffin-resolver/src/resolver.rs @@ -241,6 +241,14 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { )); continue; } + Dependencies::Unusable(reason) => { + state.add_incompatibility(Incompatibility::unusable_dependencies( + package.clone(), + version.clone(), + reason.clone(), + )); + continue; + } Dependencies::Known(constraints) if constraints.contains_key(package) => { return Err(PubGrubError::SelfDependency { package: package.clone(), @@ -457,7 +465,11 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { None, None, self.markers, - )?; + ); + if let Err(err @ ResolveError::ConflictingVersions(..)) = constraints { + return Ok(Dependencies::Unusable(Some(err.to_string()))); + } + let constraints = constraints?; for (package, version) in constraints.iter() { debug!("Adding direct dependency: {package}{version}"); @@ -862,6 +874,8 @@ impl<'a> FromIterator<&'a Url> for AllowedUrls { enum Dependencies { /// Package dependencies are unavailable. Unknown, + /// Package dependencies are not usable + Unusable(Option), /// Container for all available package versions. Known(DependencyConstraints>), }