Skip to content

Commit

Permalink
Avoid need for universal markers in requirements.txt export
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Dec 26, 2024
1 parent e6126ce commit 66b3a42
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 43 deletions.
60 changes: 54 additions & 6 deletions crates/uv-resolver/src/graph_ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<T>(
graph: &Graph<T, UniversalMarker>,
fork_markers: &[UniversalMarker],
) -> FxHashMap<NodeIndex, UniversalMarker> {
pub(crate) fn marker_reachability<Node, Edge: Reachable + Copy + PartialEq>(
graph: &Graph<Node, Edge>,
fork_markers: &[Edge],
) -> FxHashMap<NodeIndex, Edge> {
// 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);
Expand All @@ -42,11 +43,11 @@ pub(crate) fn marker_reachability<T>(
// 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
})
Expand Down Expand Up @@ -264,3 +265,50 @@ pub(crate) fn simplify_conflict_markers(
}
}
}

/// A trait for types that can be used as markers in the dependency graph.
pub(crate) trait Reachable {
fn true_marker() -> Self;

fn false_marker() -> Self;

fn and(&mut self, other: Self);

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);
}
}
42 changes: 5 additions & 37 deletions crates/uv-resolver/src/lock/requirements_txt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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::<Vec<_>>();

Expand Down

0 comments on commit 66b3a42

Please sign in to comment.