Filter out markers based on Python requirement (#4912)

## Summary

In marker normalization, we now remove any markers that are redundant
with the `requires-python` specifier (i.e., always true for the given
Python requirement).

For example, given `iniconfig ; python_version >= '3.7'`, we can remove
the `python_version >= '3.7'` marker when resolving with
`--python-version 3.8`.

Closes #4852.
This commit is contained in:
Charlie Marsh 2024-07-09 09:15:58 -07:00 committed by GitHub
parent ef120dcc54
commit 72dd34b225
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 146 additions and 48 deletions

View file

@ -1892,7 +1892,7 @@ impl Dependency {
) -> Dependency { ) -> Dependency {
let distribution_id = DistributionId::from_annotated_dist(annotated_dist); let distribution_id = DistributionId::from_annotated_dist(annotated_dist);
let extra = annotated_dist.extra.clone(); let extra = annotated_dist.extra.clone();
let marker = marker.cloned().and_then(crate::marker::normalize); let marker = marker.cloned();
Dependency { Dependency {
distribution_id, distribution_id,
extra, extra,

View file

@ -4,7 +4,7 @@ use std::collections::HashMap;
use std::ops::Bound::{self, *}; use std::ops::Bound::{self, *};
use std::ops::RangeBounds; use std::ops::RangeBounds;
use pubgrub::range::Range as PubGrubRange; use pubgrub::range::{Range as PubGrubRange, Range};
use pep440_rs::{Operator, Version, VersionSpecifier}; use pep440_rs::{Operator, Version, VersionSpecifier};
use pep508_rs::{ use pep508_rs::{
@ -82,35 +82,41 @@ fn string_is_disjoint(this: &MarkerExpression, other: &MarkerExpression) -> bool
true true
} }
/// Returns the minimum Python version that can satisfy the [`MarkerTree`], if it's constrained. pub(crate) fn python_range(expr: &MarkerExpression) -> Option<Range<Version>> {
pub(crate) fn requires_python_marker(tree: &MarkerTree) -> Option<RequiresPythonBound> { match expr {
match tree { MarkerExpression::Version {
MarkerTree::Expression(MarkerExpression::Version {
key: MarkerValueVersion::PythonFullVersion, key: MarkerValueVersion::PythonFullVersion,
specifier, specifier,
}) => { } => {
// Simplify using PEP 440 semantics. // Simplify using PEP 440 semantics.
let specifier = PubGrubSpecifier::from_pep440_specifier(specifier).ok()?; let specifier = PubGrubSpecifier::from_pep440_specifier(specifier).ok()?;
// Convert to PubGrub range and perform a union. // Convert to PubGrub.
let range = PubGrubRange::from(specifier); Some(PubGrubRange::from(specifier))
let (lower, _) = range.iter().next()?;
// Extract the lower bound.
Some(RequiresPythonBound::new(lower.clone()))
} }
MarkerTree::Expression(MarkerExpression::Version { MarkerExpression::Version {
key: MarkerValueVersion::PythonVersion, key: MarkerValueVersion::PythonVersion,
specifier, specifier,
}) => { } => {
// Simplify using release-only semantics, since `python_version` is always `major.minor`. // Simplify using release-only semantics, since `python_version` is always `major.minor`.
let specifier = PubGrubSpecifier::from_release_specifier(specifier).ok()?; let specifier = PubGrubSpecifier::from_release_specifier(specifier).ok()?;
// Convert to PubGrub range and perform a union. // Convert to PubGrub.
let range = PubGrubRange::from(specifier); Some(PubGrubRange::from(specifier))
let (lower, _) = range.iter().next()?; }
_ => None,
}
}
/// 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(expr) => {
// Extract the supported Python range.
let range = python_range(expr)?;
// Extract the lower bound. // Extract the lower bound.
let (lower, _) = range.iter().next()?;
Some(RequiresPythonBound::new(lower.clone())) Some(RequiresPythonBound::new(lower.clone()))
} }
MarkerTree::And(trees) => { MarkerTree::And(trees) => {
@ -129,7 +135,6 @@ pub(crate) fn requires_python_marker(tree: &MarkerTree) -> Option<RequiresPython
} }
min_version min_version
} }
MarkerTree::Expression(_) => None,
} }
} }
@ -137,34 +142,40 @@ pub(crate) fn requires_python_marker(tree: &MarkerTree) -> Option<RequiresPython
/// ///
/// This function does a number of operations to normalize a marker tree recursively: /// This function does a number of operations to normalize a marker tree recursively:
/// - Sort and flatten all nested expressions. /// - Sort and flatten all nested expressions.
/// - Simplify expressions. This includes combining overlapping version ranges and removing duplicate /// - Simplify expressions. This includes combining overlapping version ranges, removing duplicate
/// expressions. /// expressions, and removing redundant expressions.
/// - Normalize the order of version expressions to the form `<version key> <version op> <version>` /// - Normalize the order of version expressions to the form `<version key> <version op> <version>`
/// (i.e. not the reverse). /// (i.e., not the reverse).
/// ///
/// This is useful in cases where creating conjunctions or disjunctions might occur in a non-deterministic /// This is useful in cases where creating conjunctions or disjunctions might occur in a non-deterministic
/// order. This routine will attempt to erase the distinction created by such a construction. /// order. This routine will attempt to erase the distinction created by such a construction.
pub(crate) fn normalize(mut tree: MarkerTree) -> Option<MarkerTree> { pub(crate) fn normalize(
mut tree: MarkerTree,
bound: Option<&RequiresPythonBound>,
) -> Option<MarkerTree> {
// Filter out redundant expressions that show up before and after normalization. // Filter out redundant expressions that show up before and after normalization.
filter_all(&mut tree); filter_all(&mut tree);
let mut tree = normalize_all(tree)?; let mut tree = normalize_all(tree, bound)?;
filter_all(&mut tree); filter_all(&mut tree);
Some(tree) Some(tree)
} }
/// Normalize the marker tree recursively. /// Normalize the marker tree recursively.
pub(crate) fn normalize_all(tree: MarkerTree) -> Option<MarkerTree> { pub(crate) fn normalize_all(
tree: MarkerTree,
bound: Option<&RequiresPythonBound>,
) -> Option<MarkerTree> {
match tree { match tree {
MarkerTree::And(trees) => { MarkerTree::And(trees) => {
let mut reduced = Vec::new(); let mut reduced = Vec::new();
let mut versions: HashMap<_, Vec<_>> = HashMap::new(); let mut versions: HashMap<_, Vec<_>> = HashMap::new();
for subtree in trees { for subtree in trees {
// Simplify nested expressions as much as possible first. // Normalize nested expressions as much as possible first.
// //
// If the expression gets normalized out (e.g., `version < '3.8' and version >= '3.8'`), // If the expression gets normalized out (e.g., `version < '3.8' and version >= '3.8'`),
// omit it. // omit it.
let Some(subtree) = normalize_all(subtree) else { let Some(subtree) = normalize_all(subtree, bound) else {
continue; continue;
}; };
@ -206,11 +217,11 @@ pub(crate) fn normalize_all(tree: MarkerTree) -> Option<MarkerTree> {
let mut versions: HashMap<_, Vec<_>> = HashMap::new(); let mut versions: HashMap<_, Vec<_>> = HashMap::new();
for subtree in trees { for subtree in trees {
// Simplify nested expressions as much as possible first. // Normalize nested expressions as much as possible first.
// //
// If the expression gets normalized out (e.g., `version < '3.8' and version >= '3.8'`), // If the expression gets normalized out (e.g., `version < '3.8' and version >= '3.8'`),
// return `true`. // return `true`.
let subtree = normalize_all(subtree)?; let subtree = normalize_all(subtree, bound)?;
match subtree { match subtree {
MarkerTree::And(_) => reduced.push(subtree), MarkerTree::And(_) => reduced.push(subtree),
@ -260,6 +271,19 @@ pub(crate) fn normalize_all(tree: MarkerTree) -> Option<MarkerTree> {
} }
} }
// If the marker is redundant given the supported Python range, remove it.
//
// For example, `python_version >= '3.7'` is redundant with `requires-python: '>=3.8'`.
MarkerTree::Expression(expr)
if bound.is_some_and(|bound| {
python_range(&expr).is_some_and(|supported_range| {
Range::from(bound.clone()).subset_of(&supported_range)
})
}) =>
{
None
}
MarkerTree::Expression(ref expr) => { MarkerTree::Expression(ref expr) => {
if let Some((key, range)) = keyed_range(expr) { if let Some((key, range)) = keyed_range(expr) {
// If multiple terms are required to express the range, flatten them into an `Or` // If multiple terms are required to express the range, flatten them into an `Or`
@ -681,8 +705,8 @@ mod tests {
#[test] #[test]
fn simplify() { fn simplify() {
assert_marker_equal( assert_marker_equal(
"python_version == '3.1' or python_version == '3.1'", "python_version == '3.9' or python_version == '3.9'",
"python_version == '3.1'", "python_version == '3.9'",
); );
assert_marker_equal( assert_marker_equal(
@ -827,6 +851,21 @@ mod tests {
); );
} }
#[test]
fn requires_python() {
assert_normalizes_out("python_version >= '3.8'");
assert_normalizes_out("python_version >= '3.8' or sys_platform == 'win32'");
assert_normalizes_to(
"python_version >= '3.8' and sys_platform == 'win32'",
"sys_platform == 'win32'",
);
assert_normalizes_to("python_version == '3.8'", "python_version == '3.8'");
assert_normalizes_to("python_version <= '3.10'", "python_version <= '3.10'");
}
#[test] #[test]
fn extra_disjointness() { fn extra_disjointness() {
assert!(!is_disjoint("extra == 'a'", "python_version == '1'")); assert!(!is_disjoint("extra == 'a'", "python_version == '1'"));
@ -987,8 +1026,9 @@ mod tests {
} }
fn assert_marker_equal(one: impl AsRef<str>, two: impl AsRef<str>) { fn assert_marker_equal(one: impl AsRef<str>, two: impl AsRef<str>) {
let bound = RequiresPythonBound::new(Included(Version::new([3, 8])));
let tree1 = MarkerTree::parse_reporter(one.as_ref(), &mut TracingReporter).unwrap(); let tree1 = MarkerTree::parse_reporter(one.as_ref(), &mut TracingReporter).unwrap();
let tree1 = normalize(tree1).unwrap(); let tree1 = normalize(tree1, Some(&bound)).unwrap();
let tree2 = MarkerTree::parse_reporter(two.as_ref(), &mut TracingReporter).unwrap(); let tree2 = MarkerTree::parse_reporter(two.as_ref(), &mut TracingReporter).unwrap();
assert_eq!( assert_eq!(
tree1.to_string(), tree1.to_string(),
@ -999,17 +1039,19 @@ mod tests {
} }
fn assert_normalizes_to(before: impl AsRef<str>, after: impl AsRef<str>) { fn assert_normalizes_to(before: impl AsRef<str>, after: impl AsRef<str>) {
let bound = RequiresPythonBound::new(Included(Version::new([3, 8])));
let normalized = MarkerTree::parse_reporter(before.as_ref(), &mut TracingReporter) let normalized = MarkerTree::parse_reporter(before.as_ref(), &mut TracingReporter)
.unwrap() .unwrap()
.clone(); .clone();
let normalized = normalize(normalized).unwrap(); let normalized = normalize(normalized, Some(&bound)).unwrap();
assert_eq!(normalized.to_string(), after.as_ref()); assert_eq!(normalized.to_string(), after.as_ref());
} }
fn assert_normalizes_out(before: impl AsRef<str>) { fn assert_normalizes_out(before: impl AsRef<str>) {
let bound = RequiresPythonBound::new(Included(Version::new([3, 8])));
let normalized = MarkerTree::parse_reporter(before.as_ref(), &mut TracingReporter) let normalized = MarkerTree::parse_reporter(before.as_ref(), &mut TracingReporter)
.unwrap() .unwrap()
.clone(); .clone();
assert!(normalize(normalized).is_none()); assert!(normalize(normalized, Some(&bound)).is_none());
} }
} }

View file

@ -155,6 +155,11 @@ impl RequiresPython {
self.bound.as_ref() == Bound::Unbounded self.bound.as_ref() == Bound::Unbounded
} }
/// Returns the [`RequiresPythonBound`] for the `Requires-Python` specifier.
pub fn bound(&self) -> &RequiresPythonBound {
&self.bound
}
/// Returns this `Requires-Python` specifier as an equivalent marker /// Returns this `Requires-Python` specifier as an equivalent marker
/// expression utilizing the `python_version` marker field. /// expression utilizing the `python_version` marker field.
/// ///
@ -254,6 +259,16 @@ impl RequiresPythonBound {
} }
} }
impl From<RequiresPythonBound> for Range<Version> {
fn from(value: RequiresPythonBound) -> Self {
match value.0 {
Bound::Included(version) => Range::higher_than(version),
Bound::Excluded(version) => Range::strictly_higher_than(version),
Bound::Unbounded => Range::full(),
}
}
}
impl Deref for RequiresPythonBound { impl Deref for RequiresPythonBound {
type Target = Bound<Version>; type Target = Bound<Version>;

View file

@ -406,7 +406,7 @@ fn propagate_markers(mut graph: IntermediatePetGraph) -> IntermediatePetGraph {
} }
if let DisplayResolutionGraphNode::Dist(node) = &mut graph[index] { if let DisplayResolutionGraphNode::Dist(node) = &mut graph[index] {
node.markers = marker_tree.and_then(marker::normalize); node.markers = marker_tree.and_then(|marker| marker::normalize(marker, None));
}; };
} }

View file

@ -291,6 +291,16 @@ impl ResolutionGraph {
.and_then(PythonTarget::as_requires_python) .and_then(PythonTarget::as_requires_python)
.cloned(); .cloned();
// Normalize any markers.
for edge in petgraph.edge_indices() {
if let Some(marker) = petgraph[edge].take() {
petgraph[edge] = crate::marker::normalize(
marker,
requires_python.as_ref().map(RequiresPython::bound),
);
}
}
Ok(Self { Ok(Self {
petgraph, petgraph,
requires_python, requires_python,
@ -318,7 +328,7 @@ impl ResolutionGraph {
/// Return `true` if there are no packages in the graph. /// Return `true` if there are no packages in the graph.
pub fn is_empty(&self) -> bool { pub fn is_empty(&self) -> bool {
self.dists().any(super::AnnotatedDist::is_base) self.dists().any(AnnotatedDist::is_base)
} }
/// Returns `true` if the graph contains the given package. /// Returns `true` if the graph contains the given package.

View file

@ -578,7 +578,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
} }
forked_state.markers.and(fork.markers); forked_state.markers.and(fork.markers);
forked_state.markers = normalize(forked_state.markers) forked_state.markers = normalize(forked_state.markers, None)
.unwrap_or(MarkerTree::And(Vec::new())); .unwrap_or(MarkerTree::And(Vec::new()));
// If the fork contains a narrowed Python requirement, apply it. // If the fork contains a narrowed Python requirement, apply it.

View file

@ -1405,9 +1405,9 @@ fn update() -> Result<()> {
version = "0.1.0" version = "0.1.0"
source = { editable = "." } source = { editable = "." }
dependencies = [ dependencies = [
{ name = "requests", marker = "python_version > '3.7'" }, { name = "requests" },
{ name = "requests", extra = "socks", marker = "python_version > '3.7'" }, { name = "requests", extra = "socks" },
{ name = "requests", extra = "use-chardet-on-py3", marker = "python_version > '3.7'" }, { name = "requests", extra = "use-chardet-on-py3" },
] ]
[[distribution]] [[distribution]]

View file

@ -6366,23 +6366,23 @@ fn universal() -> Result<()> {
----- stdout ----- ----- stdout -----
# This file was autogenerated by uv via the following command: # This file was autogenerated by uv via the following command:
# uv pip compile --cache-dir [CACHE_DIR] requirements.in --universal # uv pip compile --cache-dir [CACHE_DIR] requirements.in --universal
attrs==23.2.0 ; python_version > '3.11' or sys_platform == 'win32' attrs==23.2.0
# via # via
# outcome # outcome
# trio # trio
cffi==1.16.0 ; implementation_name != 'pypy' and os_name == 'nt' and (python_version > '3.11' or sys_platform == 'win32') cffi==1.16.0 ; implementation_name != 'pypy' and os_name == 'nt'
# via trio # via trio
idna==3.6 ; python_version > '3.11' or sys_platform == 'win32' idna==3.6
# via trio # via trio
outcome==1.3.0.post0 ; python_version > '3.11' or sys_platform == 'win32' outcome==1.3.0.post0
# via trio # via trio
pycparser==2.21 ; implementation_name != 'pypy' and os_name == 'nt' and (python_version > '3.11' or sys_platform == 'win32') pycparser==2.21 ; implementation_name != 'pypy' and os_name == 'nt'
# via cffi # via cffi
sniffio==1.3.1 ; python_version > '3.11' or sys_platform == 'win32' sniffio==1.3.1
# via trio # via trio
sortedcontainers==2.4.0 ; python_version > '3.11' or sys_platform == 'win32' sortedcontainers==2.4.0
# via trio # via trio
trio==0.25.0 ; python_version > '3.11' or sys_platform == 'win32' trio==0.25.0
# via -r requirements.in # via -r requirements.in
----- stderr ----- ----- stderr -----
@ -6780,6 +6780,37 @@ fn universal_no_repeated_unconditional_distributions() -> Result<()> {
Ok(()) Ok(())
} }
/// Remove `python_version` markers that are always true.
#[test]
fn universal_unnecessary_python() -> Result<()> {
let context = TestContext::new("3.12");
let requirements_in = context.temp_dir.child("requirements.in");
requirements_in.write_str(indoc::indoc! {r"
iniconfig ; python_version >= '3.7'
"})?;
uv_snapshot!(context.filters(), windows_filters=false, context.pip_compile()
.arg("requirements.in")
.arg("-p")
.arg("3.8")
.arg("--universal"), @r###"
success: true
exit_code: 0
----- stdout -----
# This file was autogenerated by uv via the following command:
# uv pip compile --cache-dir [CACHE_DIR] requirements.in -p 3.8 --universal
iniconfig==2.0.0
# via -r requirements.in
----- stderr -----
warning: The requested Python version 3.8 is not available; 3.12.[X] will be used to build dependencies instead.
Resolved 1 package in [TIME]
"###
);
Ok(())
}
/// Resolve a package from a `requirements.in` file, with a `constraints.txt` file pinning one of /// Resolve a package from a `requirements.in` file, with a `constraints.txt` file pinning one of
/// its transitive dependencies to a specific version. /// its transitive dependencies to a specific version.
#[test] #[test]