diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index 27459f5ba3..7e643345c2 100644 --- a/src/cli/self_update.rs +++ b/src/cli/self_update.rs @@ -24,17 +24,15 @@ //! During uninstall (`rustup self uninstall`): //! //! * Delete `$RUSTUP_HOME`. -//! * Delete everything in `$CARGO_HOME`, including -//! the rustup binary and its hardlinks +//! * Delete rustup-managed 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::io::{self, Write}; use std::path::{Component, MAIN_SEPARATOR, Path, PathBuf}; use std::process::Command; use std::str::FromStr; @@ -47,7 +45,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 +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::{cleanup_cargo_bin, do_add_to_path, do_remove_from_path}; #[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::{cleanup_cargo_bin, do_add_to_path, do_remove_from_path}; #[cfg(windows)] pub(crate) use windows::{run_update, self_replace}; @@ -1048,6 +1046,39 @@ async fn maybe_install_rust(opts: InstallOpts<'_>, cfg: &mut Cfg<'_>) -> Result< Ok(()) } +pub(crate) fn do_cargo_bin_clean( + cargo_bin_path: PathBuf, + no_modify_path: bool, + process: &Process, +) -> Result<()> { + let Err(e) = fs::remove_dir(&cargo_bin_path) else { + if !no_modify_path { + do_remove_from_path(process)?; + info!("removed cargo bin from $PATH"); + } + + info!("removed empty cargo bin"); + return Ok(()); + }; + + if e.kind() == io::ErrorKind::DirectoryNotEmpty { + let cargo_bin_path = cargo_bin_path.display(); + warn!("not removing cargo bin `{cargo_bin_path}` because it is not empty"); + + return Ok(()); + } + + let cargo_bin_path = cargo_bin_path.display(); + + Err(e).with_context(|| format!("failed to remove cargo bin directory `{cargo_bin_path}`")) +} + +/// Uninstall process: +/// 1. Remove rustup home. +/// 2. Clean up rustup tool proxies. +/// 3. Remove rustup binary file. +/// 4. Try to clean up $CARGO_HOME/bin if it's empty. +/// 5. Upon successfully removing $CARGO_HOME/bin, clean up $PATH. pub(crate) fn uninstall( no_prompt: bool, no_modify_path: bool, @@ -1061,7 +1092,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 +1122,27 @@ 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 is_same_file(&proxy_path, &rustup_path).unwrap_or(false) { + 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)?; + // Delete rustup. This is tricky because this is *probably* the running executable, + // and on Windows it can't be unlinked until the process exits. + // Also remove `$CARGO_HOME/bin` if it's empty, and clean up PATH on success. + // On Windows, this happens within `windows::complete_windows_uninstall`, after + // the rustup binary is uninstalled. + cleanup_cargo_bin(process, no_modify_path)?; info!("rustup is uninstalled"); - Ok(ExitCode::SUCCESS) } diff --git a/src/cli/self_update/unix.rs b/src/cli/self_update/unix.rs index 05c598f551..1dec2ef681 100644 --- a/src/cli/self_update/unix.rs +++ b/src/cli/self_update/unix.rs @@ -1,3 +1,4 @@ +use std::env::consts::EXE_SUFFIX; use std::path::{Path, PathBuf}; use std::process::Command; @@ -47,9 +48,16 @@ pub(crate) fn do_anti_sudo_check(no_prompt: bool, process: &Process) -> Result Result<()> { - let cargo_home = process.cargo_home()?; - utils::remove_dir("cargo_home", &cargo_home) +pub(crate) fn cleanup_cargo_bin(process: &Process, no_modify_path: bool) -> Result<()> { + use super::do_cargo_bin_clean; + 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)?; + + // Clean up CARGO_HOME/bin, if it's empty now + // On success, remove it from $PATH + do_cargo_bin_clean(cargo_bin_path, no_modify_path, process) } pub(crate) fn do_remove_from_path(process: &Process) -> Result<()> { diff --git a/src/cli/self_update/windows.rs b/src/cli/self_update/windows.rs index 07578e3ec1..73de22c738 100644 --- a/src/cli/self_update/windows.rs +++ b/src/cli/self_update/windows.rs @@ -26,6 +26,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.")?; @@ -348,16 +350,26 @@ 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 { + use super::do_cargo_bin_clean; 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, rustup.exe is hopefully no longer open. + 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"); + + // Clean up CARGO_HOME/bin if it's empty now. + // On success, also remove it from $PATH. + do_cargo_bin_clean(cargo_bin_path, 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 +656,9 @@ pub(crate) fn self_replace(process: &Process) -> Result { 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 +671,7 @@ pub(crate) fn self_replace(process: &Process) -> Result { // 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 +686,7 @@ pub(crate) fn self_replace(process: &Process) -> Result { // // .. 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 cleanup_cargo_bin(process: &Process, no_modify_path: bool) -> Result<()> { use std::io; use std::ptr; use std::thread; @@ -734,6 +746,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)?; diff --git a/tests/suite/cli_self_upd.rs b/tests/suite/cli_self_upd.rs index f821659935..6c088ca2b8 100644 --- a/tests/suite/cli_self_upd.rs +++ b/tests/suite/cli_self_upd.rs @@ -259,13 +259,121 @@ 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: not removing cargo bin `[..]` because it is not empty +... +"#]]) + .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(()) + } + }; + match retry(Fibonacci::from_millis(1).map(jitter).take(23), check) { + Ok(_) => (), + Err(e) => panic!("{e}"), + } } #[tokio::test] @@ -300,7 +408,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 +455,14 @@ 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()?; + if !(name.starts_with("rustup-gc-") && name.ends_with(EXE_SUFFIX)) { + return None; + } + Some(path.to_string_lossy().to_string()) + }) .collect::>(); match garbage.len() { 0 => Ok(()),