fix: stop remove $CARGO_HOME on uninstallation

src/cli/self_update.rs

@@ -32,8 +32,6 @@
 
 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};
 use std::process::Command;
@@ -80,7 +78,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, remove_rustup_executable};
 #[cfg(unix)]
 pub(crate) use unix::{run_update, self_replace};
 
@@ -91,7 +89,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, remove_rustup_executable};
 #[cfg(windows)]
 pub(crate) use windows::{run_update, self_replace};
 
@@ -1048,6 +1046,11 @@ async fn maybe_install_rust(opts: InstallOpts<'_>, cfg: &mut Cfg<'_>) -> Result<
     Ok(())
 }
 
+// Uninstall process:
+// 1. remove rustup home
+// 2. clean up rustup tool proxys
+// 3. remove rustup binary file
+// 4. remove $CARGO_HOME/bin if it is empty
 pub(crate) fn uninstall(
     no_prompt: bool,
     no_modify_path: bool,
@@ -1061,7 +1064,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());
     }
 
@@ -1090,71 +1094,30 @@ pub(crate) fn uninstall(
         utils::remove_dir("rustup_home", &rustup_dir)?;
     }
 
-    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
-    let diriter = fs::read_dir(&cargo_home).map_err(|e| CliError::ReadDirError {
-        p: cargo_home.clone(),
-        source: e,
-    })?;
-    for dirent in diriter {
-        let dirent = dirent.map_err(|e| CliError::ReadDirError {
-            p: cargo_home.clone(),
-            source: e,
-        })?;
-        if dirent.file_name().to_str() != Some("bin") {
-            if dirent.path().is_dir() {
-                utils::remove_dir("cargo_home", &dirent.path())?;
-            } else {
-                utils::remove_file("cargo_home", &dirent.path())?;
-            }
-        }
-    }
-
-    // Then everything in bin except rustup and tools. These can't be unlinked
-    // until this process exits (on windows).
-    let tools = TOOLS
+    // Clean up rustup tool links
+    let cargo_bin_path = cargo_home.join("bin");
+    let proxy_paths = TOOLS
         .iter()
         .chain(DUP_TOOLS.iter())
