fix anyhow with_context loss other backtrace

[taikulawo]

1. Fix with_context loss command line args.
   from anyhow docs

When you print an error object using "{}" or to_string(), only the outermost
/// underlying error or context is printed, not any of the lower level causes.
/// This is exactly as if you had called the Display impl of the error from
/// which you constructed your anyhow::Error.
///
/// console /// Failed to read instrs from ./path/to/instrs.json ///
///
/// To print causes as well using anyhow's default formatting of causes, use the
/// alternate selector "{:#}".

So even we use

.with_context(|| {
      format!(
          "Command `{}` failed ({})",
          format_command(&cmd),
          output.status
      )
  })
  .with_context(|| {
      format!(
          "Failed to compile {} from {}",
          out.display(),
          source.display()
      )
  });

we are still use to_string to format anyhow error, which cause loss anyhow command line context.

we should better change

let dir = tempdir().map_err(|e| Error::Build(e.to_string()))?;

to

let dir = tempdir().map_err(|e| Error::Build(format!("{:#}", e))?;

But more nice method is change Error::Build(String) to Error::Build(anyhow::Error), that's 2. Feature

2. Feature: replace Error::Build(String) to Error::Build(anyhow::Error), keep anyhow error format to make beauty print
   

[danielocfb]

Thanks for improving the error reporting. May I ask why you adjusted Build but not Generate? The same issue seems to exist there. Also, please clean up the history as per the contribution guidelines.

[danielocfb]

I don't believe this should have a #[from] annotation. There is nothing about this variant that is tied to anyhow's Error. What happens if we were to switch the Generate variant over to using anyhow's Error as well? Now we are suddenly implicitly converting anyhow errors into Build, right? That seems wrong. #[from] is meant for cases where there is a clear relationship between a unique error type and the thing stored in the variant.

Furthermore, I believe it should have the #[source] annotation and omit {0} in the error description. Otherwise, that will just cause duplicate information from what I understand, depending on how formatting is done.

[taikulawo]

Yes, #[from] should be removed. we can use anyhow::Error directly without call into(). Generate can also use anyhow::Error if you prefer, I will do.

If you care duplicate infomation, there shouldn't have #[error("Error generating skeleton: {0}")] or #[error("Error building BPF object file: {0}")], Build/Generate always have that semantic

I have change bothBuild/Generate to #[error(transparent)] to forward Build/Generate to underlying anyhow::Error

[taikulawo]

#[error("...")] only generate Display trait not Debug, Error building BPF object file: show when you print like {}, e, but context and backtrace only show when you {:?}, e

In the most scene, libbpf-cargo only be used in build.rs, so Debug mostly used

[danielocfb]

Thanks. I am not sure #[error(transparent)] is what we want, though. We still want to convey what went wrong (building or generating), otherwise why do we keep this error type to begin with?

So I'd think we want to keep printing the "Error generating skeleton" error string but should leave out the {0} part (at least if we use the #[source] annotation as well, which would be my preference).

[taikulawo]

Thanks for improving the error reporting. May I ask why you adjusted Build but not Generate? The same issue seems to exist there. Also, please clean up the history as per the contribution guidelines.

I want to improve BPF build error report in BPF build progress, Generate progress is self-contained, less likely to make a mistakes

[danielocfb]

Thanks for improving the error reporting. May I ask why you adjusted Build but not Generate? The same issue seems to exist there. Also, please clean up the history as per the contribution guidelines.

I want to improve BPF build error report in BPF build progress, Generate progress is self-contained, less likely to make a mistakes

Let's just do it? None of this is a reason to not convert it. Now every reasonable person will wonder why one variant contains an error and the other variant a string.

[taikulawo]

All done
example output

but we would better keep {0}, or we will just get Error building BPF object file. It don't said which file failed like Error file not found, confusing user.

[danielocfb]

but we would better keep {0}, or we will just get Error building BPF object file. It don't said which file failed like Error file not found, confusing user.

I mean...it's an error chain. If you don't print the chain you won't get all the details. That's kind of the point. Here is the official stance: rust-lang/project-error-handling#27 (comment)

[danielocfb]

Seems fine to me, thanks!

[danielocfb]

I added a CHANGELOG entry and a proper commit description and merge the change. I think we should consider adding a custom Debug impl for the Error type to get rid of this Build( / Generate( prefix in the output, but this change should be able to stand on its own. Feel free to open a pull request if you want to take care of it. Thanks again.