Replace PubGrubDependencies by PubGrubDependency (#4481)

In the last PR (#4430), we flatten the requirements. In the next PR
(#4435), we want to pass `Url` around next to `PubGrubPackage` and
`Range<Version>` to keep track of which `Requirement`s added a url
across forking. This PR is a refactoring split out from #4435 that rolls
the dependency conversion into a single iterator and introduces a new
`PubGrubDependency` struct as abstraction over `(PubGrubPackage,
Range<Version>)` (or `(PubGrubPackage, Range<Version>,
VerbatimParsedUrl)` in the next PR), and it removes the now unnecessary
`PubGrubDependencies` abstraction.
This commit is contained in:
konsti 2024-06-26 00:11:52 +02:00 committed by GitHub
parent e6103dcab1
commit ff2f927579
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 122 additions and 121 deletions

View file

@ -15,76 +15,59 @@ use crate::pubgrub::{PubGrubPackage, PubGrubPackageInner};
use crate::resolver::{Locals, Urls}; use crate::resolver::{Locals, Urls};
use crate::{PubGrubSpecifier, ResolveError}; use crate::{PubGrubSpecifier, ResolveError};
#[derive(Debug)] #[derive(Clone, Debug)]
pub struct PubGrubDependencies(Vec<(PubGrubPackage, Range<Version>)>); pub(crate) struct PubGrubDependency {
pub(crate) package: PubGrubPackage,
pub(crate) version: Range<Version>,
}
impl PubGrubDependencies { impl PubGrubDependency {
/// Generate a set of PubGrub dependencies from a set of requirements. pub(crate) fn from_requirement<'a>(
#[allow(clippy::too_many_arguments)] requirement: &'a Requirement,
pub(crate) fn from_requirements( source_name: Option<&'a PackageName>,
flattened_requirements: &[&Requirement], urls: &'a Urls,
source_name: Option<&PackageName>, locals: &'a Locals,
urls: &Urls, git: &'a GitResolver,
locals: &Locals, ) -> impl Iterator<Item = Result<Self, ResolveError>> + 'a {
git: &GitResolver, // Add the package, plus any extra variants.
) -> Result<Self, ResolveError> { std::iter::once(None)
let mut dependencies = Vec::new(); .chain(requirement.extras.clone().into_iter().map(Some))
for requirement in flattened_requirements { .map(|extra| {
// Add the package, plus any extra variants. PubGrubRequirement::from_requirement(requirement, extra, urls, locals, git)
for result in std::iter::once(PubGrubRequirement::from_requirement( })
requirement, .filter_map_ok(move |pubgrub_requirement| {
None, let PubGrubRequirement { package, version } = pubgrub_requirement;
urls,
locals,
git,
))
.chain(requirement.extras.clone().into_iter().map(|extra| {
PubGrubRequirement::from_requirement(requirement, Some(extra), urls, locals, git)
})) {
let PubGrubRequirement { package, version } = result?;
match &*package { match &*package {
PubGrubPackageInner::Package { name, .. } => { PubGrubPackageInner::Package { name, .. } => {
// Detect self-dependencies. // Detect self-dependencies.
if source_name.is_some_and(|source_name| source_name == name) { if source_name.is_some_and(|source_name| source_name == name) {
warn!("{name} has a dependency on itself"); warn!("{name} has a dependency on itself");
continue; return None;
} }
dependencies.push((package.clone(), version.clone())); Some(PubGrubDependency {
} package: package.clone(),
PubGrubPackageInner::Marker { .. } => { version: version.clone(),
dependencies.push((package.clone(), version.clone())); })
} }
PubGrubPackageInner::Marker { .. } => Some(PubGrubDependency {
package: package.clone(),
version: version.clone(),
}),
PubGrubPackageInner::Extra { name, .. } => { PubGrubPackageInner::Extra { name, .. } => {
debug_assert!( debug_assert!(
!source_name.is_some_and(|source_name| source_name == name), !source_name.is_some_and(|source_name| source_name == name),
"extras not flattened for {name}" "extras not flattened for {name}"
); );
dependencies.push((package.clone(), version.clone())); Some(PubGrubDependency {
package: package.clone(),
version: version.clone(),
})
} }
_ => {} _ => None,
} }
} })
}
Ok(Self(dependencies))
}
/// Add a [`PubGrubPackage`] and [`PubGrubVersion`] range into the dependencies.
pub(crate) fn push(&mut self, package: PubGrubPackage, version: Range<Version>) {
self.0.push((package, version));
}
/// Iterate over the dependencies.
pub(crate) fn iter(&self) -> impl Iterator<Item = &(PubGrubPackage, Range<Version>)> {
self.0.iter()
}
}
/// Convert a [`PubGrubDependencies`] to a [`DependencyConstraints`].
impl From<PubGrubDependencies> for Vec<(PubGrubPackage, Range<Version>)> {
fn from(dependencies: PubGrubDependencies) -> Self {
dependencies.0
} }
} }

View file

@ -1,4 +1,4 @@
pub(crate) use crate::pubgrub::dependencies::PubGrubDependencies; pub(crate) use crate::pubgrub::dependencies::PubGrubDependency;
pub(crate) use crate::pubgrub::distribution::PubGrubDistribution; pub(crate) use crate::pubgrub::distribution::PubGrubDistribution;
pub(crate) use crate::pubgrub::package::{PubGrubPackage, PubGrubPackageInner, PubGrubPython}; pub(crate) use crate::pubgrub::package::{PubGrubPackage, PubGrubPackageInner, PubGrubPython};
pub(crate) use crate::pubgrub::priority::{PubGrubPriorities, PubGrubPriority}; pub(crate) use crate::pubgrub::priority::{PubGrubPriorities, PubGrubPriority};

View file

@ -44,8 +44,8 @@ use crate::manifest::Manifest;
use crate::pins::FilePins; use crate::pins::FilePins;
use crate::preferences::Preferences; use crate::preferences::Preferences;
use crate::pubgrub::{ use crate::pubgrub::{
PubGrubDependencies, PubGrubDistribution, PubGrubPackage, PubGrubPackageInner, PubGrubDependency, PubGrubDistribution, PubGrubPackage, PubGrubPackageInner, PubGrubPriorities,
PubGrubPriorities, PubGrubPython, PubGrubSpecifier, PubGrubPython, PubGrubSpecifier,
}; };
use crate::python_requirement::PythonRequirement; use crate::python_requirement::PythonRequirement;
use crate::resolution::ResolutionGraph; use crate::resolution::ResolutionGraph;
@ -943,13 +943,18 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
markers, markers,
); );
let dependencies = PubGrubDependencies::from_requirements( let dependencies = requirements
&requirements, .iter()
None, .flat_map(|requirement| {
&self.urls, PubGrubDependency::from_requirement(
&self.locals, requirement,
&self.git, None,
)?; &self.urls,
&self.locals,
&self.git,
)
})
.collect::<Result<Vec<_>, _>>()?;
(dependencies, None) (dependencies, None)
} }
@ -1075,13 +1080,18 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
markers, markers,
); );
let mut dependencies = PubGrubDependencies::from_requirements( let mut dependencies = requirements
&requirements, .iter()
Some(name), .flat_map(|requirement| {
&self.urls, PubGrubDependency::from_requirement(
&self.locals, requirement,
&self.git, Some(name),
)?; &self.urls,
&self.locals,
&self.git,
)
})
.collect::<Result<Vec<_>, _>>()?;
// If a package has metadata for an enabled dependency group, // If a package has metadata for an enabled dependency group,
// add a dependency from it to the same package with the group // add a dependency from it to the same package with the group
// enabled. // enabled.
@ -1090,15 +1100,15 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
if !metadata.dev_dependencies.contains_key(group) { if !metadata.dev_dependencies.contains_key(group) {
continue; continue;
} }
dependencies.push( dependencies.push(PubGrubDependency {
PubGrubPackage::from(PubGrubPackageInner::Dev { package: PubGrubPackage::from(PubGrubPackageInner::Dev {
name: name.clone(), name: name.clone(),
dev: group.clone(), dev: group.clone(),
marker: marker.clone(), marker: marker.clone(),
url: url.clone(), url: url.clone(),
}), }),
Range::singleton(version.clone()), version: Range::singleton(version.clone()),
); });
} }
} }
@ -1111,17 +1121,15 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
return Ok(Dependencies::Available( return Ok(Dependencies::Available(
[None, Some(marker)] [None, Some(marker)]
.into_iter() .into_iter()
.map(move |marker| { .map(move |marker| PubGrubDependency {
( package: PubGrubPackage::from(PubGrubPackageInner::Package {
PubGrubPackage::from(PubGrubPackageInner::Package { name: name.clone(),
name: name.clone(), extra: None,
extra: None, dev: None,
dev: None, marker: marker.cloned(),
marker: marker.cloned(), url: url.clone(),
url: url.clone(), }),
}), version: Range::singleton(version.clone()),
Range::singleton(version.clone()),
)
}) })
.collect(), .collect(),
)) ))
@ -1139,18 +1147,18 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
.into_iter() .into_iter()
.dedup() .dedup()
.flat_map(move |marker| { .flat_map(move |marker| {
[None, Some(extra)].into_iter().map(move |extra| { [None, Some(extra)]
( .into_iter()
PubGrubPackage::from(PubGrubPackageInner::Package { .map(move |extra| PubGrubDependency {
package: PubGrubPackage::from(PubGrubPackageInner::Package {
name: name.clone(), name: name.clone(),
extra: extra.cloned(), extra: extra.cloned(),
dev: None, dev: None,
marker: marker.cloned(), marker: marker.cloned(),
url: url.clone(), url: url.clone(),
}), }),
Range::singleton(version.clone()), version: Range::singleton(version.clone()),
) })
})
}) })
.collect(), .collect(),
)) ))
@ -1169,40 +1177,41 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
.into_iter() .into_iter()
.dedup() .dedup()
.flat_map(move |marker| { .flat_map(move |marker| {
[None, Some(dev)].into_iter().map(move |dev| { [None, Some(dev)]
( .into_iter()
PubGrubPackage::from(PubGrubPackageInner::Package { .map(move |dev| PubGrubDependency {
package: PubGrubPackage::from(PubGrubPackageInner::Package {
name: name.clone(), name: name.clone(),
extra: None, extra: None,
dev: dev.cloned(), dev: dev.cloned(),
marker: marker.cloned(), marker: marker.cloned(),
url: url.clone(), url: url.clone(),
}), }),
Range::singleton(version.clone()), version: Range::singleton(version.clone()),
) })
})
}) })
.collect(), .collect(),
)) ))
} }
}; };
for (dep_package, dep_version) in dependencies.iter() { for dependency in &dependencies {
let PubGrubDependency { package, version } = dependency;
if let Some(name) = name { if let Some(name) = name {
debug!("Adding transitive dependency for {name}=={version}: {dep_package}{dep_version}"); debug!("Adding transitive dependency for {name}=={version}: {package}{version}");
} else { } else {
// A dependency from the root package or requirements.txt. // A dependency from the root package or requirements.txt.
debug!("Adding direct dependency: {dep_package}{dep_version}"); debug!("Adding direct dependency: {package}{version}");
} }
// Update the package priorities. // Update the package priorities.
priorities.insert(dep_package, dep_version); priorities.insert(package, version);
// Emit a request to fetch the metadata for this package. // Emit a request to fetch the metadata for this package.
self.visit_package(dep_package, request_sink)?; self.visit_package(package, request_sink)?;
} }
Ok(Dependencies::Available(dependencies.into())) Ok(Dependencies::Available(dependencies))
} }
/// The regular and dev dependencies filtered by Python version and the markers of this fork, /// The regular and dev dependencies filtered by Python version and the markers of this fork,
@ -1648,12 +1657,12 @@ impl SolveState {
fn add_package_version_dependencies( fn add_package_version_dependencies(
&mut self, &mut self,
version: &Version, version: &Version,
dependencies: Vec<(PubGrubPackage, Range<Version>)>, dependencies: Vec<PubGrubDependency>,
prefetcher: &BatchPrefetcher, prefetcher: &BatchPrefetcher,
) -> Result<(), ResolveError> { ) -> Result<(), ResolveError> {
if dependencies if dependencies
.iter() .iter()
.any(|(dependency, _)| dependency == &self.next) .any(|dependency| dependency.package == self.next)
{ {
if enabled!(Level::DEBUG) { if enabled!(Level::DEBUG) {
prefetcher.log_tried_versions(); prefetcher.log_tried_versions();
@ -1667,7 +1676,10 @@ impl SolveState {
self.pubgrub.add_package_version_dependencies( self.pubgrub.add_package_version_dependencies(
self.next.clone(), self.next.clone(),
version.clone(), version.clone(),
dependencies, dependencies.into_iter().map(|dependency| {
let PubGrubDependency { package, version } = dependency;
(package, version)
}),
); );
Ok(()) Ok(())
} }
@ -1994,7 +2006,6 @@ enum Response {
/// This effectively distills the dependency metadata of a package down into /// This effectively distills the dependency metadata of a package down into
/// its pubgrub specific constituent parts: each dependency package has a range /// its pubgrub specific constituent parts: each dependency package has a range
/// of possible versions. /// of possible versions.
#[derive(Clone)]
enum Dependencies { enum Dependencies {
/// Package dependencies are not available. /// Package dependencies are not available.
Unavailable(UnavailableVersion), Unavailable(UnavailableVersion),
@ -2003,7 +2014,7 @@ enum Dependencies {
/// Note that in universal mode, it is possible and allowed for multiple /// Note that in universal mode, it is possible and allowed for multiple
/// `PubGrubPackage` values in this list to have the same package name. /// `PubGrubPackage` values in this list to have the same package name.
/// These conflicts are resolved via `Dependencies::fork`. /// These conflicts are resolved via `Dependencies::fork`.
Available(Vec<(PubGrubPackage, Range<Version>)>), Available(Vec<PubGrubDependency>),
} }
impl Dependencies { impl Dependencies {
@ -2022,13 +2033,16 @@ impl Dependencies {
}; };
let mut by_name: FxHashMap<&PackageName, PossibleForks> = FxHashMap::default(); let mut by_name: FxHashMap<&PackageName, PossibleForks> = FxHashMap::default();
for (index, (ref pkg, _)) in deps.iter().enumerate() { for (index, dependency) in deps.iter().enumerate() {
// A root can never be a dependency of another package, // A root can never be a dependency of another package,
// and a `Python` pubgrub package is never returned by // and a `Python` pubgrub package is never returned by
// `get_dependencies`. So a pubgrub package always has a // `get_dependencies`. So a pubgrub package always has a
// name in this context. // name in this context.
let name = pkg.name().expect("dependency always has a name"); let name = dependency
let marker = pkg.marker(); .package
.name()
.expect("dependency always has a name");
let marker = dependency.package.marker();
let Some(marker) = marker else { let Some(marker) = marker else {
// When no marker is found, it implies there is a dependency on // When no marker is found, it implies there is a dependency on
// this package that is unconditional with respect to marker // this package that is unconditional with respect to marker
@ -2157,7 +2171,7 @@ enum ForkedDependencies {
/// No forking occurred. /// No forking occurred.
/// ///
/// This is the same as `Dependencies::Available`. /// This is the same as `Dependencies::Available`.
Unforked(Vec<(PubGrubPackage, Range<Version>)>), Unforked(Vec<PubGrubDependency>),
/// Forked containers for all available package versions. /// Forked containers for all available package versions.
/// ///
/// Note that there is always at least two forks. If there would /// Note that there is always at least two forks. If there would
@ -2189,7 +2203,7 @@ struct Fork {
/// they should use `add_forked_package` or `add_nonfork_package`. Namely, /// they should use `add_forked_package` or `add_nonfork_package`. Namely,
/// it should be impossible for a package with a marker expression that is /// it should be impossible for a package with a marker expression that is
/// disjoint from the marker expression on this fork to be added. /// disjoint from the marker expression on this fork to be added.
dependencies: Vec<(PubGrubPackage, Range<Version>)>, dependencies: Vec<PubGrubDependency>,
/// The markers that provoked this fork. /// The markers that provoked this fork.
/// ///
/// So in the example above, the `a<2` fork would have /// So in the example above, the `a<2` fork would have
@ -2215,15 +2229,16 @@ impl Fork {
/// can cause the resolver to explore otherwise impossible resolutions, /// can cause the resolver to explore otherwise impossible resolutions,
/// and also run into conflicts (and thus a failed resolution) that don't /// and also run into conflicts (and thus a failed resolution) that don't
/// actually exist. /// actually exist.
fn add_forked_package(&mut self, (pkg, range): (PubGrubPackage, Range<Version>)) { fn add_forked_package(&mut self, dependency: PubGrubDependency) {
// OK because a package without a marker is unconditional and // OK because a package without a marker is unconditional and
// thus can never provoke a fork. // thus can never provoke a fork.
let marker = pkg let marker = dependency
.package
.marker() .marker()
.cloned() .cloned()
.expect("forked package always has a marker"); .expect("forked package always has a marker");
self.remove_disjoint_packages(&marker); self.remove_disjoint_packages(&marker);
self.dependencies.push((pkg, range)); self.dependencies.push(dependency);
// Each marker expression in a single fork is, // Each marker expression in a single fork is,
// by construction, overlapping with at least // by construction, overlapping with at least
// one other marker expression in this fork. // one other marker expression in this fork.
@ -2239,14 +2254,15 @@ impl Fork {
/// ///
/// It is only added if the markers on the given package are not disjoint /// It is only added if the markers on the given package are not disjoint
/// with this fork's markers. /// with this fork's markers.
fn add_nonfork_package(&mut self, (pkg, range): (PubGrubPackage, Range<Version>)) { fn add_nonfork_package(&mut self, dependency: PubGrubDependency) {
use crate::marker::is_disjoint; use crate::marker::is_disjoint;
if pkg if dependency
.package
.marker() .marker()
.map_or(true, |marker| !is_disjoint(marker, &self.markers)) .map_or(true, |marker| !is_disjoint(marker, &self.markers))
{ {
self.dependencies.push((pkg, range)); self.dependencies.push(dependency);
} }
} }
@ -2255,8 +2271,10 @@ impl Fork {
fn remove_disjoint_packages(&mut self, fork_marker: &MarkerTree) { fn remove_disjoint_packages(&mut self, fork_marker: &MarkerTree) {
use crate::marker::is_disjoint; use crate::marker::is_disjoint;
self.dependencies.retain(|(pkg, _)| { self.dependencies.retain(|dependency| {
pkg.marker() dependency
.package
.marker()
.map_or(true, |pkg_marker| !is_disjoint(pkg_marker, fork_marker)) .map_or(true, |pkg_marker| !is_disjoint(pkg_marker, fork_marker))
}); });
} }