pioduino/uv
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

Track Markers via a PubGrub package variant (#4123)

Browse Source 
## Summary

This PR adds a lowering similar to that seen in
https://github.com/astral-sh/uv/pull/3100, but this time, for markers.
Like `PubGrubPackageInner::Extra`, we now have
`PubGrubPackageInner::Marker`. The dependencies of the `Marker` are
`PubGrubPackageInner::Package` with and without the marker.

As an example of why this is useful: assume we have `urllib3>=1.22.0` as
a direct dependency. Later, we see `urllib3 ; python_version > '3.7'` as
a transitive dependency. As-is, we might (for some reason) pick a very
old version of `urllib3` to satisfy `urllib3 ; python_version > '3.7'`,
then attempt to fetch its dependencies, which could even involve
building a very old version of `urllib3 ; python_version > '3.7'`. Once
we fetch the dependencies, we would see that `urllib3` at the same
version is _also_ a dependency (because we tack it on). In the new
scheme though, as soon as we "choose" the very old version of `urllib3 ;
python_version > '3.7'`, we'd then see that `urllib3` (the base package)
is also a dependency; so we see a conflict before we even fetch the
dependencies of the old variant.

With this, I can successfully resolve the case in #4099.

Closes https://github.com/astral-sh/uv/issues/4099.
This commit is contained in:
Charlie Marsh
2024-06-07 12:57:02 -07:00
committed by GitHub
parent 0f4f3b4714
commit bcfe88dfdc
7 changed files with 233 additions and 129 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/uv-resolver/src/error.rs
+23 -10
View File
@@ -112,7 +112,7 @@ impl<T> From<tokio::sync::mpsc::error::SendError<T>> 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<PubGrubPackage, Range<Version>, 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<pubgrub::error::PubGrubError<UvDependencyProvider>> 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, .. } => {
crates/uv-resolver/src/pubgrub/dependencies.rs
+18 -18
View File
@@ -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(),
})
}
crates/uv-resolver/src/pubgrub/package.rs
+50 -1
View File
@@ -119,10 +119,19 @@ pub enum PubGrubPackageInner {
marker: Option<MarkerTree>,
url: Option<VerbatimParsedUrl>,
},
/// 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<VerbatimParsedUrl>,
},
}
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<ExtraName>,
@@ -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<ExtraName>,
mut marker: Option<MarkerTree>,
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}"),
}
crates/uv-resolver/src/pubgrub/priority.rs
+9 -2
View File
@@ -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(),
crates/uv-resolver/src/resolver/batch_prefetch.rs
+8 -4
View File
@@ -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
crates/uv-resolver/src/resolver/mod.rs
+125 -93
View File
@@ -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<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
match &**package {
PubGrubPackageInner::Root(_) => {}
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<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
request_sink.blocking_send(Request::Package(name.clone()))?;
}
}
PubGrubPackageInner::Package {
PubGrubPackageInner::Marker {
name,
url: Some(url),
..
}
| PubGrubPackageInner::Extra {
name,
url: Some(url),
..
}
| PubGrubPackageInner::Dev {
name,
url: Some(url),
..
}
| PubGrubPackageInner::Package {
name,
url: Some(url),
..
@@ -615,7 +638,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
}
/// Visit the set of [`PubGrubPackage`] candidates prior to selection. This allows us to fetch
/// metadata for all of the packages in parallel.
/// metadata for all packages in parallel.
fn pre_visit<'data>(
packages: impl Iterator<Item = (&'data PubGrubPackage, &'data Range<Version>)>,
request_sink: &Sender<Request>,
@@ -627,7 +650,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
name,
extra: None,
dev: None,
marker: _marker,
marker: None,
url: None,
} = &**package
else {
@@ -662,7 +685,12 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
Ok(None)
}
PubGrubPackageInner::Extra {
PubGrubPackageInner::Marker {
name,
url: Some(url),
..
}
| PubGrubPackageInner::Extra {
name,
url: Some(url),
..
@@ -758,7 +786,10 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
Ok(Some(ResolverVersion::Available(version.clone())))
}
PubGrubPackageInner::Extra {
PubGrubPackageInner::Marker {
name, url: None, ..
}
| PubGrubPackageInner::Extra {
name, url: None, ..
}
| PubGrubPackageInner::Dev {
@@ -888,6 +919,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
// extras are enabled, we shouldn't need to fork.
PubGrubPackageInner::Root(_) | PubGrubPackageInner::Python(_) => unreachable!(),
PubGrubPackageInner::Package { ref name, .. }
| PubGrubPackageInner::Marker { ref name, .. }
| PubGrubPackageInner::Extra { ref name, .. }
| PubGrubPackageInner::Dev { ref name, .. } => name,
};
@@ -980,21 +1012,6 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
} => {
// 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<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
}
}
// If a package has a marker, add a dependency from it to the
// same package without markers.
//
// At time of writing, AG doesn't fully understand why we need
// this, but one explanation is that without it, there is no
// way to connect two different `PubGrubPackage` values with
// the same package name but different markers. With different
// markers, they would be considered wholly distinct packages.
// But this dependency-on-itself-without-markers forces PubGrub
// to unify the constraints across what would otherwise be two
// distinct packages.
if marker.is_some() {
dependencies.push(
PubGrubPackage::from(PubGrubPackageInner::Package {
name: name.clone(),
extra: extra.clone(),
dev: dev.clone(),
marker: None,
url: url.clone(),
}),
Range::singleton(version.clone()),
);
}
Ok(Dependencies::Available(dependencies.into()))
}
// Add a dependency on both the extra and base package.
// Add a dependency on both the marker and base package.
PubGrubPackageInner::Marker { name, marker, url } => 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<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
match &**package {
PubGrubPackageInner::Root(_) => {}
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,
crates/uv/tests/lock_scenarios.rs
-1
View File
@@ -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