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

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

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 🙏

fix: preserve CARGO_HOME during Windows uninstall

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

This comment has been minimized.

Sign in to view

rami3l self-assigned this May 31, 2026

rami3l reviewed May 31, 2026

View reviewed changes

Comment thread src/cli/self_update.rs Outdated

Comment thread src/cli/self_update.rs Outdated

Comment thread src/cli/self_update.rs Outdated

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

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

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

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 3 times, most recently from afd4a48 to 6e27966 Compare June 1, 2026 13:54

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

Cloud0310 force-pushed the main branch 2 times, most recently from 2415ba4 to 5850b3e Compare June 4, 2026 13:18

renovate Bot and others added 6 commits June 4, 2026 21:23

chore(deps): lock file maintenance

93fd25a

chore: re-organize imports

ef6d201

refactor: rename delete_rustup_and_cargo_home to clean_cargo_bin

f9f7269

refactor: move PATH cleanup into clean_cargo_bin

c573da8

refactor: share cargo bin cleanup helper

bd3af44

fix: remove rustup proxies during uninstall

18a43de

Cloud0310 force-pushed the main branch from 5850b3e to 971f0ff Compare June 4, 2026 13:23

Cloud0310 added 4 commits June 4, 2026 22:21

fix: preserve CARGO_HOME during uninstall

9978bdb

fix: preserve CARGO_HOME during Windows uninstall

test: cover cargo bin removal during uninstall

0adbc03

test: cover cargo bin PATH cleanup during uninstall

5f57ff1

test: cover Windows cargo bin cleanup during uninstall

800d957

Cloud0310 force-pushed the main branch from 971f0ff to 800d957 Compare June 4, 2026 14:28

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.

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

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 • 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!

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!

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

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