diff --git a/crates/uv-resolver/src/error.rs b/crates/uv-resolver/src/error.rs index 418af506e73b..4a38e3e303c6 100644 --- a/crates/uv-resolver/src/error.rs +++ b/crates/uv-resolver/src/error.rs @@ -222,6 +222,7 @@ impl std::fmt::Display for NoSolutionError { // Transform the error tree for reporting let mut tree = self.error.clone(); + simplify_derivation_tree_markers(&self.python_requirement, &mut tree); let should_display_tree = std::env::var_os("UV_INTERNAL__SHOW_DERIVATION_TREE").is_some() || tracing::enabled!(tracing::Level::TRACE); @@ -470,6 +471,52 @@ fn collapse_redundant_depends_on_no_versions_inner( } } +/// Simplifies the markers on pubgrub packages in the given derivation tree +/// according to the given Python requirement. +/// +/// For example, when there's a dependency like `foo ; python_version >= +/// '3.11'` and `requires-python = '>=3.11'`, this simplification will remove +/// the `python_version >= '3.11'` marker since it's implied to be true by +/// the `requires-python` setting. This simplifies error messages by reducing +/// noise. +fn simplify_derivation_tree_markers( + python_requirement: &PythonRequirement, + tree: &mut DerivationTree, UnavailableReason>, +) { + match tree { + DerivationTree::External(External::NotRoot(ref mut pkg, _)) => { + pkg.simplify_markers(python_requirement); + } + DerivationTree::External(External::NoVersions(ref mut pkg, _)) => { + pkg.simplify_markers(python_requirement); + } + DerivationTree::External(External::FromDependencyOf(ref mut pkg1, _, ref mut pkg2, _)) => { + pkg1.simplify_markers(python_requirement); + pkg2.simplify_markers(python_requirement); + } + DerivationTree::External(External::Custom(ref mut pkg, _, _)) => { + pkg.simplify_markers(python_requirement); + } + DerivationTree::Derived(derived) => { + derived.terms = std::mem::take(&mut derived.terms) + .into_iter() + .map(|(mut pkg, term)| { + pkg.simplify_markers(python_requirement); + (pkg, term) + }) + .collect(); + simplify_derivation_tree_markers( + python_requirement, + Arc::make_mut(&mut derived.cause1), + ); + simplify_derivation_tree_markers( + python_requirement, + Arc::make_mut(&mut derived.cause2), + ); + } + } +} + /// Given a [`DerivationTree`], collapse incompatibilities for versions of a package that are /// unavailable for the same reason to avoid repeating the same message for every unavailable /// version. diff --git a/crates/uv-resolver/src/lock/mod.rs b/crates/uv-resolver/src/lock/mod.rs index 98ff77c91222..dab7eb54c026 100644 --- a/crates/uv-resolver/src/lock/mod.rs +++ b/crates/uv-resolver/src/lock/mod.rs @@ -39,6 +39,7 @@ use uv_workspace::{InstallTarget, Workspace}; pub use crate::lock::requirements_txt::RequirementsTxtExport; pub use crate::lock::tree::TreeDisplay; +use crate::requires_python::SimplifiedMarkerTree; use crate::resolution::{AnnotatedDist, ResolutionGraphNode}; use crate::{ExcludeNewer, PrereleaseMode, RequiresPython, ResolutionGraph, ResolutionMode}; @@ -108,7 +109,7 @@ impl Lock { continue; }; let marker = edge.weight().clone(); - package.add_dependency(dependency_dist, marker, root)?; + package.add_dependency(&requires_python, dependency_dist, marker, root)?; } let id = package.id.clone(); @@ -141,6 +142,7 @@ impl Lock { }; let marker = edge.weight().clone(); package.add_optional_dependency( + &requires_python, extra.clone(), dependency_dist, marker, @@ -163,7 +165,13 @@ impl Lock { continue; }; let marker = edge.weight().clone(); - package.add_dev_dependency(group.clone(), dependency_dist, marker, root)?; + package.add_dev_dependency( + &requires_python, + group.clone(), + dependency_dist, + marker, + root, + )?; } } } @@ -370,7 +378,19 @@ impl Lock { /// Record the supported environments that were used to generate this lock. #[must_use] pub fn with_supported_environments(mut self, supported_environments: Vec) -> Self { - self.supported_environments = supported_environments; + // We "complexify" the markers given, since the supported + // environments given might be coming directly from what's written in + // `pyproject.toml`, and those are assumed to be simplified (i.e., + // they assume `requires-python` is true). But a `Lock` always uses + // non-simplified markers internally, so we need to re-complexify them + // here. + // + // The nice thing about complexifying is that it's a no-op if the + // markers given have already been complexified. + self.supported_environments = supported_environments + .into_iter() + .map(|marker| self.requires_python.complexify_markers(marker)) + .collect(); self } @@ -424,6 +444,22 @@ impl Lock { &self.manifest.members } + /// Returns the supported environments that were used to generate this + /// lock. + /// + /// The markers returned here are "simplified" with respect to the lock + /// file's `requires-python` setting. This means these should only be used + /// for direct comparison purposes with the supported environments written + /// by a human in `pyproject.toml`. (Think of "supported environments" in + /// `pyproject.toml` as having an implicit `and python_full_version >= + /// '{requires-python-bound}'` attached to each one.) + pub fn simplified_supported_environments(&self) -> Vec { + self.supported_environments() + .iter() + .map(|marker| self.requires_python.simplify_markers(marker.clone())) + .collect() + } + /// 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] { @@ -512,7 +548,7 @@ impl Lock { )) }; for dep in deps { - if dep.marker.evaluate(marker_env, &[]) { + if dep.complexified_marker.evaluate(marker_env, &[]) { let dep_dist = self.find_by_id(&dep.package_id); if seen.insert((&dep.package_id, None)) { queue.push_back((dep_dist, None)); @@ -547,8 +583,8 @@ impl Lock { let fork_markers = each_element_on_its_line_array( self.fork_markers .iter() - .filter_map(MarkerTree::contents) - .map(|marker| marker.to_string()), + .map(|marker| SimplifiedMarkerTree::new(&self.requires_python, marker.clone())) + .filter_map(|marker| marker.try_to_string()), ); doc.insert("resolution-markers", value(fork_markers)); } @@ -557,8 +593,8 @@ impl Lock { let supported_environments = each_element_on_its_line_array( self.supported_environments .iter() - .filter_map(MarkerTree::contents) - .map(|marker| marker.to_string()), + .map(|marker| SimplifiedMarkerTree::new(&self.requires_python, marker.clone())) + .filter_map(|marker| marker.try_to_string()), ); doc.insert("supported-markers", value(supported_environments)); } @@ -682,7 +718,7 @@ impl Lock { let mut packages = ArrayOfTables::new(); for dist in &self.packages { - packages.push(dist.to_toml(&dist_count_by_name)?); + packages.push(dist.to_toml(&self.requires_python, &dist_count_by_name)?); } doc.insert("package", Item::ArrayOfTables(packages)); @@ -1174,9 +1210,9 @@ struct LockWire { /// 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. #[serde(rename = "resolution-markers", default)] - fork_markers: Vec, + fork_markers: Vec, #[serde(rename = "supported-markers", default)] - supported_environments: Vec, + supported_environments: Vec, /// We discard the lockfile if these options match. #[serde(default)] options: ResolverOptions, @@ -1186,20 +1222,6 @@ struct LockWire { packages: Vec, } -impl From for LockWire { - fn from(lock: Lock) -> LockWire { - LockWire { - version: lock.version, - requires_python: lock.requires_python, - fork_markers: lock.fork_markers, - supported_environments: lock.supported_environments, - options: lock.options, - manifest: lock.manifest, - packages: lock.packages.into_iter().map(PackageWire::from).collect(), - } - } -} - impl TryFrom for Lock { type Error = LockError; @@ -1224,17 +1246,62 @@ impl TryFrom for Lock { let packages = wire .packages .into_iter() - .map(|dist| dist.unwire(&unambiguous_package_ids)) + .map(|dist| dist.unwire(&wire.requires_python, &unambiguous_package_ids)) .collect::, _>>()?; - Lock::new( + let supported_environments = wire + .supported_environments + .into_iter() + .map(|simplified_marker| simplified_marker.into_marker(&wire.requires_python)) + .collect(); + let fork_markers = wire + .fork_markers + .into_iter() + .map(|simplified_marker| simplified_marker.into_marker(&wire.requires_python)) + .collect(); + let lock = Lock::new( wire.version, packages, wire.requires_python, wire.options, wire.manifest, - wire.supported_environments, - wire.fork_markers, - ) + supported_environments, + fork_markers, + )?; + + /* + // TODO: Use the below in tests to validate we don't produce a + // trivially incorrect lock file. + let mut name_to_markers: BTreeMap<&PackageName, Vec<(&Version, &MarkerTree)>> = + BTreeMap::new(); + for package in &lock.packages { + for dep in &package.dependencies { + name_to_markers + .entry(&dep.package_id.name) + .or_default() + .push((&dep.package_id.version, &dep.marker)); + } + } + for (name, marker_trees) in name_to_markers { + for (i, (version1, marker1)) in marker_trees.iter().enumerate() { + for (version2, marker2) in &marker_trees[i + 1..] { + if version1 == version2 { + continue; + } + if !marker1.is_disjoint(marker2) { + eprintln!("{}", lock.to_toml().unwrap()); + assert!( + false, + "[{marker1:?}] (for version {version1}) is not disjoint with \ + [{marker2:?}] (for version {version2}) \ + for package `{name}`", + ); + } + } + } + } + */ + + Ok(lock) } } @@ -1316,14 +1383,38 @@ impl Package { /// Add the [`AnnotatedDist`] as a dependency of the [`Package`]. fn add_dependency( &mut self, + requires_python: &RequiresPython, annotated_dist: &AnnotatedDist, marker: MarkerTree, root: &Path, ) -> Result<(), LockError> { - let new_dep = Dependency::from_annotated_dist(annotated_dist, marker, root)?; + let new_dep = + Dependency::from_annotated_dist(requires_python, annotated_dist, marker, root)?; for existing_dep in &mut self.dependencies { if existing_dep.package_id == new_dep.package_id - && existing_dep.marker == new_dep.marker + // It's important that we do a comparison on + // *simplified* markers here. In particular, when + // we write markers out to the lock file, we use + // "simplified" markers, or markers that are simplified + // *given* that `requires-python` is satisfied. So if + // we don't do equality based on what the simplified + // marker is, we might wind up not merging dependencies + // that ought to be merged and thus writing out extra + // entries. + // + // For example, if `requires-python = '>=3.8'` and we + // have `foo==1` and + // `foo==1 ; python_version >= '3.8'` dependencies, + // then they don't have equivalent complexified + // markers, but their simplified markers are identical. + // + // NOTE: It does seem like perhaps this should + // be implemented semantically/algebraically on + // `MarkerTree` itself, but it wasn't totally clear + // how to do that. I think `pep508` would need to + // grow a concept of "requires python" and provide an + // operation specifically for that. + && existing_dep.simplified_marker == new_dep.simplified_marker { existing_dep.extra.extend(new_dep.extra); return Ok(()); @@ -1337,15 +1428,20 @@ impl Package { /// Add the [`AnnotatedDist`] as an optional dependency of the [`Package`]. fn add_optional_dependency( &mut self, + requires_python: &RequiresPython, extra: ExtraName, annotated_dist: &AnnotatedDist, marker: MarkerTree, root: &Path, ) -> Result<(), LockError> { - let dep = Dependency::from_annotated_dist(annotated_dist, marker, root)?; + let dep = Dependency::from_annotated_dist(requires_python, annotated_dist, marker, root)?; let optional_deps = self.optional_dependencies.entry(extra).or_default(); for existing_dep in &mut *optional_deps { - if existing_dep.package_id == dep.package_id && existing_dep.marker == dep.marker { + if existing_dep.package_id == dep.package_id + // See note in add_dependency for why we use + // simplified markers here. + && existing_dep.simplified_marker == dep.simplified_marker + { existing_dep.extra.extend(dep.extra); return Ok(()); } @@ -1358,15 +1454,20 @@ impl Package { /// Add the [`AnnotatedDist`] as a development dependency of the [`Package`]. fn add_dev_dependency( &mut self, + requires_python: &RequiresPython, dev: GroupName, annotated_dist: &AnnotatedDist, marker: MarkerTree, root: &Path, ) -> Result<(), LockError> { - let dep = Dependency::from_annotated_dist(annotated_dist, marker, root)?; + let dep = Dependency::from_annotated_dist(requires_python, annotated_dist, marker, root)?; let dev_deps = self.dev_dependencies.entry(dev).or_default(); for existing_dep in &mut *dev_deps { - if existing_dep.package_id == dep.package_id && existing_dep.marker == dep.marker { + if existing_dep.package_id == dep.package_id + // See note in add_dependency for why we use + // simplified markers here. + && existing_dep.simplified_marker == dep.simplified_marker + { existing_dep.extra.extend(dep.extra); return Ok(()); } @@ -1634,7 +1735,11 @@ impl Package { Ok(Some(sdist)) } - fn to_toml(&self, dist_count_by_name: &FxHashMap) -> anyhow::Result { + fn to_toml( + &self, + requires_python: &RequiresPython, + dist_count_by_name: &FxHashMap, + ) -> anyhow::Result
{ let mut table = Table::new(); self.id.to_toml(None, &mut table); @@ -1643,28 +1748,27 @@ impl Package { let wheels = each_element_on_its_line_array( self.fork_markers .iter() - .filter_map(MarkerTree::contents) - .map(|marker| marker.to_string()), + .map(|marker| SimplifiedMarkerTree::new(requires_python, marker.clone())) + .filter_map(|marker| marker.try_to_string()), ); table.insert("resolution-markers", value(wheels)); } if !self.dependencies.is_empty() { - let deps = each_element_on_its_line_array( - self.dependencies - .iter() - .map(|dep| dep.to_toml(dist_count_by_name).into_inline_table()), - ); + let deps = each_element_on_its_line_array(self.dependencies.iter().map(|dep| { + dep.to_toml(requires_python, dist_count_by_name) + .into_inline_table() + })); table.insert("dependencies", value(deps)); } if !self.optional_dependencies.is_empty() { let mut optional_deps = Table::new(); for (extra, deps) in &self.optional_dependencies { - let deps = each_element_on_its_line_array( - deps.iter() - .map(|dep| dep.to_toml(dist_count_by_name).into_inline_table()), - ); + let deps = each_element_on_its_line_array(deps.iter().map(|dep| { + dep.to_toml(requires_python, dist_count_by_name) + .into_inline_table() + })); if !deps.is_empty() { optional_deps.insert(extra.as_ref(), value(deps)); } @@ -1677,10 +1781,10 @@ impl Package { if !self.dev_dependencies.is_empty() { let mut dev_dependencies = Table::new(); for (extra, deps) in &self.dev_dependencies { - let deps = each_element_on_its_line_array( - deps.iter() - .map(|dep| dep.to_toml(dist_count_by_name).into_inline_table()), - ); + let deps = each_element_on_its_line_array(deps.iter().map(|dep| { + dep.to_toml(requires_python, dist_count_by_name) + .into_inline_table() + })); if !deps.is_empty() { dev_dependencies.insert(extra.as_ref(), value(deps)); } @@ -1849,7 +1953,7 @@ struct PackageWire { #[serde(default)] wheels: Vec, #[serde(default, rename = "resolution-markers")] - fork_markers: Vec, + fork_markers: Vec, #[serde(default)] dependencies: Vec, #[serde(default)] @@ -1870,11 +1974,12 @@ struct PackageMetadata { impl PackageWire { fn unwire( self, + requires_python: &RequiresPython, unambiguous_package_ids: &FxHashMap, ) -> Result { let unwire_deps = |deps: Vec| -> Result, LockError> { deps.into_iter() - .map(|dep| dep.unwire(unambiguous_package_ids)) + .map(|dep| dep.unwire(requires_python, unambiguous_package_ids)) .collect() }; Ok(Package { @@ -1882,7 +1987,11 @@ impl PackageWire { metadata: self.metadata, sdist: self.sdist, wheels: self.wheels, - fork_markers: self.fork_markers, + fork_markers: self + .fork_markers + .into_iter() + .map(|simplified_marker| simplified_marker.into_marker(requires_python)) + .collect(), dependencies: unwire_deps(self.dependencies)?, optional_dependencies: self .optional_dependencies @@ -1898,32 +2007,6 @@ impl PackageWire { } } -impl From for PackageWire { - fn from(dist: Package) -> PackageWire { - let wire_deps = |deps: Vec| -> Vec { - deps.into_iter().map(DependencyWire::from).collect() - }; - PackageWire { - id: dist.id, - metadata: dist.metadata, - sdist: dist.sdist, - wheels: dist.wheels, - fork_markers: dist.fork_markers, - dependencies: wire_deps(dist.dependencies), - optional_dependencies: dist - .optional_dependencies - .into_iter() - .map(|(extra, deps)| (extra, wire_deps(deps))) - .collect(), - dev_dependencies: dist - .dev_dependencies - .into_iter() - .map(|(group, deps)| (group, wire_deps(deps))) - .collect(), - } - } -} - /// Inside the lockfile, we match a dependency entry to a package entry through a key made up /// of the name, the version and the source url. #[derive(Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord, serde::Deserialize)] @@ -3131,26 +3214,70 @@ impl TryFrom for Wheel { struct Dependency { package_id: PackageId, extra: BTreeSet, - marker: MarkerTree, + /// A marker simplified 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'`. + /// + /// Generally speaking, this marker should not be exposed to + /// anything outside this module unless it's for a specialized use + /// case. But specifically, it should never be used to evaluate + /// against a marker environment or for disjointness checks or any + /// other kind of marker algebra. + /// + /// It exists because there are some cases where we do actually + /// want to compare markers in their "simplified" form. For + /// example, when collapsing the extras on duplicate dependencies. + /// Even if a dependency has different complexified markers, + /// they might have identical markers once simplified. And since + /// `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, } impl Dependency { + fn new( + requires_python: &RequiresPython, + package_id: PackageId, + extra: BTreeSet, + complexified_marker: MarkerTree, + ) -> Dependency { + let simplified_marker = + SimplifiedMarkerTree::new(requires_python, complexified_marker.clone()); + Dependency { + package_id, + extra, + simplified_marker, + complexified_marker, + } + } + fn from_annotated_dist( + requires_python: &RequiresPython, annotated_dist: &AnnotatedDist, - marker: MarkerTree, + complexified_marker: MarkerTree, root: &Path, ) -> Result { let package_id = PackageId::from_annotated_dist(annotated_dist, root)?; let extra = annotated_dist.extra.iter().cloned().collect(); - Ok(Self { + Ok(Dependency::new( + requires_python, package_id, extra, - marker, - }) + complexified_marker, + )) } /// Returns the TOML representation of this dependency. - fn to_toml(&self, dist_count_by_name: &FxHashMap) -> Table { + fn to_toml( + &self, + _requires_python: &RequiresPython, + dist_count_by_name: &FxHashMap, + ) -> Table { let mut table = Table::new(); self.package_id .to_toml(Some(dist_count_by_name), &mut table); @@ -3162,8 +3289,8 @@ impl Dependency { .collect::(); table.insert("extra", value(extra_array)); } - if let Some(marker) = self.marker.contents() { - table.insert("marker", value(marker.to_string())); + if let Some(marker) = self.simplified_marker.try_to_string() { + table.insert("marker", value(marker)); } table @@ -3199,32 +3326,25 @@ struct DependencyWire { #[serde(default)] extra: BTreeSet, #[serde(default)] - marker: MarkerTree, + marker: SimplifiedMarkerTree, } impl DependencyWire { fn unwire( self, + requires_python: &RequiresPython, unambiguous_package_ids: &FxHashMap, ) -> Result { + let complexified_marker = self.marker.clone().into_marker(requires_python); Ok(Dependency { package_id: self.package_id.unwire(unambiguous_package_ids)?, extra: self.extra, - marker: self.marker, + simplified_marker: self.marker, + complexified_marker, }) } } -impl From for DependencyWire { - fn from(dependency: Dependency) -> DependencyWire { - DependencyWire { - package_id: PackageIdForDependency::from(dependency.package_id), - extra: dependency.extra, - marker: dependency.marker, - } - } -} - /// A single hash for a distribution artifact in a lockfile. /// /// A hash is encoded as a single TOML string in the format diff --git a/crates/uv-resolver/src/lock/requirements_txt.rs b/crates/uv-resolver/src/lock/requirements_txt.rs index e5de97673d9c..19107f78e422 100644 --- a/crates/uv-resolver/src/lock/requirements_txt.rs +++ b/crates/uv-resolver/src/lock/requirements_txt.rs @@ -102,7 +102,11 @@ impl<'lock> RequirementsTxtExport<'lock> { // Add the edge. let dep_index = inverse[&dep.package_id]; - petgraph.add_edge(index, dep_index, dep.marker.clone()); + petgraph.add_edge( + index, + dep_index, + dep.simplified_marker.as_simplified_marker_tree().clone(), + ); // Push its dependencies on the queue. if seen.insert((&dep.package_id, None)) { diff --git a/crates/uv-resolver/src/lock/tree.rs b/crates/uv-resolver/src/lock/tree.rs index 898c47fe530b..71d8c3a499db 100644 --- a/crates/uv-resolver/src/lock/tree.rs +++ b/crates/uv-resolver/src/lock/tree.rs @@ -62,7 +62,8 @@ impl<'env> TreeDisplay<'env> { Cow::Owned(Dependency { package_id: packages.id.clone(), extra: dependency.extra.clone(), - marker: dependency.marker.clone(), + simplified_marker: dependency.simplified_marker.clone(), + complexified_marker: dependency.complexified_marker.clone(), }) } else { Cow::Borrowed(dependency) @@ -72,7 +73,10 @@ impl<'env> TreeDisplay<'env> { // Skip dependencies that don't apply to the current environment. if let Some(environment_markers) = markers { - if !dependency.marker.evaluate(environment_markers, &[]) { + if !dependency + .complexified_marker + .evaluate(environment_markers, &[]) + { continue; } } @@ -91,7 +95,8 @@ impl<'env> TreeDisplay<'env> { Cow::Owned(Dependency { package_id: packages.id.clone(), extra: dependency.extra.clone(), - marker: dependency.marker.clone(), + simplified_marker: dependency.simplified_marker.clone(), + complexified_marker: dependency.complexified_marker.clone(), }) } else { Cow::Borrowed(dependency) @@ -101,7 +106,10 @@ impl<'env> TreeDisplay<'env> { // Skip dependencies that don't apply to the current environment. if let Some(environment_markers) = markers { - if !dependency.marker.evaluate(environment_markers, &[]) { + if !dependency + .complexified_marker + .evaluate(environment_markers, &[]) + { continue; } } @@ -126,7 +134,8 @@ impl<'env> TreeDisplay<'env> { Cow::Owned(Dependency { package_id: packages.id.clone(), extra: dependency.extra.clone(), - marker: dependency.marker.clone(), + simplified_marker: dependency.simplified_marker.clone(), + complexified_marker: dependency.complexified_marker.clone(), }) } else { Cow::Borrowed(dependency) @@ -136,7 +145,10 @@ impl<'env> TreeDisplay<'env> { // Skip dependencies that don't apply to the current environment. if let Some(environment_markers) = markers { - if !dependency.marker.evaluate(environment_markers, &[]) { + if !dependency + .complexified_marker + .evaluate(environment_markers, &[]) + { continue; } } diff --git a/crates/uv-resolver/src/pubgrub/package.rs b/crates/uv-resolver/src/pubgrub/package.rs index 98c7944502ed..9bb7bdc75a94 100644 --- a/crates/uv-resolver/src/pubgrub/package.rs +++ b/crates/uv-resolver/src/pubgrub/package.rs @@ -4,6 +4,8 @@ use std::sync::Arc; use pep508_rs::{MarkerTree, MarkerTreeContents}; use uv_normalize::{ExtraName, GroupName, PackageName}; +use crate::python_requirement::PythonRequirement; + /// [`Arc`] wrapper around [`PubGrubPackageInner`] to make cloning (inside PubGrub) cheap. #[derive(Debug, Clone, Eq, Hash, PartialEq, PartialOrd, Ord)] pub struct PubGrubPackage(Arc); @@ -85,7 +87,8 @@ pub enum PubGrubPackageInner { /// rather than the `Marker` variant. Marker { name: PackageName, - marker: MarkerTreeContents, + /// The marker associated with this proxy package. + marker: MarkerTree, }, } @@ -101,15 +104,16 @@ impl PubGrubPackage { // extras end up having two distinct marker expressions, which in turn // makes them two distinct packages. This results in PubGrub being // unable to unify version constraints across such packages. - let marker = marker.simplify_extras_with(|_| true).contents(); + let tree = marker.simplify_extras_with(|_| true); + let marker = tree.contents(); if let Some(extra) = extra { Self(Arc::new(PubGrubPackageInner::Extra { name, extra, marker, })) - } else if let Some(marker) = marker { - Self(Arc::new(PubGrubPackageInner::Marker { name, marker })) + } else if marker.is_some() { + Self(Arc::new(PubGrubPackageInner::Marker { name, marker: tree })) } else { Self(Arc::new(PubGrubPackageInner::Package { name, @@ -158,7 +162,7 @@ impl PubGrubPackage { | PubGrubPackageInner::Dev { marker, .. } => { marker.as_ref().map(MarkerTreeContents::as_ref) } - PubGrubPackageInner::Marker { marker, .. } => Some(marker.as_ref()), + PubGrubPackageInner::Marker { marker, .. } => Some(marker), } } @@ -171,6 +175,47 @@ impl PubGrubPackage { | PubGrubPackageInner::Marker { .. } ) } + + /// This simplifies the markers on this package (if any exist) using the + /// given Python requirement as assumed context. + /// + /// See `RequiresPython::simplify_markers` for more details. + /// + /// NOTE: This routine is kind of weird, because this should only really be + /// applied in contexts where the `PubGrubPackage` is printed as output. + /// So in theory, this should be a transformation into a new type with a + /// "printable" `PubGrubPackage` coupled with a `Requires-Python`. But at + /// time of writing, this was a larger refactor, particularly in the error + /// reporting where this routine is used. + pub(crate) fn simplify_markers(&mut self, python_requirement: &PythonRequirement) { + match *Arc::make_mut(&mut self.0) { + PubGrubPackageInner::Root(_) | PubGrubPackageInner::Python(_) => {} + PubGrubPackageInner::Package { ref mut marker, .. } + | PubGrubPackageInner::Extra { ref mut marker, .. } + | PubGrubPackageInner::Dev { ref mut marker, .. } => { + let Some(contents) = marker.as_mut() else { + return; + }; + let tree = MarkerTree::from(contents.clone()); + *marker = python_requirement.simplify_markers(tree).contents(); + } + PubGrubPackageInner::Marker { ref mut marker, .. } => { + *marker = python_requirement.simplify_markers(marker.clone()); + } + } + } + + #[allow(dead_code)] + pub(crate) fn kind(&self) -> &'static str { + match &**self { + PubGrubPackageInner::Root(_) => "root", + PubGrubPackageInner::Python(_) => "python", + PubGrubPackageInner::Package { .. } => "package", + PubGrubPackageInner::Extra { .. } => "extra", + PubGrubPackageInner::Dev { .. } => "dev", + PubGrubPackageInner::Marker { .. } => "marker", + } + } } #[derive(Debug, Copy, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)] @@ -234,7 +279,13 @@ impl std::fmt::Display for PubGrubPackageInner { } => { write!(f, "{name}[{dev}]{{{marker}}}") } - Self::Marker { name, marker, .. } => write!(f, "{name}{{{marker}}}"), + Self::Marker { name, marker, .. } => { + if let Some(marker) = marker.contents() { + write!(f, "{name}{{{marker}}}") + } else { + write!(f, "{name}") + } + } Self::Extra { name, extra, .. } => write!(f, "{name}[{extra}]"), Self::Dev { name, dev, .. } => write!(f, "{name}:{dev}"), // It is guaranteed that `extra` and `dev` are never set at the same time. diff --git a/crates/uv-resolver/src/python_requirement.rs b/crates/uv-resolver/src/python_requirement.rs index 1c286a212a0e..117442ce0dcd 100644 --- a/crates/uv-resolver/src/python_requirement.rs +++ b/crates/uv-resolver/src/python_requirement.rs @@ -1,4 +1,5 @@ use pep440_rs::Version; +use pep508_rs::MarkerTree; use uv_python::{Interpreter, PythonVersion}; use crate::{RequiresPython, RequiresPythonRange}; @@ -89,6 +90,22 @@ impl PythonRequirement { pub fn source(&self) -> PythonRequirementSource { self.source } + + /// A wrapper around `RequiresPython::simplify_markers`. See its docs for + /// more info. + /// + /// When this `PythonRequirement` isn't `RequiresPython`, the given markers + /// are returned unchanged. + pub(crate) fn simplify_markers(&self, marker: MarkerTree) -> MarkerTree { + self.target.simplify_markers(marker) + } + + /// Return a [`MarkerTree`] representing the Python requirement. + /// + /// See: [`RequiresPython::to_marker_tree`] + pub fn to_marker_tree(&self) -> MarkerTree { + self.target.to_marker_tree() + } } #[derive(Debug, Copy, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)] diff --git a/crates/uv-resolver/src/requires_python.rs b/crates/uv-resolver/src/requires_python.rs index 71cc480fbb67..9d70acd68acc 100644 --- a/crates/uv-resolver/src/requires_python.rs +++ b/crates/uv-resolver/src/requires_python.rs @@ -125,8 +125,19 @@ impl RequiresPython { } } - /// Returns the [`RequiresPython`] as a [`MarkerTree`]. - pub fn markers(&self) -> MarkerTree { + /// Returns this `Requires-Python` specifier as an equivalent + /// [`MarkerTree`] utilizing the `python_full_version` marker field. + /// + /// This is useful for comparing a `Requires-Python` specifier with + /// arbitrary marker expressions. For example, one can ask whether the + /// returned marker expression is disjoint with another marker expression. + /// If it is, then one can conclude that the `Requires-Python` specifier + /// excludes the dependency with that other marker expression. + /// + /// If this `Requires-Python` specifier has no constraints, then this + /// returns a marker tree that evaluates to `true` for all possible marker + /// environments. + pub fn to_marker_tree(&self) -> MarkerTree { match (self.range.0.as_ref(), self.range.1.as_ref()) { (Bound::Included(lower), Bound::Included(upper)) => { let mut lower = MarkerTree::expression(MarkerExpression::Version { @@ -283,6 +294,69 @@ impl RequiresPython { &self.range } + /// Simplifies the given markers in such a way as to assume that + /// the Python version is constrained by this Python version bound. + /// + /// For example, with `requires-python = '>=3.8'`, a marker like this: + /// + /// ```text + /// python_full_version >= '3.8' and python_full_version < '3.12' + /// ``` + /// + /// Will be simplified to: + /// + /// ```text + /// python_full_version < '3.12' + /// ``` + /// + /// That is, `python_full_version >= '3.8'` is assumed to be true by virtue + /// of `requires-python`, and is thus not needed in the marker. + /// + /// This should be used in contexts in which this assumption is valid to + /// make. Generally, this means it should not be used inside the resolver, + /// but instead near the boundaries of the system (like formatting error + /// messages and writing the lock file). The reason for this is that + /// this simplification fundamentally changes the meaning of the marker, + /// and the *only* correct way to interpret it is in a context in which + /// `requires-python` is known to be true. For example, when markers from + /// a lock file are deserialized and turned into a `ResolutionGraph`, the + /// markers are "complexified" to put the `requires-python` assumption back + /// into the marker explicitly. + pub(crate) fn simplify_markers(&self, marker: MarkerTree) -> MarkerTree { + let simplified = marker.simplify_python_versions(Range::from(self.range().clone())); + // FIXME: This is a hack to avoid the hidden state created by + // ADD's `restrict_versions`. I believe this is sound, but it's + // wasteful and silly. + simplified + .try_to_string() + .map(|s| s.parse().unwrap()) + .unwrap_or(MarkerTree::TRUE) + } + + /// The inverse of `simplify_markers`. + /// + /// This should be applied near the boundaries of uv when markers are + /// deserialized from a context where `requires-python` is assumed. For + /// example, with `requires-python = '>=3.8'` and a marker like: + /// + /// ```text + /// python_full_version < '3.12' + /// ``` + /// + /// It will be "complexified" to: + /// + /// ```text + /// python_full_version >= '3.8' and python_full_version < '3.12' + /// ``` + pub(crate) fn complexify_markers(&self, mut marker: MarkerTree) -> MarkerTree { + // PERF: There's likely a way to amortize this, particularly + // the construction of `to_marker_tree`. But at time of + // writing, it wasn't clear if this was an actual perf problem + // or not. If it is, try a `std::sync::OnceLock`. + marker.and(self.to_marker_tree()); + marker + } + /// Returns `false` if the wheel's tags state it can't be used in the given Python version /// range. /// @@ -477,6 +551,46 @@ impl Ord for RequiresPythonBound { } } +/// A simplified marker is just like a normal marker, except it has possibly +/// been simplified by `requires-python`. +/// +/// A simplified marker should only exist in contexts where a `requires-python` +/// setting can be assumed. In order to get a "normal" marker out of +/// a simplified marker, one must re-contextualize it by adding the +/// `requires-python` constraint back to the marker. +#[derive(Clone, Debug, Default, Eq, PartialEq, PartialOrd, Ord, serde::Deserialize)] +pub(crate) struct SimplifiedMarkerTree(MarkerTree); + +impl SimplifiedMarkerTree { + /// Simplifies the given markers by assuming the given `requires-python` + /// bound is true. + pub(crate) fn new( + requires_python: &RequiresPython, + marker: MarkerTree, + ) -> SimplifiedMarkerTree { + SimplifiedMarkerTree(requires_python.simplify_markers(marker)) + } + + /// Complexifies the given markers by adding the given `requires-python` as + /// a constraint to these simplified markers. + pub(crate) fn into_marker(self, requires_python: &RequiresPython) -> MarkerTree { + requires_python.complexify_markers(self.0) + } + + /// Attempts to convert this simplified marker to a string. + /// + /// This only returns `None` when the underlying marker is always true, + /// i.e., it matches all possible marker environments. + pub(crate) fn try_to_string(&self) -> Option { + self.0.try_to_string() + } + + /// Returns the underlying marker tree without re-complexifying them. + pub(crate) fn as_simplified_marker_tree(&self) -> &MarkerTree { + &self.0 + } +} + #[cfg(test)] mod tests { use std::cmp::Ordering; diff --git a/crates/uv-resolver/src/resolution/display.rs b/crates/uv-resolver/src/resolution/display.rs index 624d76aeea58..705d1239f228 100644 --- a/crates/uv-resolver/src/resolution/display.rs +++ b/crates/uv-resolver/src/resolution/display.rs @@ -187,7 +187,11 @@ impl std::fmt::Display for DisplayResolutionGraph<'_> { for (index, node) in nodes { // Display the node itself. let mut line = node - .to_requirements_txt(self.include_extras, self.include_markers) + .to_requirements_txt( + &self.resolution.requires_python, + self.include_extras, + self.include_markers, + ) .to_string(); // Display the distribution hashes, if any. diff --git a/crates/uv-resolver/src/resolution/requirements_txt.rs b/crates/uv-resolver/src/resolution/requirements_txt.rs index 2fa758f034b0..73ef1d7f0cbf 100644 --- a/crates/uv-resolver/src/resolution/requirements_txt.rs +++ b/crates/uv-resolver/src/resolution/requirements_txt.rs @@ -10,7 +10,10 @@ use pep508_rs::{split_scheme, MarkerTree, Scheme}; use pypi_types::HashDigest; use uv_normalize::{ExtraName, PackageName}; -use crate::resolution::AnnotatedDist; +use crate::{ + requires_python::{RequiresPython, SimplifiedMarkerTree}, + resolution::AnnotatedDist, +}; #[derive(Debug, Clone)] /// A pinned package with its resolved distribution and all the extras that were pinned for it. @@ -31,6 +34,7 @@ impl RequirementsTxtDist { /// supported in `requirements.txt`). pub(crate) fn to_requirements_txt( &self, + requires_python: &RequiresPython, include_extras: bool, include_markers: bool, ) -> Cow { @@ -90,7 +94,9 @@ impl RequirementsTxtDist { }; if let Some(given) = given { return if let Some(markers) = - self.markers.contents().filter(|_| include_markers) + SimplifiedMarkerTree::new(requires_python, self.markers.clone()) + .try_to_string() + .filter(|_| include_markers) { Cow::Owned(format!("{given} ; {markers}")) } else { @@ -101,7 +107,10 @@ impl RequirementsTxtDist { } if self.extras.is_empty() || !include_extras { - if let Some(markers) = self.markers.contents().filter(|_| include_markers) { + if let Some(markers) = SimplifiedMarkerTree::new(requires_python, self.markers.clone()) + .try_to_string() + .filter(|_| include_markers) + { Cow::Owned(format!("{} ; {}", self.dist.verbatim(), markers)) } else { self.dist.verbatim() @@ -110,7 +119,10 @@ impl RequirementsTxtDist { let mut extras = self.extras.clone(); extras.sort_unstable(); extras.dedup(); - if let Some(markers) = self.markers.contents().filter(|_| include_markers) { + if let Some(markers) = SimplifiedMarkerTree::new(requires_python, self.markers.clone()) + .try_to_string() + .filter(|_| include_markers) + { Cow::Owned(format!( "{}[{}]{} ; {}", self.name(), diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index b9bd978197bd..48e02613b806 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -562,6 +562,8 @@ impl ResolverState ResolverState ResolverState ResolverState ResolverState>, python_requirement: &PythonRequirement, ) -> Forks { + let python_marker = python_requirement.to_marker_tree(); + let mut forks = vec![Fork { dependencies: vec![], markers: MarkerTree::TRUE, @@ -2713,7 +2705,6 @@ impl Forks { continue; } for dep in deps { - // We assume that the marker has already been Python-simplified. let mut markers = dep.package.marker().cloned().unwrap_or(MarkerTree::TRUE); if markers.is_false() { // If the markers can never be satisfied, then we @@ -2740,17 +2731,25 @@ impl Forks { continue; } - let not_markers = simplify_python(markers.negate(), python_requirement); + let not_markers = markers.negate(); let mut new_markers = markers.clone(); - new_markers.and(simplify_python(fork.markers.negate(), python_requirement)); + new_markers.and(fork.markers.negate()); if !fork.markers.is_disjoint(¬_markers) { let mut new_fork = fork.clone(); new_fork.intersect(not_markers); - new.push(new_fork); + // Filter out any forks we created that are disjoint with our + // Python requirement. + if !new_fork.markers.is_disjoint(&python_marker) { + new.push(new_fork); + } } fork.dependencies.push(dep.clone()); fork.intersect(markers); - new.push(fork); + // Filter out any forks we created that are disjoint with our + // Python requirement. + if !fork.markers.is_disjoint(&python_marker) { + new.push(fork); + } markers = new_markers; } forks = new; @@ -2848,8 +2847,3 @@ impl PartialOrd for Fork { Some(self.cmp(other)) } } - -/// Simplify a [`MarkerTree`] based on a [`PythonRequirement`]. -fn simplify_python(marker: MarkerTree, python_requirement: &PythonRequirement) -> MarkerTree { - marker.simplify_python_versions(Range::from(python_requirement.target().range().clone())) -} diff --git a/crates/uv/src/commands/project/lock.rs b/crates/uv/src/commands/project/lock.rs index caf156501514..7bec6c646dab 100644 --- a/crates/uv/src/commands/project/lock.rs +++ b/crates/uv/src/commands/project/lock.rs @@ -331,7 +331,7 @@ async fn do_lock( .into_iter() .flatten() { - if requires_python.markers().is_disjoint(environment) { + if requires_python.to_marker_tree().is_disjoint(environment) { return if let Some(contents) = environment.contents() { Err(ProjectError::DisjointEnvironment( contents, @@ -637,7 +637,7 @@ impl ValidatedLock { } // If the set of supported environments has changed, we have to perform a clean resolution. - if lock.supported_environments() + if lock.simplified_supported_environments() != environments .map(SupportedEnvironments::as_markers) .unwrap_or_default() diff --git a/crates/uv/src/commands/project/sync.rs b/crates/uv/src/commands/project/sync.rs index 9d02808dd00d..eacc608ff8b8 100644 --- a/crates/uv/src/commands/project/sync.rs +++ b/crates/uv/src/commands/project/sync.rs @@ -192,7 +192,12 @@ pub(super) async fn do_sync( if !environments.is_empty() { if !environments.iter().any(|env| env.evaluate(&markers, &[])) { return Err(ProjectError::LockedPlatformIncompatibility( - environments + // For error reporting, we use the "simplified" + // supported environments, because these correspond to + // what the end user actually wrote. The non-simplified + // environments, by contrast, are explicitly + // constrained by `requires-python`. + lock.simplified_supported_environments() .iter() .filter_map(MarkerTree::contents) .map(|env| format!("`{env}`"))