[#260] Show failed nix command #310
Conversation
Problem: Many errors result from attempts to build commands which might
error with a non-zero exit code. The failed commands are not displayed
to 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 failed
command. Format each `tokyo` command using `{:?}` and save it to the
corresponding error.
Note that `Command` cannot be copied or cloned, and it's not possible to
access a command's `std` field to get the underlying command, so we
resort to putting the `Debug`-formatted command.
Problem: It's common to spawn a command and occasionally fail during running the command or because it exited, leading to code repetition. Solution: Create a `command` module to abstract some of the boilerplate by wrapping `tokio::process::Command`. Create a `run` function that abstracts most of this boilerplate. In some parts, the way commands are executed are a bit different, so instead we leave them as it is.
Problem: The output when a command failed for deployment would show the error message in the console output and _then_ the error message, which was confusing. Solution: Show the error message together with the error in an order that makes sense. This is done by capturing the output in the error message 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
| } | ||
| } | ||
|
|
||
| pub fn arg<S: AsRef<OsStr>>(&mut self, arg: S) -> &mut Command { |
There was a problem hiding this comment.
What is the motivation behind the reimplementation of arg and other methods defined for TokioCommand rather than creating a single new constructor that accepts already constructed TokioCommand and run to run the command handling the result?
There was a problem hiding this comment.
I pushed a commit with your suggestion. It simplifies command.rs, but makes the consumer code a bit more verbose.
| "Nix activation script".to_string() | ||
| } | ||
| } | ||
|
|
||
| #[derive(Error, Debug)] | ||
| pub enum ActivateError { |
There was a problem hiding this comment.
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.
Is there a reason why the run method of command::Command is not used here instead of wait_with_output?
There was a problem hiding this comment.
Because ssh_activate_child is a tokio::process::Child and not a command::Command or tokio::process::Command. Although there are patterns, the way the codebase handles children is not repetitive enough to easily abstract them into a run command for a new command::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: