-
Notifications
You must be signed in to change notification settings - Fork 984
refactor(cli/rustup-mode): merge self-update checking logic in rustup update
#4562
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
Conversation
a098dc9 to
fd92da2
Compare
rustup checkrustup update
fd92da2 to
1f0666d
Compare
| } | ||
|
|
||
| /// Whether the self-update functionality is enabled during compilation. | ||
| pub(crate) const CFG_SELF_UPDATE: bool = !cfg!(feature = "no-self-update"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is redoing something that I undid only recently. I don't see much value in it -- it just obscures the original meaning and saves a few characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@djc I still think the double negation here in !cfg!("no-*") is a bit confusing, but you can totally have different opinions.
| info!("self-update is disabled for this build of rustup"); | ||
| info!("any updates to rustup will need to be fetched with your system package manager") | ||
| match self_update_mode { | ||
| _ if !CFG_SELF_UPDATE => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the reason, I think you can cfg!() instead of an attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not having explained the full background until now, but when looking at #4565 I realized that the self-update cleanup code is very curiously written in two parts, so I want to merge them together first to make the overall logic clearer.
However, since this change is not directly related to #4565, I planned on making two separate PRs...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@djc I'm not entirely sure about your proposals, but maybe this could be simplified to just the following?
if cfg!(feature = "no-self-update") {
info!("self-update is disabled for this build of rustup");
info!("any updates to rustup will need to be fetched with your system package manager")
} else if self_update_mode == SelfUpdateMode::CheckOnly {
check_rustup_update(&dl_cfg).await?;
}... the idea is that this block covers all the cases where self_update() is not executed, and when I add the extra if self_update block in a second PR, it'll be clearer when each case is handled.
Or you might have different opinions regarding the fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made #4566 to hopefully make my original intentions clearer.
|
Closing in favor of #4566. |
No description provided.