Improve handling of dependency conflicts resulting in empty version ranges #383
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…equirements to be passed to solver This allows the solver to provide better error messages since we do not merge versions and drop information
Instead of using `PubGrubPackage::Package(.., Option<Url>)`
….. Option<Url>)` This reverts commit a510a2f in favor of a dummy `Url` package
…dencies for the root package Attaching dummy dependencies to child packages does not help us because child packages with distinct urls are not considered unique by pubgrub
zanieb
added a commit
that referenced
this pull request
Nov 16, 2023
…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>
|
Still need to investigate support for this in PubGrub... |
|
As demonstrated by this appears to be resolved on |
|
I've revived the original PubGrub change in https://github.com/zanieb/pubgrub/tree/charlie/into. |
charliermarsh
added a commit
that referenced
this pull request
Feb 22, 2024
## Summary This revives a PR from long ago (#383 and astral-sh/pubgrub#24) that modifies how we deal with dependencies that are declared multiple times within a single package. To quote from the originating PR: > Uses an experimental pubgrub branch (#370) that allows us to handle multiple version ranges for a single dependency to the solver which results in better error messages because the derivation tree contains all of the relevant versions. Previously, the version ranges were merged (by us) in the resolver before handing them to pubgrub since only one range could be provided per package. Since we don't merge the versions anymore, we no longer give the solver an empty range for conflicting requirements; instead the solver comes to that conclusion from the provided versions. You can see the improved error message for direct dependencies in [this snapshot](https://github.com/astral-sh/puffin/pull/383/files#diff-a0437f2c20cde5e2f15199a3bf81a102b92580063268417847ec9c793a115bd0). The main issue with that PR was around its handling of URL dependencies, so this PR _also_ refactors how we handle those. Previously, we stored URL dependencies on `PubGrubPackage`, but they were omitted from the hash and equality implementations of `PubGrubPackage`. This led to some really careful codepaths wherein we had to ensure that we always visited URLs before non-URL packages, so that the URL-inclusive versions were included in any hashmaps, etc. I considered preserving this approach, but it would require us to rely on lots of internal details of PubGrub (since we'd now be relying on PubGrub to merge those packages in the "right" order). So, instead, we now _always_ set the URL on a given package, whenever that package was _given_ a URL upfront. I think this is easier to reason about: if the user provided a URL for `flask`, then we should just always add the URL for `flask`. If we see some other URL for `flask`, we error, like before. If we see some unknown URL for `flask`, we error, like before. Closes #1522. Closes #1821. Closes #1615.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uses an experimental pubgrub branch (#370) that allows us to handle multiple version ranges for a single dependency to the solver which results in better error messages because the derivation tree contains all of the relevant versions. Previously, the version ranges were merged (by us) in the resolver before handing them to pubgrub since only one range could be provided per package. Since we don't merge the versions anymore, we no longer give the solver an empty range for conflicting requirements; instead the solver comes to that conclusion from the provided versions. You can see the improved error message for direct dependencies in this snapshot.
Since we are no longer merging packages, we do not raise an error on conflicting urls in dependencies anymore. Our previous solution was kind of a hack, so I'm exploring better solutions such as:
Option<Url>fromPubGrubPackage::Packagein favor of a separatePubGrubPackage::UrlPackagevariant e.g. a510a2fPubGrubPackage::Urlvariant that is added as a dummy dependency with fake conflicting version numbers for each package name and url combination e.g. 4343f66...08509a5The crux of the problem is that we must have pubgrub consider packages with the same name (specified with or without a url) as the same package with possibly compatible or incompatible versions. However, we do not have a good way to treat different urls as distinct versions while also allowing a url to satisfy versions ranges required by other dependencies relying on the package.
Supersedes #338
Requires #370