Add error context to failures in ra_project_model using anyhow crate
#3119
Conversation
adamrk
changed the title
ra_project_model using anyhow crate
| meta.current_dir(parent); | ||
| } | ||
| let meta = meta.exec().map_err(|e| format!("cargo metadata failed: {}", e))?; | ||
| let meta = meta.exec().map_err(|e| anyhow!("cargo metadata failed: {}", e))?; |
There was a problem hiding this comment.
I'd suggest to check that we get a good error message here. I am concerned because we got a report #3118 where the error message was not very helpful... I also believe meta.exec().context("cargo metadata failed") would give us a bit better error message, though not sure...
There was a problem hiding this comment.
Agree, I think this should use .context
There was a problem hiding this comment.
I ran the function with a path which wasn't pointing to a Cargo.toml file and neither setup records he path that was used in the error message. Here's what I got with your change:
cargo metadata failed
Caused by:
Error during execution of `cargo metadata`: error: the manifest-path must be a path to a Cargo.toml file
Maybe it would make sense to add a new method on MetadataCommand which returns the full command called and include that in the error message?
I can't tell from that bug report exactly what info would have clarified things.
There was a problem hiding this comment.
I think something like this should work:
meta.exec()
.with_context(|| format!("Failed to run `cargo metadata --manifest-path {}`", cargo_toml.display()))
crates/ra_project_model/Cargo.toml
Outdated
| serde = { version = "1.0.89", features = ["derive"] } | ||
| serde_json = "1.0.39" | ||
|
|
||
| anyhow = "1.0.26" |
crates/ra_project_model/src/lib.rs
Outdated
| } | ||
|
|
||
| match (|| -> Result<_> { | ||
| match (|| -> ::std::result::Result<String, Box<dyn Error + Send + Sync + 'static>> { |
There was a problem hiding this comment.
This probably wants to use anyhow::Error rather than Box<dyn Error> as well.
There was a problem hiding this comment.
@matklad , maybe anyhow::Result<String>, anyhow?
| .args(&["--print", "sysroot"]) | ||
| .output() | ||
| .map_err(|e| format!("rustc --print sysroot failed: {}", e))?; | ||
| .map_err(|e| anyhow!("rustc --print sysroot failed: {}", e))?; |
There was a problem hiding this comment.
Here .context would be more appropriate I think
| Err("failed to locate sysroot")?; | ||
| let exit_message = match rustc_output.status.code() { | ||
| Some(code) => { | ||
| anyhow!("failed to locate sysroot: rustc --print sysroot exited with code {}", code) |
There was a problem hiding this comment.
I think this can use bail! macro, rather than an Err(...)? pattern.
|
Looks like CI is failing due to |
adamrk
force-pushed
the
use-anyhow-in-ra_project_model
branch
from
5dc8045 to
d908c9a
Compare
My mistake - just mixed up changes addressing separate comments from the review. |
| let cargo_toml = PathBuf::from("/"); | ||
| let result = CargoWorkspace::from_cargo_metadata(&cargo_toml, &CargoFeatures::default()); | ||
| println!("error: {:?}", result) | ||
| } |
There was a problem hiding this comment.
This test doesn't assert anything and probably should be removed?
There was a problem hiding this comment.
Oops - forgot to take it out.
|
bors r+ |
…ate (rust-lang#3119) Add error context to failures in ra_project_model using anyhow crate
4 participants
Addresses #3085