Add an UnusableDependencies incompatibility kind and use for conflicting versions
#424
Conversation
zanieb
force-pushed
the
zanie/new-incompatibility
branch
from
20984be
to
8e4f468
Compare
vendor/pubgrub/src/report.rs
Outdated
| set.union(&r), | ||
| reason, |
There was a problem hiding this comment.
👀 this is probably not quite right because we shouldn't merge the reasons here
There was a problem hiding this comment.
Can you say a bit more about this?
There was a problem hiding this comment.
I'm not sure it matters, but this merges the two incompatibilities by taking the union of their version ranges and one of the reasons but if the reasons are different we should not merge them because they are distinct incompatibilities. Or.. we could merge them and say "{reason} and {reason}" but that seems unclear later on since you lose the reason for each range.
There was a problem hiding this comment.
Or we could make the reason it's own enum with its own merge semantics, but that requires encoding the set of reasons in the PubGrub crate.
vendor/pubgrub/src/report.rs
Outdated
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems generic enough that it could feasibly be upstreamed 🤔
There was a problem hiding this comment.
zanieb
force-pushed
the
zanie/new-incompatibility
branch
from
ac2d621
to
ada2226
Compare
| @@ -104,6 +106,17 @@ impl<P: Package, VS: VersionSet> Incompatibility<P, VS> { | |||
| } | |||
| } | |||
|
|
|||
| /// Create an incompatibility to remember | |||
| /// that a package version is not selectable | |||
| /// because its dependencies are not usable. | |||
There was a problem hiding this comment.
The line breaks here make me read this like a haiku 😂
There was a problem hiding this comment.
Me too haha I just kept the existing style but it's funny
|
I don't understand why the numpy test is failing. I'm going to look into upstreaming these changes, but I don't think it needs to block us here. |
|
(fixed the merge conflict i created) |
|
An upstream pull request has been opened for discussion at pubgrub-rs/pubgrub#153. |
|
While I intend to continue work upstream, I'd like to merge this for my own sanity? Perhaps we should create a PubGrub fork instead so we can track these changes? |
|
👍 On setting up our own fork as discussed in our 1:1. |
| @@ -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" } | |||
Extends #424 with support for URL dependency incompatibilities. Requires changes to `miette` to prevent URLs from being word wrapped; accepted upstream in zkat/miette#321
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: