fix!: stop deleting $CARGO_HOME during uninstall by Cloud0310 · Pull Request #4861 · rust-lang/rustup

Cloud0310 · 2026-05-18T07:21:32Z

fix #285
This PR fix the cargo bin nuke and the path removing possilble toutoc issue.
Here's the implementation details:

compare cargo bin dir content to rustup binary, if it's a link, remove it
then, we remove the rustup binary, on success, we then remove the cargo bin from path

This prevents rustup self uninstall from nuking out the $CARGO_HOME/bin and possible toctou problem of removing up the path.

Cloud0310 · 2026-05-19T13:42:17Z

I think I may need more tests against this PR, this includes:

$CARGO_HOME/bin with unrelated files
empty bin cleanup test

Should there be more tests? Please let me know @FranciscoTGouveia @rami3l .

Cloud0310 · 2026-05-20T18:20:13Z

I've add more tests about the PR here. Please review.

rami3l

LGTM modulo some tiny nits, nice work :D

View changes since this review

Avoid removing $CARGO_HOME when running uninstall cleanup. Combine all follow-up work for PR rust-lang#4861: - keep $CARGO_HOME/bin when it is non-empty - cover cargo bin cleanup behavior in uninstall tests - update uninstall cleanup comments and related docs - rename cleanup internals for clearer intent

djc

I think this PR needs a bunch of work, it seems to have a lot of changes together in a single commit that are pretty messy. It would be good if the commit history made it clear that there are a sequence of smaller changes being made.

View changes since this review

Avoid removing $CARGO_HOME when running uninstall cleanup. Combine all follow-up work for PR rust-lang#4861: - keep $CARGO_HOME/bin when it is non-empty - cover cargo bin cleanup behavior in uninstall tests - update uninstall cleanup comments and related docs - rename cleanup internals for clearer intent

Cloud0310 · 2026-05-30T18:34:57Z

The splitting is done. Do I need further splitting with tests?
If so, it could be more splitted into a) updated expected values, b) new tests upon keeping $CARGO_HOME.

rami3l

@Cloud0310 It is looking better, but some rework remains to be done. Especially, the 3rd commit (which used to be the 2nd commit in the last version) is still not boring enough.

The splitting is done. Do I need further splitting with tests?
If so, it could be more splitted into a) updated expected values, b) new tests upon keeping $CARGO_HOME.

Please perform the split you mentioned above as well.

Many thanks in advance :)

View changes since this review

rami3l · 2026-05-31T09:22:06Z

+            info!("removed cargo bin from $PATH");
+        }
+
+        info!("removed empty cargo bin");


Nit: The way you refer to the bindir is not consistent both in this function and elsewhere:

cargo bin

cargo bin `{cargo_bin_path}`

cargo bin directory `{cargo_bin_path}`

I suggest using the final one consistently.

rami3l · 2026-05-31T09:42:37Z

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


This new hunk has the same contents as unix::clean_cargo_bin() except the let no_modify_path line; consider further polishing the refactoring process by first moving unix::clean_cargo_bin() to the parent module and inline your current delete_cargo_bin() over there (it should have a single caller by then so the introduction of this new function delete_cargo_bin() is unnecessary).

@Cloud0310 Please delete the function delete_cargo_bin. As previously discussed, unix::clean_cargo_bin() should be moved to the parent module that contains logic common to both Windows and Unix (which includes the logic of delete_cargo_bin which is not repeated anywhere else); in Windows we use a platform specific wrapper to wrap that function in the windows module.

djc · 2026-06-01T10:35:26Z

 }

-pub(crate) fn clean_cargo_bin(process: &Process) -> Result<()> {
+pub(crate) fn clean_cargo_bin(process: &Process, no_modify_path: bool) -> Result<()> {


Nit: change the argument order (for this and other functions with the added argument), to have the more "interesting"/rare/specific argument before the less interesting/more common/less specific process argument.

View changes since the review

djc · 2026-06-01T10:39:22Z

 use anyhow::{Context, Result, bail};
 use tracing::{error, warn};

+use super::delete_cargo_bin;


Nit: this should be included in the existing super:: import, below.

View changes since the review

@djc I'm not quite sure what you are trying to say here, I assume you mean "in a first commit merge those in two groups of imports super::{...} and crate::{...}, then introduce the new import in the respective group"? Because in this file the imports are not grouped yet.

I mean that this is adding a use super::delete_cargo_bin; when there is already a use super::some_other_thing;.

@djc Okay, so we should put the single-level imports like this use super::{delete_cargo_bin, some_other_thing};

djc · 2026-06-01T10:44:18Z

-    utils::remove_dir("cargo_home", &cargo_home)
+    // Clean up CARGO_HOME/bin, if it's empty now
+    // On success, remove it from $PATH
+    delete_cargo_bin(cargo_bin_path, no_modify_path, process)


It is unclear to me why there are separate commits for the Unix and Windows paths? It seems to me like these should be in the same commit.

View changes since the review

djc · 2026-06-01T10:45:10Z

-    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())?;
-            }
-        }
-    }


