fix: stop deleting $CARGO_HOME during uninstall

src/cli/self_update.rs

@@ -24,15 +24,14 @@
 //! During uninstall (`rustup self uninstall`):
 //!
 //! * Delete `$RUSTUP_HOME`.
-//! * Delete everything in `$CARGO_HOME`, including
-//!   the rustup binary and its hardlinks
+//! * Delete rustup tool proxy binaries from `$CARGO_HOME`/bin.
+//! * Delete `$CARGO_HOME`/bin if it is empty after uninstall.
 //!
 //! Deleting the running binary during uninstall is tricky
 //! and racy on Windows.
 
 use std::borrow::Cow;
 use std::env::{self, consts::EXE_SUFFIX};
-#[cfg(not(windows))]
 use std::io;
 use std::io::Write;
 use std::path::{Component, MAIN_SEPARATOR, Path, PathBuf};
@@ -47,7 +46,7 @@ use clap::ValueEnum;
 use clap::builder::PossibleValue;
 use clap_cargo::style::{GOOD, WARN};
 use itertools::Itertools;
-use same_file::Handle;
+use same_file::{Handle, is_same_file};
 use serde::{Deserialize, Serialize};
 use tracing::{error, info, trace, warn};
 
@@ -80,7 +79,7 @@ mod shell;
 #[cfg(unix)]
 mod unix;
 #[cfg(unix)]
-use unix::{delete_rustup_and_cargo_home, do_add_to_path, do_remove_from_path};
+use unix::{do_add_to_path, do_remove_from_path};
 #[cfg(unix)]
 pub(crate) use unix::{run_update, self_replace};
 
@@ -91,7 +90,7 @@ pub use windows::complete_windows_uninstall;
 #[cfg(all(windows, feature = "test"))]
 pub use windows::{RegistryGuard, RegistryValueId, USER_PATH, get_path};
 #[cfg(windows)]
-use windows::{delete_rustup_and_cargo_home, do_add_to_path, do_remove_from_path};
+use windows::{do_add_to_path, do_remove_from_path};
 #[cfg(windows)]
 pub(crate) use windows::{run_update, self_replace};
 
@@ -1048,6 +1047,12 @@ async fn maybe_install_rust(opts: InstallOpts<'_>, cfg: &mut Cfg<'_>) -> Result<
     Ok(())
 }
 
