Improve error messages for incompatible direct requirements #338
Conversation
|
|
||
| pub fn is_empty(&self) -> bool { | ||
| return self.segments.is_empty(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Needed a way to check for emptiness...
There was a problem hiding this comment.
Note this tweaks the vendored pubgrub
crates/puffin-resolver/src/error.rs
Outdated
| #[error("Conflicting versions for package `{0}`: {1} is incompatible with {2}")] | ||
| ConflictingPackageVersions(PackageName, Range<PubGrubVersion>, Range<PubGrubVersion>), |
There was a problem hiding this comment.
Clippy is upset by the size of this variant. Suggestions?
There was a problem hiding this comment.
For e.g. error structs i ignore them
There was a problem hiding this comment.
I think then we also have to ignore in all the methods with #[allow(clippy::result_large_err)]? Seems annoying
There was a problem hiding this comment.
ugh that is significantly more annoying :/ in that case you might really want to box
|
There are some limitations here, like we will display the merged version on one side if there has been more than one previous version specifiers. |
zanieb
changed the title
| )) | ||
| } else { | ||
| Ok(version) | ||
| } |
There was a problem hiding this comment.
I think there's a problem with this approach which I also ran into with the "error if a package is requested under two different URLs" thing, which is that it doesn't let us backtrack out of the error state. So if the resolver ever sees a package with a version that has these conflicting requirements internally, it will immediately fail, rather than rejecting that version and attempting to backtrack to a working state.
Is that clear? I don't know how to solve this.
There was a problem hiding this comment.
That makes sense. Do we do try_from_requirements on requirements of dependencies i.e. indirect dependencies?
There was a problem hiding this comment.
(I tend to call those "transitive dependencies".)
There was a problem hiding this comment.
I guess the other issue here is that we won't print out a "tree" of incompatibilities, we'll just tell you which package failed, but not why that package was required. I'm wondering if we can add a new incompatibility kind to support this instead.
There was a problem hiding this comment.
Ah yes that's a good word for them. Okay... let me look for a better way to handle this then.
There was a problem hiding this comment.
Yeah it seems like it should be an incompatibility, I did not realize this was used for transitive dependencies.
zanieb
force-pushed
the
zanie/direct-incompat
branch
from
22edf48 to
7435236
Compare
…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 <[email protected]>
|
Using #424 instead |
Addresses #309 (comment)
Instead of running the resolver with a requirement with an empty range of versions, we error early with a message indicating the conflict that resulted in an empty set.
Example at https://github.com/astral-sh/puffin/pull/338/files#diff-d09432742ed63e9e76729f7fdb52772366e983f14e9551bc6f64e4329bdd20e7