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

Install script does not exit with non-zero status code on failure. #2439

Closed
eonil opened this issue Jul 25, 2020 · 5 comments
Closed

Install script does not exit with non-zero status code on failure. #2439

eonil opened this issue Jul 25, 2020 · 5 comments

Comments

@eonil
Copy link

eonil commented Jul 25, 2020

Describe the problem you are trying to solve
The install script invoked by this command curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh does not exit with non-zero status code on failure. It's hard to detect failure partial success or complete success at this point. This makes putting this command in another automation harder.

Describe the solution you'd like
Install script exits with non-zero status code on any error.

Notes
I'm using the script in automated mode like this.

curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- --default-toolchain none --no-modify-path -y; exit $?

This always exits with 0. I tested this with no network connection and script fails with curl: (6) Could not resolve host: sh.rustup.rs error but exit code is still 0.

@kinnison
Copy link
Contributor

Oh my this is not good IMO. I think this is a bug rather than an enhancement request.

@eonil
Copy link
Author

eonil commented Jul 29, 2020

IMO, if we run rustup at the end, and if it fails, we can report its exit code as final exit code. But can we regard just an execution of rustup as successful installation?

@kinnison
Copy link
Contributor

The issue as described is that our exit pathway for no network access is failing to return the error code properly. It's possible to install rustup and then not have it on-path and for that to be deliberate so we can't just try to execvp rustup afterwards.

@danpls
Copy link

danpls commented Sep 2, 2020

Actually the install script does return a non-zero status code if it fails.
We can easily test this by unsetting the PATH environment variable before running the script, forcing it to not find any of the binaries needed:

> (echo "unset PATH"; curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs) | sh; echo $?
rustup: need 'curl or wget' (command not found)
1

The cause of OP's problem was, that he ran the command without a network connection, so curl failed to even get the script from https://sh.rustup.rs, and thus exited with an error code having written nothing to stdin.
And at the other end of the pipe sh notices that stdin was closed, thus exiting with the status code of the previous command or in this case 0 as there was no previous command.

Fixing this problem is actually quite simple. We can detect if curl fails to retrieve the install script and instead write something to stdin which forces sh to exit with an error code.
E.g.:

(curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs || echo exit 1) | sh

So I am not sure if this still is an issue we need to fix, but if you do think so I'd be happy to replace all occurrences of the old command with this "fixed" one and submit a PR.

@kinnison
Copy link
Contributor

kinnison commented Sep 3, 2020

Nicely caught. My gut feeling here is that there's no great value in changing the recommended command to be even more complex. People already whine from time to time about the extra arguments to curl, let alone if we introduced a subshell and so on.

I'm going to close this and hope the issue stands as something people will find if they accidentally hit a similar situation.

@kinnison kinnison closed this as completed Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants