Skip to content

Commit

Permalink
Respect fork markers in --resolution-mode=lowest-direct (#8839)
Browse files Browse the repository at this point in the history
## Summary

Previously, given:

```toml
dependencies = [
    "pycountry >= 22.1.10",
    "setuptools >= 50.0.0; python_version>='3.12'"
]
```

We'd solve for the lowest version of setuptools (with _no_ lower-bound
constraint) in the `python_version < '3.12'` complement.

Closes #8819.
  • Loading branch information
charliermarsh authored Nov 5, 2024
1 parent d238642 commit 26e3511
Show file tree
Hide file tree
Showing 5 changed files with 119 additions and 26 deletions.
10 changes: 7 additions & 3 deletions crates/uv-resolver/src/candidate_selector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ impl CandidateSelector {
"Selecting candidate for {package_name} with range {range} with {} remote versions",
version_maps.iter().map(VersionMap::len).sum::<usize>(),
);
let highest = self.use_highest_version(package_name);
let highest = self.use_highest_version(package_name, env);

let allow_prerelease = match self.prerelease_strategy.allows(package_name, env) {
AllowPrerelease::Yes => true,
Expand Down Expand Up @@ -359,12 +359,16 @@ impl CandidateSelector {

/// By default, we select the latest version, but we also allow using the lowest version instead
/// to check the lower bounds.
pub(crate) fn use_highest_version(&self, package_name: &PackageName) -> bool {
pub(crate) fn use_highest_version(
&self,
package_name: &PackageName,
env: &ResolverEnvironment,
) -> bool {
match &self.resolution_strategy {
ResolutionStrategy::Highest => true,
ResolutionStrategy::Lowest => false,
ResolutionStrategy::LowestDirect(direct_dependencies) => {
!direct_dependencies.contains(package_name)
!direct_dependencies.contains(package_name, env)
}
}
}
Expand Down
20 changes: 9 additions & 11 deletions crates/uv-resolver/src/resolution_mode.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
use rustc_hash::FxHashSet;

use uv_normalize::PackageName;

use crate::resolver::{ForkMap, ForkSet};
use crate::{DependencyMode, Manifest, ResolverEnvironment};

#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, serde::Deserialize, serde::Serialize)]
Expand Down Expand Up @@ -39,7 +36,7 @@ pub(crate) enum ResolutionStrategy {
Lowest,
/// Resolve the lowest compatible version of any direct dependencies, and the highest
/// compatible version of any transitive dependencies.
LowestDirect(FxHashSet<PackageName>),
LowestDirect(ForkSet),
}

impl ResolutionStrategy {
Expand All @@ -52,12 +49,13 @@ impl ResolutionStrategy {
match mode {
ResolutionMode::Highest => Self::Highest,
ResolutionMode::Lowest => Self::Lowest,
ResolutionMode::LowestDirect => Self::LowestDirect(
manifest
.user_requirements(env, dependencies)
.map(|requirement| requirement.name.clone())
.collect(),
),
ResolutionMode::LowestDirect => {
let mut first_party = ForkMap::default();
for requirement in manifest.user_requirements(env, dependencies) {
first_party.add(&requirement, ());
}
Self::LowestDirect(first_party)
}
}
}
}
2 changes: 1 addition & 1 deletion crates/uv-resolver/src/resolver/batch_prefetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ impl BatchPrefetcher {
}
}
BatchPrefetchStrategy::InOrder { previous } => {
let mut range = if selector.use_highest_version(name) {
let mut range = if selector.use_highest_version(name, env) {
Range::strictly_lower_than(previous)
} else {
Range::strictly_higher_than(previous)
Expand Down
34 changes: 23 additions & 11 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ pub use crate::resolver::provider::{
use crate::resolver::reporter::Facade;
pub use crate::resolver::reporter::{BuildId, Reporter};
use crate::yanks::AllowedYanks;
use crate::{marker, DependencyMode, Exclusions, FlatIndex, Options};
use crate::{marker, DependencyMode, Exclusions, FlatIndex, Options, ResolutionMode};

mod availability;
mod batch_prefetch;
Expand Down Expand Up @@ -364,12 +364,22 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
// Walk over the selected versions, and mark them as preferences. We have to
// add forks back as to not override the preferences from the lockfile for
// the next fork
for (package, version) in &resolution.nodes {
preferences.insert(
package.name.clone(),
resolution.env.try_markers().cloned(),
version.clone(),
);
//
// If we're using a resolution mode that varies based on whether a dependency is
// direct or transitive, skip preferences, as we risk adding a preference from
// one fork (in which it's a transitive dependency) to another fork (in which
// it's direct).
if matches!(
self.options.resolution_mode,
ResolutionMode::Lowest | ResolutionMode::Highest
) {
for (package, version) in &resolution.nodes {
preferences.insert(
package.name.clone(),
resolution.env.try_markers().cloned(),
version.clone(),
);
}
}

resolutions.push(resolution);
Expand Down Expand Up @@ -1832,6 +1842,10 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
}
};

// We don't have access to the fork state when prefetching, so assume that
// pre-release versions are allowed.
let env = ResolverEnvironment::universal(vec![]);

// Try to find a compatible version. If there aren't any compatible versions,
// short-circuit.
let Some(candidate) = self.selector.select(
Expand All @@ -1841,9 +1855,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
&self.preferences,
&self.installed_packages,
&self.exclusions,
// We don't have access to the fork state when prefetching, so assume that
// pre-release versions are allowed.
&ResolverEnvironment::universal(vec![]),
&env,
) else {
return Ok(None);
};
Expand All @@ -1857,7 +1869,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
// often leads to failed attempts to build legacy versions of packages that are
// incompatible with modern build tools.
if dist.wheel().is_none() {
if !self.selector.use_highest_version(&package_name) {
if !self.selector.use_highest_version(&package_name, &env) {
if let Some((lower, _)) = range.iter().next() {
if lower == &Bound::Unbounded {
debug!("Skipping prefetch for unbounded minimum-version range: {package_name} ({range})");
Expand Down
79 changes: 79 additions & 0 deletions crates/uv/tests/it/pip_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12686,3 +12686,82 @@ fn unsupported_requires_python_dynamic_metadata() -> Result<()> {

Ok(())
}

/// Perform a universal resolution with a constraint, where the constraint itself has a marker.
#[test]
fn lowest_direct_fork() -> Result<()> {
let context = TestContext::new("3.10");
let requirements_in = context.temp_dir.child("requirements.in");
requirements_in.write_str(indoc::indoc! {r"
pycountry >= 22.1.10
setuptools >= 50.0.0 ; python_version >= '3.12'
"})?;

uv_snapshot!(context.filters(), windows_filters=false, context.pip_compile()
.arg("requirements.in")
.arg("--universal")
.arg("--resolution")
.arg("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] requirements.in --universal --resolution lowest-direct
pycountry==22.1.10
# via -r requirements.in
setuptools==50.0.0 ; python_full_version >= '3.12'
# via
# -r requirements.in
# pycountry
setuptools==69.2.0 ; python_full_version < '3.12'
# via
# -r requirements.in
# pycountry
----- stderr -----
Resolved 3 packages in [TIME]
"###
);

Ok(())
}

/// Perform a universal resolution with a constraint, where the constraint itself has a marker.
#[test]
fn lowest_fork() -> Result<()> {
let context = TestContext::new("3.10");
let requirements_in = context.temp_dir.child("requirements.in");
requirements_in.write_str(indoc::indoc! {r"
pycountry >= 22.1.10
setuptools >= 50.0.0 ; python_version >= '3.12'
"})?;

uv_snapshot!(context.filters(), windows_filters=false, context.pip_compile()
.arg("requirements.in")
.arg("--universal")
.arg("--resolution")
.arg("lowest"), @r###"
success: true
exit_code: 0
----- stdout -----
# This file was autogenerated by uv via the following command:
# uv pip compile --cache-dir [CACHE_DIR] requirements.in --universal --resolution lowest
pycountry==22.1.10
# via -r requirements.in
setuptools==0.7.2 ; python_full_version < '3.12'
# via
# -r requirements.in
# pycountry
setuptools==50.0.0 ; python_full_version >= '3.12'
# via
# -r requirements.in
# pycountry
----- stderr -----
Resolved 3 packages in [TIME]
warning: The transitive dependency `setuptools` is unpinned. Consider setting a lower bound with a constraint when using `--resolution-strategy lowest` to avoid using outdated versions.
"###
);

Ok(())
}

0 comments on commit 26e3511

Please sign in to comment.