Skip to content

Better build error messages #9660

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

Merged
zanieb merged 6 commits into from
Dec 17, 2024
Merged

Better build error messages #9660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
be9d1d5
Better build error messages
konstin Dec 4, 2024
e0bb63d
Special case build backend errors
konstin Dec 5, 2024
b67c2ef
More consistent errors
konstin Dec 6, 2024
11922eb
Unify dist error kind building
konstin Dec 6, 2024
354d6f0
Lint
konstin Dec 6, 2024
65c749c
Move error type
konstin Dec 6, 2024
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
3 changes: 2 additions & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion crates/uv-build-frontend/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ uv-types = { workspace = true }
uv-virtualenv = { workspace = true }

anstream = { workspace = true }
anyhow = { workspace = true }
fs-err = { workspace = true }
indoc = { workspace = true }
itertools = { workspace = true }
Expand Down
68 changes: 58 additions & 10 deletions crates/uv-build-frontend/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,17 @@ use std::path::PathBuf;
use std::process::ExitStatus;
use std::sync::LazyLock;

use crate::PythonRunnerOutput;
use owo_colors::OwoColorize;
use regex::Regex;
use thiserror::Error;
use tracing::error;
use uv_configuration::BuildOutput;
use uv_distribution_types::IsBuildBackendError;
use uv_fs::Simplified;
use uv_pep440::Version;
use uv_pep508::PackageName;

use crate::PythonRunnerOutput;
use uv_types::AnyErrorBuild;

