diff --git a/crates/uv-resolver/src/error.rs b/crates/uv-resolver/src/error.rs index 5b81d2244..14dd2ffee 100644 --- a/crates/uv-resolver/src/error.rs +++ b/crates/uv-resolver/src/error.rs @@ -112,7 +112,7 @@ impl From> for ResolveError { /// Given a [`DerivationTree`], collapse any [`External::FromDependencyOf`] incompatibilities /// wrap an [`PubGrubPackageInner::Extra`] package. -fn collapse_extra_proxies( +fn collapse_proxies( derivation_tree: &mut DerivationTree, UnavailableReason>, ) { match derivation_tree { @@ -125,20 +125,32 @@ fn collapse_extra_proxies( ( DerivationTree::External(External::FromDependencyOf(package, ..)), ref mut cause, - ) if matches!(&**package, PubGrubPackageInner::Extra { .. }) => { - collapse_extra_proxies(cause); + ) if matches!( + &**package, + PubGrubPackageInner::Extra { .. } + | PubGrubPackageInner::Marker { .. } + | PubGrubPackageInner::Dev { .. } + ) => + { + collapse_proxies(cause); *derivation_tree = cause.clone(); } ( ref mut cause, DerivationTree::External(External::FromDependencyOf(package, ..)), - ) if matches!(&**package, PubGrubPackageInner::Extra { .. }) => { - collapse_extra_proxies(cause); + ) if matches!( + &**package, + PubGrubPackageInner::Extra { .. } + | PubGrubPackageInner::Marker { .. } + | PubGrubPackageInner::Dev { .. } + ) => + { + collapse_proxies(cause); *derivation_tree = cause.clone(); } _ => { - collapse_extra_proxies(Arc::make_mut(&mut derived.cause1)); - collapse_extra_proxies(Arc::make_mut(&mut derived.cause2)); + collapse_proxies(Arc::make_mut(&mut derived.cause1)); + collapse_proxies(Arc::make_mut(&mut derived.cause2)); } } } @@ -156,7 +168,7 @@ impl From> for ResolveError { } pubgrub::error::PubGrubError::Failure(inner) => Self::Failure(inner), pubgrub::error::PubGrubError::NoSolution(mut derivation_tree) => { - collapse_extra_proxies(&mut derivation_tree); + collapse_proxies(&mut derivation_tree); Self::NoSolution(NoSolutionError { derivation_tree, @@ -232,8 +244,9 @@ impl NoSolutionError { let mut available_versions = IndexMap::default(); for package in self.derivation_tree.packages() { match &**package { - PubGrubPackageInner::Root(_) => {} - PubGrubPackageInner::Python(_) => {} + PubGrubPackageInner::Root { .. } => {} + PubGrubPackageInner::Python { .. } => {} + PubGrubPackageInner::Marker { .. } => {} PubGrubPackageInner::Extra { .. } => {} PubGrubPackageInner::Dev { .. } => {} PubGrubPackageInner::Package { name, .. } => { diff --git a/crates/uv-resolver/src/pubgrub/dependencies.rs b/crates/uv-resolver/src/pubgrub/dependencies.rs index 3d307ff76..5ed8a042f 100644 --- a/crates/uv-resolver/src/pubgrub/dependencies.rs +++ b/crates/uv-resolver/src/pubgrub/dependencies.rs @@ -136,6 +136,9 @@ fn add_requirements( dependencies.push((package.clone(), version.clone())); } + PubGrubPackageInner::Marker { .. } => { + dependencies.push((package.clone(), version.clone())); + } PubGrubPackageInner::Extra { name, extra, .. } => { // Recursively add the dependencies of the current package (e.g., `black` depending on // `black[colorama]`). @@ -272,13 +275,12 @@ impl PubGrubRequirement { } Ok(Self { - package: PubGrubPackage::from(PubGrubPackageInner::Package { - name: requirement.name.clone(), + package: PubGrubPackage::from_url( + requirement.name.clone(), extra, - dev: None, - marker: requirement.marker.clone(), - url: Some(expected.clone()), - }), + requirement.marker.clone(), + expected.clone(), + ), version: Range::full(), }) } @@ -299,13 +301,12 @@ impl PubGrubRequirement { } Ok(Self { - package: PubGrubPackage::from(PubGrubPackageInner::Package { - name: requirement.name.clone(), + package: PubGrubPackage::from_url( + requirement.name.clone(), extra, - dev: None, - marker: requirement.marker.clone(), - url: Some(expected.clone()), - }), + requirement.marker.clone(), + expected.clone(), + ), version: Range::full(), }) } @@ -341,13 +342,12 @@ impl PubGrubRequirement { } Ok(Self { - package: PubGrubPackage::from(PubGrubPackageInner::Package { - name: requirement.name.clone(), + package: PubGrubPackage::from_url( + requirement.name.clone(), extra, - dev: None, - marker: requirement.marker.clone(), - url: Some(expected.clone()), - }), + requirement.marker.clone(), + expected.clone(), + ), version: Range::full(), }) } diff --git a/crates/uv-resolver/src/pubgrub/package.rs b/crates/uv-resolver/src/pubgrub/package.rs index 344dd2cc7..cf116264d 100644 --- a/crates/uv-resolver/src/pubgrub/package.rs +++ b/crates/uv-resolver/src/pubgrub/package.rs @@ -119,10 +119,19 @@ pub enum PubGrubPackageInner { marker: Option, url: Option, }, + /// A proxy package for a base package with a marker (e.g., `black; python_version >= "3.6"`). + /// + /// If a requirement has an extra _and_ a marker, it will be represented via the `Extra` variant + /// rather than the `Marker` variant. + Marker { + name: PackageName, + marker: MarkerTree, + url: Option, + }, } impl PubGrubPackage { - /// Create a [`PubGrubPackage`] from a package name and optional extra name. + /// Create a [`PubGrubPackage`] from a package name and version. pub(crate) fn from_package( name: PackageName, extra: Option, @@ -143,6 +152,8 @@ impl PubGrubPackage { marker, url, })) + } else if let Some(marker) = marker { + Self(Arc::new(PubGrubPackageInner::Marker { name, marker, url })) } else { Self(Arc::new(PubGrubPackageInner::Package { name, @@ -153,6 +164,43 @@ impl PubGrubPackage { })) } } + + /// Create a [`PubGrubPackage`] from a package name and URL. + pub(crate) fn from_url( + name: PackageName, + extra: Option, + mut marker: Option, + url: VerbatimParsedUrl, + ) -> Self { + // Remove all extra expressions from the marker, since we track extras + // separately. This also avoids an issue where packages added via + // extras end up having two distinct marker expressions, which in turn + // makes them two distinct packages. This results in PubGrub being + // unable to unify version constraints across such packages. + marker = marker.and_then(|m| m.simplify_extras_with(|_| true)); + if let Some(extra) = extra { + Self(Arc::new(PubGrubPackageInner::Extra { + name, + extra, + marker, + url: Some(url), + })) + } else if let Some(marker) = marker { + Self(Arc::new(PubGrubPackageInner::Marker { + name, + marker, + url: Some(url), + })) + } else { + Self(Arc::new(PubGrubPackageInner::Package { + name, + extra, + dev: None, + marker, + url: Some(url), + })) + } + } } #[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)] @@ -202,6 +250,7 @@ impl std::fmt::Display for PubGrubPackageInner { } => { write!(f, "{name}[{extra}]{{{marker}}}") } + Self::Marker { name, marker, .. } => write!(f, "{name}{{{marker}}}"), Self::Extra { name, extra, .. } => write!(f, "{name}[{extra}]"), Self::Dev { name, dev, .. } => write!(f, "{name}:{dev}"), } diff --git a/crates/uv-resolver/src/pubgrub/priority.rs b/crates/uv-resolver/src/pubgrub/priority.rs index 92a26570b..e293e8011 100644 --- a/crates/uv-resolver/src/pubgrub/priority.rs +++ b/crates/uv-resolver/src/pubgrub/priority.rs @@ -29,7 +29,10 @@ impl PubGrubPriorities { PubGrubPackageInner::Root(_) => {} PubGrubPackageInner::Python(_) => {} - PubGrubPackageInner::Extra { + PubGrubPackageInner::Marker { + name, url: None, .. + } + | PubGrubPackageInner::Extra { name, url: None, .. } | PubGrubPackageInner::Dev { @@ -70,7 +73,10 @@ impl PubGrubPriorities { } } } - PubGrubPackageInner::Extra { + PubGrubPackageInner::Marker { + name, url: Some(_), .. + } + | PubGrubPackageInner::Extra { name, url: Some(_), .. } | PubGrubPackageInner::Dev { @@ -111,6 +117,7 @@ impl PubGrubPriorities { match &**package { PubGrubPackageInner::Root(_) => Some(PubGrubPriority::Root), PubGrubPackageInner::Python(_) => Some(PubGrubPriority::Root), + PubGrubPackageInner::Marker { name, .. } => self.0.get(name).copied(), PubGrubPackageInner::Extra { name, .. } => self.0.get(name).copied(), PubGrubPackageInner::Dev { name, .. } => self.0.get(name).copied(), PubGrubPackageInner::Package { name, .. } => self.0.get(name).copied(), diff --git a/crates/uv-resolver/src/resolver/batch_prefetch.rs b/crates/uv-resolver/src/resolver/batch_prefetch.rs index 245a919b6..0368caba7 100644 --- a/crates/uv-resolver/src/resolver/batch_prefetch.rs +++ b/crates/uv-resolver/src/resolver/batch_prefetch.rs @@ -57,7 +57,7 @@ impl BatchPrefetcher { name, extra: None, dev: None, - marker: _marker, + marker: None, url: None, } = &**next else { @@ -166,11 +166,15 @@ impl BatchPrefetcher { // Only track base packages, no virtual packages from extras. if matches!( &*package, - PubGrubPackageInner::Package { extra: Some(_), .. } + PubGrubPackageInner::Package { + extra: None, + dev: None, + marker: None, + .. + } ) { - return; + *self.tried_versions.entry(package).or_default() += 1; } - *self.tried_versions.entry(package).or_default() += 1; } /// After 5, 10, 20, 40 tried versions, prefetch that many versions to start early but not diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index 2730ba068..ece717239 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -8,6 +8,7 @@ use std::thread; use dashmap::DashMap; use futures::{FutureExt, StreamExt, TryFutureExt}; +use itertools::Itertools; use pubgrub::error::PubGrubError; use pubgrub::range::Range; use pubgrub::solver::{Incompatibility, State}; @@ -579,9 +580,16 @@ impl ResolverState {} PubGrubPackageInner::Python(_) => {} - PubGrubPackageInner::Extra { .. } => {} - PubGrubPackageInner::Dev { .. } => {} - PubGrubPackageInner::Package { + PubGrubPackageInner::Marker { + name, url: None, .. + } + | PubGrubPackageInner::Extra { + name, url: None, .. + } + | PubGrubPackageInner::Dev { + name, url: None, .. + } + | PubGrubPackageInner::Package { name, url: None, .. } => { // Verify that the package is allowed under the hash-checking policy. @@ -594,7 +602,22 @@ impl ResolverState ResolverState( packages: impl Iterator)>, request_sink: &Sender, @@ -627,7 +650,7 @@ impl ResolverState ResolverState ResolverState ResolverState unreachable!(), PubGrubPackageInner::Package { ref name, .. } + | PubGrubPackageInner::Marker { ref name, .. } | PubGrubPackageInner::Extra { ref name, .. } | PubGrubPackageInner::Dev { ref name, .. } => name, }; @@ -980,21 +1012,6 @@ impl ResolverState { // If we're excluding transitive dependencies, short-circuit. if self.dependency_mode.is_direct() { - // If a package has a marker, add a dependency from it to the - // same package without markers. - if marker.is_some() { - return Ok(Dependencies::Available(vec![( - PubGrubPackage::from(PubGrubPackageInner::Package { - name: name.clone(), - extra: extra.clone(), - dev: dev.clone(), - marker: None, - url: url.clone(), - }), - Range::singleton(version.clone()), - )])); - } - // If an extra is provided, wait for the metadata to be available, since it's // still required for generating the lock file. let dist = match url { @@ -1132,90 +1149,82 @@ impl ResolverState Ok(Dependencies::Available( + [None, Some(marker)] + .into_iter() + .map(move |marker| { + ( + PubGrubPackage::from(PubGrubPackageInner::Package { + name: name.clone(), + extra: None, + dev: None, + marker: marker.cloned(), + url: url.clone(), + }), + Range::singleton(version.clone()), + ) + }) + .collect(), + )), + + // Add a dependency on both the extra and base package, with and without the marker. PubGrubPackageInner::Extra { name, extra, marker, url, - } => Ok(Dependencies::Available(vec![ - ( - PubGrubPackage::from(PubGrubPackageInner::Package { - name: name.clone(), - extra: None, - dev: None, - marker: marker.clone(), - url: url.clone(), - }), - Range::singleton(version.clone()), - ), - ( - PubGrubPackage::from(PubGrubPackageInner::Package { - name: name.clone(), - extra: Some(extra.clone()), - dev: None, - marker: marker.clone(), - url: url.clone(), - }), - Range::singleton(version.clone()), - ), - ])), + } => Ok(Dependencies::Available( + [None, marker.as_ref()] + .into_iter() + .dedup() + .flat_map(move |marker| { + [None, Some(extra)].into_iter().map(move |extra| { + ( + PubGrubPackage::from(PubGrubPackageInner::Package { + name: name.clone(), + extra: extra.cloned(), + dev: None, + marker: marker.cloned(), + url: url.clone(), + }), + Range::singleton(version.clone()), + ) + }) + }) + .collect(), + )), - // Add a dependency on both the development dependency group and base package. + // Add a dependency on both the development dependency group and base package, with and + // without the marker. PubGrubPackageInner::Dev { name, dev, marker, url, - } => Ok(Dependencies::Available(vec![ - ( - PubGrubPackage::from(PubGrubPackageInner::Package { - name: name.clone(), - extra: None, - dev: None, - marker: marker.clone(), - url: url.clone(), - }), - Range::singleton(version.clone()), - ), - ( - PubGrubPackage::from(PubGrubPackageInner::Package { - name: name.clone(), - extra: None, - dev: Some(dev.clone()), - marker: marker.clone(), - url: url.clone(), - }), - Range::singleton(version.clone()), - ), - ])), + } => Ok(Dependencies::Available( + [None, marker.as_ref()] + .into_iter() + .dedup() + .flat_map(move |marker| { + [None, Some(dev)].into_iter().map(move |dev| { + ( + PubGrubPackage::from(PubGrubPackageInner::Package { + name: name.clone(), + extra: None, + dev: dev.cloned(), + marker: marker.cloned(), + url: url.clone(), + }), + Range::singleton(version.clone()), + ) + }) + }) + .collect(), + )), } } @@ -1444,6 +1453,7 @@ impl ResolverState {} PubGrubPackageInner::Python(_) => {} + PubGrubPackageInner::Marker { .. } => {} PubGrubPackageInner::Extra { .. } => {} PubGrubPackageInner::Dev { .. } => {} PubGrubPackageInner::Package { @@ -1569,6 +1579,28 @@ impl SolveState { dependencies.entry(names).or_default().insert(versions); } + PubGrubPackageInner::Marker { + name: ref dependency_name, + .. + } => { + if self_name == dependency_name { + continue; + } + let names = ResolutionDependencyNames { + from: self_name.clone(), + to: dependency_name.clone(), + }; + let versions = ResolutionDependencyVersions { + from_version: self_version.clone(), + from_extra: self_extra.clone(), + from_dev: self_dev.clone(), + to_version: dependency_version.clone(), + to_extra: None, + to_dev: None, + }; + dependencies.entry(names).or_default().insert(versions); + } + PubGrubPackageInner::Extra { name: ref dependency_name, extra: ref dependency_extra, diff --git a/crates/uv/tests/lock_scenarios.rs b/crates/uv/tests/lock_scenarios.rs index 2aaecd43b..3edc2a060 100644 --- a/crates/uv/tests/lock_scenarios.rs +++ b/crates/uv/tests/lock_scenarios.rs @@ -802,7 +802,6 @@ fn fork_non_local_fork_marker_transitive() -> Result<()> { warning: `uv lock` is experimental and may change without warning. × No solution found when resolving dependencies: ╰─▶ Because package-b==1.0.0 depends on package-c{sys_platform == 'darwin'}>=2.0.0 and only package-c{sys_platform == 'darwin'}<=2.0.0 is available, we can conclude that package-b==1.0.0 depends on package-c{sys_platform == 'darwin'}==2.0.0. - And because package-c{sys_platform == 'darwin'}==2.0.0 depends on package-c==2.0.0 and package-c{sys_platform == 'linux'}==1.0.0 depends on package-c==1.0.0, we can conclude that package-b==1.0.0 and package-c{sys_platform == 'linux'}==1.0.0 are incompatible. And because only the following versions of package-c{sys_platform == 'linux'} are available: package-c{sys_platform == 'linux'}==1.0.0 package-c{sys_platform == 'linux'}>=2.0.0