From b396cf43a39ff0ac807a5ba2a355debfdd426e73 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Thu, 21 Nov 2024 14:01:55 -0500 Subject: [PATCH] uv-resolver: introduce new UniversalMarker type This effectively combines a PEP 508 marker and an as-yet-specified marker for expressing conflicts among extras and groups. This just defines the type and threads it through most of the various points in the code that previously used `MarkerTree` only. Some parts do still continue to use `MarkerTree` specifically, e.g., when dealing with non-universal resolution or exporting to `requirements.txt`. This doesn't change any behavior. --- crates/uv-resolver/src/candidate_selector.rs | 8 +- crates/uv-resolver/src/graph_ops.rs | 15 +- crates/uv-resolver/src/lib.rs | 1 + crates/uv-resolver/src/lock/mod.rs | 92 ++++++++---- .../uv-resolver/src/lock/requirements_txt.rs | 35 ++++- ...missing_dependency_source_unambiguous.snap | 7 +- ...dependency_source_version_unambiguous.snap | 7 +- ...issing_dependency_version_unambiguous.snap | 7 +- crates/uv-resolver/src/lock/target.rs | 13 +- crates/uv-resolver/src/preferences.rs | 13 +- crates/uv-resolver/src/resolution/display.rs | 15 ++ crates/uv-resolver/src/resolution/mod.rs | 4 +- crates/uv-resolver/src/resolution/output.rs | 61 +++++--- .../src/resolution/requirements_txt.rs | 11 +- .../uv-resolver/src/resolver/environment.rs | 21 +++ crates/uv-resolver/src/resolver/mod.rs | 13 +- crates/uv-resolver/src/universal_marker.rs | 131 ++++++++++++++++++ crates/uv/src/commands/project/lock.rs | 13 +- 18 files changed, 376 insertions(+), 91 deletions(-) create mode 100644 crates/uv-resolver/src/universal_marker.rs diff --git a/crates/uv-resolver/src/candidate_selector.rs b/crates/uv-resolver/src/candidate_selector.rs index 5d0d824c82cb9..ed6c0690602bb 100644 --- a/crates/uv-resolver/src/candidate_selector.rs +++ b/crates/uv-resolver/src/candidate_selector.rs @@ -8,12 +8,12 @@ use uv_distribution_types::{CompatibleDist, IncompatibleDist, IncompatibleSource use uv_distribution_types::{DistributionMetadata, IncompatibleWheel, Name, PrioritizedDist}; use uv_normalize::PackageName; use uv_pep440::Version; -use uv_pep508::MarkerTree; use uv_types::InstalledPackagesProvider; use crate::preferences::Preferences; use crate::prerelease::{AllowPrerelease, PrereleaseStrategy}; use crate::resolution_mode::ResolutionStrategy; +use crate::universal_marker::UniversalMarker; use crate::version_map::{VersionMap, VersionMapDistHandle}; use crate::{Exclusions, Manifest, Options, ResolverEnvironment}; @@ -140,10 +140,10 @@ impl CandidateSelector { // first has the matching half and then the mismatching half. let preferences_match = preferences .get(package_name) - .filter(|(marker, _index, _version)| env.included_by_marker(marker)); + .filter(|(marker, _index, _version)| env.included_by_marker(marker.pep508())); let preferences_mismatch = preferences .get(package_name) - .filter(|(marker, _index, _version)| !env.included_by_marker(marker)); + .filter(|(marker, _index, _version)| !env.included_by_marker(marker.pep508())); let preferences = preferences_match.chain(preferences_mismatch).filter_map( |(marker, source, version)| { // If the package is mapped to an explicit index, only consider preferences that @@ -167,7 +167,7 @@ impl CandidateSelector { /// Return the first preference that satisfies the current range and is allowed. fn get_preferred_from_iter<'a, InstalledPackages: InstalledPackagesProvider>( &'a self, - preferences: impl Iterator, + preferences: impl Iterator, package_name: &'a PackageName, range: &Range, version_maps: &'a [VersionMap], diff --git a/crates/uv-resolver/src/graph_ops.rs b/crates/uv-resolver/src/graph_ops.rs index e09a66c753dd8..eebd6ab88e9a4 100644 --- a/crates/uv-resolver/src/graph_ops.rs +++ b/crates/uv-resolver/src/graph_ops.rs @@ -3,7 +3,8 @@ use petgraph::visit::EdgeRef; use petgraph::{Direction, Graph}; use rustc_hash::{FxBuildHasher, FxHashMap}; use std::collections::hash_map::Entry; -use uv_pep508::MarkerTree; + +use crate::universal_marker::UniversalMarker; /// Determine the markers under which a package is reachable in the dependency tree. /// @@ -13,9 +14,9 @@ use uv_pep508::MarkerTree; /// 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: &[MarkerTree], -) -> FxHashMap { + graph: &Graph, + fork_markers: &[UniversalMarker], +) -> 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); @@ -36,12 +37,12 @@ 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: MarkerTree = if fork_markers.is_empty() { - MarkerTree::TRUE + let root_markers = if fork_markers.is_empty() { + UniversalMarker::TRUE } else { fork_markers .iter() - .fold(MarkerTree::FALSE, |mut acc, marker| { + .fold(UniversalMarker::FALSE, |mut acc, marker| { acc.or(marker.clone()); acc }) diff --git a/crates/uv-resolver/src/lib.rs b/crates/uv-resolver/src/lib.rs index af0193d9a7371..f378914da501e 100644 --- a/crates/uv-resolver/src/lib.rs +++ b/crates/uv-resolver/src/lib.rs @@ -56,5 +56,6 @@ mod requires_python; mod resolution; mod resolution_mode; mod resolver; +mod universal_marker; mod version_map; mod yanks; diff --git a/crates/uv-resolver/src/lock/mod.rs b/crates/uv-resolver/src/lock/mod.rs index 66538c1bfb017..d801d70310aad 100644 --- a/crates/uv-resolver/src/lock/mod.rs +++ b/crates/uv-resolver/src/lock/mod.rs @@ -19,6 +19,7 @@ pub use crate::lock::target::InstallTarget; pub use crate::lock::tree::TreeDisplay; use crate::requires_python::SimplifiedMarkerTree; use crate::resolution::{AnnotatedDist, ResolutionGraphNode}; +use crate::universal_marker::UniversalMarker; use crate::{ ExcludeNewer, InMemoryIndex, MetadataResponse, PrereleaseMode, RequiresPython, ResolutionMode, ResolverOutput, @@ -55,23 +56,26 @@ mod tree; /// The current version of the lockfile format. pub const VERSION: u32 = 1; -static LINUX_MARKERS: LazyLock = LazyLock::new(|| { - MarkerTree::from_str( +static LINUX_MARKERS: LazyLock = LazyLock::new(|| { + let pep508 = MarkerTree::from_str( "platform_system == 'Linux' and os_name == 'posix' and sys_platform == 'linux'", ) - .unwrap() + .unwrap(); + UniversalMarker::new(pep508, MarkerTree::TRUE) }); -static WINDOWS_MARKERS: LazyLock = LazyLock::new(|| { - MarkerTree::from_str( +static WINDOWS_MARKERS: LazyLock = LazyLock::new(|| { + let pep508 = MarkerTree::from_str( "platform_system == 'Windows' and os_name == 'nt' and sys_platform == 'win32'", ) - .unwrap() + .unwrap(); + UniversalMarker::new(pep508, MarkerTree::TRUE) }); -static MAC_MARKERS: LazyLock = LazyLock::new(|| { - MarkerTree::from_str( +static MAC_MARKERS: LazyLock = LazyLock::new(|| { + let pep508 = MarkerTree::from_str( "platform_system == 'Darwin' and os_name == 'posix' and sys_platform == 'darwin'", ) - .unwrap() + .unwrap(); + UniversalMarker::new(pep508, MarkerTree::TRUE) }); #[derive(Clone, Debug, serde::Deserialize)] @@ -80,7 +84,7 @@ pub struct Lock { version: u32, /// If this lockfile was built from a forking resolution with non-identical forks, store the /// forks in the lockfile so we can recreate them in subsequent resolutions. - fork_markers: Vec, + fork_markers: Vec, /// The conflicting groups/extras specified by the user. conflicts: Conflicts, /// The list of supported environments specified by the user. @@ -315,7 +319,7 @@ impl Lock { manifest: ResolverManifest, conflicts: Conflicts, supported_environments: Vec, - fork_markers: Vec, + fork_markers: Vec, ) -> Result { // Put all dependencies for each package in a canonical order and // check for duplicates. @@ -597,7 +601,7 @@ impl Lock { /// If this lockfile was built from a forking resolution with non-identical forks, return the /// markers of those forks, otherwise `None`. - pub fn fork_markers(&self) -> &[MarkerTree] { + pub fn fork_markers(&self) -> &[UniversalMarker] { self.fork_markers.as_slice() } @@ -614,7 +618,13 @@ impl Lock { let fork_markers = each_element_on_its_line_array( self.fork_markers .iter() - .map(|marker| SimplifiedMarkerTree::new(&self.requires_python, marker.clone())) + .map(|marker| { + // TODO(ag): Consider whether `resolution-markers` should actually + // include conflicting marker info. In which case, we should serialize + // the entire `UniversalMarker` (taking care to still make the PEP 508 + // simplified). + SimplifiedMarkerTree::new(&self.requires_python, marker.pep508().clone()) + }) .filter_map(|marker| marker.try_to_string()), ); doc.insert("resolution-markers", value(fork_markers)); @@ -1434,6 +1444,9 @@ impl TryFrom for Lock { .fork_markers .into_iter() .map(|simplified_marker| simplified_marker.into_marker(&wire.requires_python)) + // TODO(ag): Consider whether this should also deserialize a conflict marker. + // We currently aren't serializing. Dropping it completely is likely to be wrong. + .map(|complexified_marker| UniversalMarker::new(complexified_marker, MarkerTree::TRUE)) .collect(); let lock = Lock::new( wire.version, @@ -1475,7 +1488,7 @@ pub struct Package { /// the next resolution. /// /// Named `resolution-markers` in `uv.lock`. - fork_markers: Vec, + fork_markers: Vec, /// The resolved dependencies of the package. dependencies: Vec, /// The resolved optional dependencies of the package. @@ -1489,7 +1502,7 @@ pub struct Package { impl Package { fn from_annotated_dist( annotated_dist: &AnnotatedDist, - fork_markers: Vec, + fork_markers: Vec, root: &Path, ) -> Result { let id = PackageId::from_annotated_dist(annotated_dist, root)?; @@ -1549,7 +1562,7 @@ impl Package { &mut self, requires_python: &RequiresPython, annotated_dist: &AnnotatedDist, - marker: MarkerTree, + marker: UniversalMarker, root: &Path, ) -> Result<(), LockError> { let new_dep = @@ -1595,7 +1608,7 @@ impl Package { requires_python: &RequiresPython, extra: ExtraName, annotated_dist: &AnnotatedDist, - marker: MarkerTree, + marker: UniversalMarker, root: &Path, ) -> Result<(), LockError> { let dep = Dependency::from_annotated_dist(requires_python, annotated_dist, marker, root)?; @@ -1621,7 +1634,7 @@ impl Package { requires_python: &RequiresPython, group: GroupName, annotated_dist: &AnnotatedDist, - marker: MarkerTree, + marker: UniversalMarker, root: &Path, ) -> Result<(), LockError> { let dep = Dependency::from_annotated_dist(requires_python, annotated_dist, marker, root)?; @@ -1957,7 +1970,13 @@ impl Package { let wheels = each_element_on_its_line_array( self.fork_markers .iter() - .map(|marker| SimplifiedMarkerTree::new(requires_python, marker.clone())) + // TODO(ag): Consider whether `resolution-markers` should actually + // include conflicting marker info. In which case, we should serialize + // the entire `UniversalMarker` (taking care to still make the PEP 508 + // simplified). + .map(|marker| { + SimplifiedMarkerTree::new(requires_python, marker.pep508().clone()) + }) .filter_map(|marker| marker.try_to_string()), ); table.insert("resolution-markers", value(wheels)); @@ -2112,7 +2131,7 @@ impl Package { } /// Return the fork markers for this package, if any. - pub fn fork_markers(&self) -> &[MarkerTree] { + pub fn fork_markers(&self) -> &[UniversalMarker] { self.fork_markers.as_slice() } @@ -2223,6 +2242,11 @@ impl PackageWire { .fork_markers .into_iter() .map(|simplified_marker| simplified_marker.into_marker(requires_python)) + // TODO(ag): Consider whether this should also deserialize a conflict marker. + // We currently aren't serializing. Dropping it completely is likely to be wrong. + .map(|complexified_marker| { + UniversalMarker::new(complexified_marker, MarkerTree::TRUE) + }) .collect(), dependencies: unwire_deps(self.dependencies)?, optional_dependencies: self @@ -3477,8 +3501,9 @@ impl TryFrom for Wheel { struct Dependency { package_id: PackageId, extra: BTreeSet, - /// A marker simplified by assuming `requires-python` is satisfied. - /// So if `requires-python = '>=3.8'`, then + /// A marker simplified from the PEP 508 marker in `complexified_marker` + /// by assuming `requires-python` is satisfied. So if + /// `requires-python = '>=3.8'`, then /// `python_version >= '3.8' and python_version < '3.12'` /// gets simplfiied to `python_version < '3.12'`. /// @@ -3496,10 +3521,10 @@ struct Dependency { /// `requires-python` applies to the entire lock file, it's /// acceptable to do comparisons on the simplified form. simplified_marker: SimplifiedMarkerTree, - /// The "complexified" marker is a marker that can stand on its - /// own independent of `requires-python`. It can be safely used - /// for any kind of marker algebra. - complexified_marker: MarkerTree, + /// The "complexified" marker is a universal marker whose PEP 508 + /// marker can stand on its own independent of `requires-python`. + /// It can be safely used for any kind of marker algebra. + complexified_marker: UniversalMarker, } impl Dependency { @@ -3507,10 +3532,10 @@ impl Dependency { requires_python: &RequiresPython, package_id: PackageId, extra: BTreeSet, - complexified_marker: MarkerTree, + complexified_marker: UniversalMarker, ) -> Dependency { let simplified_marker = - SimplifiedMarkerTree::new(requires_python, complexified_marker.clone()); + SimplifiedMarkerTree::new(requires_python, complexified_marker.pep508().clone()); Dependency { package_id, extra, @@ -3522,7 +3547,7 @@ impl Dependency { fn from_annotated_dist( requires_python: &RequiresPython, annotated_dist: &AnnotatedDist, - complexified_marker: MarkerTree, + complexified_marker: UniversalMarker, root: &Path, ) -> Result { let package_id = PackageId::from_annotated_dist(annotated_dist, root)?; @@ -3590,6 +3615,7 @@ struct DependencyWire { extra: BTreeSet, #[serde(default)] marker: SimplifiedMarkerTree, + // FIXME: Add support for representing conflict markers. } impl DependencyWire { @@ -3603,7 +3629,8 @@ impl DependencyWire { package_id: self.package_id.unwire(unambiguous_package_ids)?, extra: self.extra, simplified_marker: self.marker, - complexified_marker, + // FIXME: Support reading conflict markers. + complexified_marker: UniversalMarker::new(complexified_marker, MarkerTree::TRUE), }) } } @@ -4103,6 +4130,11 @@ enum LockErrorKind { #[source] err: DependencyGroupError, }, + /// An error that occurs when trying to export a `uv.lock` with + /// conflicting extras/groups specified to `requirements.txt`. + /// (Because `requirements.txt` cannot encode them.) + #[error("Cannot represent `uv.lock` with conflicting extras or groups as `requirements.txt`")] + ConflictsNotAllowedInRequirementsTxt, } /// An error that occurs when a source string could not be parsed. diff --git a/crates/uv-resolver/src/lock/requirements_txt.rs b/crates/uv-resolver/src/lock/requirements_txt.rs index 5c6e83fb7dd18..21635809b5304 100644 --- a/crates/uv-resolver/src/lock/requirements_txt.rs +++ b/crates/uv-resolver/src/lock/requirements_txt.rs @@ -19,7 +19,8 @@ use uv_pep508::MarkerTree; use uv_pypi_types::{ParsedArchiveUrl, ParsedGitUrl}; use crate::graph_ops::marker_reachability; -use crate::lock::{Package, PackageId, Source}; +use crate::lock::{LockErrorKind, Package, PackageId, Source}; +use crate::universal_marker::UniversalMarker; use crate::{InstallTarget, LockError}; /// An export of a [`Lock`] that renders in `requirements.txt` format. @@ -39,6 +40,9 @@ impl<'lock> RequirementsTxtExport<'lock> { hashes: bool, install_options: &'lock InstallOptions, ) -> Result { + if !target.lock().conflicts().is_empty() { + return Err(LockErrorKind::ConflictsNotAllowedInRequirementsTxt.into()); + } let size_guess = target.lock().packages.len(); let mut petgraph = Graph::with_capacity(size_guess, size_guess); let mut inverse = FxHashMap::with_capacity_and_hasher(size_guess, FxBuildHasher); @@ -64,7 +68,7 @@ impl<'lock> RequirementsTxtExport<'lock> { // Add an edge from the root. let index = inverse[&dist.id]; - petgraph.add_edge(root, index, MarkerTree::TRUE); + petgraph.add_edge(root, index, UniversalMarker::TRUE); // Push its dependencies on the queue. queue.push_back((dist, None)); @@ -110,7 +114,17 @@ impl<'lock> RequirementsTxtExport<'lock> { petgraph.add_edge( root, dep_index, - dep.simplified_marker.as_simplified_marker_tree().clone(), + UniversalMarker::new( + dep.simplified_marker.as_simplified_marker_tree().clone(), + // 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`. + MarkerTree::TRUE, + ), ); // Push its dependencies on the queue. @@ -154,7 +168,12 @@ impl<'lock> RequirementsTxtExport<'lock> { petgraph.add_edge( index, dep_index, - dep.simplified_marker.as_simplified_marker_tree().clone(), + UniversalMarker::new( + dep.simplified_marker.as_simplified_marker_tree().clone(), + // See note above for other `UniversalMarker::new` for + // why this is OK. + MarkerTree::TRUE, + ), ); // Push its dependencies on the queue. @@ -187,7 +206,13 @@ impl<'lock> RequirementsTxtExport<'lock> { }) .map(|(index, package)| Requirement { package, - marker: reachability.remove(&index).unwrap_or_default(), + // 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() + .clone(), }) .collect::>(); diff --git a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap index 066df532804a6..0ee9dabab8330 100644 --- a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap +++ b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap @@ -1,5 +1,5 @@ --- -source: crates/uv-resolver/src/lock/tests.rs +source: crates/uv-resolver/src/lock/mod.rs expression: result --- Ok( @@ -135,7 +135,10 @@ Ok( simplified_marker: SimplifiedMarkerTree( true, ), - complexified_marker: python_full_version >= '3.12', + complexified_marker: UniversalMarker { + pep508_marker: python_full_version >= '3.12', + conflict_marker: true, + }, }, ], optional_dependencies: {}, diff --git a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap index 066df532804a6..0ee9dabab8330 100644 --- a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap +++ b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap @@ -1,5 +1,5 @@ --- -source: crates/uv-resolver/src/lock/tests.rs +source: crates/uv-resolver/src/lock/mod.rs expression: result --- Ok( @@ -135,7 +135,10 @@ Ok( simplified_marker: SimplifiedMarkerTree( true, ), - complexified_marker: python_full_version >= '3.12', + complexified_marker: UniversalMarker { + pep508_marker: python_full_version >= '3.12', + conflict_marker: true, + }, }, ], optional_dependencies: {}, diff --git a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap index 066df532804a6..0ee9dabab8330 100644 --- a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap +++ b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap @@ -1,5 +1,5 @@ --- -source: crates/uv-resolver/src/lock/tests.rs +source: crates/uv-resolver/src/lock/mod.rs expression: result --- Ok( @@ -135,7 +135,10 @@ Ok( simplified_marker: SimplifiedMarkerTree( true, ), - complexified_marker: python_full_version >= '3.12', + complexified_marker: UniversalMarker { + pep508_marker: python_full_version >= '3.12', + conflict_marker: true, + }, }, ], optional_dependencies: {}, diff --git a/crates/uv-resolver/src/lock/target.rs b/crates/uv-resolver/src/lock/target.rs index 3ba08dcb398a9..1871893d5110c 100644 --- a/crates/uv-resolver/src/lock/target.rs +++ b/crates/uv-resolver/src/lock/target.rs @@ -248,7 +248,14 @@ impl<'env> InstallTarget<'env> { petgraph.add_edge( index, dep_index, - Edge::Dev(group.clone(), dep.complexified_marker.clone()), + // This is OK because we are resolving to a resolution for + // a specific marker environment and set of extras/groups. + // So at this point, we know the extras/groups have been + // satisfied, so we can safely drop the conflict marker. + // + // FIXME: Make the above true. We aren't actually checking + // the conflict marker yet. + Edge::Dev(group.clone(), dep.complexified_marker.pep508().clone()), ); // Push its dependencies on the queue. @@ -373,9 +380,9 @@ impl<'env> InstallTarget<'env> { index, dep_index, if let Some(extra) = extra { - Edge::Optional(extra.clone(), dep.complexified_marker.clone()) + Edge::Optional(extra.clone(), dep.complexified_marker.pep508().clone()) } else { - Edge::Prod(dep.complexified_marker.clone()) + Edge::Prod(dep.complexified_marker.pep508().clone()) }, ); diff --git a/crates/uv-resolver/src/preferences.rs b/crates/uv-resolver/src/preferences.rs index ef7ecab50abf6..300ebed33d0d5 100644 --- a/crates/uv-resolver/src/preferences.rs +++ b/crates/uv-resolver/src/preferences.rs @@ -11,6 +11,7 @@ use uv_pep508::{MarkerTree, VersionOrUrl}; use uv_pypi_types::{HashDigest, HashError}; use uv_requirements_txt::{RequirementEntry, RequirementsTxtRequirement}; +use crate::universal_marker::UniversalMarker; use crate::{LockError, ResolverEnvironment}; #[derive(thiserror::Error, Debug)] @@ -30,7 +31,7 @@ pub struct Preference { index: Option, /// If coming from a package with diverging versions, the markers of the forks this preference /// is part of, otherwise `None`. - fork_markers: Vec, + fork_markers: Vec, hashes: Vec, } @@ -118,7 +119,7 @@ impl Preference { #[derive(Debug, Clone)] struct Entry { - marker: MarkerTree, + marker: UniversalMarker, index: Option, pin: Pin, } @@ -169,7 +170,7 @@ impl Preferences { slf.insert( preference.name, preference.index, - MarkerTree::TRUE, + UniversalMarker::TRUE, Pin { version: preference.version, hashes: preference.hashes, @@ -198,7 +199,7 @@ impl Preferences { &mut self, package_name: PackageName, index: Option, - markers: MarkerTree, + markers: UniversalMarker, pin: impl Into, ) { self.0.entry(package_name).or_default().push(Entry { @@ -214,7 +215,7 @@ impl Preferences { ) -> impl Iterator< Item = ( &PackageName, - impl Iterator, &Version)>, + impl Iterator, &Version)>, ), > { self.0.iter().map(|(name, preferences)| { @@ -231,7 +232,7 @@ impl Preferences { pub(crate) fn get( &self, package_name: &PackageName, - ) -> impl Iterator, &Version)> { + ) -> impl Iterator, &Version)> { self.0 .get(package_name) .into_iter() diff --git a/crates/uv-resolver/src/resolution/display.rs b/crates/uv-resolver/src/resolution/display.rs index bae08bcc62f1d..81d3afe830d95 100644 --- a/crates/uv-resolver/src/resolution/display.rs +++ b/crates/uv-resolver/src/resolution/display.rs @@ -46,6 +46,11 @@ enum DisplayResolutionGraphNode<'dist> { impl<'a> DisplayResolutionGraph<'a> { /// Create a new [`DisplayResolutionGraph`] for the given graph. + /// + /// Note that this panics if any of the forks in the given resolver + /// output contain non-empty conflicting groups. That is, when using `uv + /// pip compile`, specifying conflicts is not supported because their + /// conditional logic cannot be encoded into a `requirements.txt`. #[allow(clippy::fn_params_excessive_bools)] pub fn new( underlying: &'a ResolverOutput, @@ -58,6 +63,16 @@ impl<'a> DisplayResolutionGraph<'a> { include_index_annotation: bool, annotation_style: AnnotationStyle, ) -> DisplayResolutionGraph<'a> { + for fork_marker in &underlying.fork_markers { + if !fork_marker.conflict().is_true() { + panic!( + "found fork marker {} with non-trivial conflicting marker, \ + cannot display resolver output with conflicts in \ + requirements.txt format", + fork_marker, + ); + } + } Self { resolution: underlying, env, diff --git a/crates/uv-resolver/src/resolution/mod.rs b/crates/uv-resolver/src/resolution/mod.rs index beb0a85f1106a..43eed44c29786 100644 --- a/crates/uv-resolver/src/resolution/mod.rs +++ b/crates/uv-resolver/src/resolution/mod.rs @@ -7,13 +7,13 @@ use uv_distribution_types::{ }; use uv_normalize::{ExtraName, GroupName, PackageName}; use uv_pep440::Version; -use uv_pep508::MarkerTree; use uv_pypi_types::HashDigest; pub use crate::resolution::display::{AnnotationStyle, DisplayResolutionGraph}; pub(crate) use crate::resolution::output::ResolutionGraphNode; pub use crate::resolution::output::{ConflictingDistributionError, ResolverOutput}; pub(crate) use crate::resolution::requirements_txt::RequirementsTxtDist; +use crate::universal_marker::UniversalMarker; mod display; mod output; @@ -36,7 +36,7 @@ pub(crate) struct AnnotatedDist { /// That is, when doing a traversal over all of the distributions in a /// resolution, this marker corresponds to the disjunction of all paths to /// this distribution in the resolution graph. - pub(crate) marker: MarkerTree, + pub(crate) marker: UniversalMarker, } impl AnnotatedDist { diff --git a/crates/uv-resolver/src/resolution/output.rs b/crates/uv-resolver/src/resolution/output.rs index da681d67ac0ce..e9d01e1714589 100644 --- a/crates/uv-resolver/src/resolution/output.rs +++ b/crates/uv-resolver/src/resolution/output.rs @@ -28,6 +28,7 @@ use crate::redirect::url_to_precise; use crate::resolution::AnnotatedDist; use crate::resolution_mode::ResolutionStrategy; use crate::resolver::{Resolution, ResolutionDependencyEdge, ResolutionPackage}; +use crate::universal_marker::UniversalMarker; use crate::{ InMemoryIndex, MetadataResponse, Options, PythonRequirement, RequiresPython, ResolveError, VersionsResponse, @@ -40,12 +41,12 @@ use crate::{ #[derive(Debug)] pub struct ResolverOutput { /// The underlying graph. - pub(crate) graph: Graph, + pub(crate) graph: Graph, /// The range of supported Python versions. pub(crate) requires_python: RequiresPython, /// If the resolution had non-identical forks, store the forks in the lockfile so we can /// recreate them in subsequent resolutions. - pub(crate) fork_markers: Vec, + pub(crate) fork_markers: Vec, /// Any diagnostics that were encountered while building the graph. pub(crate) diagnostics: Vec, /// The requirements that were used to build the graph. @@ -65,9 +66,9 @@ pub(crate) enum ResolutionGraphNode { } impl ResolutionGraphNode { - pub(crate) fn marker(&self) -> &MarkerTree { + pub(crate) fn marker(&self) -> &UniversalMarker { match self { - ResolutionGraphNode::Root => &MarkerTree::TRUE, + ResolutionGraphNode::Root => &UniversalMarker::TRUE, ResolutionGraphNode::Dist(dist) => &dist.marker, } } @@ -108,7 +109,7 @@ impl ResolverOutput { options: Options, ) -> Result { let size_guess = resolutions[0].nodes.len(); - let mut graph: Graph = + let mut graph: Graph = Graph::with_capacity(size_guess, size_guess); let mut inverse: FxHashMap> = FxHashMap::with_capacity_and_hasher(size_guess, FxBuildHasher); @@ -141,7 +142,7 @@ impl ResolverOutput { let mut seen = FxHashSet::default(); for resolution in resolutions { - let marker = resolution.env.try_markers().cloned().unwrap_or_default(); + let marker = resolution.env.try_universal_markers().unwrap_or_default(); // Add every edge to the graph, propagating the marker for the current fork, if // necessary. @@ -158,12 +159,19 @@ impl ResolverOutput { // Extract the `Requires-Python` range, if provided. let requires_python = python.target().clone(); - let fork_markers: Vec = if let [resolution] = resolutions { - resolution.env.try_markers().cloned().into_iter().collect() + let fork_markers: Vec = if let [resolution] = resolutions { + resolution + .env + .try_markers() + .cloned() + .into_iter() + .map(|marker| UniversalMarker::new(marker, MarkerTree::TRUE)) + .collect() } else { resolutions .iter() .map(|resolution| resolution.env.try_markers().cloned().unwrap_or_default()) + .map(|marker| UniversalMarker::new(marker, MarkerTree::TRUE)) .collect() }; @@ -206,6 +214,9 @@ impl ResolverOutput { // the same time. At which point, uv will report an error, // thereby sidestepping the possibility of installing different // versions of the same package into the same virtualenv. ---AG + // + // FIXME: When `UniversalMarker` supports extras/groups, we can + // re-enable this. if conflicts.is_empty() { #[allow(unused_mut, reason = "Used in debug_assertions below")] let mut conflicting = output.find_conflicting_distributions(); @@ -234,11 +245,11 @@ impl ResolverOutput { } fn add_edge( - graph: &mut Graph, + graph: &mut Graph, inverse: &mut FxHashMap, NodeIndex>, root_index: NodeIndex, edge: &ResolutionDependencyEdge, - marker: MarkerTree, + marker: UniversalMarker, ) { let from_index = edge.from.as_ref().map_or(root_index, |from| { inverse[&PackageRef { @@ -260,25 +271,25 @@ impl ResolverOutput { }]; let edge_marker = { - let mut edge_marker = edge.marker.clone(); + let mut edge_marker = edge.universal_marker(); edge_marker.and(marker); edge_marker }; - if let Some(marker) = graph + if let Some(weight) = graph .find_edge(from_index, to_index) .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); + weight.or(edge_marker); } else { graph.update_edge(from_index, to_index, edge_marker); } } fn add_version<'a>( - graph: &mut Graph, + graph: &mut Graph, inverse: &mut FxHashMap, NodeIndex>, diagnostics: &mut Vec, preferences: &Preferences, @@ -339,7 +350,7 @@ impl ResolverOutput { dev: dev.clone(), hashes, metadata, - marker: MarkerTree::TRUE, + marker: UniversalMarker::TRUE, })); inverse.insert( PackageRef { @@ -703,7 +714,7 @@ impl ResolverOutput { /// an installation in that marker environment could wind up trying to /// install different versions of the same package, which is not allowed. fn find_conflicting_distributions(&self) -> Vec { - let mut name_to_markers: BTreeMap<&PackageName, Vec<(&Version, &MarkerTree)>> = + let mut name_to_markers: BTreeMap<&PackageName, Vec<(&Version, &UniversalMarker)>> = BTreeMap::new(); for node in self.graph.node_weights() { let annotated_dist = match node { @@ -749,8 +760,8 @@ pub struct ConflictingDistributionError { name: PackageName, version1: Version, version2: Version, - marker1: MarkerTree, - marker2: MarkerTree, + marker1: UniversalMarker, + marker2: UniversalMarker, } impl std::error::Error for ConflictingDistributionError {} @@ -767,8 +778,8 @@ impl Display for ConflictingDistributionError { write!( f, "found conflicting versions for package `{name}`: - `{marker1:?}` (for version `{version1}`) is not disjoint with \ - `{marker2:?}` (for version `{version2}`)", + `{marker1}` (for version `{version1}`) is not disjoint with \ + `{marker2}` (for version `{version2}`)", ) } } @@ -824,7 +835,11 @@ impl From for uv_distribution_types::Resolution { // 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(); + // OK to ignore conflicting marker because we've asserted + // above that we aren't in universal mode. If we aren't in + // universal mode, then there can be no conflicts since + // conflicts imply forks and forks imply universal mode. + let marker = graph[edge].pep508().clone(); match (&graph[source], &graph[target]) { (ResolutionGraphNode::Root, ResolutionGraphNode::Dist(target_dist)) => { @@ -860,7 +875,7 @@ impl From for uv_distribution_types::Resolution { /// Find any packages that don't have any lower bound on them when in resolution-lowest mode. fn report_missing_lower_bounds( - graph: &Graph, + graph: &Graph, diagnostics: &mut Vec, ) { for node_index in graph.node_indices() { @@ -887,7 +902,7 @@ fn report_missing_lower_bounds( fn has_lower_bound( node_index: NodeIndex, package_name: &PackageName, - graph: &Graph, + graph: &Graph, ) -> bool { for neighbor_index in graph.neighbors_directed(node_index, Direction::Incoming) { let neighbor_dist = match graph.node_weight(neighbor_index).unwrap() { diff --git a/crates/uv-resolver/src/resolution/requirements_txt.rs b/crates/uv-resolver/src/resolution/requirements_txt.rs index 3117fe32b501c..6a956ce9d091d 100644 --- a/crates/uv-resolver/src/resolution/requirements_txt.rs +++ b/crates/uv-resolver/src/resolution/requirements_txt.rs @@ -167,11 +167,20 @@ impl<'dist> RequirementsTxtDist<'dist> { } pub(crate) fn from_annotated_dist(annotated: &'dist AnnotatedDist) -> Self { + assert!( + annotated.marker.conflict().is_true(), + "found dist {annotated} with non-trivial conflicting marker {marker}, \ + which cannot be represented in a `requirements.txt` format", + marker = annotated.marker, + ); Self { dist: &annotated.dist, version: &annotated.version, hashes: &annotated.hashes, - markers: &annotated.marker, + // OK because we've asserted above that this dist + // does not have a non-trivial conflicting marker + // that we would otherwise need to care about. + markers: annotated.marker.pep508(), extras: if let Some(extra) = annotated.extra.clone() { vec![extra] } else { diff --git a/crates/uv-resolver/src/resolver/environment.rs b/crates/uv-resolver/src/resolver/environment.rs index 91fa9d2745c59..d8a6fccd66917 100644 --- a/crates/uv-resolver/src/resolver/environment.rs +++ b/crates/uv-resolver/src/resolver/environment.rs @@ -6,6 +6,7 @@ use uv_pypi_types::{ConflictItem, ConflictItemRef, ResolverMarkerEnvironment}; use crate::pubgrub::{PubGrubDependency, PubGrubPackage}; use crate::requires_python::RequiresPythonRange; use crate::resolver::ForkState; +use crate::universal_marker::UniversalMarker; use crate::PythonRequirement; /// Represents one or more marker environments for a resolution. @@ -333,6 +334,26 @@ impl ResolverEnvironment { } } + /// Creates a universal marker expression corresponding to the fork that is + /// represented by this resolver environment. A universal marker includes + /// not just the standard PEP 508 marker, but also a marker based on + /// conflicting extras/groups. + /// + /// This returns `None` when this does not correspond to a fork. + pub(crate) fn try_universal_markers(&self) -> Option { + match self.kind { + Kind::Specific { .. } => None, + Kind::Universal { ref markers, .. } => { + if markers.is_true() { + None + } else { + // FIXME: Support conflicts. + Some(UniversalMarker::new(markers.clone(), MarkerTree::TRUE)) + } + } + } + } + /// Returns a requires-python version range derived from the marker /// expression describing this resolver environment. /// diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index e7ed401a8dd23..09b08debe0d12 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -63,6 +63,7 @@ pub(crate) use crate::resolver::availability::{ }; use crate::resolver::batch_prefetch::BatchPrefetcher; pub use crate::resolver::derivation::DerivationChainBuilder; +use crate::universal_marker::UniversalMarker; use crate::resolver::groups::Groups; pub use crate::resolver::index::InMemoryIndex; @@ -384,9 +385,8 @@ impl ResolverState UniversalMarker { + // FIXME: Account for extras and groups here. + UniversalMarker::new(self.marker.clone(), MarkerTree::TRUE) + } +} + /// Fetch the metadata for an item #[derive(Debug)] #[allow(clippy::large_enum_variant)] diff --git a/crates/uv-resolver/src/universal_marker.rs b/crates/uv-resolver/src/universal_marker.rs new file mode 100644 index 0000000000000..74366ad760f72 --- /dev/null +++ b/crates/uv-resolver/src/universal_marker.rs @@ -0,0 +1,131 @@ +use uv_normalize::ExtraName; +use uv_pep508::{MarkerEnvironment, MarkerTree}; + +/// A representation of a marker for use in universal resolution. +/// +/// (This also degrades gracefully to a standard PEP 508 marker in the case of +/// non-universal resolution.) +/// +/// This universal marker is meant to combine both a PEP 508 marker and a +/// marker for conflicting extras/groups. The latter specifically expresses +/// whether a particular edge in a dependency graph should be followed +/// depending on the activated extras and groups. +/// +/// A universal marker evaluates to true only when *both* its PEP 508 marker +/// and its conflict marker evaluate to true. +#[derive(Debug, Default, Clone, Eq, Hash, PartialEq, PartialOrd, Ord)] +pub struct UniversalMarker { + pep508_marker: MarkerTree, + conflict_marker: MarkerTree, +} + +impl UniversalMarker { + /// A constant universal marker that always evaluates to `true`. + pub(crate) const TRUE: UniversalMarker = UniversalMarker { + pep508_marker: MarkerTree::TRUE, + conflict_marker: MarkerTree::TRUE, + }; + + /// A constant universal marker that always evaluates to `false`. + pub(crate) const FALSE: UniversalMarker = UniversalMarker { + pep508_marker: MarkerTree::FALSE, + conflict_marker: MarkerTree::FALSE, + }; + + /// Creates a new universal marker from its constituent pieces. + pub(crate) fn new(pep508_marker: MarkerTree, conflict_marker: MarkerTree) -> UniversalMarker { + UniversalMarker { + pep508_marker, + conflict_marker, + } + } + + /// Combine this universal marker with the one given in a way that unions + /// them. That is, the updated marker will evaluate to `true` if `self` or + /// `other` evaluate to `true`. + pub(crate) fn or(&mut self, other: UniversalMarker) { + self.pep508_marker.or(other.pep508_marker); + self.conflict_marker.or(other.conflict_marker); + } + + /// Combine this universal marker with the one given in a way that + /// intersects them. That is, the updated marker will evaluate to `true` if + /// `self` and `other` evaluate to `true`. + pub(crate) fn and(&mut self, other: UniversalMarker) { + self.pep508_marker.and(other.pep508_marker); + self.conflict_marker.and(other.conflict_marker); + } + + /// Returns true if this universal marker will always evaluate to `true`. + pub(crate) fn is_true(&self) -> bool { + self.pep508_marker.is_true() && self.conflict_marker.is_true() + } + + /// Returns true if this universal marker will always evaluate to `false`. + pub(crate) fn is_false(&self) -> bool { + self.pep508_marker.is_false() || self.conflict_marker.is_false() + } + + /// Returns true if this universal marker is disjoint with the one given. + /// + /// Two universal markers are disjoint when it is impossible for them both + /// to evaluate to `true` simultaneously. + pub(crate) fn is_disjoint(&self, other: &UniversalMarker) -> bool { + self.pep508_marker.is_disjoint(&other.pep508_marker) + || self.conflict_marker.is_disjoint(&other.conflict_marker) + } + + /// Returns true if this universal marker is satisfied by the given + /// marker environment and list of activated extras. + /// + /// FIXME: This also needs to accept a list of groups. + pub(crate) fn evaluate(&self, env: &MarkerEnvironment, extras: &[ExtraName]) -> bool { + self.pep508_marker.evaluate(env, extras) && self.conflict_marker.evaluate(env, extras) + } + + /// Returns the PEP 508 marker for this universal marker. + /// + /// One should be cautious using this. Generally speaking, it should only + /// be used when one knows universal resolution isn't in effect. When + /// universal resolution is enabled (i.e., there may be multiple forks + /// producing different versions of the same package), then one should + /// always use a universal marker since it accounts for all possible ways + /// for a package to be installed. + pub fn pep508(&self) -> &MarkerTree { + &self.pep508_marker + } + + /// Returns the non-PEP 508 marker expression that represents conflicting + /// extras/groups. + /// + /// Like with `UniversalMarker::pep508`, one should be cautious when using + /// this. It is generally always wrong to consider conflicts in isolation + /// from PEP 508 markers. But this can be useful for detecting failure + /// cases. For example, the code for emitting a `ResolverOutput` (even a + /// universal one) in a `requirements.txt` format checks for the existence + /// of non-trivial conflict markers and fails if any are found. (Because + /// conflict markers cannot be represented in the `requirements.txt` + /// format.) + pub fn conflict(&self) -> &MarkerTree { + &self.conflict_marker + } +} + +impl std::fmt::Display for UniversalMarker { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + if self.pep508_marker.is_false() || self.conflict_marker.is_false() { + return write!(f, "`false`"); + } + match ( + self.pep508_marker.contents(), + self.conflict_marker.contents(), + ) { + (None, None) => write!(f, "`true`"), + (Some(pep508), None) => write!(f, "`{pep508}`"), + (None, Some(conflict)) => write!(f, "`true` (conflict marker: `{conflict}`"), + (Some(pep508), Some(conflict)) => { + write!(f, "`{pep508}` (conflict marker: `{conflict}`") + } + } + } +} diff --git a/crates/uv/src/commands/project/lock.rs b/crates/uv/src/commands/project/lock.rs index c0bb000eaf3ba..1eefe96f163ae 100644 --- a/crates/uv/src/commands/project/lock.rs +++ b/crates/uv/src/commands/project/lock.rs @@ -588,7 +588,18 @@ async fn do_lock( // the environment changed, e.g. the python bound check above can lead to different forking. let resolver_env = ResolverEnvironment::universal( forks_lock - .map(|lock| lock.fork_markers().to_vec()) + .map(|lock| { + // TODO(ag): Consider whether we should be capturing + // conflicting extras/groups for every fork. If + // we did, then we'd be able to use them here, + // which would in turn flow into construction of + // `ResolverEnvironment`. + lock.fork_markers() + .iter() + .map(|marker| marker.pep508()) + .cloned() + .collect() + }) .unwrap_or_else(|| { environments .cloned()