Skip to content
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

Proper exit codes #127

Closed
somehowchris opened this issue Apr 30, 2022 · 3 comments · Fixed by #151
Closed

Proper exit codes #127

somehowchris opened this issue Apr 30, 2022 · 3 comments · Fixed by #151

Comments

@somehowchris
Copy link
Contributor

somehowchris commented Apr 30, 2022

As mentioned in #113 and #116 many statments in main.rs after an if clause to determine if it should abort use return Ok(()); which ends the process but exits with the exit code 0. This makes it hard for scripts like Dockerfile or github actions to abort if cargo binstall couldn't find a correct version.

Changing this to return Err(anyhow::anyhow!("Installation cancelled")); would be a easy thing to do but in case of the provided example this would output the following thing:

gitpod /workspace/cargo-binstall (fix/package-version-abort-logic) $ cargo binstall cargo-watch --version 8.1.0 --no-confirm
09:50:31 [INFO] Installing package: 'cargo-watch'
09:50:32 [WARN] You specified `--version 8.1.0` but the package resolved that to '8.1.1', use `=8.1.0` if you want an exact match
09:50:32 [WARN] Installation cancelled
Error: Installation cancelled

This means once the message via a warn!, nicely formatted and once plain as it is returned from main() -> Result<(), anyhow::Error>.

My 2 cents about a solution:

  • It would be awesome to be able to format the output of anyhow, but I haven't found a solution yet
  • Aiming for developers, this might be a no brainer as humans can read, but aiming for ci solutions, exit codes are critical

I appreciate comments and opinions, as I currently do not have a nice solution to propose

@somehowchris
Copy link
Contributor Author

somehowchris commented May 1, 2022

Would one be to have an Errors enum something along the lines like reqwest, so its actually typed, debuggable.

I could imagine cargo-binstall also being useful for other clis, so expsing a crate api would be cool (and just a random though), so typed errors would also be useful.

Using this approach, the errors then could be "catched" by some formatting logic to output it nicely formatted and exit the cli with a non 0 exit code. This would also mean that the main function would be main() {} and handle all the things inside

@somehowchris
Copy link
Contributor Author

@passcod
Copy link
Member

passcod commented May 2, 2022

I think specifically there's two different things here to think about:

  • a human aborting the process by saying "no" to a prompt is a correct, expected end to the process and should return 0
    • in the case of a dry run, if the options don't imply a script use, I think that's still an expected end. You shouldn't really do a dry-run in a script, I think?
  • the process exiting on its own in CI without installing the package is unexpected and should exit with an error code

So I think the first concrete improvement here will be to split those two scenarios and exit differently. There's really two options I see here:

  • continue to return Ok(()) after printing a message for the former and return an error otherwise
  • make up a special error somehow (not sure how to specialise like that in anyhow, might need some figuring out or wrapping) that has (initially) two variants: UserAbort and Failure (or whatever naming), and do our own result handling in main instead of returning an error so we exit UserAbort gracefully and print a pretty error with Failure.

I prefer the second because it's cleaner and clearer at the type level but it may be a bit more work.

Once we've got that going and well separated, we can look into adding:

  • more detail to errors
  • different error codes for different errors (I'm sure there's a standard somewhere we can take inspiration from)
  • this from the mega thread: "(thinking about feedback, we could have a if this doesn't work for you, please open an issue with the following information <link> in the help or something to help point people back here when things go wrong?)"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants