uv-resolver: add new UniversalMarker type
#9334
Merged
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
BurntSushi
force-pushed
the
ag/add-universal-marker
branch
from
b396cf4 to
1be6b78
Compare
This test demonstrates the difference between `extra != "foo"` and `sys_platform != "foo"`. I wrote this test down to test the extra simplification logic was correct. And I also wanted to test whether we could somehow hackily encode `group` (as opposed to just `extra`) logic into marker expressions by reusing another field. But I don't think we can.
This filtering is now redundant, since forking now avoids these degenerate cases by construction. The main change to forking that enables skipping over "always false" forks is that forking now starts with the parent's markers instead of starting with MarkerTree::TRUE and trying to combine them with the parent's markers later. This in turn leads to skipping over anything that "can't" happen when combined with the parents markers. So we never hit the case of generating a fork that, when combined with the parent's markers, results in a marker that is always false. We just avoid it in the first place.
This doesn't change any behavior. My guess is that this code was a casualty of refactoring. But basically, it was doing redundant case analysis and iterating over all resolutions (even though it's in the branch that can only occur when there is only one resolution).
This was completely unused. I noticed this while trying to read and understand the code. It's unclear when or how this happened.
This doesn't change any behavior. But this makes it a bit clearer in the code that `uv pip compile` does not support specifying conflicts. Indeed, we always pass an empty set of conflicts to the resolver. This is because there is no way to encode the conditional logic of conflicts into the `requirements.txt` format. This is unlike markers.
This effectively combines a PEP 508 marker and an as-yet-specified marker for expressing conflicts among extras and groups. This just defines the type and threads it through most of the various points in the code that previously used `MarkerTree` only. Some parts do still continue to use `MarkerTree` specifically, e.g., when dealing with non-universal resolution or exporting to `requirements.txt`. This doesn't change any behavior.
BurntSushi
force-pushed
the
ag/add-universal-marker
branch
from
1be6b78 to
4645037
Compare
|
@charliermarsh Since this is just a refactoring PR, I'm going to bring this in. If you leave any feedback, I'm happy to follow-up! |
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.
This effectively combines a PEP 508 marker and an as-yet-specified
marker for expressing conflicts among extras and groups.
This just defines the type and threads it through most of the various
points in the code that previously used
MarkerTreeonly. Some partsdo still continue to use
MarkerTreespecifically, e.g., when dealingwith non-universal resolution or exporting to
requirements.txt.This doesn't change any behavior.
There are some other smaller commits in this PR will smaller changes,
so I'd recommend reviewing commit-by-commit.
Ref #9289