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

uv-resolver: use Requires-Python to filter dependencies during universal resolution #4273

Merged
merged 4 commits into from
Jun 12, 2024
Merged
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
35 changes: 34 additions & 1 deletion crates/uv-resolver/src/pubgrub/dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use tracing::warn;

use distribution_types::Verbatim;
use pep440_rs::Version;
use pep508_rs::MarkerEnvironment;
use pep508_rs::{MarkerEnvironment, MarkerTree};
use pypi_types::{
ParsedArchiveUrl, ParsedGitUrl, ParsedPathUrl, ParsedUrl, Requirement, RequirementSource,
};
Expand Down Expand Up @@ -39,6 +39,7 @@ impl PubGrubDependencies {
locals: &Locals,
git: &GitResolver,
env: Option<&MarkerEnvironment>,
requires_python: Option<&MarkerTree>,
) -> Result<Self, ResolveError> {
let mut dependencies = Vec::default();
let mut seen = FxHashSet::default();
Expand All @@ -55,6 +56,7 @@ impl PubGrubDependencies {
locals,
git,
env,
requires_python,
&mut dependencies,
&mut seen,
)?;
Expand Down Expand Up @@ -87,6 +89,7 @@ fn add_requirements(
locals: &Locals,
git: &GitResolver,
env: Option<&MarkerEnvironment>,
requires_python: Option<&MarkerTree>,
dependencies: &mut Vec<(PubGrubPackage, Range<Version>)>,
seen: &mut FxHashSet<ExtraName>,
) -> Result<(), ResolveError> {
Expand All @@ -96,6 +99,11 @@ fn add_requirements(
} else {
Either::Right(requirements.iter())
}) {
// If the requirement would not be selected with any Python version
// supported by the root, skip it.
if !satisfies_requires_python(requires_python, requirement) {
continue;
}
// If the requirement isn't relevant for the current platform, skip it.
match source_extra {
Some(source_extra) => {
Expand Down Expand Up @@ -157,6 +165,7 @@ fn add_requirements(
locals,
git,
env,
requires_python,
dependencies,
seen,
)?;
Expand All @@ -170,6 +179,11 @@ fn add_requirements(

// If the requirement was constrained, add those constraints.
for constraint in constraints.get(&requirement.name).into_iter().flatten() {
// If the requirement would not be selected with any Python
// version supported by the root, skip it.
if !satisfies_requires_python(requires_python, constraint) {
continue;
}
// If the requirement isn't relevant for the current platform, skip it.
match source_extra {
Some(source_extra) => {
Expand Down Expand Up @@ -381,3 +395,22 @@ impl PubGrubRequirement {
Self::from_requirement(constraint, None, urls, locals, git)
}
}

/// Returns true if and only if the given requirement's marker expression has a
/// possible true value given the `requires_python` specifier given.
///
/// While this is always called, a `requires_python` is only non-None when in
/// universal resolution mode. In non-universal mode, `requires_python` is
/// `None` and this always returns `true`.
fn satisfies_requires_python(
requires_python: Option<&MarkerTree>,
requirement: &Requirement,
) -> bool {
let Some(requires_python) = requires_python else {
return true;
};
let Some(marker) = requirement.marker.as_ref() else {
return true;
};
!crate::marker::is_disjoint(requires_python, marker)
}
6 changes: 6 additions & 0 deletions crates/uv-resolver/src/python_requirement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@ impl PythonRequirement {
pub fn target(&self) -> Option<&PythonTarget> {
self.target.as_ref()
}

/// Return the target version of Python as a "requires python" type,
/// if available.
pub(crate) fn requires_python(&self) -> Option<&RequiresPython> {
self.target().and_then(|target| target.as_requires_python())
}
}

#[derive(Debug, Clone, Eq, PartialEq)]
Expand Down
63 changes: 62 additions & 1 deletion crates/uv-resolver/src/requires_python.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ use std::collections::Bound;
use itertools::Itertools;
use pubgrub::range::Range;

use pep440_rs::{Version, VersionSpecifier, VersionSpecifiers};
use pep440_rs::{Operator, Version, VersionSpecifier, VersionSpecifiers};
use pep508_rs::{MarkerExpression, MarkerTree, MarkerValueVersion};

#[derive(thiserror::Error, Debug)]
pub enum RequiresPythonError {
Expand Down Expand Up @@ -169,6 +170,66 @@ impl RequiresPython {
pub fn bound(&self) -> &Bound<Version> {
&self.bound
}

/// Returns this `Requires-Python` specifier as an equivalent marker
/// expression utilizing the `python_version` marker field.
///
/// This is useful for comparing a `Requires-Python` specifier with
/// arbitrary marker expressions. For example, one can ask whether the
/// returned marker expression is disjoint with another marker expression.
/// If it is, then one can conclude that the `Requires-Python` specifier
/// excludes the dependency with that other marker expression.
///
/// If this `Requires-Python` specifier has no constraints, then this
/// 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 {
// 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) => {
(Operator::GreaterThanEqual, version.clone().without_local())
}
};
// For the `python_version` marker expression, it specifically only
// supports truncate major/minor versions of Python. This means that
// a `Requires-Python: 3.10.1` is satisfied by `python_version ==
// '3.10'`. So for disjointness checking, we need to ensure that the
// marker expression we generate for `Requires-Python` doesn't try to
// be overly selective about the patch version. We do this by keeping
// this part of our marker limited to the major and minor version
// components only.
let version_major_minor_only = Version::new(version.release().iter().take(2));
let expr_python_version = MarkerExpression::Version {
key: MarkerValueVersion::PythonVersion,
// OK because a version specifier is only invalid when the
// version is local (which is impossible here because we
// strip it above) or if the operator is ~= (which is also
// impossible here).
specifier: VersionSpecifier::from_version(op, version_major_minor_only).unwrap(),
};
let expr_python_full_version = MarkerExpression::Version {
key: MarkerValueVersion::PythonFullVersion,
// For `python_full_version`, we can use the entire
// version as-is.
//
// OK because a version specifier is only invalid when the
// version is local (which is impossible here because we
// strip it above) or if the operator is ~= (which is also
// impossible here).
specifier: VersionSpecifier::from_version(op, version).unwrap(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can this one unwrap?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reason as the one above. I duplicated the comment to make this clearer.

Although I suppose the constructor of RequiresPython permits any Version which means it could technically have a local segment. Likely a logic error, but I added without_local() to strip it from the version anyway.

};
MarkerTree::And(vec![
MarkerTree::Expression(expr_python_version),
MarkerTree::Expression(expr_python_full_version),
])
}
}

impl std::fmt::Display for RequiresPython {
Expand Down
22 changes: 22 additions & 0 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ use crate::pubgrub::{
PubGrubPriorities, PubGrubPython, PubGrubSpecifier,
};
use crate::python_requirement::PythonRequirement;
use crate::requires_python::RequiresPython;
use crate::resolution::ResolutionGraph;
pub(crate) use crate::resolver::availability::{
IncompletePackage, ResolverVersion, UnavailablePackage, UnavailableReason, UnavailableVersion,
Expand Down Expand Up @@ -94,6 +95,14 @@ struct ResolverState<InstalledPackages: InstalledPackagesProvider> {
/// When not set, the resolver is in "universal" mode.
markers: Option<MarkerEnvironment>,
python_requirement: PythonRequirement,
/// This is derived from `PythonRequirement` once at initialization
/// time. It's used in universal mode to filter our dependencies with
/// a `python_version` marker expression that has no overlap with the
/// `Requires-Python` specifier.
///
/// This is non-None if and only if the resolver is operating in
/// universal mode. (i.e., when `markers` is `None`.)
requires_python: Option<MarkerTree>,
selector: CandidateSelector,
index: InMemoryIndex,
installed_packages: InstalledPackages,
Expand Down Expand Up @@ -181,6 +190,16 @@ impl<Provider: ResolverProvider, InstalledPackages: InstalledPackagesProvider>
provider: Provider,
installed_packages: InstalledPackages,
) -> Result<Self, ResolveError> {
let requires_python = if markers.is_some() {
None
} else {
Some(
python_requirement
.requires_python()
.map(RequiresPython::to_marker_tree)
.unwrap_or_else(|| MarkerTree::And(vec![])),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this evaluate to None if requires_python is empty? What's the thinking behind MarkerTree::And(vec![])?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it would be correct for this to be None. But that's only because we treat None and "marker expression is always true" as equivalent. It just makes the contract a little nicer I guess:

    /// This is derived from `PythonRequirement` once at initialization
    /// time. It's used in universal mode to filter our dependencies with
    /// a `python_version` marker expression that has no overlap with the
    /// `Requires-Python` specifier.
    ///
    /// This is non-None if and only if the resolver is operating in
    /// universal mode. (i.e., when `markers` is `None`.)
    requires_python: Option<MarkerTree>,

If we used None here then this would need to be rephrased.

I don't have a strong opinion here or anything, but this just made the most sense to me at the time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I sometimes wonder if we could get away with not having Option<MarkerTree> anywhere.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I was hoping to avoid ever allowing And(vec![]) or Or(vec![]). Ideally that would be impossible in the future...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I had similar hopes for regex initially (e.g., banning "" and [^\s\S]), and it just turns out that it's useful to have types like this be able to represent "always match" and "never match." For example, you might be able to ban And(vec![]), but 1) you'll have to juggle it with Option<MarkerTree> which is annoying and 2) there an infinite number of MarkerTree values that have the same truth table as And(vec![]). Because of (2), you kinda need to deal with "always true" (and also, "never true") expressions anyway. So you might as well allow the trivial examples of them.

)
};
let state = ResolverState {
index: index.clone(),
git: git.clone(),
Expand All @@ -200,6 +219,7 @@ impl<Provider: ResolverProvider, InstalledPackages: InstalledPackagesProvider>
hasher: hasher.clone(),
markers: markers.cloned(),
python_requirement: python_requirement.clone(),
requires_python,
reporter: None,
installed_packages,
};
Expand Down Expand Up @@ -959,6 +979,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
&self.locals,
&self.git,
self.markers.as_ref(),
self.requires_python.as_ref(),
);

let dependencies = match dependencies {
Expand Down Expand Up @@ -1109,6 +1130,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
&self.locals,
&self.git,
self.markers.as_ref(),
self.requires_python.as_ref(),
)?;

for (dep_package, dep_version) in dependencies.iter() {
Expand Down
2 changes: 1 addition & 1 deletion crates/uv/tests/common/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub static EXCLUDE_NEWER: &str = "2024-03-25T00:00:00Z";
/// Using a find links url allows using `--index-url` instead of `--extra-index-url` in tests
/// to prevent dependency confusion attacks against our test suite.
pub const BUILD_VENDOR_LINKS_URL: &str =
"https://raw.githubusercontent.com/astral-sh/packse/0.3.18/vendor/links.html";
"https://raw.githubusercontent.com/astral-sh/packse/0.3.24/vendor/links.html";

#[doc(hidden)] // Macro and test context only, don't use directly.
pub const INSTA_FILTERS: &[(&str, &str)] = &[
Expand Down
Loading
Loading