Remove derivation chain special casing (#9678)

Instead of modifying the error to replace a dummy derivation chain from
construction with the real one, build the error with the real derivation
chain directly.
This commit is contained in:
konsti 2024-12-06 14:05:03 +01:00 committed by GitHub
parent a286e95f44
commit 400839c527
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 117 additions and 159 deletions

View File

@ -30,8 +30,8 @@ use uv_installer::{Installer, Plan, Planner, Preparer, SitePackages};
use uv_pypi_types::{Conflicts, Requirement}; use uv_pypi_types::{Conflicts, Requirement};
use uv_python::{Interpreter, PythonEnvironment}; use uv_python::{Interpreter, PythonEnvironment};
use uv_resolver::{ use uv_resolver::{
DerivationChainBuilder, ExcludeNewer, FlatIndex, Flexibility, InMemoryIndex, Manifest, ExcludeNewer, FlatIndex, Flexibility, InMemoryIndex, Manifest, OptionsBuilder,
OptionsBuilder, PythonRequirement, Resolver, ResolverEnvironment, PythonRequirement, Resolver, ResolverEnvironment,
}; };
use uv_types::{BuildContext, BuildIsolation, EmptyInstalledPackages, HashStrategy, InFlight}; use uv_types::{BuildContext, BuildIsolation, EmptyInstalledPackages, HashStrategy, InFlight};
@ -274,18 +274,8 @@ impl<'a> BuildContext for BuildDispatch<'a> {
); );
preparer preparer
.prepare(remote, &self.shared_state.in_flight) .prepare(remote, &self.shared_state.in_flight, resolution)
.await .await?
.map_err(|err| match err {
uv_installer::PrepareError::Dist(kind, dist, chain, err) => {
debug_assert!(chain.is_empty());
let chain =
DerivationChainBuilder::from_resolution(resolution, (&*dist).into())
.unwrap_or_default();
uv_installer::PrepareError::Dist(kind, dist, chain, err)
}
_ => err,
})?
}; };
// Remove any unnecessary packages. // Remove any unnecessary packages.

View File