+/// Uninstall process:
+/// 1. Remove rustup home.
+/// 2. Remove rustup tool links.
+/// 3. Remove rustup binary.
+/// 3. Try to remove $CARGO_HOME/bin directory if it's empty.
+/// 4. Upon successfully removing $CARGO_HOME/bin, clean up $PATH.
 pub(crate) fn uninstall(
     no_prompt: bool,
     no_modify_path: bool,
@@ -1061,7 +1066,8 @@ pub(crate) fn uninstall(
 
     let cargo_home = process.cargo_home()?;
 
-    if !cargo_home.join(format!("bin/rustup{EXE_SUFFIX}")).exists() {
+    let rustup_path = cargo_home.join(format!("bin/rustup{EXE_SUFFIX}"));
+    if !rustup_path.exists() {
         return Err(CliError::NotSelfInstalled { p: cargo_home }.into());
     }
 
@@ -1092,11 +1098,6 @@ pub(crate) fn uninstall(
 
     info!("removing cargo home");
 
-    // Remove CARGO_HOME/bin from PATH
-    if !no_modify_path {
-        do_remove_from_path(process)?;
-    }
-
     // Delete everything in CARGO_HOME *except* the rustup bin
 
     // First everything except the bin directory
@@ -1148,16 +1149,64 @@ pub(crate) fn uninstall(
 
     info!("removing rustup binaries");
 
-    // Delete rustup. This is tricky because this is *probably*
+    // Delete rustup.
+    #[cfg(unix)]
+    clean_cargo_bin(no_modify_path, process)?;
+    // NOTE: On windows, This is tricky because this is *probably*
     // the running executable and on Windows can't be unlinked until
     // the process exits.
-    delete_rustup_and_cargo_home(process)?;
+    // see: windows::{complete_windows_uninstall,clean_cargo_bin}
+    #[cfg(windows)]
+    windows::clean_cargo_bin(no_modify_path, process)?;
 
     info!("rustup is uninstalled");
-
     Ok(ExitCode::SUCCESS)
 }
 
+/// Remove rustup tool links and executable, then remove `$CARGO_HOME/bin` directory if empty.
+/// On success, remove it from `$PATH` unless `no_modify_path` is set.
+/// If the directory is not empty, emit a warning and return success.
+fn clean_cargo_bin(no_modify_path: bool, process: &Process) -> Result<()> {
+    let cargo_bin_path = process.cargo_home()?.join("bin");
+    let rustup_path = cargo_bin_path.join(format!("rustup{EXE_SUFFIX}"));
+
+    // Remove rustup tool links
+    let proxy_paths = TOOLS
+        .iter()
+        .chain(DUP_TOOLS.iter())
+        .map(|tool| cargo_bin_path.join(format!("{tool}{EXE_SUFFIX}")));
+    for proxy_path in proxy_paths {
+        if is_same_file(&proxy_path, &rustup_path).unwrap_or(false) {
+            utils::remove_file("rustup tool proxy", &proxy_path)?;
+        }
+    }
+
+    // Remove rustup binary
+    utils::remove_file("rustup_bin", &rustup_path)?;
+
+    // Remove $CARGO_HOME/bin
+    let cargo_bin_path_display = cargo_bin_path.display();
+    info!("removing empty cargo bin directory `{cargo_bin_path_display}`");
+
+    let Err(e) = fs::remove_dir(&cargo_bin_path) else {
+        if !no_modify_path {
+            info!("removing cargo bin directory `{cargo_bin_path_display}` from $PATH");
+            do_remove_from_path(process)?;
+        }
+
+        return Ok(());
+    };
+
+    if e.kind() == io::ErrorKind::DirectoryNotEmpty {
+        warn!("keeping non-empty cargo bin directory `{cargo_bin_path_display}`");
+
+        return Ok(());
+    }
+
+    Err(e)
+        .with_context(|| format!("failed to remove cargo bin directory `{cargo_bin_path_display}`"))
+}
+
 #[derive(Clone, Copy, Debug)]
 pub(crate) enum SelfUpdatePermission {
     HardFail,

src/cli/self_update/unix.rs

@@ -47,11 +47,6 @@ pub(crate) fn do_anti_sudo_check(no_prompt: bool, process: &Process) -> Result<u
     Ok(utils::ExitCode(0))
 }
 
-pub(crate) fn delete_rustup_and_cargo_home(process: &Process) -> Result<()> {
-    let cargo_home = process.cargo_home()?;
-    utils::remove_dir("cargo_home", &cargo_home)
-}
-
 pub(crate) fn do_remove_from_path(process: &Process) -> Result<()> {
     for sh in shell::get_available_shells(process) {
         let source_bytes = format!("{}\n", sh.source_string(process)?).into_bytes();

src/cli/self_update/windows.rs

@@ -348,16 +348,18 @@ fn has_windows_sdk_libs(process: &Process) -> bool {
     false
 }
 
-/// Run by rustup-gc-$num.exe to delete CARGO_HOME
+/// Run by rustup-gc-$num.exe to delete rustup binary
 #[tracing::instrument(level = "trace")]
 pub fn complete_windows_uninstall(process: &Process) -> Result<utils::ExitCode> {
     use std::process::Stdio;
 
     wait_for_parent()?;
 
-    // Now that the parent has exited there are hopefully no more files open in CARGO_HOME
-    let cargo_home = process.cargo_home()?;
-    utils::remove_dir("cargo_home", &cargo_home)?;
+    let no_modify_path = process.var_os(GC_MODIFY_PATH).as_deref() != Some(OsStr::new("1"));
+
+    // Clean up CARGO_HOME/bin if it's empty now
+    // On success, also remove it from $PATH.
+    super::clean_cargo_bin(no_modify_path, process)?;
 
     // Now, run a *system* binary to inherit the DELETE_ON_CLOSE
     // handle to *this* process, then exit. The OS will delete the gc
@@ -644,9 +646,9 @@ pub(crate) fn self_replace(process: &Process) -> Result<utils::ExitCode> {
     Ok(utils::ExitCode(0))
 }
 
-// The last step of uninstallation is to delete *this binary*,
-// rustup.exe and the CARGO_HOME that contains it. On Unix, this
-// works fine. On Windows you can't delete files while they are open,
+// The last step of uninstallation is to delete *this binary*, rustup.exe.
+// On Unix, this works fine.
+// On Windows you can't delete files while they are open,
 // like when they are running.
 //
 // Here's what we're going to do:
@@ -659,7 +661,7 @@ pub(crate) fn self_replace(process: &Process) -> Result<utils::ExitCode> {
 //   processes created with the option to inherit handles
 //   will also keep them open.
 // - Run the gc exe, which waits for the original rustup.exe
-//   process to close, then deletes CARGO_HOME. This process
+//   process to close, then deletes rustup.exe. This process
 //   has inherited a FILE_FLAG_DELETE_ON_CLOSE handle to itself.
 // - Finally, spawn yet another system binary with the inherit handles
 //   flag, so *it* inherits the FILE_FLAG_DELETE_ON_CLOSE handle to
@@ -674,7 +676,7 @@ pub(crate) fn self_replace(process: &Process) -> Result<utils::ExitCode> {
 //
 // .. augmented with this SO answer
 // https://stackoverflow.com/questions/10319526/understanding-a-self-deleting-program-in-c
-pub(crate) fn delete_rustup_and_cargo_home(process: &Process) -> Result<()> {
+pub(crate) fn clean_cargo_bin(no_modify_path: bool, process: &Process) -> Result<()> {
     use std::io;
     use std::ptr;
     use std::thread;
@@ -734,6 +736,7 @@ pub(crate) fn delete_rustup_and_cargo_home(process: &Process) -> Result<()> {
     };
 
     Command::new(gc_exe)
+        .env(GC_MODIFY_PATH, if no_modify_path { "0" } else { "1" })
         .spawn()
         .context(CliError::WindowsUninstallMadness)?;
 
@@ -749,6 +752,10 @@ pub(crate) fn delete_rustup_and_cargo_home(process: &Process) -> Result<()> {
     Ok(())
 }
 
+// The rustup-gc executable cannot accept normal function call here,
+// so we use env var here, notifying it if we need to remove $CARGO_HOME/bin from $PATH
+const GC_MODIFY_PATH: &str = "RUSTUP_GC_MODIFY_PATH";
+
 #[cfg(any(test, feature = "test"))]
 pub fn get_path() -> Result<Option<Value>> {
     USER_PATH.get()

tests/suite/cli_self_upd.rs

@@ -259,13 +259,118 @@ async fn uninstall_works_if_rustup_home_doesnt_exist() {
 }
 
 #[tokio::test]
-async fn uninstall_deletes_cargo_home() {
+async fn uninstall_keeps_cargo_home() {
     let cx = setup_empty_installed().await;
     cx.config
         .expect(["rustup", "self", "uninstall", "-y"])
         .await
         .is_ok();
-    assert!(!cx.config.cargodir.exists());
+    assert!(cx.config.cargodir.exists());
+}
+
+#[tokio::test]
+async fn uninstall_removes_empty_cargo_bin() {
+    let cx = setup_empty_installed().await;
+    cx.config
+        .expect(["rustup", "self", "uninstall", "-y"])
+        .await
+        .is_ok();
+    assert!(!cx.config.cargodir.join("bin").exists());
+}
+
+#[tokio::test]
+async fn uninstall_keeps_non_empty_cargo_bin() {
+    let cx = setup_empty_installed().await;
+    let cargo_bin = cx.config.cargodir.join("bin");
+
+    let mock_file = cargo_bin.join(".DS_Store");
+    fs::write(&mock_file, "").unwrap();
+
+    cx.config
+        .expect(["rustup", "self", "uninstall", "-y"])
+        .await
+        .with_stderr(snapbox::str![[r#"
+...
+warn: keeping non-empty cargo bin directory `[..]`
+...
+"#]])
+        .is_ok();
+    assert!(cargo_bin.exists());
+    assert!(mock_file.exists());
+}
+
+#[cfg(not(windows))]
+#[tokio::test]
+async fn uninstall_removes_path_only_when_bin_removed() {
+    async fn install(cx: &CliTestContext) {
+        cx.config
+            .expect(["rustup-init", "-y", "--default-toolchain", "none"])
+            .await
+            .is_ok();
+    }
+
+    fn source_line(cx: &CliTestContext) -> String {
+        format!(
+            r#". "{}/env"
+"#,
+            cx.config.cargodir.display()
+        )
+    }
+
+    let cx = CliTestContext::new(Scenario::Empty).await;
+    install(&cx).await;
+    let profile = cx.config.homedir.join(".profile");
+    assert!(
+        fs::read_to_string(&profile)
+            .unwrap()
+            .contains(&source_line(&cx))
+    );
+    cx.config
+        .expect(["rustup", "self", "uninstall", "-y"])
+        .await
+        .is_ok();
+    assert!(!cx.config.cargodir.join("bin").exists());
+    assert!(
+        !fs::read_to_string(&profile)
+            .unwrap()
+            .contains(&source_line(&cx))
+    );
+
+    let cx = CliTestContext::new(Scenario::Empty).await;
+    install(&cx).await;
+    let cargo_bin = cx.config.cargodir.join("bin");
+    let profile = cx.config.homedir.join(".profile");
+    fs::write(cargo_bin.join("custom-tool"), "").unwrap();
+    cx.config
+        .expect(["rustup", "self", "uninstall", "-y"])
+        .await
+        .is_ok();
+    assert!(cargo_bin.exists());
+    assert!(
+        fs::read_to_string(&profile)
+            .unwrap()
+            .contains(&source_line(&cx))
+    );
+}
+
+#[cfg(windows)]
+#[tokio::test]
+async fn windows_complete_uninstall_removes_empty_cargo_bin() {
+    let cx = setup_empty_installed().await;
+    let cargo_bin = cx.config.cargodir.join("bin");
+    cx.config
+        .expect(["rustup", "self", "uninstall", "-y"])
+        .await
+        .is_ok();
+
+    let check = || {
+        if cargo_bin.exists() {
+            Err(format!("cargo bin still exists: {}", cargo_bin.display()))
+        } else {
+            Ok(())
+        }
+    };
+    retry(Fibonacci::from_millis(1).map(jitter).take(23), check).unwrap()
 }
 
 #[tokio::test]
@@ -300,7 +405,7 @@ async fn uninstall_self_delete_works() {
 
     assert!(out.status.success());
     assert!(!rustup.exists());
-    assert!(!cx.config.cargodir.exists());
+    assert!(cx.config.cargodir.exists());
 
     let rustc = cx.config.cargodir.join(format!("bin/rustc{EXE_SUFFIX}"));
     let rustdoc = cx.config.cargodir.join(format!("bin/rustdoc{EXE_SUFFIX}"));
@@ -347,7 +452,15 @@ async fn uninstall_doesnt_leave_gc_file() {
 fn ensure_empty(dir: &Path) -> Result<(), GcErr> {
     let garbage = fs::read_dir(dir)
         .unwrap()
-        .map(|d| d.unwrap().path().to_string_lossy().to_string())
+        .filter_map(|entry| {
+            let path = entry.unwrap().path();
+            let name = path.file_name()?.to_str()?;
+            // On Windows, this binary is cleaned up on exit
+            if !(name.starts_with("rustup-gc-") && name.ends_with(EXE_SUFFIX)) {
+                return None;
+            }
+            Some(path.to_string_lossy().to_string())
+        })
         .collect::<Vec<_>>();
     match garbage.len() {
         0 => Ok(()),