Skip to content

Commit

Permalink
Handle universal vs. fork markers with ResolverMarkers (#5099)
Browse files Browse the repository at this point in the history
* Use a dedicated `ResolverMarkers` check in the fork state. This is
better than the `MarkerTree::And(Vec::new())` check.
* Report the timing correct naming universal resolution instead of two
spaces around an empty string when there are no markers.
* On resolution error, show the split that we're in. I'm not sure how to
word this, since we're doing a universal resolution until we fork, so
the trace may contain information from requirements that are not part of
this fork.
  • Loading branch information
konstin authored Jul 17, 2024
1 parent fe40357 commit a6dfd39
Show file tree
Hide file tree
Showing 19 changed files with 220 additions and 133 deletions.
4 changes: 2 additions & 2 deletions crates/bench/benches/uv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ mod resolver {
use uv_python::PythonEnvironment;
use uv_resolver::{
FlatIndex, InMemoryIndex, Manifest, OptionsBuilder, PythonRequirement, ResolutionGraph,
Resolver,
Resolver, ResolverMarkers,
};
use uv_types::{BuildIsolation, EmptyInstalledPackages, HashStrategy, InFlight};

Expand Down Expand Up @@ -175,7 +175,7 @@ mod resolver {
manifest,
options,
&python_requirement,
Some(&MARKERS),
ResolverMarkers::SpecificEnvironment(MARKERS.clone()),
Some(&TAGS),
&flat_index,
&index,
Expand Down
3 changes: 2 additions & 1 deletion crates/uv-dispatch/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use uv_installer::{Installer, Plan, Planner, Preparer, SitePackages};
use uv_python::{Interpreter, PythonEnvironment};
use uv_resolver::{
ExcludeNewer, FlatIndex, InMemoryIndex, Manifest, OptionsBuilder, PythonRequirement, Resolver,
ResolverMarkers,
};
use uv_types::{BuildContext, BuildIsolation, EmptyInstalledPackages, HashStrategy, InFlight};

Expand Down Expand Up @@ -146,7 +147,7 @@ impl<'a> BuildContext for BuildDispatch<'a> {
.index_strategy(self.index_strategy)
.build(),
&python_requirement,
Some(markers),
ResolverMarkers::SpecificEnvironment(markers.clone()),
Some(tags),
self.flat_index,
self.index,
Expand Down
11 changes: 6 additions & 5 deletions crates/uv-requirements/src/lookahead.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,12 @@ use thiserror::Error;
use tracing::trace;

use distribution_types::{BuiltDist, Dist, DistributionMetadata, GitSourceDist, SourceDist};
use pep508_rs::MarkerEnvironment;
use pypi_types::{Requirement, RequirementSource};
use uv_configuration::{Constraints, Overrides};
use uv_distribution::{DistributionDatabase, Reporter};
use uv_git::GitUrl;
use uv_normalize::GroupName;
use uv_resolver::{InMemoryIndex, MetadataResponse};
use uv_resolver::{InMemoryIndex, MetadataResponse, ResolverMarkers};
use uv_types::{BuildContext, HashStrategy, RequestedRequirements};

#[derive(Debug, Error)]
Expand Down Expand Up @@ -98,7 +97,7 @@ impl<'a, Context: BuildContext> LookaheadResolver<'a, Context> {
/// to "only evaluate marker expressions that reference an extra name.")
pub async fn resolve(
self,
markers: Option<&MarkerEnvironment>,
markers: &ResolverMarkers,
) -> Result<Vec<RequestedRequirements>, LookaheadError> {
let mut results = Vec::new();
let mut futures = FuturesUnordered::new();
Expand All @@ -108,7 +107,7 @@ impl<'a, Context: BuildContext> LookaheadResolver<'a, Context> {
let mut queue: VecDeque<_> = self
.constraints
.apply(self.overrides.apply(self.requirements))
.filter(|requirement| requirement.evaluate_markers(markers, &[]))
.filter(|requirement| requirement.evaluate_markers(markers.marker_environment(), &[]))
.map(|requirement| (*requirement).clone())
.collect();

Expand All @@ -127,7 +126,9 @@ impl<'a, Context: BuildContext> LookaheadResolver<'a, Context> {
.constraints
.apply(self.overrides.apply(lookahead.requirements()))
{
if requirement.evaluate_markers(markers, lookahead.extras()) {
if requirement
.evaluate_markers(markers.marker_environment(), lookahead.extras())
{
queue.push_back((*requirement).clone());
}
}
Expand Down
20 changes: 17 additions & 3 deletions crates/uv-resolver/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use crate::pubgrub::{
PubGrubPackage, PubGrubPackageInner, PubGrubReportFormatter, PubGrubSpecifierError,
};
use crate::python_requirement::PythonRequirement;
use crate::resolver::{IncompletePackage, UnavailablePackage, UnavailableReason};
use crate::resolver::{IncompletePackage, ResolverMarkers, UnavailablePackage, UnavailableReason};

#[derive(Debug, thiserror::Error)]
pub enum ResolveError {
Expand Down Expand Up @@ -50,10 +50,10 @@ pub enum ResolveError {
ConflictingOverrideUrls(PackageName, String, String),

#[error("Requirements contain conflicting URLs for package `{0}`:\n- {}", _1.join("\n- "))]
ConflictingUrls(PackageName, Vec<String>),
ConflictingUrlsUniversal(PackageName, Vec<String>),

#[error("Requirements contain conflicting URLs for package `{package_name}` in split `{fork_markers}`:\n- {}", urls.join("\n- "))]
ConflictingUrlsInFork {
ConflictingUrlsFork {
package_name: PackageName,
urls: Vec<String>,
fork_markers: MarkerTree,
Expand Down Expand Up @@ -125,9 +125,21 @@ pub struct NoSolutionError {
unavailable_packages: FxHashMap<PackageName, UnavailablePackage>,
incomplete_packages: FxHashMap<PackageName, BTreeMap<Version, IncompletePackage>>,
fork_urls: ForkUrls,
markers: ResolverMarkers,
}

impl NoSolutionError {
pub fn header(&self) -> String {
match &self.markers {
ResolverMarkers::Universal | ResolverMarkers::SpecificEnvironment(_) => {
"No solution found when resolving dependencies:".to_string()
}
ResolverMarkers::Fork(markers) => {
format!("No solution found when resolving dependencies for split ({markers}):")
}
}
}

pub(crate) fn new(
error: pubgrub::error::NoSolutionError<UvDependencyProvider>,
available_versions: FxHashMap<PubGrubPackage, BTreeSet<Version>>,
Expand All @@ -137,6 +149,7 @@ impl NoSolutionError {
unavailable_packages: FxHashMap<PackageName, UnavailablePackage>,
incomplete_packages: FxHashMap<PackageName, BTreeMap<Version, IncompletePackage>>,
fork_urls: ForkUrls,
markers: ResolverMarkers,
) -> Self {
Self {
error,
Expand All @@ -147,6 +160,7 @@ impl NoSolutionError {
unavailable_packages,
incomplete_packages,
fork_urls,
markers,
}
}

Expand Down
29 changes: 16 additions & 13 deletions crates/uv-resolver/src/fork_urls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ use std::collections::hash_map::Entry;
use rustc_hash::FxHashMap;

use distribution_types::Verbatim;
use pep508_rs::MarkerTree;
use pypi_types::VerbatimParsedUrl;
use uv_normalize::PackageName;

use crate::resolver::ResolverMarkers;
use crate::ResolveError;

/// See [`crate::resolver::SolveState`].
Expand All @@ -29,7 +29,7 @@ impl ForkUrls {
&mut self,
package_name: &PackageName,
url: &VerbatimParsedUrl,
fork_markers: &MarkerTree,
fork_markers: &ResolverMarkers,
) -> Result<(), ResolveError> {
match self.0.entry(package_name.clone()) {
Entry::Occupied(previous) => {
Expand All @@ -39,17 +39,20 @@ impl ForkUrls {
url.verbatim.verbatim().to_string(),
];
conflicting_url.sort();
return if fork_markers.is_universal() {
Err(ResolveError::ConflictingUrls(
package_name.clone(),
conflicting_url,
))
} else {
Err(ResolveError::ConflictingUrlsInFork {
package_name: package_name.clone(),
urls: conflicting_url,
fork_markers: fork_markers.clone(),
})
return match fork_markers {
ResolverMarkers::Universal | ResolverMarkers::SpecificEnvironment(_) => {
Err(ResolveError::ConflictingUrlsUniversal(
package_name.clone(),
conflicting_url,
))
}
ResolverMarkers::Fork(fork_markers) => {
Err(ResolveError::ConflictingUrlsFork {
package_name: package_name.clone(),
urls: conflicting_url,
fork_markers: fork_markers.clone(),
})
}
};
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-resolver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub use resolution::{AnnotationStyle, DisplayResolutionGraph, ResolutionGraph};
pub use resolution_mode::ResolutionMode;
pub use resolver::{
BuildId, DefaultResolverProvider, InMemoryIndex, MetadataResponse, PackageVersionsResult,
Reporter as ResolverReporter, Resolver, ResolverProvider, VersionsResponse,
Reporter as ResolverReporter, Resolver, ResolverMarkers, ResolverProvider, VersionsResponse,
WheelMetadataResult,
};
pub use version_map::VersionMap;
Expand Down
28 changes: 15 additions & 13 deletions crates/uv-resolver/src/resolution/display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ use petgraph::Direction;
use rustc_hash::{FxBuildHasher, FxHashMap};

use distribution_types::{DistributionMetadata, Name, SourceAnnotation, SourceAnnotations};
use pep508_rs::MarkerEnvironment;
use pep508_rs::MarkerTree;
use uv_normalize::PackageName;

use crate::resolution::{RequirementsTxtDist, ResolutionGraphNode};
use crate::{marker, ResolutionGraph};
use crate::{marker, ResolutionGraph, ResolverMarkers};

static UNIVERSAL_MARKERS: ResolverMarkers = ResolverMarkers::Universal;

/// A [`std::fmt::Display`] implementation for the resolution graph.
#[derive(Debug)]
Expand All @@ -21,7 +22,7 @@ pub struct DisplayResolutionGraph<'a> {
/// The underlying graph.
resolution: &'a ResolutionGraph,
/// The marker environment, used to determine the markers that apply to each package.
marker_env: Option<&'a MarkerEnvironment>,
marker_env: &'a ResolverMarkers,
/// The packages to exclude from the output.
no_emit_packages: &'a [PackageName],
/// Whether to include hashes in the output.
Expand Down Expand Up @@ -50,7 +51,7 @@ impl<'a> From<&'a ResolutionGraph> for DisplayResolutionGraph<'a> {
fn from(resolution: &'a ResolutionGraph) -> Self {
Self::new(
resolution,
None,
&UNIVERSAL_MARKERS,
&[],
false,
false,
Expand All @@ -67,7 +68,7 @@ impl<'a> DisplayResolutionGraph<'a> {
#[allow(clippy::fn_params_excessive_bools)]
pub fn new(
underlying: &'a ResolutionGraph,
marker_env: Option<&'a MarkerEnvironment>,
marker_env: &'a ResolverMarkers,
no_emit_packages: &'a [PackageName],
show_hashes: bool,
include_extras: bool,
Expand Down Expand Up @@ -97,12 +98,9 @@ impl std::fmt::Display for DisplayResolutionGraph<'_> {
let sources = if self.include_annotations {
let mut sources = SourceAnnotations::default();

for requirement in self
.resolution
.requirements
.iter()
.filter(|requirement| requirement.evaluate_markers(self.marker_env, &[]))
{
for requirement in self.resolution.requirements.iter().filter(|requirement| {
requirement.evaluate_markers(self.marker_env.marker_environment(), &[])
}) {
if let Some(origin) = &requirement.origin {
sources.add(
&requirement.name,
Expand All @@ -115,7 +113,9 @@ impl std::fmt::Display for DisplayResolutionGraph<'_> {
.resolution
.constraints
.requirements()
.filter(|requirement| requirement.evaluate_markers(self.marker_env, &[]))
.filter(|requirement| {
requirement.evaluate_markers(self.marker_env.marker_environment(), &[])
})
{
if let Some(origin) = &requirement.origin {
sources.add(
Expand All @@ -129,7 +129,9 @@ impl std::fmt::Display for DisplayResolutionGraph<'_> {
.resolution
.overrides
.requirements()
.filter(|requirement| requirement.evaluate_markers(self.marker_env, &[]))
.filter(|requirement| {
requirement.evaluate_markers(self.marker_env.marker_environment(), &[])
})
{
if let Some(origin) = &requirement.origin {
sources.add(
Expand Down
Loading

0 comments on commit a6dfd39

Please sign in to comment.