It is not clear from reviewing this (presumably Unix-only) commit why this (presumably platform-independent) logic could be removed.

View changes since the review

djc · 2026-06-01T10:45:29Z


 use super::super::errors::CliError;
 use super::common;
+use super::delete_cargo_bin;


Nit: use existing import.

View changes since the review

@Cloud0310 Same as #4861 (comment).

djc · 2026-06-01T10:46:45Z

    Ok(utils::ExitCode(0))
 }

+const GC_MODIFY_PATH: &str = "RUSTUP_GC_MODIFY_PATH";


Perhaps this should be introduced independently to make it more clear why it is introduced/what it's purpose is? (It should be defined below all callers.)

View changes since the review

djc · 2026-06-01T10:47:28Z

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


Suggest introducing this in a separate first commit, and then updating it in each commit (where necessary) as a way of documenting what is changed.

View changes since the review

rustbot · 2026-06-01T12:50:48Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

fix: preserve CARGO_HOME during Windows uninstall

rami3l · 2026-06-01T14:37:09Z

@Cloud0310 Since your PR needs more work, I've marked this PR as draft.

Please address or at least respond to all open threads before marking this PR as "ready for review" again, at which point the team will return for another round of review, thanks 🙏

Cloud0310 force-pushed the main branch from 64f3d84 to 1e3a7f7 Compare May 18, 2026 12:11

FranciscoTGouveia reviewed May 19, 2026

View reviewed changes

Comment thread tests/suite/cli_self_upd.rs

Cloud0310 marked this pull request as ready for review May 19, 2026 10:37

Cloud0310 force-pushed the main branch from 07d31c8 to b6195c7 Compare May 19, 2026 13:22

rami3l reviewed May 19, 2026

View reviewed changes

Comment thread src/cli/self_update.rs Outdated

Cloud0310 requested a review from FranciscoTGouveia May 20, 2026 03:55

Cloud0310 force-pushed the main branch from b6195c7 to 48acf08 Compare May 20, 2026 06:54

djc reviewed May 20, 2026

View reviewed changes

Comment thread src/cli/self_update/windows.rs Outdated

Comment thread src/cli/self_update/windows.rs Outdated

Comment thread src/cli/self_update/windows.rs Outdated

Comment thread src/cli/self_update.rs Outdated

Comment thread src/cli/self_update.rs Outdated

Cloud0310 force-pushed the main branch from 48acf08 to b509a72 Compare May 20, 2026 14:03

Cloud0310 force-pushed the main branch from 5a91816 to 905c540 Compare May 20, 2026 18:33

rami3l reviewed May 21, 2026

View reviewed changes

Comment thread src/cli/self_update/unix.rs Outdated

rami3l approved these changes May 21, 2026

View reviewed changes

Comment thread src/cli/self_update.rs Outdated

Comment thread tests/suite/cli_self_upd.rs Outdated

Comment thread tests/suite/cli_self_upd.rs

Cloud0310 force-pushed the main branch 2 times, most recently from 4a4880a to 971dffa Compare May 22, 2026 08:13

Cloud0310 force-pushed the main branch from 971dffa to b7bddd9 Compare May 22, 2026 08:26

Cloud0310 force-pushed the main branch from b7bddd9 to 01cfd07 Compare May 22, 2026 08:45

This comment has been minimized.

Sign in to view

Cloud0310 force-pushed the main branch from 01cfd07 to 61b3e49 Compare May 22, 2026 09:00

djc reviewed May 22, 2026

View reviewed changes

Comment thread src/cli/self_update/unix.rs Outdated

Comment thread src/cli/self_update/windows.rs Outdated

