From 8620b5a52f8ccd2a030cc60068c110ffc5e568a6 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 5 Mar 2024 09:25:06 -0800 Subject: [PATCH] Make direct dependency detection respect markers (#2207) ## Summary When determining "direct" dependencies, we need to ensure that we respect markers. In the linked issue, the user had an optional dependency like: ```toml [project.optional-dependencies] dev = [ "setuptools>=64", "setuptools_scm>=8" ] ``` By not respecting markers, we tried to resolve `setuptools` to the lowest-available version. However, since `setuptools>=64` _isn't_ enabled (since it's optional), we won't respect _that_ constraint. To be consistent, we need to omit optional dependencies just as we will at resolution time. Closes https://github.com/astral-sh/uv/issues/2203. ## Test Plan `cargo test` --- crates/uv-resolver/src/candidate_selector.rs | 39 ++++++++++----- crates/uv-resolver/src/prerelease_mode.rs | 32 +++++++------ crates/uv-resolver/src/resolution_mode.rs | 19 +++++--- crates/uv-resolver/src/resolver/mod.rs | 13 +---- crates/uv-resolver/src/yanks.rs | 39 ++++++++++----- crates/uv/tests/pip_compile.rs | 50 ++++++++++++++++++++ 6 files changed, 135 insertions(+), 57 deletions(-) diff --git a/crates/uv-resolver/src/candidate_selector.rs b/crates/uv-resolver/src/candidate_selector.rs index d62db6940..c83b8565e 100644 --- a/crates/uv-resolver/src/candidate_selector.rs +++ b/crates/uv-resolver/src/candidate_selector.rs @@ -4,7 +4,7 @@ use rustc_hash::FxHashMap; use distribution_types::CompatibleDist; use distribution_types::{DistributionMetadata, IncompatibleWheel, Name, PrioritizedDist}; use pep440_rs::Version; -use pep508_rs::{Requirement, VersionOrUrl}; +use pep508_rs::{MarkerEnvironment, Requirement, VersionOrUrl}; use uv_normalize::PackageName; use crate::prerelease_mode::PreReleaseStrategy; @@ -21,11 +21,23 @@ pub(crate) struct CandidateSelector { impl CandidateSelector { /// Return a [`CandidateSelector`] for the given [`Manifest`]. - pub(crate) fn for_resolution(manifest: &Manifest, options: Options) -> Self { + pub(crate) fn for_resolution( + options: Options, + manifest: &Manifest, + markers: &MarkerEnvironment, + ) -> Self { Self { - resolution_strategy: ResolutionStrategy::from_mode(options.resolution_mode, manifest), - prerelease_strategy: PreReleaseStrategy::from_mode(options.prerelease_mode, manifest), - preferences: Preferences::from(manifest.preferences.as_slice()), + resolution_strategy: ResolutionStrategy::from_mode( + options.resolution_mode, + manifest, + markers, + ), + prerelease_strategy: PreReleaseStrategy::from_mode( + options.prerelease_mode, + manifest, + markers, + ), + preferences: Preferences::from_requirements(manifest.preferences.as_slice(), markers), } } @@ -47,17 +59,15 @@ impl CandidateSelector { struct Preferences(FxHashMap); impl Preferences { - fn get(&self, package_name: &PackageName) -> Option<&Version> { - self.0.get(package_name) - } -} - -impl From<&[Requirement]> for Preferences { - fn from(requirements: &[Requirement]) -> Self { + /// Create a set of [`Preferences`] from a set of requirements. + fn from_requirements(requirements: &[Requirement], markers: &MarkerEnvironment) -> Self { Self( requirements .iter() .filter_map(|requirement| { + if !requirement.evaluate_markers(markers, &[]) { + return None; + } let Some(VersionOrUrl::VersionSpecifier(version_specifiers)) = requirement.version_or_url.as_ref() else { @@ -74,6 +84,11 @@ impl From<&[Requirement]> for Preferences { .collect(), ) } + + /// Return the pinned version for a package, if any. + fn get(&self, package_name: &PackageName) -> Option<&Version> { + self.0.get(package_name) + } } #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] diff --git a/crates/uv-resolver/src/prerelease_mode.rs b/crates/uv-resolver/src/prerelease_mode.rs index d37912fdf..c8942d333 100644 --- a/crates/uv-resolver/src/prerelease_mode.rs +++ b/crates/uv-resolver/src/prerelease_mode.rs @@ -1,6 +1,6 @@ use rustc_hash::FxHashSet; -use pep508_rs::VersionOrUrl; +use pep508_rs::{MarkerEnvironment, VersionOrUrl}; use uv_normalize::PackageName; use crate::Manifest; @@ -50,7 +50,11 @@ pub(crate) enum PreReleaseStrategy { } impl PreReleaseStrategy { - pub(crate) fn from_mode(mode: PreReleaseMode, manifest: &Manifest) -> Self { + pub(crate) fn from_mode( + mode: PreReleaseMode, + manifest: &Manifest, + markers: &MarkerEnvironment, + ) -> Self { match mode { PreReleaseMode::Disallow => Self::Disallow, PreReleaseMode::Allow => Self::Allow, @@ -61,12 +65,12 @@ impl PreReleaseStrategy { .iter() .chain(manifest.constraints.iter()) .chain(manifest.overrides.iter()) - .chain( - manifest - .editables - .iter() - .flat_map(|(_editable, metadata)| metadata.requires_dist.iter()), - ) + .filter(|requirement| requirement.evaluate_markers(markers, &[])) + .chain(manifest.editables.iter().flat_map(|(editable, metadata)| { + metadata.requires_dist.iter().filter(|requirement| { + requirement.evaluate_markers(markers, &editable.extras) + }) + })) .filter(|requirement| { let Some(version_or_url) = &requirement.version_or_url else { return false; @@ -90,12 +94,12 @@ impl PreReleaseStrategy { .iter() .chain(manifest.constraints.iter()) .chain(manifest.overrides.iter()) - .chain( - manifest - .editables - .iter() - .flat_map(|(_editable, metadata)| metadata.requires_dist.iter()), - ) + .filter(|requirement| requirement.evaluate_markers(markers, &[])) + .chain(manifest.editables.iter().flat_map(|(editable, metadata)| { + metadata.requires_dist.iter().filter(|requirement| { + requirement.evaluate_markers(markers, &editable.extras) + }) + })) .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 4bd6083b7..c121598e2 100644 --- a/crates/uv-resolver/src/resolution_mode.rs +++ b/crates/uv-resolver/src/resolution_mode.rs @@ -1,5 +1,6 @@ use rustc_hash::FxHashSet; +use pep508_rs::MarkerEnvironment; use uv_normalize::PackageName; use crate::Manifest; @@ -31,7 +32,11 @@ pub(crate) enum ResolutionStrategy { } impl ResolutionStrategy { - pub(crate) fn from_mode(mode: ResolutionMode, manifest: &Manifest) -> Self { + pub(crate) fn from_mode( + mode: ResolutionMode, + manifest: &Manifest, + markers: &MarkerEnvironment, + ) -> Self { match mode { ResolutionMode::Highest => Self::Highest, ResolutionMode::Lowest => Self::Lowest, @@ -40,12 +45,12 @@ impl ResolutionStrategy { manifest .requirements .iter() - .chain( - manifest - .editables - .iter() - .flat_map(|(_editable, metadata)| metadata.requires_dist.iter()), - ) + .filter(|requirement| requirement.evaluate_markers(markers, &[])) + .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(), ), diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index c8c98a388..5867b743c 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -158,21 +158,12 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> { index: &'a InMemoryIndex, provider: Provider, ) -> Result { - let selector = CandidateSelector::for_resolution(&manifest, options); - - // Determine the allowed yanked package versions - let allowed_yanks = manifest - .requirements - .iter() - .chain(manifest.constraints.iter()) - .collect(); - Ok(Self { index, unavailable_packages: DashMap::default(), visited: DashSet::default(), - selector, - allowed_yanks, + selector: CandidateSelector::for_resolution(options, &manifest, markers), + allowed_yanks: AllowedYanks::from_manifest(&manifest, markers), dependency_mode: options.dependency_mode, urls: Urls::from_manifest(&manifest, markers)?, project: manifest.project, diff --git a/crates/uv-resolver/src/yanks.rs b/crates/uv-resolver/src/yanks.rs index c6618d5b8..8c092bb3c 100644 --- a/crates/uv-resolver/src/yanks.rs +++ b/crates/uv-resolver/src/yanks.rs @@ -1,28 +1,33 @@ use rustc_hash::{FxHashMap, FxHashSet}; use pep440_rs::Version; -use pep508_rs::Requirement; +use pep508_rs::MarkerEnvironment; use uv_normalize::PackageName; +use crate::Manifest; + /// A set of package versions that are permitted, even if they're marked as yanked by the /// relevant index. #[derive(Debug, Default)] pub(crate) struct AllowedYanks(FxHashMap>); impl AllowedYanks { - /// Returns `true` if the given package version is allowed, even if it's marked as yanked by - /// the relevant index. - pub(crate) fn allowed(&self, package_name: &PackageName, version: &Version) -> bool { - self.0 - .get(package_name) - .is_some_and(|allowed_yanks| allowed_yanks.contains(version)) - } -} - -impl<'a> FromIterator<&'a Requirement> for AllowedYanks { - fn from_iter>(iter: T) -> Self { + pub(crate) fn from_manifest(manifest: &Manifest, markers: &MarkerEnvironment) -> Self { let mut allowed_yanks = FxHashMap::>::default(); - for requirement in iter { + for requirement in manifest + .requirements + .iter() + .chain(manifest.constraints.iter()) + .chain(manifest.overrides.iter()) + .chain(manifest.preferences.iter()) + .filter(|requirement| requirement.evaluate_markers(markers, &[])) + .chain(manifest.editables.iter().flat_map(|(editable, metadata)| { + metadata + .requires_dist + .iter() + .filter(|requirement| requirement.evaluate_markers(markers, &editable.extras)) + })) + { let Some(pep508_rs::VersionOrUrl::VersionSpecifier(specifiers)) = &requirement.version_or_url else { @@ -43,4 +48,12 @@ impl<'a> FromIterator<&'a Requirement> for AllowedYanks { } Self(allowed_yanks) } + + /// Returns `true` if the given package version is allowed, even if it's marked as yanked by + /// the relevant index. + pub(crate) fn allowed(&self, package_name: &PackageName, version: &Version) -> bool { + self.0 + .get(package_name) + .is_some_and(|allowed_yanks| allowed_yanks.contains(version)) + } } diff --git a/crates/uv/tests/pip_compile.rs b/crates/uv/tests/pip_compile.rs index 9aea78cbe..d3d295de4 100644 --- a/crates/uv/tests/pip_compile.rs +++ b/crates/uv/tests/pip_compile.rs @@ -4804,3 +4804,53 @@ dev = [ Ok(()) } + +/// Under `--resolution=lowest-direct`, ignore optional dependencies. +/// +/// In the below example, ensure that `setuptools` does not resolve to the lowest-available version. +#[test] +fn editable_optional_lowest_direct() -> Result<()> { + let context = TestContext::new("3.12"); + + // Create an editable package with an optional URL dependency. + let pyproject_toml = context.temp_dir.child("pyproject.toml"); + pyproject_toml.write_str( + r#"[project] +name = "example" +version = "0.0.0" +dependencies = ["setuptools-scm>=8.0.0"] +requires-python = '>=3.8' + +[project.optional-dependencies] +dev = ["setuptools"] +"#, + )?; + + // Write to a requirements file. + let requirements_in = context.temp_dir.child("requirements.in"); + requirements_in.write_str("-e .")?; + + uv_snapshot!(context.compile() + .arg("requirements.in") + .arg("--resolution=lowest-direct"), @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 2023-11-18T12:00:00Z requirements.in --resolution=lowest-direct + -e . + packaging==23.2 + # via setuptools-scm + setuptools==68.2.2 + # via setuptools-scm + setuptools-scm==8.0.1 + # via example + + ----- stderr ----- + Built 1 editable in [TIME] + Resolved 4 packages in [TIME] + "### + ); + + Ok(()) +}