-
Notifications
You must be signed in to change notification settings - Fork 115
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
Surface the failing command's name from the build error dump #253
Conversation
I've also added |
Instead of using panic, maybe using eprintln!() + std::process::exit(1) would work? openssl-src is probably not called in any context where recovering from an unexpected build error is necessary, and you added try_build for cases where build errors are expected to happen. |
Exiting directly without a panic would be cleaner, but I'm leaving that to the caller of |
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.
Thanks for the PR! If try_build
is being introduced could this result-ify all the unwraps/expects? The existence of try_build
and build
implies that one never panics while the other might, so I'd prefer to head off confusion.
Also I agree with @bjorn3 that if the goal is a "clean exit" for the build
method, could that be updated to print+exit instead of a formatted panic?
fd39574
to
3cf146f
Compare
I've updated it with a clean exit, and converted other panics into errors. |
Thanks for this! |
Because Cargo's formatting of build errors is very noisy users struggle to understand what is causing the build to fail.
Here's a recent case where the failure reason was simple, but the user could not see it through all the noise.
Debug
impl ofCommand
obscures the command name that has been run. This change prints the command name alone.cargo:warning
to also print the message above the stdout and stderr dumps.