Better build error messages (#9660)

Build failures are one of the most common user facing failures that
aren't "obivous" errors (such as typos) or resolver errors. Currently,
they show more technical details than being focussed on this being an
error in a subprocess that is either on the side of the package or -
more likely - in the build environment, e.g. the user needs to install a
dev package or their python version is incompatible.

The new error message clearly delineates the part that's important (this
is a build backend problem) from the internals (we called this hook) and
is consistent about which part of the dist building stage failed. We
have to calibrate the exact wording of the error message some more. Most
of the implementation is working around the orphan rule, (this)error
rules and trait rules, so it came out more of a refactoring than
intended.

Example:


![image](https://github.com/user-attachments/assets/2bc12992-db79-4362-a444-fd0d94594b77)
This commit is contained in:
konsti 2024-12-17 16:44:32 +01:00 committed by GitHub
parent b7df5dbaf3
commit ebc6d20d9d
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
24 changed files with 426 additions and 262 deletions

View file

@ -1059,36 +1059,11 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
MetadataResponse::Error(dist, err) => {
// TODO(charlie): Add derivation chain for URL dependencies. In practice, this isn't
// critical since we fetch URL dependencies _prior_ to invoking the resolver.
let chain = DerivationChain::default();
let (kind, dist) = match &**dist {
Dist::Built(built_dist @ BuiltDist::Path(_)) => {
(DistErrorKind::Read, Dist::Built(built_dist.clone()))
}
Dist::Source(source_dist @ SourceDist::Path(_)) => {
(DistErrorKind::Build, Dist::Source(source_dist.clone()))
}
Dist::Source(source_dist @ SourceDist::Directory(_)) => {
(DistErrorKind::Build, Dist::Source(source_dist.clone()))
}
Dist::Built(built_dist) => {
(DistErrorKind::Download, Dist::Built(built_dist.clone()))
}
Dist::Source(source_dist) => {
if source_dist.is_local() {
(DistErrorKind::Build, Dist::Source(source_dist.clone()))
} else {
(
DistErrorKind::DownloadAndBuild,
Dist::Source(source_dist.clone()),
)
}
}
};
return Err(ResolveError::Dist(
kind,
Box::new(dist),
chain,
(*err).clone(),
DistErrorKind::from_dist_and_err(dist, &**err),
dist.clone(),
DerivationChain::default(),
err.clone(),
));
}
};
@ -1446,35 +1421,11 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
MetadataResponse::Error(dist, err) => {
let chain = DerivationChainBuilder::from_state(id, version, pubgrub)
.unwrap_or_default();
let (kind, dist) = match &**dist {
Dist::Built(built_dist @ BuiltDist::Path(_)) => {
(DistErrorKind::Read, Dist::Built(built_dist.clone()))
}
Dist::Source(source_dist @ SourceDist::Path(_)) => {
(DistErrorKind::Build, Dist::Source(source_dist.clone()))
}
Dist::Source(source_dist @ SourceDist::Directory(_)) => {
(DistErrorKind::Build, Dist::Source(source_dist.clone()))
}
Dist::Built(built_dist) => {
(DistErrorKind::Download, Dist::Built(built_dist.clone()))
}
Dist::Source(source_dist) => {
if source_dist.is_local() {
(DistErrorKind::Build, Dist::Source(source_dist.clone()))
} else {
(
DistErrorKind::DownloadAndBuild,
Dist::Source(source_dist.clone()),
)
}
}
};
return Err(ResolveError::Dist(
kind,
Box::new(dist),
DistErrorKind::from_dist_and_err(dist, &**err),
dist.clone(),
chain,
(*err).clone(),
err.clone(),
));
}
};
@ -2353,22 +2304,6 @@ impl ForkState {
// A dependency from the root package or requirements.txt.
debug!("Adding direct dependency: {package}{version}");
let name = package.name_no_root().unwrap();
// Catch cases where we pass a package once by name with extras and then once as
// URL for the specific distribution.
has_url = has_url
|| dependencies
.iter()
.filter(|other_dep| *other_dep != dependency)
.filter(|other_dep| {
other_dep
.package
.name()
.is_some_and(|other_name| other_name == name)
})
.any(|other_dep| other_dep.url.is_some());
// Warn the user if a direct dependency lacks a lower bound in `--lowest` resolution.
let missing_lower_bound = version
.bounding_range()
@ -2383,6 +2318,7 @@ impl ForkState {
"The direct dependency `{name}` is unpinned. \
Consider setting a lower bound when using `--resolution lowest` \
to avoid using outdated versions.",
name = package.name_no_root().unwrap(),
);
}
}