Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defer reporting of build failures in resolver #9098

Merged
merged 1 commit into from
Nov 13, 2024
Merged

Defer reporting of build failures in resolver #9098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions crates/uv-resolver/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ pub enum ResolveError {
#[error(transparent)]
Client(#[from] uv_client::Error),

#[error(transparent)]
Distribution(#[from] uv_distribution::Error),

#[error("The channel closed unexpectedly")]
ChannelClosed,

Expand Down Expand Up @@ -94,20 +97,20 @@ pub enum ResolveError {
ParsedUrl(#[from] uv_pypi_types::ParsedUrlError),

#[error("Failed to download `{0}`")]
Download(Box<BuiltDist>, #[source] uv_distribution::Error),
Download(Box<BuiltDist>, #[source] Arc<uv_distribution::Error>),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I can avoid this Arc... I need to be able to clone this, because it's stored in the OnceMap, but then needs to be cloned to be returned as a Result in the resolver. And uv_distribution::Error doesn't implement Clone.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arc inside an error is totally natural: https://github.com/BurntSushi/jiff/blob/16376c7175bca14a8ccb37a178bde92598af1a5f/src/error.rs#L47-L57

The main alternative here, and one I would suggest if possible, is to make uv_distribution::Error embed an Arc internally so that it can implement Clone itself. Then you don't need to spread Arcs out everywhere else.


#[error("Failed to download and build `{0}`")]
DownloadAndBuild(Box<SourceDist>, #[source] uv_distribution::Error),
DownloadAndBuild(Box<SourceDist>, #[source] Arc<uv_distribution::Error>),

#[error("Failed to read `{0}`")]
Read(Box<BuiltDist>, #[source] uv_distribution::Error),
Read(Box<BuiltDist>, #[source] Arc<uv_distribution::Error>),

// TODO(zanieb): Use `thiserror` in `InstalledDist` so we can avoid chaining `anyhow`
#[error("Failed to read metadata from installed package `{0}`")]
ReadInstalled(Box<InstalledDist>, #[source] anyhow::Error),

#[error("Failed to build `{0}`")]
Build(Box<SourceDist>, #[source] uv_distribution::Error),
Build(Box<SourceDist>, #[source] Arc<uv_distribution::Error>),

#[error(transparent)]
NoSolution(#[from] NoSolutionError),
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-resolver/src/lock/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1241,7 +1241,7 @@ enum TagPolicy<'tags> {
/// Exclusively consider wheels that match the specified platform tags.
Required(&'tags Tags),
/// Prefer wheels that match the specified platform tags, but fall back to incompatible wheels
/// if necessary.
/// if necessary.
Preferred(&'tags Tags),
}

Expand Down
104 changes: 57 additions & 47 deletions crates/uv-resolver/src/resolver/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -945,6 +945,32 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
MetadataResponse::RequiresPython(..) => {
unreachable!("`requires-python` is only known upfront for registry distributions")
}
MetadataResponse::Error(dist, err) => {
return Err(match &**dist {
Dist::Built(built_dist @ BuiltDist::Path(_)) => {
ResolveError::Read(Box::new(built_dist.clone()), (*err).clone())
}
Dist::Source(source_dist @ SourceDist::Path(_)) => {
ResolveError::Build(Box::new(source_dist.clone()), (*err).clone())
}
Dist::Source(source_dist @ SourceDist::Directory(_)) => {
ResolveError::Build(Box::new(source_dist.clone()), (*err).clone())
}
Dist::Built(built_dist) => {
ResolveError::Download(Box::new(built_dist.clone()), (*err).clone())
}
Dist::Source(source_dist) => {
if source_dist.is_local() {
ResolveError::Build(Box::new(source_dist.clone()), (*err).clone())
} else {
ResolveError::DownloadAndBuild(
Box::new(source_dist.clone()),
(*err).clone(),
)
}
}
});
}
};

let version = &metadata.version;
Expand Down Expand Up @@ -1331,6 +1357,35 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
UnavailableVersion::RequiresPython(requires_python.clone()),
));
}
MetadataResponse::Error(dist, err) => {
return Err(match &**dist {
Dist::Built(built_dist @ BuiltDist::Path(_)) => {
ResolveError::Read(Box::new(built_dist.clone()), (*err).clone())
}
Dist::Source(source_dist @ SourceDist::Path(_)) => {
ResolveError::Build(Box::new(source_dist.clone()), (*err).clone())
}
Dist::Source(source_dist @ SourceDist::Directory(_)) => {
ResolveError::Build(Box::new(source_dist.clone()), (*err).clone())
}
Dist::Built(built_dist) => {
ResolveError::Download(Box::new(built_dist.clone()), (*err).clone())
}
Dist::Source(source_dist) => {
if source_dist.is_local() {
ResolveError::Build(
Box::new(source_dist.clone()),
(*err).clone(),
)
} else {
ResolveError::DownloadAndBuild(
Box::new(source_dist.clone()),
(*err).clone(),
)
}
}
});
}
};

if let Some(err) =
Expand Down Expand Up @@ -1791,28 +1846,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
let metadata = provider
.get_or_build_wheel_metadata(&dist)
.boxed_local()
.await
.map_err(|err| match dist.clone() {
Dist::Built(built_dist @ BuiltDist::Path(_)) => {
ResolveError::Read(Box::new(built_dist), err)
}
Dist::Source(source_dist @ SourceDist::Path(_)) => {
ResolveError::Build(Box::new(source_dist), err)
}
Dist::Source(source_dist @ SourceDist::Directory(_)) => {
ResolveError::Build(Box::new(source_dist), err)
}
Dist::Built(built_dist) => {
ResolveError::Download(Box::new(built_dist), err)
}
Dist::Source(source_dist) => {
if source_dist.is_local() {
ResolveError::Build(Box::new(source_dist), err)
} else {
ResolveError::DownloadAndBuild(Box::new(source_dist), err)
}
}
})?;
.await?;

Ok(Some(Response::Dist { dist, metadata }))
}
Expand Down Expand Up @@ -1928,31 +1962,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
let metadata = provider
.get_or_build_wheel_metadata(&dist)
.boxed_local()
.await
.map_err(|err| match dist.clone() {
Dist::Built(built_dist @ BuiltDist::Path(_)) => {
ResolveError::Read(Box::new(built_dist), err)
}
Dist::Source(source_dist @ SourceDist::Path(_)) => {
ResolveError::Build(Box::new(source_dist), err)
}
Dist::Source(source_dist @ SourceDist::Directory(_)) => {
ResolveError::Build(Box::new(source_dist), err)
}
Dist::Built(built_dist) => {
ResolveError::Download(Box::new(built_dist), err)
}
Dist::Source(source_dist) => {
if source_dist.is_local() {
ResolveError::Build(Box::new(source_dist), err)
} else {
ResolveError::DownloadAndBuild(
Box::new(source_dist),
err,
)
}
}
})?;
.await?;

Response::Dist { dist, metadata }
}
Expand Down
8 changes: 7 additions & 1 deletion crates/uv-resolver/src/resolver/provider.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use std::future::Future;
use std::sync::Arc;

use uv_configuration::BuildOptions;
use uv_distribution::{ArchiveMetadata, DistributionDatabase};
Expand Down Expand Up @@ -46,6 +47,8 @@ pub enum MetadataResponse {
/// The source distribution has a `requires-python` requirement that is not met by the installed
/// Python version (and static metadata is not available).
RequiresPython(VersionSpecifiers, Version),
/// The distribution could not be built or downloaded.
Error(Box<Dist>, Arc<uv_distribution::Error>),
}

pub trait ResolverProvider {
Expand Down Expand Up @@ -210,7 +213,10 @@ impl<'a, Context: BuildContext> ResolverProvider for DefaultResolverProvider<'a,
uv_distribution::Error::RequiresPython(requires_python, version) => {
Ok(MetadataResponse::RequiresPython(requires_python, version))
}
err => Err(err),
err => Ok(MetadataResponse::Error(
Box::new(dist.clone()),
Arc::new(err),
)),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method now _never_returns an error. But I left it as a Result since it's implementing the provider trait, and different clients could do different things here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

},
}
}
Expand Down
Loading