From 4cc91cc6bb4ed158048d3dcdc0ac5a10c5b22a6b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 27 Mar 2024 21:03:44 -0400 Subject: [PATCH] Add convenience methods to `Manifest` to iterate over requirements (#2701) ## Summary These are repeated a bunch. It's nice to DRY them up and ensure the ordering is consistent. --- crates/uv-resolver/src/manifest.rs | 76 +++++++++++++++++++++- crates/uv-resolver/src/prerelease_mode.rs | 32 +-------- crates/uv-resolver/src/resolution_mode.rs | 22 +------ crates/uv-resolver/src/resolver/locals.rs | 21 +----- crates/uv-resolver/src/version_map.rs | 6 +- crates/uv-resolver/src/yanks.rs | 24 ++----- crates/uv/tests/pip_compile.rs | 79 +++++++++++++++++++++++ 7 files changed, 168 insertions(+), 92 deletions(-) diff --git a/crates/uv-resolver/src/manifest.rs b/crates/uv-resolver/src/manifest.rs index 213a5d166171..f8e3434644f5 100644 --- a/crates/uv-resolver/src/manifest.rs +++ b/crates/uv-resolver/src/manifest.rs @@ -1,5 +1,5 @@ use distribution_types::LocalEditable; -use pep508_rs::Requirement; +use pep508_rs::{MarkerEnvironment, Requirement}; use pypi_types::Metadata23; use uv_normalize::PackageName; use uv_types::RequestedRequirements; @@ -74,4 +74,78 @@ impl Manifest { lookaheads: Vec::new(), } } + + /// Return an iterator over all requirements, constraints, and overrides, in priority order, + /// such that requirements come first, followed by constraints, followed by overrides. + /// + /// At time of writing, this is used for: + /// - Determining which requirements should allow yanked versions. + /// - Determining which requirements should allow pre-release versions (e.g., `torch>=2.2.0a1`). + /// - Determining which requirements should allow direct URLs (e.g., `torch @ https://...`). + /// - Determining which requirements should allow local version specifiers (e.g., `torch==2.2.0+cpu`). + pub fn requirements<'a>( + &'a self, + markers: &'a MarkerEnvironment, + ) -> impl Iterator { + self.lookaheads + .iter() + .flat_map(|lookahead| { + lookahead + .requirements() + .iter() + .filter(|requirement| requirement.evaluate_markers(markers, lookahead.extras())) + }) + .chain(self.editables.iter().flat_map(|(editable, metadata)| { + metadata + .requires_dist + .iter() + .filter(|requirement| requirement.evaluate_markers(markers, &editable.extras)) + })) + .chain( + self.requirements + .iter() + .filter(|requirement| requirement.evaluate_markers(markers, &[])), + ) + .chain( + self.constraints + .iter() + .filter(|requirement| requirement.evaluate_markers(markers, &[])), + ) + .chain( + self.overrides + .iter() + .filter(|requirement| requirement.evaluate_markers(markers, &[])), + ) + } + + /// Return an iterator over the names of all direct dependency requirements. + /// + /// At time of writing, this is used for: + /// - Determining which packages should use the "lowest-compatible version" of a package, when + /// the `lowest-direct` strategy is in use. + pub fn direct_dependencies<'a>( + &'a self, + markers: &'a MarkerEnvironment, + ) -> impl Iterator { + self.lookaheads + .iter() + .flat_map(|lookahead| { + lookahead + .requirements() + .iter() + .filter(|requirement| requirement.evaluate_markers(markers, lookahead.extras())) + }) + .chain(self.editables.iter().flat_map(|(editable, metadata)| { + metadata + .requires_dist + .iter() + .filter(|requirement| requirement.evaluate_markers(markers, &editable.extras)) + })) + .chain( + self.requirements + .iter() + .filter(|requirement| requirement.evaluate_markers(markers, &[])), + ) + .map(|requirement| &requirement.name) + } } diff --git a/crates/uv-resolver/src/prerelease_mode.rs b/crates/uv-resolver/src/prerelease_mode.rs index 905d7910bb5e..7d911ef2a51d 100644 --- a/crates/uv-resolver/src/prerelease_mode.rs +++ b/crates/uv-resolver/src/prerelease_mode.rs @@ -61,21 +61,7 @@ impl PreReleaseStrategy { PreReleaseMode::IfNecessary => Self::IfNecessary, PreReleaseMode::Explicit => Self::Explicit( manifest - .requirements - .iter() - .chain(manifest.constraints.iter()) - .chain(manifest.overrides.iter()) - .filter(|requirement| requirement.evaluate_markers(markers, &[])) - .chain(manifest.lookaheads.iter().flat_map(|lookahead| { - lookahead.requirements().iter().filter(|requirement| { - requirement.evaluate_markers(markers, lookahead.extras()) - }) - })) - .chain(manifest.editables.iter().flat_map(|(editable, metadata)| { - metadata.requires_dist.iter().filter(|requirement| { - requirement.evaluate_markers(markers, &editable.extras) - }) - })) + .requirements(markers) .filter(|requirement| { let Some(version_or_url) = &requirement.version_or_url else { return false; @@ -95,21 +81,7 @@ impl PreReleaseStrategy { ), PreReleaseMode::IfNecessaryOrExplicit => Self::IfNecessaryOrExplicit( manifest - .requirements - .iter() - .chain(manifest.constraints.iter()) - .chain(manifest.overrides.iter()) - .filter(|requirement| requirement.evaluate_markers(markers, &[])) - .chain(manifest.lookaheads.iter().flat_map(|lookahead| { - lookahead.requirements().iter().filter(|requirement| { - requirement.evaluate_markers(markers, lookahead.extras()) - }) - })) - .chain(manifest.editables.iter().flat_map(|(editable, metadata)| { - metadata.requires_dist.iter().filter(|requirement| { - requirement.evaluate_markers(markers, &editable.extras) - }) - })) + .requirements(markers) .filter(|requirement| { let Some(version_or_url) = &requirement.version_or_url else { return false; diff --git a/crates/uv-resolver/src/resolution_mode.rs b/crates/uv-resolver/src/resolution_mode.rs index 97a823e16ffb..c750dff78d06 100644 --- a/crates/uv-resolver/src/resolution_mode.rs +++ b/crates/uv-resolver/src/resolution_mode.rs @@ -40,25 +40,9 @@ impl ResolutionStrategy { match mode { ResolutionMode::Highest => Self::Highest, ResolutionMode::Lowest => Self::Lowest, - ResolutionMode::LowestDirect => Self::LowestDirect( - // Consider `requirements` and dependencies of any local requirements to be "direct" dependencies. - manifest - .requirements - .iter() - .filter(|requirement| requirement.evaluate_markers(markers, &[])) - .chain(manifest.lookaheads.iter().flat_map(|lookahead| { - lookahead.requirements().iter().filter(|requirement| { - requirement.evaluate_markers(markers, lookahead.extras()) - }) - })) - .chain(manifest.editables.iter().flat_map(|(editable, metadata)| { - metadata.requires_dist.iter().filter(|requirement| { - requirement.evaluate_markers(markers, &editable.extras) - }) - })) - .map(|requirement| requirement.name.clone()) - .collect(), - ), + ResolutionMode::LowestDirect => { + Self::LowestDirect(manifest.direct_dependencies(markers).cloned().collect()) + } } } } diff --git a/crates/uv-resolver/src/resolver/locals.rs b/crates/uv-resolver/src/resolver/locals.rs index dffd8e2398b7..263c4ae6d633 100644 --- a/crates/uv-resolver/src/resolver/locals.rs +++ b/crates/uv-resolver/src/resolver/locals.rs @@ -23,25 +23,8 @@ impl Locals { let mut required: FxHashMap = FxHashMap::default(); // Add all direct requirements and constraints. There's no need to look for conflicts, - // since conflicting versions will be tracked upstream. - for requirement in - manifest - .requirements - .iter() - .chain(manifest.constraints.iter()) - .chain(manifest.overrides.iter()) - .filter(|requirement| requirement.evaluate_markers(markers, &[])) - .chain(manifest.lookaheads.iter().flat_map(|lookahead| { - lookahead.requirements().iter().filter(|requirement| { - requirement.evaluate_markers(markers, lookahead.extras()) - }) - })) - .chain(manifest.editables.iter().flat_map(|(editable, metadata)| { - metadata.requires_dist.iter().filter(|requirement| { - requirement.evaluate_markers(markers, &editable.extras) - }) - })) - { + // since conflicts will be enforced by the solver. + for requirement in manifest.requirements(markers) { if let Some(version_or_url) = requirement.version_or_url.as_ref() { for local in iter_locals(version_or_url) { required.insert(requirement.name.clone(), local); diff --git a/crates/uv-resolver/src/version_map.rs b/crates/uv-resolver/src/version_map.rs index c58b4e8b01f9..8b803a3ce662 100644 --- a/crates/uv-resolver/src/version_map.rs +++ b/crates/uv-resolver/src/version_map.rs @@ -136,10 +136,10 @@ impl VersionMap { /// stored in this map. For example, the versions `1.2.0` and `1.2` are /// semantically equivalent, but when converted to strings, they are /// distinct. - pub(crate) fn get_with_version<'a>( - &'a self, + pub(crate) fn get_with_version( + &self, version: &Version, - ) -> Option<(&'a Version, &'a PrioritizedDist)> { + ) -> Option<(&Version, &PrioritizedDist)> { match self.inner { VersionMapInner::Eager(ref map) => map.get_key_value(version), VersionMapInner::Lazy(ref lazy) => lazy.get_with_version(version), diff --git a/crates/uv-resolver/src/yanks.rs b/crates/uv-resolver/src/yanks.rs index 139426c35e6c..2ebb6f1252ce 100644 --- a/crates/uv-resolver/src/yanks.rs +++ b/crates/uv-resolver/src/yanks.rs @@ -4,8 +4,7 @@ use pep440_rs::Version; use pep508_rs::{MarkerEnvironment, VersionOrUrl}; use uv_normalize::PackageName; -use crate::preferences::Preference; -use crate::Manifest; +use crate::{Manifest, Preference}; /// A set of package versions that are permitted, even if they're marked as yanked by the /// relevant index. @@ -15,24 +14,9 @@ pub struct AllowedYanks(FxHashMap>); impl AllowedYanks { pub fn from_manifest(manifest: &Manifest, markers: &MarkerEnvironment) -> Self { let mut allowed_yanks = FxHashMap::>::default(); - for requirement in - manifest - .requirements - .iter() - .chain(manifest.constraints.iter()) - .chain(manifest.overrides.iter()) - .chain(manifest.preferences.iter().map(Preference::requirement)) - .filter(|requirement| requirement.evaluate_markers(markers, &[])) - .chain(manifest.lookaheads.iter().flat_map(|lookahead| { - lookahead.requirements().iter().filter(|requirement| { - requirement.evaluate_markers(markers, lookahead.extras()) - }) - })) - .chain(manifest.editables.iter().flat_map(|(editable, metadata)| { - metadata.requires_dist.iter().filter(|requirement| { - requirement.evaluate_markers(markers, &editable.extras) - }) - })) + for requirement in manifest + .requirements(markers) + .chain(manifest.preferences.iter().map(Preference::requirement)) { let Some(VersionOrUrl::VersionSpecifier(specifiers)) = &requirement.version_or_url else { diff --git a/crates/uv/tests/pip_compile.rs b/crates/uv/tests/pip_compile.rs index 48f27f025cf9..3ebf022ba9a3 100644 --- a/crates/uv/tests/pip_compile.rs +++ b/crates/uv/tests/pip_compile.rs @@ -1815,6 +1815,85 @@ fn allowed_transitive_url_path_dependency() -> Result<()> { Ok(()) } +/// A dependency with conflicting URLs in `requirements.in` and `constraints.txt` should arguably +/// be ignored if the dependency has an override. However, we currently error in this case. +#[test] +fn requirement_constraint_override_url() -> Result<()> { + let context = TestContext::new("3.12"); + + let requirements_in = context.temp_dir.child("requirements.in"); + requirements_in.write_str("anyio @ https://files.pythonhosted.org/packages/db/4d/3970183622f0330d3c23d9b8a5f52e365e50381fd484d08e3285104333d3/anyio-4.3.0.tar.gz")?; + + let constraints_txt = context.temp_dir.child("constraints.txt"); + constraints_txt.write_str("anyio @ https://files.pythonhosted.org/packages/14/fd/2f20c40b45e4fb4324834aea24bd4afdf1143390242c0b33774da0e2e34f/anyio-4.3.0-py3-none-any.whl")?; + + let overrides_txt = context.temp_dir.child("overrides.txt"); + overrides_txt.write_str("anyio==3.7.0")?; + + uv_snapshot!(context.compile() + .arg("requirements.in") + .arg("--constraint") + .arg("constraints.txt") + .arg("--override") + .arg("overrides.txt"), @r###" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + error: Requirements contain conflicting URLs for package `anyio`: + - https://files.pythonhosted.org/packages/db/4d/3970183622f0330d3c23d9b8a5f52e365e50381fd484d08e3285104333d3/anyio-4.3.0.tar.gz + - https://files.pythonhosted.org/packages/14/fd/2f20c40b45e4fb4324834aea24bd4afdf1143390242c0b33774da0e2e34f/anyio-4.3.0-py3-none-any.whl + "### + ); + + Ok(()) +} + +/// A dependency that uses a pre-release marker in `requirements.in` should be overridden by a +/// non-pre-release version in `overrides.txt`. We currently allow Flask to use a pre-release below, +/// but probably shouldn't. +#[test] +fn requirement_override_prerelease() -> Result<()> { + let context = TestContext::new("3.12"); + + let requirements_in = context.temp_dir.child("requirements.in"); + requirements_in.write_str("flask<2.0.0rc4")?; + + let overrides_txt = context.temp_dir.child("overrides.txt"); + overrides_txt.write_str("flask<2.0.1,!=2.0.0")?; + + uv_snapshot!(context.compile() + .arg("requirements.in") + .arg("--override") + .arg("overrides.txt"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + # This file was autogenerated by uv via the following command: + # uv pip compile --cache-dir [CACHE_DIR] --exclude-newer 2024-03-25T00:00:00Z requirements.in --override overrides.txt + click==8.1.7 + # via flask + flask==2.0.0rc2 + itsdangerous==2.1.2 + # via flask + jinja2==3.1.3 + # via flask + markupsafe==2.1.5 + # via + # jinja2 + # werkzeug + werkzeug==3.0.1 + # via flask + + ----- stderr ----- + Resolved 6 packages in [TIME] + "### + ); + + Ok(()) +} + /// Resolve packages from all optional dependency groups in a `pyproject.toml` file. #[test] fn compile_pyproject_toml_all_extras() -> Result<()> {