Skip to content

Commit

Permalink
reverts
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh
charliermarsh committed Jan 17, 2025
1 parent dabc782 commit 9c22987
Show file tree
Hide file tree
Showing 6 changed files with 111 additions and 82 deletions.
105 changes: 73 additions & 32 deletions crates/uv-resolver/src/marker.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use pubgrub::{Range, Ranges};
use pubgrub::Ranges;

use uv_pep440::Version;
use uv_pep508::{CanonicalMarkerValueVersion, MarkerTree, MarkerTreeKind};

Expand All @@ -10,7 +11,11 @@ pub(crate) fn requires_python(tree: MarkerTree) -> Option<RequiresPythonRange> {
///
/// 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>>) {
fn collect_python_markers(
tree: MarkerTree,
markers: &mut Vec<Vec<Ranges<Version>>>,
current_path: &mut Vec<Ranges<Version>>,
) {
match tree.kind() {
MarkerTreeKind::True => {
markers.push(current_path.clone());
Expand Down Expand Up @@ -60,11 +65,13 @@ pub(crate) fn requires_python(tree: MarkerTree) -> Option<RequiresPythonRange> {
let range = markers
.into_iter()
.map(|ranges| {
ranges.into_iter().fold(Ranges::full(), |acc: Range<Version>, range| {
acc.intersection(&range)
})
ranges
.into_iter()
.fold(Ranges::full(), |acc: Ranges<Version>, range| {
acc.intersection(&range)
})
})
.fold(Ranges::empty(), |acc: Range<Version>, range| {
.fold(Ranges::empty(), |acc: Ranges<Version>, range| {
acc.union(&range)
});

Expand All @@ -76,56 +83,90 @@ pub(crate) fn requires_python(tree: MarkerTree) -> Option<RequiresPythonRange> {
))
}



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

use super::*;

#[test]
fn test_requires_python() {
// Test exact version match
// An 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();
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()))
);

// A 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();
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()))
);

// A 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
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()))
);

// A version with a 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.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
// A version with an 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();
assert_eq!(
*range.upper(),
UpperBound::new(Bound::Excluded(Version::from_str("3.9").unwrap()))
);

// A disjunction with a non-Python marker (i.e., an 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
// A complex mix of conjunctions and disjunctions.
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())));
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: 1 addition & 3 deletions crates/uv-resolver/src/resolution/output.rs
View file
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,8 +165,6 @@ 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: 1 addition & 29 deletions crates/uv-resolver/src/resolver/environment.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use std::sync::Arc;
use tracing::{debug, trace};
use tracing::trace;
use uv_pep440::VersionSpecifiers;
use uv_pep508::{MarkerEnvironment, MarkerTree};
use uv_pypi_types::{ConflictItem, ConflictItemRef, ResolverMarkerEnvironment};
Expand Down Expand Up @@ -544,41 +544,13 @@ 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: 2 additions & 16 deletions crates/uv-resolver/src/resolver/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -542,13 +542,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
let version = match version {
ResolverVersion::Unforked(version) => version,
ResolverVersion::Forked(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));
forked_states.extend(self.version_forks_to_fork_states(state, forks));
continue 'FORK;
}
ResolverVersion::Unavailable(version, reason) => {
Expand Down Expand Up @@ -1240,12 +1234,6 @@ 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 @@ -2772,12 +2760,10 @@ 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
34 changes: 34 additions & 0 deletions crates/uv/tests/it/pip_compile.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14143,6 +14143,40 @@ fn compile_lowest_extra_unpinned_warning() -> Result<()> {
Ok(())
}

#[test]
fn disjoint_requires_python() -> Result<()> {
let context = TestContext::new("3.8");

let requirements_in = context.temp_dir.child("requirements.in");
requirements_in.write_str(indoc::indoc! {r"
iniconfig ; platform_python_implementation == 'CPython' and python_version >= '3.10'
coverage
"})?;

uv_snapshot!(context.filters(), context.pip_compile()
.arg("--universal")
.arg(requirements_in.path())
.env_remove(EnvVars::UV_EXCLUDE_NEWER), @r###"
success: true
exit_code: 0
----- stdout -----
# This file was autogenerated by uv via the following command:
# uv pip compile --cache-dir [CACHE_DIR] --universal [TEMP_DIR]/requirements.in
coverage==7.6.1 ; python_full_version < '3.9'
# via -r requirements.in
coverage==7.6.10 ; python_full_version >= '3.9'
# via -r requirements.in
iniconfig==2.0.0 ; python_full_version >= '3.10' and platform_python_implementation == 'CPython'
# via -r requirements.in
----- stderr -----
Resolved 3 packages in [TIME]
"###
);

Ok(())
}

/// Test that we use the version in the source distribution filename for compiling, even if the
/// version is declared as dynamic.
///
Expand Down
2 changes: 0 additions & 2 deletions req.in
View file

This file was deleted.

0 comments on commit 9c22987

Please sign in to comment.