Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fork resolution when Python requirement is narrowed #4712

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions crates/pep440-rs/src/version_specifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ impl VersionSpecifier {
}

/// Returns a version specifier representing the given lower bound.
fn from_lower_bound(bound: &Bound<Version>) -> Option<VersionSpecifier> {
pub fn from_lower_bound(bound: &Bound<Version>) -> Option<VersionSpecifier> {
match bound {
Bound::Included(version) => Some(
VersionSpecifier::from_version(Operator::GreaterThanEqual, version.clone())
Expand All @@ -450,7 +450,7 @@ impl VersionSpecifier {
}

/// Returns a version specifier representing the given upper bound.
fn from_upper_bound(bound: &Bound<Version>) -> Option<VersionSpecifier> {
pub fn from_upper_bound(bound: &Bound<Version>) -> Option<VersionSpecifier> {
match bound {
Bound::Included(version) => Some(
VersionSpecifier::from_version(Operator::LessThanEqual, version.clone()).unwrap(),
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-resolver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ pub use preferences::{Preference, PreferenceError, Preferences};
pub use prerelease_mode::PreReleaseMode;
pub use pubgrub::{PubGrubSpecifier, PubGrubSpecifierError};
pub use python_requirement::PythonRequirement;
pub use requires_python::{RequiresPython, RequiresPythonError};
pub use requires_python::{RequiresPython, RequiresPythonBound, RequiresPythonError};
pub use resolution::{AnnotationStyle, DisplayResolutionGraph, ResolutionGraph};
pub use resolution_mode::ResolutionMode;
pub use resolver::{
Expand Down
40 changes: 39 additions & 1 deletion crates/uv-resolver/src/marker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,16 @@ use std::collections::HashMap;
use std::ops::Bound::{self, *};
use std::ops::RangeBounds;

use pubgrub::range::Range as PubGrubRange;

use pep440_rs::{Operator, Version, VersionSpecifier};
use pep508_rs::{
ExtraName, ExtraOperator, MarkerExpression, MarkerOperator, MarkerTree, MarkerValueString,
MarkerValueVersion,
};

use crate::pubgrub::PubGrubSpecifier;
use pubgrub::range::Range as PubGrubRange;
use crate::RequiresPythonBound;

/// Returns `true` if there is no environment in which both marker trees can both apply, i.e.
/// the expression `first and second` is always false.
Expand Down Expand Up @@ -80,6 +82,42 @@ fn string_is_disjoint(this: &MarkerExpression, other: &MarkerExpression) -> bool
true
}

/// Returns the minimum Python version that can satisfy the [`MarkerTree`], if it's constrained.
pub(crate) fn requires_python_marker(tree: &MarkerTree) -> Option<RequiresPythonBound> {
match tree {
MarkerTree::Expression(MarkerExpression::Version {
key: MarkerValueVersion::PythonFullVersion | MarkerValueVersion::PythonVersion,
specifier,
}) => {
let specifier = PubGrubSpecifier::try_from(specifier).ok()?;

// Convert to PubGrub range and perform a union.
let range = PubGrubRange::from(specifier);
let (lower, _) = range.iter().next()?;

// Extract the lower bound.
Some(RequiresPythonBound::new(lower.clone()))
}
MarkerTree::And(trees) => {
// Take the maximum of any nested expressions.
trees.iter().filter_map(requires_python_marker).max()
}
MarkerTree::Or(trees) => {
// If all subtrees have a bound, take the minimum.
let mut min_version = None;
for tree in trees {
let version = requires_python_marker(tree)?;
min_version = match min_version {
Some(min_version) => Some(std::cmp::min(min_version, version)),
None => Some(version),
};
}
min_version
}
MarkerTree::Expression(_) => None,
}
}

/// Normalizes this marker tree.
///
/// This function does a number of operations to normalize a marker tree recursively:
Expand Down
7 changes: 3 additions & 4 deletions crates/uv-resolver/src/pubgrub/package.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use std::fmt::{Display, Formatter};
use std::ops::Deref;
use std::sync::Arc;

Expand All @@ -17,9 +16,9 @@ impl Deref for PubGrubPackage {
}
}

impl Display for PubGrubPackage {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
Display::fmt(&self.0, f)
impl std::fmt::Display for PubGrubPackage {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
std::fmt::Display::fmt(&self.0, f)
}
}

Expand Down
15 changes: 14 additions & 1 deletion crates/uv-resolver/src/python_requirement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use pep440_rs::VersionSpecifiers;
use pep508_rs::{MarkerTree, StringVersion};
use uv_toolchain::{Interpreter, PythonVersion};

use crate::RequiresPython;
use crate::{RequiresPython, RequiresPythonBound};

#[derive(Debug, Clone, Eq, PartialEq)]
pub struct PythonRequirement {
Expand Down Expand Up @@ -49,6 +49,19 @@ impl PythonRequirement {
}
}

/// Narrow the [`PythonRequirement`] to the given version, if it's stricter (i.e., greater)
/// than the current `Requires-Python` minimum.
pub fn narrow(&self, target: &RequiresPythonBound) -> Option<Self> {
let Some(PythonTarget::RequiresPython(requires_python)) = self.target.as_ref() else {
return None;
};
let requires_python = requires_python.narrow(target)?;
Some(Self {
installed: self.installed.clone(),
target: Some(PythonTarget::RequiresPython(requires_python)),
})
}

/// Return the installed version of Python.
pub fn installed(&self) -> &StringVersion {
&self.installed
Expand Down
94 changes: 72 additions & 22 deletions crates/uv-resolver/src/requires_python.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use std::cmp::Ordering;
use std::collections::Bound;
use std::ops::Deref;

use itertools::Itertools;
use pubgrub::range::Range;
Expand All @@ -24,7 +26,7 @@ pub enum RequiresPythonError {
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct RequiresPython {
specifiers: VersionSpecifiers,
bound: Bound<Version>,
bound: RequiresPythonBound,
}

impl RequiresPython {
Expand All @@ -34,7 +36,7 @@ impl RequiresPython {
specifiers: VersionSpecifiers::from(VersionSpecifier::greater_than_equal_version(
version.clone(),
)),
bound: Bound::Included(version),
bound: RequiresPythonBound(Bound::Included(version)),
}
}

Expand All @@ -61,11 +63,13 @@ impl RequiresPython {
};

// Extract the lower bound.
let bound = range
.iter()
.next()
.map(|(lower, _)| lower.clone())
.unwrap_or(Bound::Unbounded);
let bound = RequiresPythonBound(
range
.iter()
.next()
.map(|(lower, _)| lower.clone())
.unwrap_or(Bound::Unbounded),
);

// Convert back to PEP 440 specifiers.
let specifiers = range
Expand All @@ -76,6 +80,16 @@ impl RequiresPython {
Ok(Some(Self { specifiers, bound }))
}

/// Narrow the [`RequiresPython`] to the given version, if it's stricter (i.e., greater) than
/// the current target.
pub fn narrow(&self, target: &RequiresPythonBound) -> Option<Self> {
let target = VersionSpecifiers::from(VersionSpecifier::from_lower_bound(target)?);
Self::union(std::iter::once(&target))
.ok()
.flatten()
.filter(|next| next.bound > self.bound)
}

/// Returns `true` if the `Requires-Python` is compatible with the given version.
pub fn contains(&self, version: &Version) -> bool {
self.specifiers.contains(version)
Expand Down Expand Up @@ -140,7 +154,7 @@ impl RequiresPython {
// 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, &self.bound) {
match (target, self.bound.as_ref()) {
(Bound::Included(target_lower), Bound::Included(requires_python_lower)) => {
target_lower.release() <= requires_python_lower.release()
}
Expand All @@ -166,9 +180,9 @@ impl RequiresPython {
&self.specifiers
}

/// Returns the lower [`Bound`] for the `Requires-Python` specifier.
pub fn bound(&self) -> &Bound<Version> {
&self.bound
/// Returns `true` if the `Requires-Python` specifier is unbounded.
pub fn is_unbounded(&self) -> bool {
self.bound.as_ref() == Bound::Unbounded
}

/// Returns this `Requires-Python` specifier as an equivalent marker
Expand All @@ -184,16 +198,14 @@ impl RequiresPython {
/// returns a marker tree that evaluates to `true` for all possible marker
/// environments.
pub fn to_marker_tree(&self) -> MarkerTree {
let (op, version) = match self.bound {
let (op, version) = match self.bound.as_ref() {
// If we see this anywhere, then it implies the marker
// tree we would generate would always evaluate to
// `true` because every possible Python version would
// satisfy it.
Bound::Unbounded => return MarkerTree::And(vec![]),
Bound::Excluded(ref version) => {
(Operator::GreaterThan, version.clone().without_local())
}
Bound::Included(ref version) => {
Bound::Excluded(version) => (Operator::GreaterThan, version.clone().without_local()),
Bound::Included(version) => {
(Operator::GreaterThanEqual, version.clone().without_local())
}
};
Expand Down Expand Up @@ -247,12 +259,50 @@ impl serde::Serialize for RequiresPython {
impl<'de> serde::Deserialize<'de> for RequiresPython {
fn deserialize<D: serde::Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
let specifiers = VersionSpecifiers::deserialize(deserializer)?;
let bound = crate::pubgrub::PubGrubSpecifier::try_from(&specifiers)
.map_err(serde::de::Error::custom)?
.iter()
.next()
.map(|(lower, _)| lower.clone())
.unwrap_or(Bound::Unbounded);
let bound = RequiresPythonBound(
crate::pubgrub::PubGrubSpecifier::try_from(&specifiers)
.map_err(serde::de::Error::custom)?
.iter()
.next()
.map(|(lower, _)| lower.clone())
.unwrap_or(Bound::Unbounded),
);
Ok(Self { specifiers, bound })
}
}

#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct RequiresPythonBound(Bound<Version>);

impl RequiresPythonBound {
pub fn new(bound: Bound<Version>) -> Self {
Self(bound)
}
}

impl Deref for RequiresPythonBound {
type Target = Bound<Version>;

fn deref(&self) -> &Self::Target {
&self.0
}
}

impl PartialOrd for RequiresPythonBound {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.cmp(other))
}
}

impl Ord for RequiresPythonBound {
fn cmp(&self, other: &Self) -> Ordering {
match (self.as_ref(), other.as_ref()) {
(Bound::Included(a), Bound::Included(b)) => a.cmp(b),
(Bound::Included(_), Bound::Excluded(_)) => Ordering::Less,
(Bound::Excluded(_), Bound::Included(_)) => Ordering::Greater,
(Bound::Excluded(a), Bound::Excluded(b)) => a.cmp(b),
(Bound::Unbounded, _) => Ordering::Less,
(_, Bound::Unbounded) => Ordering::Greater,
}
}
}
Loading
Loading