From 79dce7391e17c9872a96e3d2cfe186c3a94ee1e0 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 26 Dec 2024 10:32:32 -0500 Subject: [PATCH] Avoid need for universal markers in `requirements.txt` export (#10171) --- crates/uv-resolver/src/graph_ops.rs | 64 +++++++++++++++++-- .../uv-resolver/src/lock/requirements_txt.rs | 42 ++---------- 2 files changed, 63 insertions(+), 43 deletions(-) diff --git a/crates/uv-resolver/src/graph_ops.rs b/crates/uv-resolver/src/graph_ops.rs index 020100de58ea..03264ff2f534 100644 --- a/crates/uv-resolver/src/graph_ops.rs +++ b/crates/uv-resolver/src/graph_ops.rs @@ -5,6 +5,7 @@ use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet}; use std::collections::hash_map::Entry; use uv_normalize::{ExtraName, GroupName, PackageName}; +use uv_pep508::MarkerTree; use uv_pypi_types::{ConflictItem, Conflicts}; use crate::resolution::ResolutionGraphNode; @@ -17,10 +18,10 @@ use crate::universal_marker::UniversalMarker; /// marker), we re-queue the node and update all its children. This implicitly handles cycles, /// whenever we re-reach a node through a cycle the marker we have is a more /// specific marker/longer path, so we don't update the node and don't re-queue it. -pub(crate) fn marker_reachability( - graph: &Graph, - fork_markers: &[UniversalMarker], -) -> FxHashMap { +pub(crate) fn marker_reachability( + graph: &Graph, + fork_markers: &[Edge], +) -> FxHashMap { // Note that we build including the virtual packages due to how we propagate markers through // the graph, even though we then only read the markers for base packages. let mut reachability = FxHashMap::with_capacity_and_hasher(graph.node_count(), FxBuildHasher); @@ -42,11 +43,11 @@ pub(crate) fn marker_reachability( // The root nodes are always applicable, unless the user has restricted resolver // environments with `tool.uv.environments`. let root_markers = if fork_markers.is_empty() { - UniversalMarker::TRUE + Edge::true_marker() } else { fork_markers .iter() - .fold(UniversalMarker::FALSE, |mut acc, marker| { + .fold(Edge::false_marker(), |mut acc, marker| { acc.or(*marker); acc }) @@ -264,3 +265,54 @@ pub(crate) fn simplify_conflict_markers( } } } + +/// A trait for types that can be used as markers in the dependency graph. +pub(crate) trait Reachable { + /// The marker representing the "true" value. + fn true_marker() -> Self; + + /// The marker representing the "false" value. + fn false_marker() -> Self; + + /// Perform a logical AND operation with another marker. + fn and(&mut self, other: Self); + + /// Perform a logical OR operation with another marker. + fn or(&mut self, other: Self); +} + +impl Reachable for UniversalMarker { + fn true_marker() -> Self { + Self::TRUE + } + + fn false_marker() -> Self { + Self::FALSE + } + + fn and(&mut self, other: Self) { + self.and(other); + } + + fn or(&mut self, other: Self) { + self.or(other); + } +} + +impl Reachable for MarkerTree { + fn true_marker() -> Self { + Self::TRUE + } + + fn false_marker() -> Self { + Self::FALSE + } + + fn and(&mut self, other: Self) { + self.and(other); + } + + fn or(&mut self, other: Self) { + self.or(other); + } +} diff --git a/crates/uv-resolver/src/lock/requirements_txt.rs b/crates/uv-resolver/src/lock/requirements_txt.rs index 2209ed89f207..7290f61ba20b 100644 --- a/crates/uv-resolver/src/lock/requirements_txt.rs +++ b/crates/uv-resolver/src/lock/requirements_txt.rs @@ -20,7 +20,6 @@ use uv_pypi_types::{ParsedArchiveUrl, ParsedGitUrl}; use crate::graph_ops::marker_reachability; use crate::lock::{Package, PackageId, Source}; -use crate::universal_marker::{ConflictMarker, UniversalMarker}; use crate::{Installable, LockError}; /// An export of a [`Lock`] that renders in `requirements.txt` format. @@ -70,7 +69,7 @@ impl<'lock> RequirementsTxtExport<'lock> { // Add an edge from the root. let index = inverse[&dist.id]; - petgraph.add_edge(root, index, UniversalMarker::TRUE); + petgraph.add_edge(root, index, MarkerTree::TRUE); // Push its dependencies on the queue. queue.push_back((dist, None)); @@ -110,17 +109,7 @@ impl<'lock> RequirementsTxtExport<'lock> { petgraph.add_edge( root, dep_index, - UniversalMarker::new( - dep.simplified_marker.as_simplified_marker_tree(), - // OK because we've verified above that we do not have any - // conflicting extras/groups. - // - // So why use `UniversalMarker`? Because that's what - // `marker_reachability` wants and it (probably) isn't - // worth inventing a new abstraction so that it can accept - // graphs with either `MarkerTree` or `UniversalMarker`. - ConflictMarker::TRUE, - ), + dep.simplified_marker.as_simplified_marker_tree(), ); // Push its dependencies on the queue. @@ -205,21 +194,7 @@ impl<'lock> RequirementsTxtExport<'lock> { // Add an edge from the root. let dep_index = inverse[&dist.id]; - petgraph.add_edge( - root, - dep_index, - UniversalMarker::new( - marker, - // OK because we've verified above that we do not have any - // conflicting extras/groups. - // - // So why use `UniversalMarker`? Because that's what - // `marker_reachability` wants and it (probably) isn't - // worth inventing a new abstraction so that it can accept - // graphs with either `MarkerTree` or `UniversalMarker`. - ConflictMarker::TRUE, - ), - ); + petgraph.add_edge(root, dep_index, marker); // Push its dependencies on the queue. if seen.insert((&dist.id, None)) { @@ -267,12 +242,7 @@ impl<'lock> RequirementsTxtExport<'lock> { petgraph.add_edge( index, dep_index, - UniversalMarker::new( - dep.simplified_marker.as_simplified_marker_tree(), - // See note above for other `UniversalMarker::new` for - // why this is OK. - ConflictMarker::TRUE, - ), + dep.simplified_marker.as_simplified_marker_tree(), ); // Push its dependencies on the queue. @@ -305,9 +275,7 @@ impl<'lock> RequirementsTxtExport<'lock> { }) .map(|(index, package)| Requirement { package, - // As above, we've verified that there are no conflicting extras/groups - // specified, so it's safe to completely ignore the conflict marker. - marker: reachability.remove(&index).unwrap_or_default().pep508(), + marker: reachability.remove(&index).unwrap_or_default(), }) .collect::>();