diff --git a/crates/uv-resolver/src/error.rs b/crates/uv-resolver/src/error.rs index 8411d98dbbc6c..c490fbae51d25 100644 --- a/crates/uv-resolver/src/error.rs +++ b/crates/uv-resolver/src/error.rs @@ -19,6 +19,7 @@ use crate::pubgrub::{ PubGrubPackage, PubGrubPackageInner, PubGrubReportFormatter, PubGrubSpecifierError, }; use crate::python_requirement::PythonRequirement; +use crate::resolution::ConflictingDistributionError; use crate::resolver::{IncompletePackage, ResolverMarkers, UnavailablePackage, UnavailableReason}; #[derive(Debug, thiserror::Error)] @@ -102,6 +103,9 @@ pub enum ResolveError { #[error("In `--require-hashes` mode, all requirements must be pinned upfront with `==`, but found: `{0}`")] UnhashedPackage(PackageName), + #[error("found conflicting distribution in resolution: {0}")] + ConflictingDistribution(ConflictingDistributionError), + /// Something unexpected happened. #[error("{0}")] Failure(String), diff --git a/crates/uv-resolver/src/lib.rs b/crates/uv-resolver/src/lib.rs index 950eb8a0ca432..82345e6bce1ca 100644 --- a/crates/uv-resolver/src/lib.rs +++ b/crates/uv-resolver/src/lib.rs @@ -13,7 +13,9 @@ pub use prerelease::PrereleaseMode; pub use pubgrub::{PubGrubSpecifier, PubGrubSpecifierError}; pub use python_requirement::PythonRequirement; pub use requires_python::{RequiresPython, RequiresPythonError, RequiresPythonRange}; -pub use resolution::{AnnotationStyle, DisplayResolutionGraph, ResolutionGraph}; +pub use resolution::{ + AnnotationStyle, ConflictingDistributionError, DisplayResolutionGraph, ResolutionGraph, +}; pub use resolution_mode::ResolutionMode; pub use resolver::{ BuildId, DefaultResolverProvider, InMemoryIndex, MetadataResponse, PackageVersionsResult, diff --git a/crates/uv-resolver/src/lock/mod.rs b/crates/uv-resolver/src/lock/mod.rs index 28fdc01488a0c..06a0f03d36af9 100644 --- a/crates/uv-resolver/src/lock/mod.rs +++ b/crates/uv-resolver/src/lock/mod.rs @@ -428,7 +428,7 @@ impl Lock { } } } - Ok(Self { + let lock = Self { version, fork_markers, supported_environments, @@ -437,7 +437,8 @@ impl Lock { packages, by_id, manifest, - }) + }; + Ok(lock) } /// Record the requirements that were used to generate this lock. @@ -1465,38 +1466,6 @@ impl TryFrom for Lock { 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) { - assert!( - false, - "[{marker1:?}] (for version {version1}) is not disjoint with \ - [{marker2:?}] (for version {version2}) \ - for package `{name}`", - ); - } - } - } - } - */ - Ok(lock) } } diff --git a/crates/uv-resolver/src/resolution/graph.rs b/crates/uv-resolver/src/resolution/graph.rs index f6ff3312b0966..c54f0f030ad92 100644 --- a/crates/uv-resolver/src/resolution/graph.rs +++ b/crates/uv-resolver/src/resolution/graph.rs @@ -1,3 +1,6 @@ +use std::collections::BTreeMap; +use std::fmt::{Display, Formatter}; + use distribution_types::{ Dist, DistributionMetadata, Name, ResolutionDiagnostic, ResolvedDist, VersionId, VersionOrUrlRef, @@ -11,7 +14,6 @@ use petgraph::{ }; use pypi_types::{HashDigest, ParsedUrlError, Requirement, VerbatimParsedUrl, Yanked}; use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet}; -use std::fmt::{Display, Formatter}; use uv_configuration::{Constraints, Overrides}; use uv_distribution::Metadata; use uv_git::GitResolver; @@ -233,7 +235,7 @@ impl ResolutionGraph { report_missing_lower_bounds(&petgraph, &mut diagnostics); } - Ok(Self { + let graph = Self { petgraph, requires_python, package_markers, @@ -243,7 +245,29 @@ impl ResolutionGraph { overrides: overrides.clone(), options, fork_markers, - }) + }; + let mut conflicting = graph.find_conflicting_distributions(); + if !conflicting.is_empty() { + tracing::warn!( + "found {} conflicting distributions in resolution, \ + please report this as a bug at \ + https://github.com/astral-sh/uv/issues/new", + conflicting.len() + ); + } + // When testing, we materialize any conflicting distributions as an + // error to ensure any relevant tests fail. Otherwise, we just leave + // it at the warning message above. The reason for not returning an + // error "in production" is that an incorrect resolution may only be + // incorrect in certain marker environments, but fine in most others. + // Returning an error in that case would make `uv` unusable whenever + // the bug occurs, but letting it through means `uv` *could* still be + // usable. + #[cfg(debug_assertions)] + if let Some(err) = conflicting.pop() { + return Err(ResolveError::ConflictingDistribution(err)); + } + Ok(graph) } fn add_edge( @@ -681,6 +705,84 @@ impl ResolutionGraph { Some(&package_markers[&(version.clone(), url.cloned())]) } } + + /// Returns a sequence of conflicting distribution errors from this + /// resolution. + /// + /// Correct resolutions always return an empty sequence. A non-empty + /// sequence implies there is a package with two distinct versions in the + /// same marker environment in this resolution. This in turn implies that + /// 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)>> = + BTreeMap::new(); + for node in self.petgraph.node_weights() { + let annotated_dist = match node { + ResolutionGraphNode::Root => continue, + ResolutionGraphNode::Dist(ref annotated_dist) => annotated_dist, + }; + name_to_markers + .entry(&annotated_dist.name) + .or_default() + .push((&annotated_dist.version, &annotated_dist.marker)); + } + let mut dupes = vec![]; + 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) { + dupes.push(ConflictingDistributionError { + name: name.clone(), + version1: (*version1).clone(), + version2: (*version2).clone(), + marker1: (*marker1).clone(), + marker2: (*marker2).clone(), + }); + } + } + } + } + dupes + } +} + +/// An error that occurs for conflicting versions of the same package. +/// +/// Specifically, this occurs when two distributions with the same package +/// name are found with distinct versions in at least one possible marker +/// environment. This error reflects an error that could occur when installing +/// the corresponding resolution into that marker environment. +#[derive(Debug)] +pub struct ConflictingDistributionError { + name: PackageName, + version1: Version, + version2: Version, + marker1: MarkerTree, + marker2: MarkerTree, +} + +impl std::error::Error for ConflictingDistributionError {} + +impl std::fmt::Display for ConflictingDistributionError { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + let ConflictingDistributionError { + ref name, + ref version1, + ref version2, + ref marker1, + ref marker2, + } = *self; + write!( + f, + "found conflicting versions for package `{name}`: + `{marker1:?}` (for version `{version1}`) is not disjoint with \ + `{marker2:?}` (for version `{version2}`)", + ) + } } impl From for distribution_types::Resolution { diff --git a/crates/uv-resolver/src/resolution/mod.rs b/crates/uv-resolver/src/resolution/mod.rs index e64cf68f35ff4..df9e33f8e5e38 100644 --- a/crates/uv-resolver/src/resolution/mod.rs +++ b/crates/uv-resolver/src/resolution/mod.rs @@ -11,8 +11,8 @@ use uv_distribution::Metadata; use uv_normalize::{ExtraName, GroupName, PackageName}; pub use crate::resolution::display::{AnnotationStyle, DisplayResolutionGraph}; -pub use crate::resolution::graph::ResolutionGraph; pub(crate) use crate::resolution::graph::ResolutionGraphNode; +pub use crate::resolution::graph::{ConflictingDistributionError, ResolutionGraph}; pub(crate) use crate::resolution::requirements_txt::RequirementsTxtDist; mod display; @@ -31,6 +31,11 @@ pub(crate) struct AnnotatedDist { pub(crate) dev: Option, pub(crate) hashes: Vec, pub(crate) metadata: Option, + /// The "full" marker for this distribution. It precisely describes all + /// marker environments for which this distribution _can_ be installed. + /// 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, }