- Notifications
You must be signed in to change notification settings - Fork137
[#260] Show failed nix command#310
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
[#260] Show failed nix command#310
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Problem: Many errors result from attempts to build commands which mighterror with a non-zero exit code. The failed commands are not displayedto the user, however.Solution: For every `([A-Za-z])Exit(Option<i32>)` error, change it to`\1Exit(Option<i32>, String)`, where the `String` contains the failedcommand. Format each `tokyo` command using `{:?}` and save it to thecorresponding error.Note that `Command` cannot be copied or cloned, and it's not possible toaccess a command's `std` field to get the underlying command, so weresort to putting the `Debug`-formatted command.Problem: It's common to spawn a command and occasionally fail duringrunning the command or because it exited, leading to code repetition.Solution: Create a `command` module to abstract some of the boilerplateby wrapping `tokio::process::Command`. Create a `run` function thatabstracts most of this boilerplate.In some parts, the way commands are executed are a bit different, soinstead we leave them as it is.
Problem: The output when a command failed for deployment would show theerror message in the console output and _then_ the error message, whichwas confusing.Solution: Show the error message together with the error in an orderthat makes sense. This is done by capturing the output in the errormessage and printinting it in an exit error. We also use `Stdio::null`rather than `Stdio::piped` to avoid printing to the console.
src/command.rs Outdated
| } | ||
| } | ||
| pubfnarg<S:AsRef<OsStr>>(&mutself,arg:S) ->&mutCommand{ |
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.
What is the motivation behind the reimplementation ofarg and other methods defined forTokioCommand rather than creating a singlenew constructor that accepts already constructedTokioCommand andrun to run the command handling the 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.
I pushed a commit with your suggestion. It simplifiescommand.rs, but makes the consumer code a bit more verbose.
| } | ||
| #[derive(Error,Debug)] | ||
| pubenumActivateError{ |
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.
I like the fact that this error type became simpler
| let ssh_activate_exit_status = ssh_activate_child | ||
| .wait() | ||
| .wait_with_output() |
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.
Is there a reason why therun method ofcommand::Command is not used here instead ofwait_with_output?
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.
Becausessh_activate_child is atokio::process::Child and not acommand::Command ortokio::process::Command. Although there are patterns, the way the codebase handles children is not repetitive enough to easily abstract them into arun command for a newcommand::Child, but I can put some more thought into this if you'd like.
This PR displays the command that failed when an error is produced after a command exits with a non-zero code. The output is also cleared a bit. Example: