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`
This commit is contained in:
Charlie Marsh 2024-03-05 09:25:06 -08:00 committed by GitHub
parent 7f07ada24c
commit 8620b5a52f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 135 additions and 57 deletions

View file

@ -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<PackageName, Version>);
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)]

View file

@ -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;

View file

@ -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(),
),

View file

@ -158,21 +158,12 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
index: &'a InMemoryIndex,
provider: Provider,
) -> Result<Self, ResolveError> {
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,

View file

@ -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<PackageName, FxHashSet<Version>>);
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<T: IntoIterator<Item = &'a Requirement>>(iter: T) -> Self {
pub(crate) fn from_manifest(manifest: &Manifest, markers: &MarkerEnvironment) -> Self {
let mut allowed_yanks = FxHashMap::<PackageName, FxHashSet<Version>>::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))
}
}