Skip to content

Commit

Permalink
Add an UnusableDependencies incompatibility kind and use for confli…
Browse files Browse the repository at this point in the history
…cting versions (#424)

Addresses
#309 (comment)

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
pubgrub-rs/pubgrub#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
pubgrub-rs/pubgrub#153.

Related to:
- pubgrub-rs/pubgrub#152
- #338 
- #383

---------

Co-authored-by: konsti <konstin@mailbox.org>
  • Loading branch information
zanieb and konstin committed Nov 16, 2023
1 parent 832058d commit 0d9d4f9
Show file tree
Hide file tree
Showing 7 changed files with 61 additions and 10 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 @@ -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" }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,16 @@ 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
----- stdout -----

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

3 changes: 3 additions & 0 deletions crates/puffin-resolver/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),

Expand Down
17 changes: 13 additions & 4 deletions crates/puffin-resolver/src/pubgrub/dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)? {
Expand Down Expand Up @@ -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)? {
Expand Down Expand Up @@ -178,10 +178,19 @@ fn to_pubgrub(

/// Merge two [`PubGrubVersion`] ranges.
fn merge_versions(
package: &PubGrubPackage,
left: &Range<PubGrubVersion>,
right: &Range<PubGrubVersion>,
) -> Range<PubGrubVersion> {
left.intersection(right)
) -> Result<Range<PubGrubVersion>, 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.
Expand Down
24 changes: 24 additions & 0 deletions crates/puffin-resolver/src/pubgrub/report.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,8 @@ enum PuffinExternal {
NoVersions(PubGrubPackage, Range<PubGrubVersion>),
/// Dependencies of the package are unavailable for versions in that set.
UnavailableDependencies(PubGrubPackage, Range<PubGrubVersion>),
/// Dependencies of the package are unusable for versions in that set.
UnusableDependencies(PubGrubPackage, Range<PubGrubVersion>, Option<String>),
/// Incompatibility coming from the dependencies of a given package.
FromDependencyOf(
PubGrubPackage,
Expand All @@ -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)
}
Expand Down Expand Up @@ -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}")
Expand Down
16 changes: 15 additions & 1 deletion crates/puffin-resolver/src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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}");
Expand Down Expand Up @@ -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<String>),
/// Container for all available package versions.
Known(DependencyConstraints<PubGrubPackage, Range<PubGrubVersion>>),
}

0 comments on commit 0d9d4f9

Please sign in to comment.