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

fix(rustup-mode): return ExitCode(1) when update() fails #3952

Merged
merged 3 commits into from
Jul 16, 2024

Conversation

rami3l
Copy link
Member

@rami3l rami3l commented Jul 16, 2024

Closes #3476, verified via local testing.

@rami3l rami3l requested a review from djc July 16, 2024 11:55
src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems okay. Personally I feel like use of the custom ExitCode is a bad abstraction. One idea I've played with is that we should change types to be Result<T, Option<E>>, where the Some represents an error that should be yielded by the top-level caller while None means the error has already been reported. (We then report ExitCode::FAILURE if the Result is Err.)

src/utils/utils.rs Outdated Show resolved Hide resolved
@rami3l
Copy link
Member Author

rami3l commented Jul 16, 2024

This seems okay. Personally I feel like use of the custom ExitCode is a bad abstraction. One idea I've played with is that we should change types to be Result<T, Option<E>>, where the Some represents an error that should be yielded by the top-level caller while None means the error has already been reported. (We then report ExitCode::FAILURE if the Result is Err.)

@djc Indeed, in other projects that I've worked with my principle is that we should never expose the exit code with the single exception of the main module, where some thiserror is converted to ExitStatus via an impl Termination.

But that kind of change is clearly out of the scope of this PR...

@rami3l rami3l force-pushed the fix/weird-exit-code branch 3 times, most recently from 5d54796 to cecc7e0 Compare July 16, 2024 12:33
@rami3l rami3l requested a review from djc July 16, 2024 12:38
src/utils/utils.rs Show resolved Hide resolved
@rami3l rami3l force-pushed the fix/weird-exit-code branch from cecc7e0 to ad271f1 Compare July 16, 2024 13:08
@rami3l rami3l enabled auto-merge July 16, 2024 13:11
@rami3l rami3l added this pull request to the merge queue Jul 16, 2024
Merged via the queue into rust-lang:master with commit 3fa1790 Jul 16, 2024
27 checks passed
@rami3l rami3l deleted the fix/weird-exit-code branch July 16, 2024 13:46
@rami3l rami3l modified the milestone: 1.28.0 Jul 17, 2024
@rami3l rami3l mentioned this pull request Oct 6, 2024
3 tasks
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 this pull request may close these issues.

When rustup update fails, it doesn't return an exit code that indicate failure
2 participants