-        .map(|t| format!("{t}{EXE_SUFFIX}"));
-    let tools: Vec<_> = tools.chain(vec![format!("rustup{EXE_SUFFIX}")]).collect();
-    let bin_dir = cargo_home.join("bin");
-    let diriter = fs::read_dir(&bin_dir).map_err(|e| CliError::ReadDirError {
-        p: bin_dir.clone(),
-        source: e,
-    })?;
-    for dirent in diriter {
-        let dirent = dirent.map_err(|e| CliError::ReadDirError {
-            p: bin_dir.clone(),
-            source: e,
-        })?;
-        let name = dirent.file_name();
-        let file_is_tool = name.to_str().map(|n| tools.iter().any(|t| *t == n));
-        if file_is_tool == Some(false) {
-            if dirent.path().is_dir() {
-                utils::remove_dir("cargo_home", &dirent.path())?;
-            } else {
-                utils::remove_file("cargo_home", &dirent.path())?;
-            }
+        .map(|tool| cargo_bin_path.join(format!("{tool}{EXE_SUFFIX}")));
+
+    for proxy_path in proxy_paths {
+        if matches!((
+            Handle::from_path(&proxy_path),
+            Handle::from_path(&rustup_path),
+        ), (Ok(proxy_path), Ok(rustup_path)) if proxy_path == rustup_path)
+        {
+            utils::remove_file("rustup tool proxy", &proxy_path)?;
         }
     }
 
-    info!("removing rustup binaries");
-
     // Delete rustup. 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)?;
+    // Also remove the `$CARGO_HOME/bin` dir if it's empty, and clean up the PATH on success
+    remove_rustup_executable(process, no_modify_path)?;
 
     info!("rustup is uninstalled");
-
     Ok(ExitCode::SUCCESS)
 }
 
@@ -1181,7 +1144,7 @@ pub(crate) fn self_update_permitted(explicit: bool) -> Result<SelfUpdatePermissi
         .tempdir_in(current_exe_dir)
     {
         match e.kind() {
-            io::ErrorKind::PermissionDenied => {
+            std::io::ErrorKind::PermissionDenied => {
                 trace!("Skipping self-update because we cannot write to the rustup dir");
                 if explicit {
                     return Ok(SelfUpdatePermission::HardFail);

src/cli/self_update/unix.rs

@@ -1,8 +1,10 @@
+use std::env::consts::EXE_SUFFIX;
+use std::fs;
 use std::path::{Path, PathBuf};
 use std::process::Command;
 
 use anyhow::{Context, Result, bail};
-use tracing::{error, warn};
+use tracing::{error, info, warn};
 
 use super::install_bins;
 use super::shell::{self, Posix, UnixShell};
@@ -47,9 +49,31 @@ 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 remove_rustup_executable(process: &Process, no_modify_path: bool) -> Result<()> {
+    let cargo_bin_path = process.cargo_home()?.join("bin");
+    let rustup_path = cargo_bin_path.join(format!("rustup{EXE_SUFFIX}"));
+
+    utils::remove_file("rustup_bin", &rustup_path)?;
+
+    match fs::remove_dir(&cargo_bin_path) {
+        Ok(()) => {
+            if !no_modify_path {
+                do_remove_from_path(process)?;
+                info!("remove cargo bin from path");
+            }
+
+            info!("removed empty cargo bin");
+        }
+        Err(e) => {
+            let cargo_bin_path = cargo_bin_path.display();
+
+            return Err(e).with_context(|| {
+                format!("failed to remove cargo bin directory `{cargo_bin_path}`")
+            });
+        }
+    }
+
+    Ok(())
 }
 
 pub(crate) fn do_remove_from_path(process: &Process) -> Result<()> {

src/cli/self_update/windows.rs

@@ -1,6 +1,7 @@
 use std::env::{consts::EXE_SUFFIX, split_paths};
 use std::ffi::{OsStr, OsString};
 use std::fmt;
+use std::fs;
 use std::io::Write;
 use std::os::windows::ffi::OsStrExt;
 use std::path::Path;
@@ -26,6 +27,8 @@ use crate::download::DownloadOptions;
 use crate::process::{ColorableTerminal, Process};
 use crate::utils;
 
+const GC_NO_MODIFY_PATH: &str = "RUSTUP_GC_NO_MODIFY_PATH";
+
 pub(crate) fn ensure_prompt(process: &Process) -> Result<()> {
     writeln!(process.stdout().lock(),)?;
     writeln!(process.stdout().lock(), "Press the Enter key to continue.")?;
@@ -347,17 +350,21 @@ 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 bianry
 #[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)?;
+    // Now that the parent has exited there are hopefully not opened
+    let cargo_home_dir = process.cargo_home()?;
+    let rustup_bin = cargo_home_dir.join(format!("bin/rustup{EXE_SUFFIX}"));
+    utils::remove_file("rustup_bin", &rustup_bin)?;
+
+    // Best effort remove for cargo bin
+    let no_modify_path = process.var_os(GC_NO_MODIFY_PATH).as_deref() == Some(OsStr::new("1"));
+    let cargo_bin_path = cargo_home_dir.join("bin");
 
     // Now, run a *system* binary to inherit the DELETE_ON_CLOSE
     // handle to *this* process, then exit. The OS will delete the gc
@@ -371,6 +378,25 @@ pub fn complete_windows_uninstall(process: &Process) -> Result<utils::ExitCode>
         .spawn()
         .context(CliError::WindowsUninstallMadness)?;
 
+    // clean up PATH and dir
+    match fs::remove_dir(&cargo_bin_path) {
+        Ok(()) => {
+            if !no_modify_path {
+                do_remove_from_path(process)?;
+                info!("remove cargo bin from path");
+            }
+
+            info!("removed empty cargo bin");
+        }
+        Err(e) => {
+            let cargo_bin_path = cargo_bin_path.display();
+
+            return Err(e).with_context(|| {
+                format!("failed to remove cargo bin directory `{cargo_bin_path}`")
+            });
+        }
+    }
+
     Ok(utils::ExitCode(0))
 }
 
@@ -645,7 +671,7 @@ pub(crate) fn self_replace(process: &Process) -> Result<utils::ExitCode> {
 }
 
 // The last step of uninstallation is to delete *this binary*,
-// rustup.exe and the CARGO_HOME that contains it. On Unix, this
+// rustup.exe. On Unix, this
 // works fine. On Windows you can't delete files while they are open,
 // like when they are running.
 //
@@ -674,7 +700,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 remove_rustup_executable(process: &Process, no_modify_path: bool) -> Result<()> {
     use std::io;
     use std::ptr;
     use std::thread;
@@ -734,6 +760,7 @@ pub(crate) fn delete_rustup_and_cargo_home(process: &Process) -> Result<()> {
     };
 
     Command::new(gc_exe)
+        .env(GC_NO_MODIFY_PATH, if no_modify_path { "1" } else { "0" })
         .spawn()
         .context(CliError::WindowsUninstallMadness)?;
 

tests/suite/cli_self_upd.rs

@@ -258,16 +258,6 @@ async fn uninstall_works_if_rustup_home_doesnt_exist() {
         .is_ok();
 }
 
-#[tokio::test]
-async fn uninstall_deletes_cargo_home() {
-    let cx = setup_empty_installed().await;
-    cx.config
-        .expect(["rustup", "self", "uninstall", "-y"])
-        .await
-        .is_ok();
-    assert!(!cx.config.cargodir.exists());
-}
-
 #[tokio::test]
 async fn uninstall_fails_if_not_installed() {
     let cx = setup_empty_installed().await;
@@ -300,7 +290,6 @@ async fn uninstall_self_delete_works() {
 
     assert!(out.status.success());
     assert!(!rustup.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}"));
@@ -337,17 +326,24 @@ async fn uninstall_doesnt_leave_gc_file() {
     // 100ms, but during the contention of test suites can be substantially
     // longer while still succeeding.
 
-    let check = || ensure_empty(parent);
+    let check = || ensure_no_gc_file(parent);
     match retry(Fibonacci::from_millis(1).map(jitter).take(23), check) {
         Ok(_) => (),
         Err(e) => panic!("{e}"),
     }
 }
 
-fn ensure_empty(dir: &Path) -> Result<(), GcErr> {
+fn ensure_no_gc_file(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 is_gc_file = path
+                .file_name()
+                .and_then(|name| name.to_str())
+                .is_some_and(|name| name.starts_with("rustup-gc-") && name.ends_with(EXE_SUFFIX));
+            is_gc_file.then(|| path.to_string_lossy().to_string())
+        })
         .collect::<Vec<_>>();
     match garbage.len() {
         0 => Ok(()),