/// e.g. `pygraphviz/graphviz_wrap.c:3020:10: fatal error: graphviz/cgraph.h: No such file or directory`
static MISSING_HEADER_RE_GCC: LazyLock<Regex> = LazyLock::new(|| {
Expand Down Expand Up @@ -68,19 +69,47 @@ pub enum Error {
#[error("Editable installs with setup.py legacy builds are unsupported, please specify a build backend in pyproject.toml")]
EditableSetupPy,
#[error("Failed to resolve requirements from {0}")]
RequirementsResolve(&'static str, #[source] anyhow::Error),
RequirementsResolve(&'static str, #[source] AnyErrorBuild),
#[error("Failed to install requirements from {0}")]
RequirementsInstall(&'static str, #[source] anyhow::Error),
RequirementsInstall(&'static str, #[source] AnyErrorBuild),
Copy link
Copy Markdown
Member

@BurntSushi BurntSushi Dec 5, 2024

Choose a reason for hiding this comment

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

Same as below, but this might be dropping any chain that might have been on anyhow::Error? Not 100% sure

Copy link
Copy Markdown
Member Author

@konstin konstin Dec 6, 2024

Choose a reason for hiding this comment

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

AnyErrorBuild preserves the chain, it passes .source() calls through to its wrapped type.

Copy link
Copy Markdown
Member

@BurntSushi BurntSushi Dec 6, 2024

Choose a reason for hiding this comment

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

I mean its Display impl. The Display impl for a Box<dyn Error> doesn't emit the chain. But a anyhow::Error will.

Copy link
Copy Markdown
Member Author

@konstin konstin Dec 6, 2024

Choose a reason for hiding this comment

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

afaik all the error types only show their inner message, but not the chain, over which you have to iterate manually. When testing,

use thiserror::Error;

#[derive(Error, Debug)]
#[error("Wrapper Err")]
struct WrappedErr;

fn main() {
    let err: anyhow::Error = WrappedErr.into();
    println!("{}", err);
    println!("{}", err.context("2"));
}

shows

Wrapper Err
2

and we don't seem to be losing and context in our snapshot tests.

Copy link
Copy Markdown
Member Author

@konstin konstin Dec 6, 2024

Choose a reason for hiding this comment

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

anyhow does show the chain, but only in alternate mode:

use thiserror::Error;

#[derive(Error, Debug)]
#[error("Inner Err")]
struct InnerErr;

#[derive(Error, Debug)]
#[error("Wrapper Err")]
struct WrappedErr(#[source] InnerErr);

fn main() {
    let err = WrappedErr(InnerErr);
    println!("{:#}", err);
    let err: anyhow::Error = WrappedErr(InnerErr).into();
    println!("{:#}", err);
    println!("{:#}", err.context("2"));
}

shows

Wrapper Err
Wrapper Err: Inner Err
2: Wrapper Err: Inner Err

Copy link
Copy Markdown
Member

@BurntSushi BurntSushi Dec 6, 2024

Choose a reason for hiding this comment

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

Ah okay, so anyhow::Error's Display impl is the same as Box<dyn Error>. Interesting. All righty.

And yeah, I realize the snapshot tests indicate nothing untoward is happening here. I don't have a full grasp of the surrounding code and I wasn't sure how easy it would be to break something in the future. But I think you've demonstrated my concern is unfounded. Thank you. :)

#[error("Failed to create temporary virtualenv")]
Virtualenv(#[from] uv_virtualenv::Error),
// Build backend errors
#[error("Failed to run `{0}`")]
CommandFailed(PathBuf, #[source] io::Error),
#[error(transparent)]
#[error("The build backend returned an error")]
BuildBackend(#[from] BuildBackendError),
#[error(transparent)]
#[error("The build backend returned an error")]
MissingHeader(#[from] MissingHeaderError),
#[error("Failed to build PATH for build script")]
BuildScriptPath(#[source] env::JoinPathsError),
// For the convenience of typing `setup_build` properly.
#[error("Building source distributions for {0} is disabled")]
NoSourceDistBuild(PackageName),
#[error("Building source distributions is disabled")]
NoSourceDistBuilds,
}

impl IsBuildBackendError for Error {
fn is_build_backend_error(&self) -> bool {
match self {
Self::Io(_)
| Self::Lowering(_)
| Self::InvalidSourceDist(_)
| Self::InvalidPyprojectTomlSyntax(_)
| Self::InvalidPyprojectTomlSchema(_)
| Self::EditableSetupPy
| Self::RequirementsResolve(_, _)
| Self::RequirementsInstall(_, _)
| Self::Virtualenv(_)
| Self::NoSourceDistBuild(_)
| Self::NoSourceDistBuilds => false,
Self::CommandFailed(_, _)
| Self::BuildBackend(_)
| Self::MissingHeader(_)
| Self::BuildScriptPath(_) => true,
}
}
}

#[derive(Debug)]
Expand Down Expand Up @@ -247,6 +276,13 @@ impl Display for BuildBackendError {
writeln!(f)?;
}

write!(
f,
"\n{}{} This usually indicates a problem with the package or the build environment.",
"hint".bold().cyan(),
":".bold()
)?;

Ok(())
}
}
Expand Down Expand Up @@ -416,7 +452,10 @@ mod test {

assert!(matches!(err, Error::MissingHeader { .. }));
// Unix uses exit status, Windows uses exit code.
let formatted = err.to_string().replace("exit status: ", "exit code: ");
let formatted = std::error::Error::source(&err)
.unwrap()
.to_string()
.replace("exit status: ", "exit code: ");
let formatted = anstream::adapter::strip_str(&formatted);
insta::assert_snapshot!(formatted, @r###"
Failed building wheel through setup.py (exit code: 0)
Expand Down Expand Up @@ -471,7 +510,10 @@ mod test {
);
assert!(matches!(err, Error::MissingHeader { .. }));
// Unix uses exit status, Windows uses exit code.
let formatted = err.to_string().replace("exit status: ", "exit code: ");
let formatted = std::error::Error::source(&err)
.unwrap()
.to_string()
.replace("exit status: ", "exit code: ");
let formatted = anstream::adapter::strip_str(&formatted);
insta::assert_snapshot!(formatted, @r###"
Failed building wheel through setup.py (exit code: 0)
Expand Down Expand Up @@ -516,7 +558,10 @@ mod test {
);
assert!(matches!(err, Error::MissingHeader { .. }));
// Unix uses exit status, Windows uses exit code.
let formatted = err.to_string().replace("exit status: ", "exit code: ");
let formatted = std::error::Error::source(&err)
.unwrap()
.to_string()
.replace("exit status: ", "exit code: ");
let formatted = anstream::adapter::strip_str(&formatted);
insta::assert_snapshot!(formatted, @r###"
Failed building wheel through setup.py (exit code: 0)
Expand Down Expand Up @@ -559,7 +604,10 @@ mod test {
);
assert!(matches!(err, Error::MissingHeader { .. }));
// Unix uses exit status, Windows uses exit code.
let formatted = err.to_string().replace("exit status: ", "exit code: ");
let formatted = std::error::Error::source(&err)
.unwrap()
.to_string()
.replace("exit status: ", "exit code: ");
let formatted = anstream::adapter::strip_str(&formatted);
insta::assert_snapshot!(formatted, @r###"
Failed building wheel through setup.py (exit code: 0)
Expand Down
97 changes: 45 additions & 52 deletions crates/uv-build-frontend/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ mod error;
use fs_err as fs;
use indoc::formatdoc;
use itertools::Itertools;
use owo_colors::OwoColorize;
use rustc_hash::FxHashMap;
use serde::de::{value, IntoDeserializer, SeqAccess, Visitor};
use serde::{de, Deserialize, Deserializer};
Expand Down Expand Up @@ -36,7 +35,7 @@ use uv_pep508::PackageName;
use uv_pypi_types::{Requirement, VerbatimParsedUrl};
use uv_python::{Interpreter, PythonEnvironment};
use uv_static::EnvVars;
use uv_types::{BuildContext, BuildIsolation, SourceBuildTrait};
use uv_types::{AnyErrorBuild, BuildContext, BuildIsolation, SourceBuildTrait};

pub use crate::error::{Error, MissingHeaderCause};

Expand Down Expand Up @@ -325,7 +324,7 @@ impl SourceBuild {
build_context
.install(&resolved_requirements, &venv)
.await
.map_err(|err| Error::RequirementsInstall("`build-system.requires`", err))?;
.map_err(|err| Error::RequirementsInstall("`build-system.requires`", err.into()))?;
} else {
debug!("Proceeding without build isolation");
}
Expand Down Expand Up @@ -423,15 +422,19 @@ impl SourceBuild {
let resolved_requirements = build_context
.resolve(&default_backend.requirements)
.await
.map_err(|err| Error::RequirementsResolve("`setup.py` build", err))?;
.map_err(|err| {
Error::RequirementsResolve("`setup.py` build", err.into())
})?;
*resolution = Some(resolved_requirements.clone());
resolved_requirements
}
} else {
build_context
.resolve(&pep517_backend.requirements)
.await
.map_err(|err| Error::RequirementsResolve("`build-system.requires`", err))?
.map_err(|err| {
Error::RequirementsResolve("`build-system.requires`", err.into())
})?
},
)
}
Expand Down Expand Up @@ -622,8 +625,8 @@ impl SourceBuild {
if !output.status.success() {
return Err(Error::from_command_output(
format!(
"Build backend failed to determine metadata through `{}`",
format!("prepare_metadata_for_build_{}", self.build_kind).green()
"Call to `{}.prepare_metadata_for_build_{}` failed",
self.pep517_backend.backend, self.build_kind
),
&output,
self.level,
Expand Down Expand Up @@ -745,9 +748,8 @@ impl SourceBuild {
if !output.status.success() {
return Err(Error::from_command_output(
format!(
"Build backend failed to build {} through `{}`",
self.build_kind,
format!("build_{}", self.build_kind).green(),
"Call to `{}.build_{}` failed",
pep517_backend.backend, self.build_kind
),
&output,
self.level,
Expand All @@ -761,8 +763,8 @@ impl SourceBuild {
if !output_dir.join(&distribution_filename).is_file() {
return Err(Error::from_command_output(
format!(
"Build backend failed to produce {} through `{}`: `{distribution_filename}` not found",
self.build_kind, format!("build_{}", self.build_kind).green(),
"Call to `{}.build_{}` failed",
pep517_backend.backend, self.build_kind
),
&output,
self.level,
Expand All @@ -776,11 +778,11 @@ impl SourceBuild {
}

impl SourceBuildTrait for SourceBuild {
async fn metadata(&mut self) -> anyhow::Result<Option<PathBuf>> {
async fn metadata(&mut self) -> Result<Option<PathBuf>, AnyErrorBuild> {
Ok(self.get_metadata_without_build().await?)
}

async fn wheel<'a>(&'a self, wheel_dir: &'a Path) -> anyhow::Result<String> {
async fn wheel<'a>(&'a self, wheel_dir: &'a Path) -> Result<String, AnyErrorBuild> {
Ok(self.build(wheel_dir).await?)
}
}
Expand Down Expand Up @@ -858,8 +860,8 @@ async fn create_pep517_build_environment(
if !output.status.success() {
return Err(Error::from_command_output(
format!(
"Build backend failed to determine requirements with `{}`",
format!("build_{build_kind}()").green()
"Call to `{}.build_{}` failed",
pep517_backend.backend, build_kind
),
&output,
level,
Expand All @@ -869,37 +871,27 @@ async fn create_pep517_build_environment(
));
}

// Read the requirements from the output file.
let contents = fs_err::read(&outfile).map_err(|err| {
Error::from_command_output(
format!(
"Build backend failed to read requirements from `{}`: {err}",
format!("get_requires_for_build_{build_kind}").green(),
),
&output,
level,
package_name,
package_version,
version_id,
)
})?;

// Deserialize the requirements from the output file.
let extra_requires: Vec<uv_pep508::Requirement<VerbatimParsedUrl>> =
serde_json::from_slice::<Vec<uv_pep508::Requirement<VerbatimParsedUrl>>>(&contents)
.map_err(|err| {
Error::from_command_output(
format!(
"Build backend failed to return requirements from `{}`: {err}",
format!("get_requires_for_build_{build_kind}").green(),
),
&output,
level,
package_name,
package_version,
version_id,
)
})?;
// Read and deserialize the requirements from the output file.
let read_requires_result = fs_err::read(&outfile)
.map_err(|err| err.to_string())
.and_then(|contents| serde_json::from_slice(&contents).map_err(|err| err.to_string()));
let extra_requires: Vec<uv_pep508::Requirement<VerbatimParsedUrl>> = match read_requires_result
{
Ok(extra_requires) => extra_requires,
Err(err) => {
return Err(Error::from_command_output(
format!(
"Call to `{}.get_requires_for_build_{}` failed: {}",
pep517_backend.backend, build_kind, err
),
&output,
level,
package_name,
package_version,
version_id,
))
}
};

// If necessary, lower the requirements.
let extra_requires = match source_strategy {
Expand Down Expand Up @@ -937,15 +929,16 @@ async fn create_pep517_build_environment(
.cloned()
.chain(extra_requires)
.collect();
let resolution = build_context
.resolve(&requirements)
.await
.map_err(|err| Error::RequirementsResolve("`build-system.requires`", err))?;
let resolution = build_context.resolve(&requirements).await.map_err(|err| {
Error::RequirementsResolve("`build-system.requires`", AnyErrorBuild::from(err))
})?;

build_context
.install(&resolution, venv)
.await
.map_err(|err| Error::RequirementsInstall("`build-system.requires`", err))?;
.map_err(|err| {
Error::RequirementsInstall("`build-system.requires`", AnyErrorBuild::from(err))
})?;
}

Ok(())
Expand Down
2 changes: 2 additions & 0 deletions crates/uv-dispatch/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ uv-distribution-types = { workspace = true }
uv-git = { workspace = true }
uv-install-wheel = { workspace = true }
uv-installer = { workspace = true }
uv-platform-tags = { workspace = true }
uv-pypi-types = { workspace = true }
uv-python = { workspace = true }
uv-resolver = { workspace = true }
Expand All @@ -38,5 +39,6 @@ anyhow = { workspace = true }
futures = { workspace = true }
itertools = { workspace = true }
rustc-hash = { workspace = true }
thiserror = { workspace = true }
tokio = { workspace = true }
tracing = { workspace = true }
Loading