-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
Panic-less code and user prompt for toolchain installation #30
Conversation
8adf52a
to
02311f1
Compare
02311f1
to
ee1993a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick fix about main
logging the error chain and producing a backtrace for better debugging.
reason = "CRAB GOOD. CRAB IMPORTANT." | ||
)] | ||
{ | ||
print!("🦀 "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can totally change this BTW. I just used Ferris to as an example of what we could do.
crates/cargo-gpu/src/main.rs
Outdated
} | ||
} | ||
|
||
fn main() -> anyhow::Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I think it's fine for a cli program to panic
when it hits an unrecoverable error. There are benefits to this behavior like knowing exactly where in the program the failure occurred since the line number is included in the print statement and a backtrace is produced.
But wrapping all possible errors in Result
can be nice for testing and is a generally good practice.
So - my only concern here is that if main
returns the Result
, we have no way of logging the error (and the errors further up the chain of errors) or the backtrace and instead we rely on whatever Rust's default behavior is - which I think is likely just panicking with the text of the top-level error, erasing the backtrace.
So I think here we should probably not return Result
and instead if let Err(msg) = result { ... }
making sure to use all of anyhow
's machinery to print the relevant information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points, I agree. I've refactored so that main()
wraps and captures the error from a run()
function, then writes the error message to stderr
and logs the backtrace when not a release build.
Also add some user output for the major steps of installing toolchains, compiling `spirv-builder-cli` and compiling the actual shader. Surprisingly there's no way in the standard library to get a single keypress from the user! There's ways to get a line, therefore a keypress then a newline, but that's not so conventional for responding to a "y/n" prompt. Fixes Rust-GPU#14
ee1993a
to
9dc0c03
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! 🙇
There's now prompts for when Rust toolchain assets need to be installed. Surprisingly responding to a prompt with a single keypress requires a whole crate, namely
crossterm
.Also all the code is now "panic-less", so no
unwrap()
,panic()
, etc. Everything goes through?
thanks to theanyhow
crate.Fixes #14