From a552f7430828e18e7ce98dfc5d96fb1592f9b1bc Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 14 Nov 2024 15:25:34 -0500 Subject: [PATCH] Refactor `Resolution` type to retain dependency graph (#9106) ## Summary This PR should not contain any user-visible changes, but the goal is to refactor the `Resolution` type to retain a dependency graph. We want to be able to explain _why_ a given package was excluded on error (see: https://github.com/astral-sh/uv/issues/8962), which in turn requires that at install time, we can go back and figure out the dependency chain. At present, `Resolution` is just a map from package name to distribution; this PR remodels it as a graph in which each node is a package, and the edges contain markers plus extras or dependency groups. --- Cargo.lock | 1 + crates/uv-distribution-types/Cargo.toml | 1 + .../uv-distribution-types/src/resolution.rs | 151 +++++---- .../uv-resolver/src/lock/requirements_txt.rs | 11 +- crates/uv-resolver/src/lock/target.rs | 290 +++++++++++++----- crates/uv-resolver/src/resolution/display.rs | 18 +- crates/uv-resolver/src/resolution/output.rs | 123 ++++++-- crates/uv-types/src/hash.rs | 3 +- crates/uv/src/commands/pip/operations.rs | 4 +- crates/uv/src/commands/project/sync.rs | 14 +- 10 files changed, 433 insertions(+), 183 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0eb453856302..e09d62343ddd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4884,6 +4884,7 @@ dependencies = [ "fs-err", "itertools 0.13.0", "jiff", + "petgraph", "rkyv", "rustc-hash", "schemars", diff --git a/crates/uv-distribution-types/Cargo.toml b/crates/uv-distribution-types/Cargo.toml index 92b9c18712b5..123a55ad41f2 100644 --- a/crates/uv-distribution-types/Cargo.toml +++ b/crates/uv-distribution-types/Cargo.toml @@ -33,6 +33,7 @@ bitflags = { workspace = true } fs-err = { workspace = true } itertools = { workspace = true } jiff = { workspace = true } +petgraph = { workspace = true } rkyv = { workspace = true } rustc-hash = { workspace = true } schemars = { workspace = true, optional = true } diff --git a/crates/uv-distribution-types/src/resolution.rs b/crates/uv-distribution-types/src/resolution.rs index 46922af9f96c..8950e991cf9c 100644 --- a/crates/uv-distribution-types/src/resolution.rs +++ b/crates/uv-distribution-types/src/resolution.rs @@ -1,55 +1,76 @@ -use std::collections::BTreeMap; use uv_distribution_filename::DistExtension; use uv_normalize::{ExtraName, GroupName, PackageName}; +use uv_pep508::MarkerTree; use uv_pypi_types::{HashDigest, RequirementSource}; use crate::{BuiltDist, Diagnostic, Dist, Name, ResolvedDist, SourceDist}; /// A set of packages pinned at specific versions. +/// +/// This is similar to [`ResolverOutput`], but represents a resolution for a subset of all +/// marker environments. For example, the resolution is guaranteed to contain at most one version +/// for a given package. #[derive(Debug, Default, Clone)] pub struct Resolution { - packages: BTreeMap, - hashes: BTreeMap>, + graph: petgraph::graph::DiGraph, diagnostics: Vec, } impl Resolution { /// Create a new resolution from the given pinned packages. - pub fn new( - packages: BTreeMap, - hashes: BTreeMap>, - diagnostics: Vec, - ) -> Self { + pub fn new(graph: petgraph::graph::DiGraph) -> Self { Self { - packages, - hashes, - diagnostics, + graph, + diagnostics: Vec::new(), } } - /// Return the hashes for the given package name, if they exist. - pub fn get_hashes(&self, package_name: &PackageName) -> &[HashDigest] { - self.hashes.get(package_name).map_or(&[], Vec::as_slice) + /// Return the underlying graph of the resolution. + pub fn graph(&self) -> &petgraph::graph::DiGraph { + &self.graph + } + + /// Add [`Diagnostics`] to the resolution. + #[must_use] + pub fn with_diagnostics(mut self, diagnostics: Vec) -> Self { + self.diagnostics.extend(diagnostics); + self } - /// Iterate over the [`PackageName`] entities in this resolution. - pub fn packages(&self) -> impl Iterator { - self.packages.keys() + /// Return the hashes for the given package name, if they exist. + pub fn hashes(&self) -> impl Iterator { + self.graph + .node_indices() + .filter_map(move |node| match &self.graph[node] { + Node::Dist { + dist, + hashes, + install, + .. + } if *install => Some((dist, hashes.as_slice())), + _ => None, + }) } /// Iterate over the [`ResolvedDist`] entities in this resolution. pub fn distributions(&self) -> impl Iterator { - self.packages.values() + self.graph + .raw_nodes() + .iter() + .filter_map(|node| match &node.weight { + Node::Dist { dist, install, .. } if *install => Some(dist), + _ => None, + }) } /// Return the number of distributions in this resolution. pub fn len(&self) -> usize { - self.packages.len() + self.distributions().count() } /// Return `true` if there are no pinned packages in this resolution. pub fn is_empty(&self) -> bool { - self.packages.is_empty() + self.distributions().next().is_none() } /// Return the [`ResolutionDiagnostic`]s that were produced during resolution. @@ -59,44 +80,31 @@ impl Resolution { /// Filter the resolution to only include packages that match the given predicate. #[must_use] - pub fn filter(self, predicate: impl Fn(&ResolvedDist) -> bool) -> Self { - let packages = self - .packages - .into_iter() - .filter(|(_, dist)| predicate(dist)) - .collect::>(); - let hashes = self - .hashes - .into_iter() - .filter(|(name, _)| packages.contains_key(name)) - .collect(); - let diagnostics = self.diagnostics.clone(); - Self { - packages, - hashes, - diagnostics, + pub fn filter(mut self, predicate: impl Fn(&ResolvedDist) -> bool) -> Self { + for node in self.graph.node_weights_mut() { + if let Node::Dist { dist, install, .. } = node { + if !predicate(dist) { + *install = false; + } + } } + self } /// Map over the resolved distributions in this resolution. + /// + /// For efficiency, the map function should return `None` if the resolved distribution is + /// unchanged. #[must_use] - pub fn map(self, predicate: impl Fn(ResolvedDist) -> ResolvedDist) -> Self { - let packages = self - .packages - .into_iter() - .map(|(name, dist)| (name, predicate(dist))) - .collect::>(); - let hashes = self - .hashes - .into_iter() - .filter(|(name, _)| packages.contains_key(name)) - .collect(); - let diagnostics = self.diagnostics.clone(); - Self { - packages, - hashes, - diagnostics, + pub fn map(mut self, predicate: impl Fn(&ResolvedDist) -> Option) -> Self { + for node in self.graph.node_weights_mut() { + if let Node::Dist { dist, .. } = node { + if let Some(transformed) = predicate(dist) { + *dist = transformed; + } + } } + self } } @@ -166,15 +174,46 @@ impl Diagnostic for ResolutionDiagnostic { } } +/// A node in the resolution, along with whether its been filtered out. +/// +/// We retain filtered nodes as we still need to be able to trace dependencies through the graph +/// (e.g., to determine why a package was included in the resolution). +#[derive(Debug, Clone)] +pub enum Node { + Root, + Dist { + dist: ResolvedDist, + hashes: Vec, + install: bool, + }, +} + +/// An edge in the resolution graph, along with the marker that must be satisfied to traverse it. +#[derive(Debug, Clone)] +pub enum Edge { + Prod(MarkerTree), + Optional(ExtraName, MarkerTree), + Dev(GroupName, MarkerTree), +} + +impl Edge { + /// Return the [`MarkerTree`] for this edge. + pub fn marker(&self) -> &MarkerTree { + match self { + Self::Prod(marker) => marker, + Self::Optional(_, marker) => marker, + Self::Dev(_, marker) => marker, + } + } +} + impl From<&ResolvedDist> for RequirementSource { fn from(resolved_dist: &ResolvedDist) -> Self { match resolved_dist { - ResolvedDist::Installable { dist, .. } => match dist { + ResolvedDist::Installable { dist, version } => match dist { Dist::Built(BuiltDist::Registry(wheels)) => RequirementSource::Registry { specifier: uv_pep440::VersionSpecifiers::from( - uv_pep440::VersionSpecifier::equals_version( - wheels.best_wheel().filename.version.clone(), - ), + uv_pep440::VersionSpecifier::equals_version(version.clone()), ), index: Some(wheels.best_wheel().index.url().clone()), }, diff --git a/crates/uv-resolver/src/lock/requirements_txt.rs b/crates/uv-resolver/src/lock/requirements_txt.rs index df7f900c63d0..e3edcfd01d27 100644 --- a/crates/uv-resolver/src/lock/requirements_txt.rs +++ b/crates/uv-resolver/src/lock/requirements_txt.rs @@ -6,7 +6,7 @@ use std::path::{Component, Path, PathBuf}; use either::Either; use petgraph::visit::IntoNodeReferences; -use petgraph::{Directed, Graph}; +use petgraph::Graph; use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet}; use url::Url; @@ -22,8 +22,6 @@ use crate::graph_ops::marker_reachability; use crate::lock::{Package, PackageId, Source}; use crate::{InstallTarget, LockError}; -type LockGraph<'lock> = Graph, Edge, Directed>; - /// An export of a [`Lock`] that renders in `requirements.txt` format. #[derive(Debug)] pub struct RequirementsTxtExport<'lock> { @@ -42,7 +40,7 @@ impl<'lock> RequirementsTxtExport<'lock> { install_options: &'lock InstallOptions, ) -> Result { let size_guess = target.lock().packages.len(); - let mut petgraph = LockGraph::with_capacity(size_guess, size_guess); + let mut petgraph = Graph::with_capacity(size_guess, size_guess); let mut inverse = FxHashMap::with_capacity_and_hasher(size_guess, FxBuildHasher); let mut queue: VecDeque<(&Package, Option<&ExtraName>)> = VecDeque::new(); @@ -282,16 +280,13 @@ impl std::fmt::Display for RequirementsTxtExport<'_> { } } -/// A node in the [`LockGraph`]. +/// A node in the graph. #[derive(Debug, Clone, PartialEq, Eq)] enum Node<'lock> { Root, Package(&'lock Package), } -/// The edges of the [`LockGraph`]. -type Edge = MarkerTree; - /// A flat requirement, with its associated marker. #[derive(Debug, Clone, PartialEq, Eq)] struct Requirement<'lock> { diff --git a/crates/uv-resolver/src/lock/target.rs b/crates/uv-resolver/src/lock/target.rs index d0fdd4ea4d51..03bfabcef876 100644 --- a/crates/uv-resolver/src/lock/target.rs +++ b/crates/uv-resolver/src/lock/target.rs @@ -1,11 +1,13 @@ -use std::collections::{BTreeMap, VecDeque}; - use either::Either; -use rustc_hash::FxHashSet; +use petgraph::Graph; +use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet}; +use std::collections::hash_map::Entry; +use std::collections::{BTreeMap, VecDeque}; use uv_configuration::{BuildOptions, DevGroupsManifest, ExtrasSpecification, InstallOptions}; -use uv_distribution_types::{Resolution, ResolvedDist}; +use uv_distribution_types::{Edge, Node, Resolution, ResolvedDist}; use uv_normalize::{ExtraName, GroupName, PackageName, DEV_DEPENDENCIES}; +use uv_pep508::MarkerTree; use uv_platform_tags::Tags; use uv_pypi_types::{ResolverMarkerEnvironment, VerbatimParsedUrl}; use uv_workspace::dependency_groups::{DependencyGroupError, FlatDependencyGroups}; @@ -155,12 +157,18 @@ impl<'env> InstallTarget<'env> { build_options: &BuildOptions, install_options: &InstallOptions, ) -> Result { + let size_guess = self.lock().packages.len(); + let mut petgraph = Graph::with_capacity(size_guess, size_guess); + let mut inverse = FxHashMap::with_capacity_and_hasher(size_guess, FxBuildHasher); + let mut queue: VecDeque<(&Package, Option<&ExtraName>)> = VecDeque::new(); let mut seen = FxHashSet::default(); + let root = petgraph.add_node(Node::Root); + // Add the workspace packages to the queue. for root_name in self.packages() { - let root = self + let dist = self .lock() .find_by_name(root_name) .map_err(|_| LockErrorKind::MultipleRootPackages { @@ -170,21 +178,30 @@ impl<'env> InstallTarget<'env> { name: root_name.clone(), })?; - if dev.prod() { - // Add the base package. - queue.push_back((root, None)); + // Add the workspace package to the graph. + let index = petgraph.add_node(if dev.prod() { + self.package_to_node(dist, tags, build_options, install_options)? + } else { + self.non_installable_node(dist, tags)? + }); + inverse.insert(&dist.id, index); - // Add any extras. + // Add an edge from the root. + petgraph.add_edge(root, index, Edge::Prod(MarkerTree::TRUE)); + + if dev.prod() { + // Push its dependencies on the queue. + queue.push_back((dist, None)); match extras { ExtrasSpecification::None => {} ExtrasSpecification::All => { - for extra in root.optional_dependencies.keys() { - queue.push_back((root, Some(extra))); + for extra in dist.optional_dependencies.keys() { + queue.push_back((dist, Some(extra))); } } ExtrasSpecification::Some(extras) => { for extra in extras { - queue.push_back((root, Some(extra))); + queue.push_back((dist, Some(extra))); } } } @@ -192,16 +209,44 @@ impl<'env> InstallTarget<'env> { // Add any dev dependencies. for group in dev.iter() { - for dep in root.dependency_groups.get(group).into_iter().flatten() { - if dep.complexified_marker.evaluate(marker_env, &[]) { - let dep_dist = self.lock().find_by_id(&dep.package_id); - if seen.insert((&dep.package_id, None)) { - queue.push_back((dep_dist, None)); + for dep in dist.dependency_groups.get(group).into_iter().flatten() { + if !dep.complexified_marker.evaluate(marker_env, &[]) { + continue; + } + + let dep_dist = self.lock().find_by_id(&dep.package_id); + + // Add the dependency to the graph. + let dep_index = match inverse.entry(&dep.package_id) { + Entry::Vacant(entry) => { + let index = petgraph.add_node(self.package_to_node( + dep_dist, + tags, + build_options, + install_options, + )?); + entry.insert(index); + index } - for extra in &dep.extra { - if seen.insert((&dep.package_id, Some(extra))) { - queue.push_back((dep_dist, Some(extra))); - } + Entry::Occupied(entry) => { + let index = *entry.get(); + index + } + }; + + petgraph.add_edge( + index, + dep_index, + Edge::Dev(group.clone(), dep.complexified_marker.clone()), + ); + + // Push its dependencies on the queue. + if seen.insert((&dep.package_id, None)) { + queue.push_back((dep_dist, None)); + } + for extra in &dep.extra { + if seen.insert((&dep.package_id, Some(extra))) { + queue.push_back((dep_dist, Some(extra))); } } } @@ -215,70 +260,173 @@ impl<'env> InstallTarget<'env> { .map_err(|err| LockErrorKind::DependencyGroup { err })?; for group in dev.iter() { for dependency in groups.get(group).into_iter().flatten() { - if dependency.marker.evaluate(marker_env, &[]) { - let root_name = &dependency.name; - let root = self - .lock() - .find_by_markers(root_name, marker_env) - .map_err(|_| LockErrorKind::MultipleRootPackages { - name: root_name.clone(), - })? - .ok_or_else(|| LockErrorKind::MissingRootPackage { - name: root_name.clone(), - })?; - - // Add the base package. - queue.push_back((root, None)); - - // Add any extras. - for extra in &dependency.extras { - queue.push_back((root, Some(extra))); + if !dependency.marker.evaluate(marker_env, &[]) { + continue; + } + + let root_name = &dependency.name; + let dist = self + .lock() + .find_by_markers(root_name, marker_env) + .map_err(|_| LockErrorKind::MultipleRootPackages { + name: root_name.clone(), + })? + .ok_or_else(|| LockErrorKind::MissingRootPackage { + name: root_name.clone(), + })?; + + // Add the workspace package to the graph. + let index = match inverse.entry(&dist.id) { + Entry::Vacant(entry) => { + let index = petgraph.add_node(self.package_to_node( + dist, + tags, + build_options, + install_options, + )?); + entry.insert(index); + index + } + Entry::Occupied(entry) => { + let index = *entry.get(); + index } + }; + + // Add an edge from the root. + petgraph.add_edge( + root, + index, + Edge::Dev(group.clone(), dependency.marker.clone()), + ); + + // Push its dependencies on the queue. + queue.push_back((dist, None)); + for extra in &dependency.extras { + queue.push_back((dist, Some(extra))); } } } - let mut map = BTreeMap::default(); - let mut hashes = BTreeMap::default(); - while let Some((dist, extra)) = queue.pop_front() { + while let Some((package, extra)) = queue.pop_front() { let deps = if let Some(extra) = extra { - Either::Left(dist.optional_dependencies.get(extra).into_iter().flatten()) + Either::Left( + package + .optional_dependencies + .get(extra) + .into_iter() + .flatten(), + ) } else { - Either::Right(dist.dependencies.iter()) + Either::Right(package.dependencies.iter()) }; for dep in deps { - if dep.complexified_marker.evaluate(marker_env, &[]) { - let dep_dist = self.lock().find_by_id(&dep.package_id); - if seen.insert((&dep.package_id, None)) { - queue.push_back((dep_dist, None)); - } - for extra in &dep.extra { - if seen.insert((&dep.package_id, Some(extra))) { - queue.push_back((dep_dist, Some(extra))); - } - } + if !dep.complexified_marker.evaluate(marker_env, &[]) { + continue; } - } - if install_options.include_package( - &dist.id.name, - self.project_name(), - self.lock().members(), - ) { - map.insert( - dist.id.name.clone(), - ResolvedDist::Installable { - dist: dist.to_dist( - self.workspace().install_path(), - TagPolicy::Required(tags), + + let dep_dist = self.lock().find_by_id(&dep.package_id); + + // Add the dependency to the graph. + let dep_index = match inverse.entry(&dep.package_id) { + Entry::Vacant(entry) => { + let index = petgraph.add_node(self.package_to_node( + dep_dist, + tags, build_options, - )?, - version: dist.id.version.clone(), + install_options, + )?); + entry.insert(index); + index + } + Entry::Occupied(entry) => { + let index = *entry.get(); + index + } + }; + + // Add the edge. + let index = inverse[&package.id]; + petgraph.add_edge( + index, + dep_index, + if let Some(extra) = extra { + Edge::Optional(extra.clone(), dep.complexified_marker.clone()) + } else { + Edge::Prod(dep.complexified_marker.clone()) }, ); - hashes.insert(dist.id.name.clone(), dist.hashes()); + + // Push its dependencies on the queue. + if seen.insert((&dep.package_id, None)) { + queue.push_back((dep_dist, None)); + } + for extra in &dep.extra { + if seen.insert((&dep.package_id, Some(extra))) { + queue.push_back((dep_dist, Some(extra))); + } + } } } - let diagnostics = vec![]; - Ok(Resolution::new(map, hashes, diagnostics)) + + Ok(Resolution::new(petgraph)) + } + + /// Create an installable [`Node`] from a [`Package`]. + fn installable_node( + &self, + package: &Package, + tags: &Tags, + build_options: &BuildOptions, + ) -> Result { + let dist = package.to_dist( + self.workspace().install_path(), + TagPolicy::Required(tags), + build_options, + )?; + let version = package.version().clone(); + let dist = ResolvedDist::Installable { dist, version }; + let hashes = package.hashes(); + Ok(Node::Dist { + dist, + hashes, + install: true, + }) + } + + /// Create a non-installable [`Node`] from a [`Package`]. + fn non_installable_node(&self, package: &Package, tags: &Tags) -> Result { + let dist = package.to_dist( + self.workspace().install_path(), + TagPolicy::Preferred(tags), + &BuildOptions::default(), + )?; + let version = package.version().clone(); + let dist = ResolvedDist::Installable { dist, version }; + let hashes = package.hashes(); + Ok(Node::Dist { + dist, + hashes, + install: false, + }) + } + + /// Convert a lockfile entry to a graph [`Node`]. + fn package_to_node( + &self, + package: &Package, + tags: &Tags, + build_options: &BuildOptions, + install_options: &InstallOptions, + ) -> Result { + if install_options.include_package( + package.name(), + self.project_name(), + self.lock().members(), + ) { + self.installable_node(package, tags, build_options) + } else { + self.non_installable_node(package, tags) + } } } diff --git a/crates/uv-resolver/src/resolution/display.rs b/crates/uv-resolver/src/resolution/display.rs index c811c3e68501..bae08bcc62f1 100644 --- a/crates/uv-resolver/src/resolution/display.rs +++ b/crates/uv-resolver/src/resolution/display.rs @@ -2,12 +2,10 @@ use std::collections::BTreeSet; use owo_colors::OwoColorize; use petgraph::visit::EdgeRef; -use petgraph::Direction; +use petgraph::{Directed, Direction, Graph}; use rustc_hash::{FxBuildHasher, FxHashMap}; -use uv_distribution_types::{ - DistributionMetadata, Name, SourceAnnotation, SourceAnnotations, VersionId, -}; +use uv_distribution_types::{DistributionMetadata, Name, SourceAnnotation, SourceAnnotations}; use uv_normalize::PackageName; use uv_pep508::MarkerTree; @@ -305,11 +303,9 @@ pub enum AnnotationStyle { } /// We don't need the edge markers anymore since we switched to propagated markers. -type IntermediatePetGraph<'dist> = - petgraph::graph::Graph, (), petgraph::Directed>; +type IntermediatePetGraph<'dist> = Graph, (), Directed>; -type RequirementsTxtGraph<'dist> = - petgraph::graph::Graph, (), petgraph::Directed>; +type RequirementsTxtGraph<'dist> = Graph, (), Directed>; /// Reduce the graph, such that all nodes for a single package are combined, regardless of /// the extras, as long as they have the same version and markers. @@ -324,8 +320,10 @@ type RequirementsTxtGraph<'dist> = /// We also remove the root node, to simplify the graph structure. fn combine_extras<'dist>(graph: &IntermediatePetGraph<'dist>) -> RequirementsTxtGraph<'dist> { /// Return the key for a node. - fn version_marker(dist: &RequirementsTxtDist) -> (VersionId, MarkerTree) { - (dist.version_id(), dist.markers.clone()) + fn version_marker<'dist>( + dist: &'dist RequirementsTxtDist, + ) -> (&'dist PackageName, &'dist MarkerTree) { + (dist.name(), dist.markers) } let mut next = RequirementsTxtGraph::with_capacity(graph.node_count(), graph.edge_count()); diff --git a/crates/uv-resolver/src/resolution/output.rs b/crates/uv-resolver/src/resolution/output.rs index bb4f2f928807..b654c656498b 100644 --- a/crates/uv-resolver/src/resolution/output.rs +++ b/crates/uv-resolver/src/resolution/output.rs @@ -10,8 +10,8 @@ use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet}; use uv_configuration::{Constraints, Overrides}; use uv_distribution::Metadata; use uv_distribution_types::{ - Dist, DistributionMetadata, IndexUrl, Name, ResolutionDiagnostic, ResolvedDist, VersionId, - VersionOrUrlRef, + Dist, DistributionMetadata, Edge, IndexUrl, Name, Node, ResolutionDiagnostic, ResolvedDist, + VersionId, VersionOrUrlRef, }; use uv_git::GitResolver; use uv_normalize::{ExtraName, GroupName, PackageName}; @@ -275,7 +275,7 @@ impl ResolverOutput { } fn add_edge( - petgraph: &mut Graph, + graph: &mut Graph, inverse: &mut FxHashMap, NodeIndex>, root_index: NodeIndex, edge: &ResolutionDependencyEdge, @@ -306,20 +306,20 @@ impl ResolverOutput { edge_marker }; - if let Some(marker) = petgraph + if let Some(marker) = graph .find_edge(from_index, to_index) - .and_then(|edge| petgraph.edge_weight_mut(edge)) + .and_then(|edge| graph.edge_weight_mut(edge)) { // If either the existing marker or new marker is `true`, then the dependency is // included unconditionally, and so the combined marker is `true`. marker.or(edge_marker); } else { - petgraph.update_edge(from_index, to_index, edge_marker); + graph.update_edge(from_index, to_index, edge_marker); } } fn add_version<'a>( - petgraph: &mut Graph, + graph: &mut Graph, inverse: &mut FxHashMap, NodeIndex>, diagnostics: &mut Vec, preferences: &Preferences, @@ -372,7 +372,7 @@ impl ResolverOutput { } // Add the distribution to the graph. - let node = petgraph.add_node(ResolutionGraphNode::Dist(AnnotatedDist { + let node = graph.add_node(ResolutionGraphNode::Dist(AnnotatedDist { dist, name: name.clone(), version: version.clone(), @@ -814,29 +814,98 @@ impl Display for ConflictingDistributionError { } } +/// Convert a [`ResolverOutput`] into a [`uv_distribution_types::Resolution`]. +/// +/// This involves converting [`ResolutionGraphNode`]s into [`Node`]s, which in turn involves +/// dropping any extras and dependency groups from the graph nodes. Instead, each package is +/// collapsed into a single node, with extras and dependency groups annotating the _edges_, rather +/// than being represented as separate nodes. This is a more natural representation, but a further +/// departure from the PubGrub model. +/// +/// For simplicity, this transformation makes the assumption that the resolution only applies to a +/// subset of markers, i.e., it shouldn't be called on universal resolutions, and expects only a +/// single version of each package to be present in the graph. impl From for uv_distribution_types::Resolution { - fn from(graph: ResolverOutput) -> Self { - Self::new( - graph - .dists() - .map(|node| (node.name().clone(), node.dist.clone())) - .collect(), - graph - .dists() - .map(|node| (node.name().clone(), node.hashes.clone())) - .collect(), - graph.diagnostics, - ) + fn from(output: ResolverOutput) -> Self { + let ResolverOutput { + graph, + diagnostics, + fork_markers, + .. + } = output; + + assert!( + fork_markers.is_empty(), + "universal resolutions are not supported" + ); + + let mut transformed = Graph::with_capacity(graph.node_count(), graph.edge_count()); + let mut inverse = FxHashMap::with_capacity_and_hasher(graph.node_count(), FxBuildHasher); + + // Create the root node. + let root = transformed.add_node(Node::Root); + + // Re-add the nodes to the reduced graph. + for index in graph.node_indices() { + let ResolutionGraphNode::Dist(dist) = &graph[index] else { + continue; + }; + if dist.is_base() { + inverse.insert( + &dist.name, + transformed.add_node(Node::Dist { + dist: dist.dist.clone(), + hashes: dist.hashes.clone(), + install: true, + }), + ); + } + } + + // Re-add the edges to the reduced graph. + for edge in graph.edge_indices() { + let (source, target) = graph.edge_endpoints(edge).unwrap(); + let marker = graph[edge].clone(); + + match (&graph[source], &graph[target]) { + (ResolutionGraphNode::Root, ResolutionGraphNode::Dist(target_dist)) => { + let target = inverse[&target_dist.name()]; + transformed.update_edge(root, target, Edge::Prod(marker)); + } + ( + ResolutionGraphNode::Dist(source_dist), + ResolutionGraphNode::Dist(target_dist), + ) => { + let source = inverse[&source_dist.name()]; + let target = inverse[&target_dist.name()]; + + let edge = if let Some(extra) = source_dist.extra.as_ref() { + Edge::Optional(extra.clone(), marker) + } else if let Some(dev) = source_dist.dev.as_ref() { + Edge::Dev(dev.clone(), marker) + } else { + Edge::Prod(marker) + }; + + transformed.add_edge(source, target, edge); + } + _ => { + unreachable!("root should not contain incoming edges"); + } + } + } + + uv_distribution_types::Resolution::new(transformed).with_diagnostics(diagnostics) } } /// Find any packages that don't have any lower bound on them when in resolution-lowest mode. fn report_missing_lower_bounds( - petgraph: &Graph, + graph: &Graph, diagnostics: &mut Vec, ) { - for node_index in petgraph.node_indices() { - let ResolutionGraphNode::Dist(dist) = petgraph.node_weight(node_index).unwrap() else { + for node_index in graph.node_indices() { + let ResolutionGraphNode::Dist(dist) = graph.node_weight(node_index).unwrap() else { // Ignore the root package. continue; }; @@ -847,7 +916,7 @@ fn report_missing_lower_bounds( // have to drop. continue; } - if !has_lower_bound(node_index, dist.name(), petgraph) { + if !has_lower_bound(node_index, dist.name(), graph) { diagnostics.push(ResolutionDiagnostic::MissingLowerBound { package_name: dist.name().clone(), }); @@ -859,10 +928,10 @@ fn report_missing_lower_bounds( fn has_lower_bound( node_index: NodeIndex, package_name: &PackageName, - petgraph: &Graph, + graph: &Graph, ) -> bool { - for neighbor_index in petgraph.neighbors_directed(node_index, Direction::Incoming) { - let neighbor_dist = match petgraph.node_weight(neighbor_index).unwrap() { + for neighbor_index in graph.neighbors_directed(node_index, Direction::Incoming) { + let neighbor_dist = match graph.node_weight(neighbor_index).unwrap() { ResolutionGraphNode::Root => { // We already handled direct dependencies with a missing constraint // separately. diff --git a/crates/uv-types/src/hash.rs b/crates/uv-types/src/hash.rs index 8696538f0082..2ac08effa3b5 100644 --- a/crates/uv-types/src/hash.rs +++ b/crates/uv-types/src/hash.rs @@ -274,8 +274,7 @@ impl HashStrategy { ) -> Result { let mut hashes = FxHashMap::>::default(); - for dist in resolution.distributions() { - let digests = resolution.get_hashes(dist.name()); + for (dist, digests) in resolution.hashes() { if digests.is_empty() { // Under `--require-hashes`, every requirement must include a hash. if mode.is_require() { diff --git a/crates/uv/src/commands/pip/operations.rs b/crates/uv/src/commands/pip/operations.rs index 3bc3eab72bde..e427922c19eb 100644 --- a/crates/uv/src/commands/pip/operations.rs +++ b/crates/uv/src/commands/pip/operations.rs @@ -733,8 +733,8 @@ pub(crate) fn diagnose_environment( for diagnostic in site_packages.diagnostics(markers)? { // Only surface diagnostics that are "relevant" to the current resolution. if resolution - .packages() - .any(|package| diagnostic.includes(package)) + .distributions() + .any(|dist| diagnostic.includes(dist.name())) { writeln!( printer.stderr(), diff --git a/crates/uv/src/commands/project/sync.rs b/crates/uv/src/commands/project/sync.rs index abeb535cb5a8..8f8593a83f55 100644 --- a/crates/uv/src/commands/project/sync.rs +++ b/crates/uv/src/commands/project/sync.rs @@ -510,19 +510,19 @@ fn apply_editable_mode( version, } = dist else { - return dist; + return None; }; - ResolvedDist::Installable { + Some(ResolvedDist::Installable { dist: Dist::Source(SourceDist::Directory(DirectorySourceDist { - name, - install_path, + name: name.clone(), + install_path: install_path.clone(), editable: false, r#virtual: false, - url, + url: url.clone(), })), - version, - } + version: version.clone(), + }) }), } }