Skip to content

Commit

Permalink
Fix requires-python
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Jan 17, 2025
1 parent dce7b9d commit 6e7dd55
Show file tree
Hide file tree
Showing 5 changed files with 131 additions and 22 deletions.
99 changes: 81 additions & 18 deletions crates/uv-resolver/src/marker.rs
Original file line number Diff line number Diff line change
@@ -1,63 +1,72 @@
use pubgrub::Range;
use pubgrub::{Range, Ranges};
use uv_pep440::Version;
use uv_pep508::{CanonicalMarkerValueVersion, MarkerTree, MarkerTreeKind};

use crate::requires_python::{LowerBound, RequiresPythonRange, UpperBound};

/// Returns the bounding Python versions that can satisfy the [`MarkerTree`], if it's constrained.
pub(crate) fn requires_python(tree: MarkerTree) -> Option<RequiresPythonRange> {
fn collect_python_markers(tree: MarkerTree, markers: &mut Vec<Range<Version>>) {
/// Collect the Python version markers from the tree.
///
/// Specifically, performs a DFS to collect all Python requirements on the path to every
/// `MarkerTreeKind::True` node.
fn collect_python_markers(tree: MarkerTree, markers: &mut Vec<Vec<Range<Version>>>, current_path: &mut Vec<Range<Version>>) {
match tree.kind() {
MarkerTreeKind::True | MarkerTreeKind::False => {}
MarkerTreeKind::True => {
markers.push(current_path.clone());
}
MarkerTreeKind::False => {}
MarkerTreeKind::Version(marker) => match marker.key() {
CanonicalMarkerValueVersion::PythonFullVersion => {
for (range, tree) in marker.edges() {
if !tree.is_false() {
markers.push(range.clone());
}
current_path.push(range.clone());
collect_python_markers(tree, markers, current_path);
current_path.pop();
}
}
CanonicalMarkerValueVersion::ImplementationVersion => {
for (_, tree) in marker.edges() {
collect_python_markers(tree, markers);
collect_python_markers(tree, markers, current_path);
}
}
},
MarkerTreeKind::String(marker) => {
for (_, tree) in marker.children() {
collect_python_markers(tree, markers);
collect_python_markers(tree, markers, current_path);
}
}
MarkerTreeKind::In(marker) => {
for (_, tree) in marker.children() {
collect_python_markers(tree, markers);
collect_python_markers(tree, markers, current_path);
}
}
MarkerTreeKind::Contains(marker) => {
for (_, tree) in marker.children() {
collect_python_markers(tree, markers);
collect_python_markers(tree, markers, current_path);
}
}
MarkerTreeKind::Extra(marker) => {
for (_, tree) in marker.children() {
collect_python_markers(tree, markers);
collect_python_markers(tree, markers, current_path);
}
}
}
}

let mut markers = Vec::new();
collect_python_markers(tree, &mut markers);
collect_python_markers(tree, &mut markers, &mut Vec::new());

// Take the union of all Python version markers.
// Take the union of the intersections of the Python version markers.
let range = markers
.into_iter()
.fold(None, |acc: Option<Range<Version>>, range| {
Some(match acc {
Some(acc) => acc.union(&range),
None => range.clone(),
.map(|ranges| {
ranges.into_iter().fold(Ranges::full(), |acc: Range<Version>, range| {
acc.intersection(&range)
})
})?;
})
.fold(Ranges::empty(), |acc: Range<Version>, range| {
acc.union(&range)
});

let (lower, upper) = range.bounding_range()?;

Expand All @@ -66,3 +75,57 @@ pub(crate) fn requires_python(tree: MarkerTree) -> Option<RequiresPythonRange> {
UpperBound::new(upper.cloned()),
))
}



#[cfg(test)]
mod tests {
use std::ops::Bound;
use std::str::FromStr;
use super::*;

#[test]
fn test_requires_python() {
// Test exact version match
let tree = MarkerTree::from_str("python_full_version == '3.8.*'").unwrap();
let range = requires_python(tree).unwrap();
assert_eq!(*range.lower(), LowerBound::new(Bound::Included(Version::from_str("3.8").unwrap())));
assert_eq!(*range.upper(), UpperBound::new(Bound::Excluded(Version::from_str("3.9").unwrap())));

// Test version range with exclusive bounds
let tree = MarkerTree::from_str("python_full_version > '3.8' and python_full_version < '3.9'").unwrap();
let range = requires_python(tree).unwrap();
assert_eq!(*range.lower(), LowerBound::new(Bound::Excluded(Version::from_str("3.8").unwrap())));
assert_eq!(*range.upper(), UpperBound::new(Bound::Excluded(Version::from_str("3.9").unwrap())));

// Test version range with inclusive bounds
let tree = MarkerTree::from_str("python_full_version >= '3.8' and python_full_version <= '3.9'").unwrap();
let range = requires_python(tree).unwrap();
assert_eq!(*range.lower(), LowerBound::new(Bound::Included(Version::from_str("3.8").unwrap())));
assert_eq!(*range.upper(), UpperBound::new(Bound::Included(Version::from_str("3.9").unwrap())));

// Test version with only lower bound
let tree = MarkerTree::from_str("python_full_version >= '3.8'").unwrap();
let range = requires_python(tree).unwrap();
assert_eq!(*range.lower(), LowerBound::new(Bound::Included(Version::from_str("3.8").unwrap())));
assert_eq!(*range.upper(), UpperBound::new(Bound::Unbounded));

// Test version with only upper bound
let tree = MarkerTree::from_str("python_full_version < '3.9'").unwrap();
let range = requires_python(tree).unwrap();
assert_eq!(*range.lower(), LowerBound::new(Bound::Unbounded));
assert_eq!(*range.upper(), UpperBound::new(Bound::Excluded(Version::from_str("3.9").unwrap())));

// Test OR with non-Python marker (should result in unbounded range)
let tree = MarkerTree::from_str("python_full_version > '3.8' or sys_platform == 'win32'").unwrap();
let range = requires_python(tree).unwrap();
assert_eq!(*range.lower(), LowerBound::new(Bound::Unbounded));
assert_eq!(*range.upper(), UpperBound::new(Bound::Unbounded));

// Test complex AND/OR combination
let tree = MarkerTree::from_str("(python_full_version >= '3.8' and python_full_version < '3.9') or (python_full_version >= '3.10' and python_full_version < '3.11')").unwrap();
let range = requires_python(tree).unwrap();
assert_eq!(*range.lower(), LowerBound::new(Bound::Included(Version::from_str("3.8").unwrap())));
assert_eq!(*range.upper(), UpperBound::new(Bound::Excluded(Version::from_str("3.11").unwrap())));
}
}
4 changes: 3 additions & 1 deletion crates/uv-resolver/src/resolution/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use petgraph::{
Directed, Direction,
};
use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet};

use tracing::debug;
use uv_configuration::{Constraints, Overrides};
use uv_distribution::Metadata;
use uv_distribution_types::{
Expand Down Expand Up @@ -165,6 +165,8 @@ impl ResolverOutput {
for resolution in resolutions {
let marker = resolution.env.try_universal_markers().unwrap_or_default();

debug!("Got resolution with: {:?}", marker);

// Add every edge to the graph, propagating the marker for the current fork, if
// necessary.
for edge in &resolution.edges {
Expand Down
30 changes: 29 additions & 1 deletion crates/uv-resolver/src/resolver/environment.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use std::sync::Arc;
use tracing::trace;
use tracing::{debug, trace};
use uv_pep440::VersionSpecifiers;
use uv_pep508::{MarkerEnvironment, MarkerTree};
use uv_pypi_types::{ConflictItem, ConflictItemRef, ResolverMarkerEnvironment};
Expand Down Expand Up @@ -544,13 +544,41 @@ pub(crate) fn fork_version_by_python_requirement(
panic!("resolver must be in universal mode for forking")
};

debug!("Using env marker: {:?}", env_marker);
debug!("Using requires-python: {:?}", python_requirement.target());

let mut envs = vec![];
if !env_marker.is_disjoint(lower.to_marker_tree()) {
debug!("Narrowing to: {:?}", lower.to_marker_tree());
debug!("Narrowed env: {:?}", env.narrow_environment(lower.to_marker_tree()));
envs.push(env.narrow_environment(lower.to_marker_tree()));
}
if !env_marker.is_disjoint(upper.to_marker_tree()) {
debug!("Narrowing to: {:?}", upper.to_marker_tree());
debug!("Narrowed env: {:?}", env.narrow_environment(upper.to_marker_tree()));
envs.push(env.narrow_environment(upper.to_marker_tree()));
}

// The big issue here is: if `python_requirement` is more narrow than the environment marker,
// the split will _not_ cover the entire environment. So, for example, with the current bug,
// if marker is `python_version >= '3.10' or sys_platform == 'linux'`, and the Python
// requirement is `>=3.11`, then we lose the portion of the environment that is `sys_platform
// == 'linux'`.
//
// So it's only an issue if we have a broken invariant, I think.

// // Compute the remainder.
// let mut remainder = env_marker.clone();
// remainder.and(lower.to_marker_tree().negate());
// remainder.and(upper.to_marker_tree().negate());
// // remainder.and(python_requirement.to_marker_tree());
// if !remainder.is_false() {
// println!("Remainder: {:?}", remainder);
// println!("python_requirement: {:?}", python_requirement);
// envs.push(env.narrow_environment(remainder));
// // return vec![];
// }

debug_assert!(!envs.is_empty(), "at least one fork should be produced");
envs
}
Expand Down
18 changes: 16 additions & 2 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,13 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
let version = match version {
ResolverVersion::Unforked(version) => version,
ResolverVersion::Forked(forks) => {
forked_states.extend(self.version_forks_to_fork_states(state, forks));
debug!("Currently solving: {}", state.env);

for y in self.version_forks_to_fork_states(state, forks) {
debug!("Forking to solve: {}", y.env);
forked_states.push(y);
}
// forked_states.extend(self.version_forks_to_fork_states(state, forks));
continue 'FORK;
}
ResolverVersion::Unavailable(version, reason) => {
Expand Down Expand Up @@ -1234,6 +1240,12 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
{
if matches!(self.options.fork_strategy, ForkStrategy::RequiresPython) {
if env.marker_environment().is_none() {
// So here, we have:
// `python_full_version < '3.10' or platform_python_implementation != 'CPython'`
//
// We then run into a dependency (`coverage==7.6.10`) that has `>=3.9`.
//
// So we want to solve separately for `3.8` and `3.9`.
let forks = fork_version_by_python_requirement(
requires_python,
python_requirement,
Expand Down Expand Up @@ -2760,10 +2772,12 @@ impl ForkState {
/// If the fork should be dropped (e.g., because its markers can never be true for its
/// Python requirement), then this returns `None`.
fn with_env(mut self, env: ResolverEnvironment) -> Self {
debug!("Creating env: {env}");
debug!("Current python requirement: {:?}", self.python_requirement);
self.env = env;
// If the fork contains a narrowed Python requirement, apply it.
if let Some(req) = self.env.narrow_python_requirement(&self.python_requirement) {
debug!("Narrowed `requires-python` bound to: {}", req.target());
debug!("Narrowed `requires-python` bound to: {:?}", req.target());
self.python_requirement = req;
}
self
Expand Down
2 changes: 2 additions & 0 deletions req.in
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
iniconfig ; platform_python_implementation == "CPython" and python_version >= "3.10"
coverage

0 comments on commit 6e7dd55

Please sign in to comment.