Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
+22 −19
Closed

refactor(cli/rustup-mode): merge self-update checking logic in rustup update #4562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d6b6a2
refactor(cli/self-update): extract `CFG_SELF_UPDATE` predicate
rami3l Oct 24, 2025
1f0666d
refactor(cli/rustup-mode): merge self-update checking logic in `rustu…
rami3l Oct 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions src/cli/rustup_mode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ use crate::{
show_help, toolchain_help, toolchain_install_help, toolchain_link_help, topic_arg_help,
update_help,
},
self_update::{self, SelfUpdateMode, check_rustup_update},
self_update::{self, CFG_SELF_UPDATE, SelfUpdateMode, check_rustup_update},
topical_doc,
},
command, component_for_bin,
Expand Down Expand Up @@ -916,9 +916,8 @@ async fn check_updates(cfg: &Cfg<'_>, opts: CheckOpts) -> Result<ExitCode> {
// Check for update only if rustup does **not** have the no-self-update feature,
// and auto-self-update is configured to **enable**
// and has **no** no-self-update parameter.
let self_update = !cfg!(feature = "no-self-update")
&& self_update_mode == SelfUpdateMode::Enable
&& !opts.no_self_update;
let self_update =
CFG_SELF_UPDATE && self_update_mode == SelfUpdateMode::Enable && !opts.no_self_update;

if self_update && check_rustup_update(&DownloadCfg::new(cfg)).await? {
update_available = true;
Expand All @@ -941,9 +940,8 @@ async fn update(
// Update only if rustup does **not** have the no-self-update feature,
// and auto-self-update is configured to **enable**
// and has **no** no-self-update parameter.
let self_update = !cfg!(feature = "no-self-update")
&& self_update_mode == SelfUpdateMode::Enable
&& !opts.no_self_update;
let self_update =
CFG_SELF_UPDATE && self_update_mode == SelfUpdateMode::Enable && !opts.no_self_update;
let force_non_host = opts.force_non_host;
cfg.profile_override = opts.profile;

Expand Down Expand Up @@ -1023,13 +1021,15 @@ async fn update(
cfg.tmp_cx.clean();
}

if !cfg!(feature = "no-self-update") && self_update_mode == SelfUpdateMode::CheckOnly {
check_rustup_update(&dl_cfg).await?;
}

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")
match self_update_mode {
_ if !CFG_SELF_UPDATE => {
Copy link
Contributor

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.

Copy link
Member Author

@rami3l rami3l Oct 25, 2025 •
edited
Loading

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...

Copy link
Member Author

@rami3l rami3l Oct 25, 2025 •
edited
Loading

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?

Copy link
Member Author

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.

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");
}
SelfUpdateMode::CheckOnly => {
check_rustup_update(&dl_cfg).await?;
}
_ => (),
}

Ok(exit_code)
Expand Down Expand Up @@ -1828,7 +1828,7 @@ fn set_auto_self_update(
cfg: &mut Cfg<'_>,
auto_self_update_mode: SelfUpdateMode,
) -> Result<ExitCode> {
if cfg!(feature = "no-self-update") {
if !CFG_SELF_UPDATE {
let mut args = cfg.process.args_os();
let arg0 = args.next().map(PathBuf::from);
let arg0 = arg0
Expand Down
11 changes: 7 additions & 4 deletions src/cli/self_update.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -979,12 +979,15 @@ async fn maybe_install_rust(opts: InstallOpts<'_>, cfg: &mut Cfg<'_>) -> Result<
Ok(())
}

/// Whether the self-update functionality is enabled during compilation.
pub(crate) const CFG_SELF_UPDATE: bool = !cfg!(feature = "no-self-update");
Copy link
Contributor

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.

Copy link
Member Author

@rami3l rami3l Oct 25, 2025 •
edited
Loading

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.


pub(crate) fn uninstall(
no_prompt: bool,
no_modify_path: bool,
process: &Process,
) -> Result<utils::ExitCode> {
if cfg!(feature = "no-self-update") {
if !CFG_SELF_UPDATE {
error!("self-uninstall is disabled for this build of rustup");
error!("you should probably use your system package manager to uninstall rustup");
return Ok(utils::ExitCode(1));
Expand Down Expand Up @@ -1169,10 +1172,10 @@ pub(crate) async fn update(cfg: &Cfg<'_>) -> Result<utils::ExitCode> {
common::warn_if_host_is_emulated(cfg.process);

use SelfUpdatePermission::*;
let update_permitted = if cfg!(feature = "no-self-update") {
HardFail
} else {
let update_permitted = if CFG_SELF_UPDATE {
self_update_permitted(true)?
} else {
HardFail
};
match update_permitted {
HardFail => {
Expand Down