make universal resolver fork only when markers are disjoint#4135
Merged
charliermarsh merged 3 commits intomainfrom Jun 7, 2024
Merged
make universal resolver fork only when markers are disjoint#4135charliermarsh merged 3 commits intomainfrom
charliermarsh merged 3 commits intomainfrom
Conversation
17dd0e1 to
7f97d14
Compare
7f97d14 to
fe4ee84
Compare
charliermarsh
approved these changes
Jun 7, 2024
Member
charliermarsh
left a comment
There was a problem hiding this comment.
This looks good to me! I'd like to see some tests eventually for combinations of forks and markers, like...
flask
flask[dotenv] ; python_version > '3.7'
flask[async] ; python_version <= '3.7'
| @@ -1,5 +1,7 @@ | |||
| //! Given a set of requirements, find a set of compatible packages. | |||
|
|
|||
| #![allow(warnings)] | |||
Member
There was a problem hiding this comment.
I removed it and fixed the lint issues, hope that's ok.
Member
Author
There was a problem hiding this comment.
Ug yeah. Sometimes I add this while writing new code (to silence lots of "unused code" warnings) and forget to remove it. Yes, it's okay!
| // *only* when the markers between two identically named packages are disjoint. | ||
| // So if there are, say, multiple `Extra` packages for the same name, they should | ||
| // all have the same markers (I believe), and thus they are not disjoint and thus | ||
| // they should not provoke a fork. |
Member
There was a problem hiding this comment.
We can probably remove these comments then.
Member
|
I'm gonna merge this so that I can build atop it if need be. |
…ypes This commit does not change any behavior, but instead changes `get_dependencies_forking` (and its caller) to use more structured data types describing the result of forking (or not). This commit is a precursor to forking based only on whether markers between conflicting dependency specifications are disjoint.
fe4ee84 to
12c08cb
Compare
The previous way we did forking was merely by looking at whether there were duplicate dependency specifications (with respect to package name), and if their version constraints were different, we forked. But this is incorrect (and was known to be, it was a shortcut). We only want to fork when we can prove that the resolution produced will never result in installing two different versions of the same package. That in turn can only happen when there are marker expressions attached to conflicting dependency specifications that never overlap. This does slightly change the test output, but only the ordering of packages. This might be because of the fork refactoring. We may want to investigate making the test output more stable, but we punt on that for now.
12c08cb to
ead960a
Compare
| PubGrubPackageInner::Package { name, marker, .. } | ||
| | PubGrubPackageInner::Extra { name, marker, .. } | ||
| | PubGrubPackageInner::Dev { name, marker, .. } => (name, marker.as_ref()), | ||
| PubGrubPackageInner::Marker { name, marker, .. } => (name, Some(marker)), |
Member
There was a problem hiding this comment.
I removed the ref name and ref marker here and now use marker.as_ref(), because the PubGrubPackageInner::Marker arm needs needs to return Option<&Marker> not &Option<Marker>.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The basic idea here is to make it so forking can only ever result in a resolution that, for a particular marker environment, will only install at most one version of a package. We can guarantee this by ensuring we only fork on conflicting dependency specifications only when their corresponding markers are completely disjoint. If they aren't, then resolution must find a single version of the package in the intersection of the two dependency specifications.
A test for this case has been added to packse here: astral-sh/packse#182. Previously, that test would result in a resolution with two different unconditional versions of the same package. With this change, resolution fails (as it should).
A commit-by-commit review should be helpful here, since the first commit is a refactor to make the second commit a bit more digestible.
Closes #3926