@ -1,3 +1,8 @@
use crate::{DistRef, Edge, Name, Node, Resolution, ResolvedDist};
use petgraph::prelude::EdgeRef;
use petgraph::Direction;
use rustc_hash::FxHashSet;
use std::collections::VecDeque;
use std::fmt::{Display, Formatter}; use std::fmt::{Display, Formatter};
use uv_normalize::{ExtraName, GroupName, PackageName}; use uv_normalize::{ExtraName, GroupName, PackageName};
use uv_pep440::Version; use uv_pep440::Version;
@ -37,6 +42,70 @@ impl FromIterator<DerivationStep> for DerivationChain {
} }
impl DerivationChain { impl DerivationChain {
/// Compute a [`DerivationChain`] from a resolution graph.
///
/// This is used to construct a derivation chain upon install failure in the `uv pip` context,
/// where we don't have a lockfile describing the resolution.
pub fn from_resolution(
resolution: &Resolution,
target: DistRef<'_>,
) -> Option<DerivationChain> {
// Find the target distribution in the resolution graph.
let target = resolution.graph().node_indices().find(|node| {
let Node::Dist {
dist: ResolvedDist::Installable { dist, .. },
..
} = &resolution.graph()[*node]
else {
return false;
};
target == dist.as_ref()
})?;
// Perform a BFS to find the shortest path to the root.
let mut queue = VecDeque::new();
queue.push_back((target, None, None, Vec::new()));
// TODO(charlie): Consider respecting markers here.
let mut seen = FxHashSet::default();
while let Some((node, extra, group, mut path)) = queue.pop_front() {
if !seen.insert(node) {
continue;
}
match &resolution.graph()[node] {
Node::Root => {
path.reverse();
path.pop();
return Some(DerivationChain::from_iter(path));
}
Node::Dist { dist, .. } => {
for edge in resolution.graph().edges_directed(node, Direction::Incoming) {
let mut path = path.clone();
path.push(DerivationStep::new(
dist.name().clone(),
extra.clone(),
group.clone(),
dist.version().clone(),
Ranges::empty(),
));
let target = edge.source();
let extra = match edge.weight() {
Edge::Optional(extra, ..) => Some(extra.clone()),
_ => None,
};
let group = match edge.weight() {
Edge::Dev(group, ..) => Some(group.clone()),
_ => None,
};
queue.push_back((target, extra, group, path));
}
}
}
}
None
}
/// Returns the length of the derivation chain. /// Returns the length of the derivation chain.
pub fn len(&self) -> usize { pub fn len(&self) -> usize {
self.0.len() self.0.len()

View File

@ -10,7 +10,7 @@ use uv_configuration::BuildOptions;
use uv_distribution::{DistributionDatabase, LocalWheel}; use uv_distribution::{DistributionDatabase, LocalWheel};
use uv_distribution_types::{ use uv_distribution_types::{
BuildableSource, CachedDist, DerivationChain, Dist, DistErrorKind, Hashed, Identifier, Name, BuildableSource, CachedDist, DerivationChain, Dist, DistErrorKind, Hashed, Identifier, Name,
RemoteSource, RemoteSource, Resolution,
}; };
use uv_pep508::PackageName; use uv_pep508::PackageName;
use uv_platform_tags::Tags; use uv_platform_tags::Tags;
@ -65,11 +65,15 @@ impl<'a, Context: BuildContext> Preparer<'a, Context> {
&'stream self, &'stream self,
distributions: Vec<Dist>, distributions: Vec<Dist>,
in_flight: &'stream InFlight, in_flight: &'stream InFlight,
resolution: &'stream Resolution,
) -> impl Stream<Item = Result<CachedDist, Error>> + 'stream { ) -> impl Stream<Item = Result<CachedDist, Error>> + 'stream {
distributions distributions
.into_iter() .into_iter()
.map(|dist| async { .map(|dist| async {
let wheel = self.get_wheel(dist, in_flight).boxed_local().await?; let wheel = self
.get_wheel(dist, in_flight, resolution)
.boxed_local()
.await?;
if let Some(reporter) = self.reporter.as_ref() { if let Some(reporter) = self.reporter.as_ref() {
reporter.on_progress(&wheel); reporter.on_progress(&wheel);
} }
@ -84,13 +88,14 @@ impl<'a, Context: BuildContext> Preparer<'a, Context> {
&self, &self,
mut distributions: Vec<Dist>, mut distributions: Vec<Dist>,
in_flight: &InFlight, in_flight: &InFlight,
resolution: &Resolution,
) -> Result<Vec<CachedDist>, Error> { ) -> Result<Vec<CachedDist>, Error> {
// Sort the distributions by size. // Sort the distributions by size.
distributions distributions
.sort_unstable_by_key(|distribution| Reverse(distribution.size().unwrap_or(u64::MAX))); .sort_unstable_by_key(|distribution| Reverse(distribution.size().unwrap_or(u64::MAX)));
let wheels = self let wheels = self
.prepare_stream(distributions, in_flight) .prepare_stream(distributions, in_flight, resolution)
.try_collect() .try_collect()
.await?; .await?;
@ -102,7 +107,12 @@ impl<'a, Context: BuildContext> Preparer<'a, Context> {
} }
/// Download, build, and unzip a single wheel. /// Download, build, and unzip a single wheel.
#[instrument(skip_all, fields(name = % dist, size = ? dist.size(), url = dist.file().map(| file | file.url.to_string()).unwrap_or_default()))] #[instrument(skip_all, fields(name = % dist, size = ? dist.size(), url = dist.file().map(| file | file.url.to_string()).unwrap_or_default()))]
pub async fn get_wheel(&self, dist: Dist, in_flight: &InFlight) -> Result<CachedDist, Error> { pub async fn get_wheel(
&self,
dist: Dist,
in_flight: &InFlight,
resolution: &Resolution,
) -> Result<CachedDist, Error> {
// Validate that the distribution is compatible with the build options. // Validate that the distribution is compatible with the build options.
match dist { match dist {
Dist::Built(ref dist) => { Dist::Built(ref dist) => {
@ -129,7 +139,7 @@ impl<'a, Context: BuildContext> Preparer<'a, Context> {
.database .database
.get_or_build_wheel(&dist, self.tags, policy) .get_or_build_wheel(&dist, self.tags, policy)
.boxed_local() .boxed_local()
.map_err(|err| Error::from_dist(dist.clone(), err)) .map_err(|err| Error::from_dist(dist.clone(), err, resolution))
.await .await
.and_then(|wheel: LocalWheel| { .and_then(|wheel: LocalWheel| {
if wheel.satisfies(policy) { if wheel.satisfies(policy) {
@ -140,7 +150,7 @@ impl<'a, Context: BuildContext> Preparer<'a, Context> {
policy.digests(), policy.digests(),
wheel.hashes(), wheel.hashes(),
); );
Err(Error::from_dist(dist, err)) Err(Error::from_dist(dist, err, resolution))
} }
}) })
.map(CachedDist::from); .map(CachedDist::from);
@ -178,7 +188,7 @@ impl<'a, Context: BuildContext> Preparer<'a, Context> {
given: dist.name().clone(), given: dist.name().clone(),
metadata: cached.filename().name.clone(), metadata: cached.filename().name.clone(),
}; };
return Err(Error::from_dist(dist, err)); return Err(Error::from_dist(dist, err, resolution));
} }
if let Some(version) = dist.version() { if let Some(version) = dist.version() {
if *version != cached.filename().version { if *version != cached.filename().version {
@ -186,7 +196,7 @@ impl<'a, Context: BuildContext> Preparer<'a, Context> {
given: version.clone(), given: version.clone(),
metadata: cached.filename().version.clone(), metadata: cached.filename().version.clone(),
}; };
return Err(Error::from_dist(dist, err)); return Err(Error::from_dist(dist, err, resolution));
} }
} }
Ok(cached.clone()) Ok(cached.clone())
@ -216,7 +226,7 @@ pub enum Error {
impl Error { impl Error {
/// Create an [`Error`] from a distribution error. /// Create an [`Error`] from a distribution error.
fn from_dist(dist: Dist, cause: uv_distribution::Error) -> Self { fn from_dist(dist: Dist, cause: uv_distribution::Error, resolution: &Resolution) -> Self {
let kind = match &dist { let kind = match &dist {
Dist::Built(_) => DistErrorKind::Download, Dist::Built(_) => DistErrorKind::Download,
Dist::Source(dist) => { Dist::Source(dist) => {
@ -227,7 +237,9 @@ impl Error {
} }
} }
}; };
Self::Dist(kind, Box::new(dist), DerivationChain::default(), cause) let chain =
DerivationChain::from_resolution(resolution, (&dist).into()).unwrap_or_default();
Self::Dist(kind, Box::new(dist), chain, cause)
} }
} }

View File

@ -5,7 +5,7 @@ pub use crate::sources::*;
pub use crate::specification::*; pub use crate::specification::*;
pub use crate::unnamed::*; pub use crate::unnamed::*;
use uv_distribution_types::{DerivationChain, Dist, DistErrorKind, GitSourceDist, SourceDist}; use uv_distribution_types::{Dist, DistErrorKind, GitSourceDist, SourceDist};
use uv_git::GitUrl; use uv_git::GitUrl;
use uv_pypi_types::{Requirement, RequirementSource}; use uv_pypi_types::{Requirement, RequirementSource};
@ -20,12 +20,7 @@ pub mod upgrade;
#[derive(Debug, thiserror::Error)] #[derive(Debug, thiserror::Error)]
pub enum Error { pub enum Error {
#[error("{0} `{1}`")] #[error("{0} `{1}`")]
Dist( Dist(DistErrorKind, Box<Dist>, #[source] uv_distribution::Error),
DistErrorKind,
Box<Dist>,
DerivationChain,
#[source] uv_distribution::Error,
),
#[error(transparent)] #[error(transparent)]
Distribution(#[from] uv_distribution::Error), Distribution(#[from] uv_distribution::Error),
@ -41,24 +36,16 @@ impl Error {
/// Create an [`Error`] from a distribution error. /// Create an [`Error`] from a distribution error.
pub(crate) fn from_dist(dist: Dist, cause: uv_distribution::Error) -> Self { pub(crate) fn from_dist(dist: Dist, cause: uv_distribution::Error) -> Self {
match dist { match dist {
Dist::Built(dist) => Self::Dist( Dist::Built(dist) => {
DistErrorKind::Download, Self::Dist(DistErrorKind::Download, Box::new(Dist::Built(dist)), cause)
Box::new(Dist::Built(dist)), }
DerivationChain::default(),
cause,
),
Dist::Source(dist) => { Dist::Source(dist) => {
let kind = if dist.is_local() { let kind = if dist.is_local() {
DistErrorKind::Build DistErrorKind::Build
} else { } else {
DistErrorKind::DownloadAndBuild DistErrorKind::DownloadAndBuild
}; };
Self::Dist( Self::Dist(kind, Box::new(Dist::Source(dist)), cause)
kind,
Box::new(Dist::Source(dist)),
DerivationChain::default(),
cause,
)
} }
} }
} }

View File

@ -106,11 +106,7 @@ pub enum ResolveError {
), ),
#[error("Failed to read metadata from installed package `{0}`")] #[error("Failed to read metadata from installed package `{0}`")]
ReadInstalled( ReadInstalled(Box<InstalledDist>, #[source] InstalledDistError),
Box<InstalledDist>,
DerivationChain,
#[source] InstalledDistError,
),
#[error(transparent)] #[error(transparent)]
NoSolution(#[from] NoSolutionError), NoSolution(#[from] NoSolutionError),

View File

@ -1,88 +1,18 @@
use std::collections::VecDeque; use pubgrub::{Id, Kind, State};
use rustc_hash::FxHashMap;
use petgraph::visit::EdgeRef; use uv_distribution_types::{DerivationChain, DerivationStep};
use petgraph::Direction;
use pubgrub::{Id, Kind, Range, State};
use rustc_hash::{FxHashMap, FxHashSet};
use uv_distribution_types::{
DerivationChain, DerivationStep, DistRef, Edge, Name, Node, Resolution, ResolvedDist,
};
use uv_pep440::Version; use uv_pep440::Version;
use crate::dependency_provider::UvDependencyProvider; use crate::dependency_provider::UvDependencyProvider;
use crate::pubgrub::PubGrubPackage; use crate::pubgrub::PubGrubPackage;
/// A chain of derivation steps from the root package to the current package, to explain why a /// Build a [`DerivationChain`] from the pubgrub state, which is available in `uv-resolver`, but not
/// package is included in the resolution. /// in `uv-distribution-types`.
#[derive(Debug, Default, Clone, PartialEq, Eq, Hash)] #[derive(Debug, Default, Clone, PartialEq, Eq, Hash)]
pub struct DerivationChainBuilder; pub struct DerivationChainBuilder;
impl DerivationChainBuilder { impl DerivationChainBuilder {
/// Compute a [`DerivationChain`] from a resolution graph.
///
/// This is used to construct a derivation chain upon install failure in the `uv pip` context,
/// where we don't have a lockfile describing the resolution.
pub fn from_resolution(
resolution: &Resolution,
target: DistRef<'_>,
) -> Option<DerivationChain> {
// Find the target distribution in the resolution graph.
let target = resolution.graph().node_indices().find(|node| {
let Node::Dist {
dist: ResolvedDist::Installable { dist, .. },
..
} = &resolution.graph()[*node]
else {
return false;
};
target == dist.as_ref()
})?;
// Perform a BFS to find the shortest path to the root.
let mut queue = VecDeque::new();
queue.push_back((target, None, None, Vec::new()));
// TODO(charlie): Consider respecting markers here.
let mut seen = FxHashSet::default();
while let Some((node, extra, group, mut path)) = queue.pop_front() {
if !seen.insert(node) {
continue;
}
match &resolution.graph()[node] {
Node::Root => {
path.reverse();
path.pop();
return Some(DerivationChain::from_iter(path));
}
Node::Dist { dist, .. } => {
for edge in resolution.graph().edges_directed(node, Direction::Incoming) {
let mut path = path.clone();
path.push(DerivationStep::new(
dist.name().clone(),
extra.clone(),
group.clone(),
dist.version().clone(),
Range::empty(),
));
let target = edge.source();
let extra = match edge.weight() {
Edge::Optional(extra, ..) => Some(extra.clone()),
_ => None,
};
let group = match edge.weight() {
Edge::Dev(group, ..) => Some(group.clone()),
_ => None,
};
queue.push_back((target, extra, group, path));
}
}
}
}
None
}
/// Compute a [`DerivationChain`] from the current PubGrub state. /// Compute a [`DerivationChain`] from the current PubGrub state.
/// ///
/// This is used to construct a derivation chain upon resolution failure. /// This is used to construct a derivation chain upon resolution failure.

View File

@ -1914,13 +1914,9 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
Request::Installed(dist) => { Request::Installed(dist) => {
// TODO(charlie): This should be return a `MetadataResponse`. // TODO(charlie): This should be return a `MetadataResponse`.
let metadata = dist.metadata().map_err(|err| { let metadata = dist
ResolveError::ReadInstalled( .metadata()
Box::new(dist.clone()), .map_err(|err| ResolveError::ReadInstalled(Box::new(dist.clone()), err))?;
DerivationChain::default(),
err,
)
})?;
Ok(Some(Response::Installed { dist, metadata })) Ok(Some(Response::Installed { dist, metadata }))
} }
@ -2035,11 +2031,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
} }
ResolvedDist::Installed { dist } => { ResolvedDist::Installed { dist } => {
let metadata = dist.metadata().map_err(|err| { let metadata = dist.metadata().map_err(|err| {
ResolveError::ReadInstalled( ResolveError::ReadInstalled(Box::new(dist.clone()), err)
Box::new(dist.clone()),
DerivationChain::default(),
err,
)
})?; })?;
Response::Installed { dist, metadata } Response::Installed { dist, metadata }
} }

View File

@ -81,13 +81,8 @@ impl OperationDiagnostic {
dist_error(kind, dist, &chain, err); dist_error(kind, dist, &chain, err);
None None
} }
pip::operations::Error::Requirements(uv_requirements::Error::Dist( pip::operations::Error::Requirements(uv_requirements::Error::Dist(kind, dist, err)) => {
kind, dist_error(kind, dist, &DerivationChain::default(), Arc::new(err));
dist,
chain,
err,
)) => {
dist_error(kind, dist, &chain, Arc::new(err));
None None
} }
pip::operations::Error::Prepare(uv_installer::PrepareError::Dist( pip::operations::Error::Prepare(uv_installer::PrepareError::Dist(

View File

@ -36,9 +36,8 @@ use uv_requirements::{
SourceTreeResolver, SourceTreeResolver,
}; };
use uv_resolver::{ use uv_resolver::{
DependencyMode, DerivationChainBuilder, Exclusions, FlatIndex, InMemoryIndex, Manifest, DependencyMode, Exclusions, FlatIndex, InMemoryIndex, Manifest, Options, Preference,
Options, Preference, Preferences, PythonRequirement, Resolver, ResolverEnvironment, Preferences, PythonRequirement, Resolver, ResolverEnvironment, ResolverOutput,
ResolverOutput,
}; };
use uv_types::{HashStrategy, InFlight, InstalledPackagesProvider}; use uv_types::{HashStrategy, InFlight, InstalledPackagesProvider};
use uv_warnings::warn_user; use uv_warnings::warn_user;
@ -464,20 +463,8 @@ pub(crate) async fn install(
.with_reporter(PrepareReporter::from(printer).with_length(remote.len() as u64)); .with_reporter(PrepareReporter::from(printer).with_length(remote.len() as u64));
let wheels = preparer let wheels = preparer
.prepare(remote.clone(), in_flight) .prepare(remote.clone(), in_flight, resolution)
.await .await?;
.map_err(Error::from)
.map_err(|err| match err {
// Attach resolution context to the error.
Error::Prepare(uv_installer::PrepareError::Dist(kind, dist, chain, err)) => {
debug_assert!(chain.is_empty());
let chain =
DerivationChainBuilder::from_resolution(resolution, (&*dist).into())
.unwrap_or_default();
Error::Prepare(uv_installer::PrepareError::Dist(kind, dist, chain, err))
}
_ => err,
})?;
logger.on_prepare(wheels.len(), start, printer)?; logger.on_prepare(wheels.len(), start, printer)?;