Skip to content

Fix handling of != intersections in requires-python #7897

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.

Sign up for GitHub

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

Merged
charliermarsh merged 1 commit into from
Oct 9, 2024
Merged

Fix handling of != intersections in requires-python #7897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
78 changes: 30 additions & 48 deletions crates/uv-resolver/src/requires_python.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
use std::cmp::Ordering;
use std::collections::Bound;
use std::collections::{BTreeSet, Bound};
use std::ops::Deref;

use itertools::Itertools;
use pubgrub::Range;

use uv_distribution_filename::WheelFilename;
Expand Down Expand Up @@ -70,37 +69,39 @@ impl RequiresPython {
pub fn intersection<'a>(
specifiers: impl Iterator<Item = &'a VersionSpecifiers>,
) -> Result<Option<Self>, RequiresPythonError> {
// Convert to PubGrub range and perform an intersection.
let range = specifiers
.into_iter()
.map(crate::pubgrub::PubGrubSpecifier::from_release_specifiers)
.fold_ok(None, |range: Option<Range<Version>>, requires_python| {
if let Some(range) = range {
Some(range.intersection(&requires_python.into()))
} else {
Some(requires_python.into())
let mut combined: BTreeSet<VersionSpecifier> = BTreeSet::new();
let mut lower_bound: LowerBound = LowerBound(Bound::Unbounded);
let mut upper_bound: UpperBound = UpperBound(Bound::Unbounded);

for specifier in specifiers {
// Convert to PubGrub range and perform an intersection.
let requires_python =
crate::pubgrub::PubGrubSpecifier::from_release_specifiers(specifier)?;
if let Some((lower, upper)) = requires_python.bounding_range() {
let lower = LowerBound(lower.cloned());
let upper = UpperBound(upper.cloned());
if lower > lower_bound {
lower_bound = lower;
}
if upper < upper_bound {
upper_bound = upper;
}
})?;
}

// Track all specifiers for the final result.
combined.extend(specifier.iter().cloned());
}

let Some(range) = range else {
if combined.is_empty() {
return Ok(None);
};
}

// Extract the bounds.
let (lower_bound, upper_bound) = range
.bounding_range()
.map(|(lower_bound, upper_bound)| (lower_bound.cloned(), upper_bound.cloned()))
.unwrap_or((Bound::Unbounded, Bound::Unbounded));

// Convert back to PEP 440 specifiers.
let specifiers = range
.iter()
.flat_map(VersionSpecifier::from_release_only_bounds)
.collect();
Copy link
Copy Markdown
Member Author

@charliermarsh charliermarsh Oct 3, 2024

Choose a reason for hiding this comment

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

This isn't quite correct -- we can't just AND these, since a != specifier gets converted to < and > the value. So instead we retain the actual specifiers now.

// Compute the intersection by combining the specifiers.
let specifiers = combined.into_iter().collect();

Ok(Some(Self {
specifiers,
Copy link
Copy Markdown
Member

@zanieb zanieb Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Does the doc comment for this function (intersection) need to be updated?

range: RequiresPythonRange(LowerBound(lower_bound), UpperBound(upper_bound)),
range: RequiresPythonRange(lower_bound, upper_bound),
}))
}

Expand Down Expand Up @@ -231,29 +232,10 @@ impl RequiresPython {
.map(|(lower, _)| lower)
.unwrap_or(&Bound::Unbounded);

// We want, e.g., `requires_python_lower` to be `>=3.8` and `version_lower` to be
// `>=3.7`.
// We want, e.g., `self.range.lower()` to be `>=3.8` and `target` to be `>=3.7`.
//
// That is: `version_lower` should be less than or equal to `requires_python_lower`.
match (target, self.range.lower().as_ref()) {
(Bound::Included(target_lower), Bound::Included(requires_python_lower)) => {
target_lower <= requires_python_lower
}
(Bound::Excluded(target_lower), Bound::Included(requires_python_lower)) => {
target_lower < requires_python_lower
}
(Bound::Included(target_lower), Bound::Excluded(requires_python_lower)) => {
target_lower <= requires_python_lower
}
(Bound::Excluded(target_lower), Bound::Excluded(requires_python_lower)) => {
target_lower < requires_python_lower
}
// If the dependency has no lower bound, then it supports all versions.
(Bound::Unbounded, _) => true,
// If we have no lower bound, then there must be versions we support that the
// dependency does not.
(_, Bound::Unbounded) => false,
}
// That is: `target` should be less than or equal to `self.range.lower()`.
*self.range.lower() >= LowerBound(target.clone())
}

/// Returns the [`VersionSpecifiers`] for the `Requires-Python` specifier.
Expand Down
85 changes: 83 additions & 2 deletions crates/uv/tests/lock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3814,6 +3814,87 @@ fn lock_requires_python_star() -> Result<()> {
Ok(())
}

/// Lock a requirement from PyPI, respecting the `Requires-Python` metadata. In this case,
/// `Requires-Python` uses the != operator.
#[test]
fn lock_requires_python_not_equal() -> Result<()> {
let context = TestContext::new("3.11");

let lockfile = context.temp_dir.join("uv.lock");

let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str(
r#"
[project]
name = "project"
version = "0.1.0"
requires-python = ">3.10, !=3.10.9, <3.13"
dependencies = ["iniconfig"]

[build-system]
requires = ["setuptools>=42"]
build-backend = "setuptools.build_meta"
"#,
)?;

uv_snapshot!(context.filters(), context.lock(), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Resolved 2 packages in [TIME]
"###);

let lock = fs_err::read_to_string(&lockfile).unwrap();

insta::with_settings!({
filters => context.filters(),
}, {
assert_snapshot!(
lock, @r###"
version = 1
requires-python = ">3.10, !=3.10.9, <3.13"

[options]
exclude-newer = "2024-03-25T00:00:00Z"

[[package]]
name = "iniconfig"
version = "2.0.0"
source = { registry = "https://pypi.org/simple" }
sdist = { url = "https://files.pythonhosted.org/packages/d7/4b/cbd8e699e64a6f16ca3a8220661b5f83792b3017d0f79807cb8708d33913/iniconfig-2.0.0.tar.gz", hash = "sha256:2d91e135bf72d31a410b17c16da610a82cb55f6b0477d1a902134b24a455b8b3", size = 4646 }
wheels = [
{ url = "https://files.pythonhosted.org/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl", hash = "sha256:b6a85871a79d2e3b22d2d1b94ac2824226a63c6b741c88f7ae975f18b6778374", size = 5892 },
]

[[package]]
name = "project"
version = "0.1.0"
source = { editable = "." }
dependencies = [
{ name = "iniconfig" },
]

[package.metadata]
requires-dist = [{ name = "iniconfig" }]
"###
);
});

// Re-run with `--locked`.
uv_snapshot!(context.filters(), context.lock().arg("--locked"), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Resolved 2 packages in [TIME]
"###);

Ok(())
}

/// Lock a requirement from PyPI, respecting the `Requires-Python` metadata. In this case,
/// `Requires-Python` uses a pre-release specifier, but it's effectively ignored, as `>=3.11.0b1`
/// is interpreted as equivalent to `>=3.11.0`.
Expand Down Expand Up @@ -3855,7 +3936,7 @@ fn lock_requires_python_pre() -> Result<()> {
assert_snapshot!(
lock, @r###"
version = 1
requires-python = ">=3.11"
requires-python = ">=3.11b1"
Copy link
Copy Markdown
Member Author

@charliermarsh charliermarsh Oct 3, 2024

Choose a reason for hiding this comment

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

We now retain this more-or-less verbatim. I think this is ok since we'll ignore these anywhere that it matters when we actually use it.


[options]
exclude-newer = "2024-03-25T00:00:00Z"
Expand Down Expand Up @@ -12873,7 +12954,7 @@ fn lock_simplified_environments() -> Result<()> {
assert_snapshot!(
lock, @r###"
version = 1
requires-python = "==3.11.*"
requires-python = ">=3.11, <3.12"
resolution-markers = [
"sys_platform == 'darwin'",
"sys_platform != 'darwin'",
Expand Down
2 changes: 1 addition & 1 deletion crates/uv/tests/sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ fn mixed_requires_python() -> Result<()> {

----- stderr -----
Using CPython 3.8.[X] interpreter at: [PYTHON-3.8]
error: The requested interpreter resolved to Python 3.8.[X], which is incompatible with the project's Python requirement: `>=3.12`. However, a workspace member (`bird-feeder`) supports Python >=3.8. To install the workspace member on its own, navigate to `packages/bird-feeder`, then run `uv venv --python 3.8.[X]` followed by `uv pip install -e .`.
error: The requested interpreter resolved to Python 3.8.[X], which is incompatible with the project's Python requirement: `>=3.8, >=3.12`. However, a workspace member (`bird-feeder`) supports Python >=3.8. To install the workspace member on its own, navigate to `packages/bird-feeder`, then run `uv venv --python 3.8.[X]` followed by `uv pip install -e .`.
Copy link
Copy Markdown
Member Author

@charliermarsh charliermarsh Oct 3, 2024

Choose a reason for hiding this comment

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

This is a pain. Maybe we do want to apply some simplifications here...? Like ignoring redundant clauses?

Copy link
Copy Markdown
Member Author

@charliermarsh charliermarsh Oct 3, 2024

Choose a reason for hiding this comment

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

I guess we could do something like: only add the specifier if it changes the computed bounds? That could work.

Copy link
Copy Markdown
Member

@zanieb zanieb Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can you use Range::subset_of?

Copy link
Copy Markdown
Member

@zanieb zanieb Oct 8, 2024

Choose a reason for hiding this comment

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

Here's a POC of using subset_of. I'm guessing this can be done in a simpler way?

#8026

Copy link
Copy Markdown
Member Author

@charliermarsh charliermarsh Oct 9, 2024

Choose a reason for hiding this comment

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

Good idea. I will do something like that to at least avoid these common cases.

Copy link
Copy Markdown
Member

@konstin konstin Oct 9, 2024

Choose a reason for hiding this comment

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

My expectation is that we should be able to represent RequiresPython as a single pubgrub range, extracting the range's lower or upper bound as needed; My quick attempt at that failed though because there seem other constraints too.

Copy link
Copy Markdown
Member Author

@charliermarsh charliermarsh Oct 9, 2024

Choose a reason for hiding this comment

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

I ended up writing a routine to convert to an "optimal" PEP 440 specifier using similar logic to the existing method, but for multiple bounds. It's kind of tedious but does work as expected.

"###);

Ok(())
Expand Down