From 34198290f9afaebcf11e511340a7a3ac44b7418d Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Thu, 21 Nov 2024 08:26:55 -0500 Subject: [PATCH 1/6] uv-pep508: add a clarifying test This test demonstrates the difference between `extra != "foo"` and `sys_platform != "foo"`. I wrote this test down to test the extra simplification logic was correct. And I also wanted to test whether we could somehow hackily encode `group` (as opposed to just `extra`) logic into marker expressions by reusing another field. But I don't think we can. --- crates/uv-pep508/src/marker/tree.rs | 33 +++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/crates/uv-pep508/src/marker/tree.rs b/crates/uv-pep508/src/marker/tree.rs index 5c5502e31a34..e312022d9abf 100644 --- a/crates/uv-pep508/src/marker/tree.rs +++ b/crates/uv-pep508/src/marker/tree.rs @@ -2451,6 +2451,39 @@ mod test { assert!(m("sys_platform == 'win32' and sys_platform != 'win32'").is_false()); } + /// This tests the difference between simplifying extras and simplifying + /// other stringly-valued marker attributes. + /// + /// In particular, this demonstrates that `extra != "foo"` would actually + /// be more clearly written as `"foo" not in extras`. And similarly, `extra + /// == "foo"` would be written as `"foo" in extras`. This is different from + /// other attributes, like `sys_platform`, where the test is just string + /// equality. That is, `extra` is a set where as `sys_platform` is just a + /// single value. + #[test] + fn test_simplification_extra_versus_other() { + // Here, the `extra != 'foo'` cannot be simplified out, because + // `extra == 'foo'` can be true even when `extra == 'bar`' is true. + assert_simplifies( + r#"extra != "foo" and (extra == "bar" or extra == "baz")"#, + "(extra == 'bar' and extra != 'foo') or (extra == 'baz' and extra != 'foo')", + ); + // But here, the `sys_platform != 'foo'` can be simplified out, because + // it is strictly disjoint with + // `sys_platform == "bar" or sys_platform == "baz"`. + assert_simplifies( + r#"sys_platform != "foo" and (sys_platform == "bar" or sys_platform == "baz")"#, + "sys_platform == 'bar' or sys_platform == 'baz'", + ); + + // Another case I experimented with and wanted to verify. + assert_simplifies( + r#"(extra != "bar" and (extra == "foo" or extra == "baz")) + or ((extra != "foo" and extra != "bar") and extra == "baz")"#, + "(extra != 'bar' and extra == 'baz') or (extra != 'bar' and extra == 'foo')", + ); + } + #[test] fn test_marker_negation() { assert_eq!( From c36ad34fc274474586410724976203701aa2104c Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Thu, 21 Nov 2024 08:35:12 -0500 Subject: [PATCH 2/6] uv-resolver: remove 'fork.is_false()' filtering This filtering is now redundant, since forking now avoids these degenerate cases by construction. The main change to forking that enables skipping over "always false" forks is that forking now starts with the parent's markers instead of starting with MarkerTree::TRUE and trying to combine them with the parent's markers later. This in turn leads to skipping over anything that "can't" happen when combined with the parents markers. So we never hit the case of generating a fork that, when combined with the parent's markers, results in a marker that is always false. We just avoid it in the first place. --- crates/uv-resolver/src/resolution/output.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/crates/uv-resolver/src/resolution/output.rs b/crates/uv-resolver/src/resolution/output.rs index ffa8175672da..1f7b33746969 100644 --- a/crates/uv-resolver/src/resolution/output.rs +++ b/crates/uv-resolver/src/resolution/output.rs @@ -188,8 +188,6 @@ impl ResolverOutput { .expect("A non-forking resolution exists in forking mode") .clone() }) - // Any unsatisfiable forks were skipped. - .filter(|fork| !fork.is_false()) .collect() }) .unwrap_or_else(Vec::new) @@ -203,8 +201,6 @@ impl ResolverOutput { .cloned() .unwrap_or(MarkerTree::TRUE) }) - // Any unsatisfiable forks were skipped. - .filter(|fork| !fork.is_false()) .collect() }; From fbe5f50a00479de3f9acf124dda54f380bf57d29 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Thu, 21 Nov 2024 08:56:00 -0500 Subject: [PATCH 3/6] uv-resolver: simplify `fork_markers` construction This doesn't change any behavior. My guess is that this code was a casualty of refactoring. But basically, it was doing redundant case analysis and iterating over all resolutions (even though it's in the branch that can only occur when there is only one resolution). --- crates/uv-resolver/src/resolution/output.rs | 27 +++------------------ 1 file changed, 3 insertions(+), 24 deletions(-) diff --git a/crates/uv-resolver/src/resolution/output.rs b/crates/uv-resolver/src/resolution/output.rs index 1f7b33746969..93ac6ddeed20 100644 --- a/crates/uv-resolver/src/resolution/output.rs +++ b/crates/uv-resolver/src/resolution/output.rs @@ -174,33 +174,12 @@ impl ResolverOutput { // Extract the `Requires-Python` range, if provided. let requires_python = python.target().clone(); - let fork_markers = if let [resolution] = resolutions { - resolution - .env - .try_markers() - .map(|_| { - resolutions - .iter() - .map(|resolution| { - resolution - .env - .try_markers() - .expect("A non-forking resolution exists in forking mode") - .clone() - }) - .collect() - }) - .unwrap_or_else(Vec::new) + let fork_markers: Vec = if let [resolution] = resolutions { + resolution.env.try_markers().cloned().into_iter().collect() } else { resolutions .iter() - .map(|resolution| { - resolution - .env - .try_markers() - .cloned() - .unwrap_or(MarkerTree::TRUE) - }) + .map(|resolution| resolution.env.try_markers().cloned().unwrap_or_default()) .collect() }; From 8c190c20633e3db5fbe33339246a5b0b7ff6cce2 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Thu, 21 Nov 2024 10:31:07 -0500 Subject: [PATCH 4/6] uv-resolver: remove 'package_markers' This was completely unused. I noticed this while trying to read and understand the code. It's unclear when or how this happened. --- crates/uv-resolver/src/resolution/output.rs | 16 ---------------- crates/uv-resolver/src/resolver/mod.rs | 6 ------ 2 files changed, 22 deletions(-) diff --git a/crates/uv-resolver/src/resolution/output.rs b/crates/uv-resolver/src/resolution/output.rs index 93ac6ddeed20..da681d67ac0c 100644 --- a/crates/uv-resolver/src/resolution/output.rs +++ b/crates/uv-resolver/src/resolution/output.rs @@ -33,8 +33,6 @@ use crate::{ VersionsResponse, }; -pub(crate) type MarkersForDistribution = Vec; - /// The output of a successful resolution. /// /// Includes a complete resolution graph in which every node represents a pinned package and every @@ -119,24 +117,10 @@ impl ResolverOutput { // Add the root node. let root_index = graph.add_node(ResolutionGraphNode::Root); - let mut package_markers: FxHashMap = - FxHashMap::default(); - let mut seen = FxHashSet::default(); for resolution in resolutions { // Add every package to the graph. for (package, version) in &resolution.nodes { - if package.is_base() { - // For packages with diverging versions, store which version comes from which - // fork. - if let Some(markers) = resolution.env.try_markers() { - package_markers - .entry(package.name.clone()) - .or_default() - .push(markers.clone()); - } - } - if !seen.insert((package, version)) { // Insert each node only once. continue; diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index 14acd60cb79f..e7ed401a8dd2 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -2612,12 +2612,6 @@ pub(crate) struct ResolutionDependencyEdge { pub(crate) marker: MarkerTree, } -impl ResolutionPackage { - pub(crate) fn is_base(&self) -> bool { - self.extra.is_none() && self.dev.is_none() - } -} - /// Fetch the metadata for an item #[derive(Debug)] #[allow(clippy::large_enum_variant)] From 0e4ced639fc329ea18776a11e2eb0d518cf43280 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Thu, 21 Nov 2024 13:23:39 -0500 Subject: [PATCH 5/6] uv/pip/compile: slightly simplify conflicts This doesn't change any behavior. But this makes it a bit clearer in the code that `uv pip compile` does not support specifying conflicts. Indeed, we always pass an empty set of conflicts to the resolver. This is because there is no way to encode the conditional logic of conflicts into the `requirements.txt` format. This is unlike markers. --- crates/uv/src/commands/pip/compile.rs | 12 +++--------- crates/uv/src/lib.rs | 2 -- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/crates/uv/src/commands/pip/compile.rs b/crates/uv/src/commands/pip/compile.rs index f022f31583f4..885cef6e4edc 100644 --- a/crates/uv/src/commands/pip/compile.rs +++ b/crates/uv/src/commands/pip/compile.rs @@ -54,7 +54,6 @@ pub(crate) async fn pip_compile( constraints_from_workspace: Vec, overrides_from_workspace: Vec, environments: SupportedEnvironments, - conflicts: Conflicts, extras: ExtrasSpecification, output_file: Option<&Path>, resolution_mode: ResolutionMode, @@ -252,20 +251,15 @@ pub(crate) async fn pip_compile( }; // Determine the environment for the resolution. - let (tags, resolver_env, conflicting_groups) = if universal { + let (tags, resolver_env) = if universal { ( None, ResolverEnvironment::universal(environments.into_markers()), - conflicts, ) } else { let (tags, marker_env) = resolution_environment(python_version, python_platform, &interpreter)?; - ( - Some(tags), - ResolverEnvironment::specific(marker_env), - Conflicts::empty(), - ) + (Some(tags), ResolverEnvironment::specific(marker_env)) }; // Generate, but don't enforce hashes for the requirements. @@ -400,7 +394,7 @@ pub(crate) async fn pip_compile( tags.as_deref(), resolver_env.clone(), python_requirement, - conflicting_groups, + Conflicts::empty(), &client, &flat_index, &top_level_index, diff --git a/crates/uv/src/lib.rs b/crates/uv/src/lib.rs index 8ddecaa4a30d..f095ce86d380 100644 --- a/crates/uv/src/lib.rs +++ b/crates/uv/src/lib.rs @@ -24,7 +24,6 @@ use uv_cli::{PythonCommand, PythonNamespace, ToolCommand, ToolNamespace, TopLeve #[cfg(feature = "self-update")] use uv_cli::{SelfCommand, SelfNamespace, SelfUpdateArgs}; use uv_fs::CWD; -use uv_pypi_types::Conflicts; use uv_requirements::RequirementsSource; use uv_scripts::{Pep723Item, Pep723Metadata, Pep723Script}; use uv_settings::{Combine, FilesystemOptions, Options}; @@ -333,7 +332,6 @@ async fn run(mut cli: Cli) -> Result { args.constraints_from_workspace, args.overrides_from_workspace, args.environments, - Conflicts::empty(), args.settings.extras, args.settings.output_file.as_deref(), args.settings.resolution, From 4645037a4cd446bd5ccb228567e12aa598486e76 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Thu, 21 Nov 2024 14:01:55 -0500 Subject: [PATCH 6/6] 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 | 2 + 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 | 12 ++ 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 | 16 ++- 18 files changed, 376 insertions(+), 92 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 5d0d824c82cb..ed6c0690602b 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 e09a66c753dd..eebd6ab88e9a 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 af0193d9a737..90cd4634ce79 100644 --- a/crates/uv-resolver/src/lib.rs +++ b/crates/uv-resolver/src/lib.rs @@ -22,6 +22,7 @@ pub use resolver::{ PackageVersionsResult, Reporter as ResolverReporter, Resolver, ResolverEnvironment, ResolverProvider, VersionsResponse, WheelMetadataResult, }; +pub use universal_marker::UniversalMarker; pub use version_map::VersionMap; pub use yanks::AllowedYanks; @@ -56,5 +57,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 66538c1bfb01..d801d70310aa 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 5c6e83fb7dd1..21635809b530 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 066df532804a..0ee9dabab833 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 066df532804a..0ee9dabab833 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 066df532804a..0ee9dabab833 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 3ba08dcb398a..1871893d5110 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 ef7ecab50abf..300ebed33d0d 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 bae08bcc62f1..cd0d6703f63d 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,13 @@ impl<'a> DisplayResolutionGraph<'a> { include_index_annotation: bool, annotation_style: AnnotationStyle, ) -> DisplayResolutionGraph<'a> { + for fork_marker in &underlying.fork_markers { + assert!( + fork_marker.conflict().is_true(), + "found fork marker {fork_marker} with non-trivial conflicting marker, \ + cannot display resolver output with conflicts in requirements.txt format", + ); + } Self { resolution: underlying, env, diff --git a/crates/uv-resolver/src/resolution/mod.rs b/crates/uv-resolver/src/resolution/mod.rs index beb0a85f1106..43eed44c2978 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 da681d67ac0c..e9d01e171458 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 3117fe32b501..6a956ce9d091 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 91fa9d2745c5..d8a6fccd6691 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 e7ed401a8dd2..09b08debe0d1 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 000000000000..74366ad760f7 --- /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 c0bb000eaf3b..76b8dd36f122 100644 --- a/crates/uv/src/commands/project/lock.rs +++ b/crates/uv/src/commands/project/lock.rs @@ -28,7 +28,8 @@ use uv_requirements::upgrade::{read_lock_requirements, LockedRequirements}; use uv_requirements::ExtrasResolver; use uv_resolver::{ FlatIndex, InMemoryIndex, Lock, LockVersion, Options, OptionsBuilder, PythonRequirement, - RequiresPython, ResolverEnvironment, ResolverManifest, SatisfiesResult, VERSION, + RequiresPython, ResolverEnvironment, ResolverManifest, SatisfiesResult, UniversalMarker, + VERSION, }; use uv_settings::PythonInstallMirrors; use uv_types::{BuildContext, BuildIsolation, EmptyInstalledPackages, HashStrategy}; @@ -588,7 +589,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(UniversalMarker::pep508) + .cloned() + .collect() + }) .unwrap_or_else(|| { environments .cloned()