Comment thread src/cli/self_update/windows.rs Outdated

Comment thread src/cli/self_update/windows.rs

Cloud0310 mentioned this pull request May 22, 2026

Update contribute docs with cross platform code development guide #4864

Closed

rami3l mentioned this pull request May 22, 2026

Clarify in the dev guide what is expected from a PR #4865

Closed

Cloud0310 force-pushed the main branch from 61b3e49 to a80abc9 Compare May 22, 2026 16:28

Cloud0310 force-pushed the main branch from 31659d4 to 1c70ffe Compare May 26, 2026 19:10

This comment has been minimized.

Sign in to view

Cloud0310 force-pushed the main branch from 1c70ffe to dfe1a7c Compare May 26, 2026 19:15

This comment has been minimized.

Sign in to view

Cloud0310 force-pushed the main branch 2 times, most recently from 67d8653 to 9f17fce Compare May 30, 2026 17:18

This comment has been minimized.

Sign in to view

rami3l self-assigned this May 31, 2026

rami3l reviewed May 31, 2026

View reviewed changes

rami3l changed the title ~~fix: stop remove $CARGO_HOME on uninstallation~~ fix: stop deleting $CARGO_HOME during uninstall May 31, 2026

rami3l changed the title ~~fix: stop deleting $CARGO_HOME during uninstall~~ fix!: stop deleting $CARGO_HOME during uninstall May 31, 2026

rami3l reviewed May 31, 2026

View reviewed changes

Comment thread src/cli/self_update.rs

Cloud0310 force-pushed the main branch from 9f17fce to 6e1aa36 Compare May 31, 2026 15:39

Cloud0310 added 2 commits June 1, 2026 16:43

refactor: rename delete_rustup_and_cargo_home to clean_cargo_bin

2eee69f

refactor: move PATH cleanup into clean_cargo_bin

09a089d

djc reviewed Jun 1, 2026

View reviewed changes

Cloud0310 force-pushed the main branch from 6e1aa36 to 6fee395 Compare June 1, 2026 12:50

Cloud0310 force-pushed the main branch from 6fee395 to bd43ee6 Compare June 1, 2026 13:12

Cloud0310 added 2 commits June 1, 2026 21:29

refactor: share cargo bin cleanup helper

ca3ae0e

fix: remove rustup proxies during uninstall

7a62073

Cloud0310 force-pushed the main branch from bd43ee6 to afd4a48 Compare June 1, 2026 13:50

Cloud0310 added 4 commits June 1, 2026 21:54

fix: preserve CARGO_HOME during uninstall

2109e81

fix: preserve CARGO_HOME during Windows uninstall

test: cover cargo bin removal during uninstall

19bd33c

test: cover cargo bin PATH cleanup during uninstall

2e52ec8

test: cover Windows cargo bin cleanup during uninstall

6e27966

Cloud0310 force-pushed the main branch from afd4a48 to 6e27966 Compare June 1, 2026 13:54

rami3l marked this pull request as draft June 1, 2026 14:17

Conversation

Cloud0310 commented May 18, 2026

Uh oh!

Uh oh!

Cloud0310 commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Cloud0310 commented May 20, 2026

Uh oh!

Uh oh!

rami3l left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

djc left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Cloud0310 commented May 30, 2026

Uh oh!

rami3l left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rami3l May 31, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rami3l May 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rami3l Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

djc Jun 1, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djc Jun 1, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rami3l Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

djc Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

rami3l Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

djc Jun 1, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Cloud0310 commented May 19, 2026 •

edited

Loading

rami3l left a comment •

edited by rustbot

Loading

djc left a comment •

edited by rustbot

Loading

rami3l left a comment •

edited

Loading

rami3l May 31, 2026 •

edited

Loading

rami3l Jun 1, 2026 •

edited

Loading

djc Jun 1, 2026 •

edited by rustbot

Loading

djc Jun 1, 2026 •

edited by rustbot

Loading

djc Jun 1, 2026 •

edited by rustbot

Loading

djc Jun 1, 2026 •

edited by rustbot

Loading

djc Jun 1, 2026 •

edited by rustbot

Loading

rami3l Jun 1, 2026 •

edited

Loading

djc Jun 1, 2026 •

edited by rustbot

Loading

djc Jun 1, 2026 •

edited by rustbot

Loading