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

Exit status incorrect when output reporting error #180

Closed
yanganto opened this issue Apr 29, 2024 · 5 comments
Closed

Exit status incorrect when output reporting error #180

yanganto opened this issue Apr 29, 2024 · 5 comments
Assignees
Milestone

Comments

@yanganto
Copy link
Contributor

yanganto commented Apr 29, 2024

moreover, I noticed the exit status of the command is 0 even if the reported error gets output and I think this should be fixed

Originally posted by @nicbus in #173 (comment)

@dr-orlovsky dr-orlovsky added this to the v0.11.0 milestone May 1, 2024
@yanganto
Copy link
Contributor Author

The exec of commands, pay function of WalletProvider return errors.
The current design is really good without bugs, this issue is also possibly out of the scope of this crate, in there we swallow an error and just print it out.

@nicbus
Copy link
Contributor

nicbus commented May 28, 2024

The exec of commands, pay function of WalletProvider return errors.
The current design is really good without bugs, this issue is also possibly out of the scope of this crate, in there we swallow an error and just print it out.

I'm having a hard time understanding what you mean, so I'll explain what I think is the issue in more detail.

In my opinion the issue here is that when a non-recoverable error is encountered, the command exits without completing the task it was supposed to, but exits with a zero status code, which means everything went well. This is a problem because a CLI user (or pipeline, or script) then assumes that the command worked and produced the expected results, while this is not the case and further operations down the line will fail because of this.

IMO it's important to report that the command did not produce the expected result by exiting with a non-zero status code, which can be handled by the user (e.g. by checking $?) or automatically (e.g. setting the shell's -e option).

As a more concrete example, in the original issue the command to create a wallet was the one failing with an error, which means the intended result of the command (creating a wallet) had not been achieved, which is why I think the exit status should have been non-zero. With a zero exit code, the user (or script) continues with the following commands (e.g. trying to generate an address) but they will fail because the wallet is not there.

@yanganto
Copy link
Contributor Author

yanganto commented Jun 3, 2024

The $? thing is shell operation, I know what you mean. We do not have a shell (.sh) in the repo.

This happens when $rgb something and an error occurs with successful exit status. This is about the rust program and not about the shell. Did I misunderstand something?

The main.rs returns an error code if the error is raised.
https://github.com/RGB-WG/rgb/blob/76a227730abb710cdd02f09207c2b36c64d938f6/cli/src/main.rs#L47C1-L47C26

If there is an error report with a successful exit status, it means the system swallows an error somewhere, because Result is a primary type and can be used for Rust function return, if some error happens and is swallowed it will not pass to the return.

@nicbus
Copy link
Contributor

nicbus commented Jun 3, 2024

The main.rs returns an error code if the error is raised.
https://github.com/RGB-WG/rgb/blob/76a227730abb710cdd02f09207c2b36c64d938f6/cli/src/main.rs#L47C1-L47C26

If there is an error report with a successful exit status, it means the system swallows an error somewhere, because Result is a primary type and can be used for Rust function return, if some error happens and is swallowed it will not pass to the return.

You're right, the exit code was being masked by the fact that some rgb commands were executed inside a subshell or pipe. I didn't spot that difference initially, sorry for that, this can be closed.

@yanganto
Copy link
Contributor Author

yanganto commented Jun 4, 2024

@nicbus Thanks for your confirm. If this issue still happens please kindly reopen this issue, we can dig into this more and make it better.

@yanganto yanganto closed this as completed Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

3 participants