Skip to content

Commit

Permalink
Cap Requires-Python comparisons at the patch version (#4150)
Browse files Browse the repository at this point in the history
## Summary

See the long comment inline. I think this is debatable but probably
right for now. The other options have their own problems, but there are
a few alternate ideas in the comment.

Closes #4132.
  • Loading branch information
charliermarsh authored Jun 8, 2024
1 parent ac1ddf5 commit eb239ff
Show file tree
Hide file tree
Showing 2 changed files with 378 additions and 4 deletions.
47 changes: 43 additions & 4 deletions crates/uv-resolver/src/requires_python.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,18 +99,57 @@ impl RequiresPython {
// `>=3.7`.
//
// That is: `version_lower` should be less than or equal to `requires_python_lower`.
//
// When comparing, we also limit the comparison to the release segment, ignoring
// pre-releases and such. This may or may not be correct.
//
// Imagine `target_lower` is `3.13.0b1`, and `requires_python_lower` is `3.13`.
// That would be fine, since we're saying we support `3.13.0` and later, and `target_lower`
// supports more than that.
//
// Next, imagine `requires_python_lower` is `3.13.0b1`, and `target_lower` is `3.13`.
// Technically, that would _not_ be fine, since we're saying we support `3.13.0b1` and
// later, but `target_lower` does not support that. For example, `target_lower` does not
// support `3.13.0b1`, `3.13.0rc1`, etc.
//
// In practice, this is most relevant for cases like: `requires_python = "==3.8.*"`, with
// `target = ">=3.8"`. In this case, `requires_python_lower` is actually `3.8.0.dev0`,
// because `==3.8.*` allows development and pre-release versions. So there are versions we
// want to support that aren't explicitly supported by `target`, which does _not_ include
// pre-releases.
//
// Since this is a fairly common `Requires-Python` specifier, we handle it pragmatically
// by only enforcing Python compatibility at the patch-release level.
//
// There are some potentially-bad outcomes here. For example, maybe the user _did_ request
// `>=3.13.0b1`. In that case, maybe we _shouldn't_ allow resolution that only support
// `3.13.0` and later, because we're saying we support the beta releases, but the dependency
// does not. But, it's debatable.
//
// If this scheme proves problematic, we could explore using different semantics when
// converting to PubGrub. For example, we could parse `>=3.8.*` as `>=3.8,<3.9`. But this
// too could be problematic. Imagine that the user requests `>=3.8.0b0`, and the target
// declares `==3.8.*`. In this case, we _do_ want to allow resolution, because the target
// is saying it supports all versions of `3.8`, including pre-releases. But under those
// modified parsing semantics, we would fail. (You could argue, though, that users declaring
// `==3.8.*` are not intending to support pre-releases, and so failing there is fine, but
// it's also incorrect in its own way.)
//
// Alternatively, we could vary the semantics depending on whether or not the user included
// a pre-release in their specifier, enforcing pre-release compatibility only if the user
// explicitly requested it.
match (target_lower, requires_python_lower) {
(Bound::Included(target_lower), Bound::Included(requires_python_lower)) => {
target_lower <= requires_python_lower
target_lower.release() <= requires_python_lower.release()
}
(Bound::Excluded(target_lower), Bound::Included(requires_python_lower)) => {
target_lower < requires_python_lower
target_lower.release() < requires_python_lower.release()
}
(Bound::Included(target_lower), Bound::Excluded(requires_python_lower)) => {
target_lower <= requires_python_lower
target_lower.release() <= requires_python_lower.release()
}
(Bound::Excluded(target_lower), Bound::Excluded(requires_python_lower)) => {
target_lower < requires_python_lower
target_lower.release() < requires_python_lower.release()
}
// If the dependency has no lower bound, then it supports all versions.
(Bound::Unbounded, _) => true,
Expand Down
Loading

0 comments on commit eb239ff

Please sign in to comment.