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

…odule

djc

Definitely getting better, still some suggested improvements though.

View changes since this review

djc · 2026-06-11T08:29:29Z

 // .. augmented with this SO answer
 // https://stackoverflow.com/questions/10319526/understanding-a-self-deleting-program-in-c
-pub(crate) fn clean_cargo_bin(process: &Process) -> Result<()> {
+pub(crate) fn spawn_uninstall_gc(process: &Process) -> Result<()> {


Suggest moving this commit earlier in the series (maybe the very first) to make this more obvious during the changes made earlier.

djc · 2026-06-11T08:36:41Z

 }

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


I think this rename should be postponed until the commit where the change actually happens (from home to just bin).

@djc For extra clarity for @Cloud0310, do you mean right before the commit X where the change happens, after X, or within X?

Probably in the same commit?

rami3l · 2026-06-11T15:56:11Z

-/// 2. Remove other tools in $CARGO_HOME/bin, but skipping rustup binary file and links to rustup.
-/// 3. If no `--no-modify-path` is passed $CARGO_HOME/bin, clean up $PATH.
-/// 4. Remove $CARGO_HOME
+/// 2. Remove rustup tool links.


In the commit "fix: preserve CARGO_HOME during uninstall", we are not deleting rustup tool links yet.

The comments should really reflect what the code is doing in that particular commit.

View changes since the review

rami3l · 2026-06-12T07:41:11Z

-    info!("removing cargo home");
-
-    // 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())?;
-            }
-        }
-    }


@Cloud0310 Really sorry for the back and forth, what you have implemented here matches the expected semantics in a future version, there's nothing wrong about it; but I think it might cause a downgrade in terms of UX within the scope of this PR, so it would be better if:

The code that "removes everything except the bin directory" is kept;

The code that "removes everything in bin except rustup and tools" is removed as you are doing right now.

When we move further into XDG support it is indeed intended to do this more precisely, but for now it might still feel surprising if self uninstall is not removing a bunch of cargo-specific stuff.

The comments and the tests should be updated accordingly.

My apologies for not having noticed this in advance 🙏

View changes since the review

rami3l · 2026-06-12T07:43:35Z

    info!("removing empty cargo bin directory `{cargo_bin_path_display}`");

    let Err(e) = fs::remove_dir(&cargo_bin_path) else {
        if !no_modify_path {


Similar to 6aa45d2 (this PR), the user might want to see $CARGO_HOME being removed if it ends up empty at this point. In most cases this will be a no-op.

View changes since the review

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 dac53d8 to 3c0d7e4 Compare June 7, 2026 15:43

refactor(cli/self-update): move unix::clean_cargo_bin() to parent m…

f2bfa78

…odule

Cloud0310 force-pushed the main branch 2 times, most recently from af1c3ac to 60c433c Compare June 8, 2026 18:02

Cloud0310 marked this pull request as ready for review June 8, 2026 18:04

Cloud0310 changed the title ~~fix!: stop deleting $CARGO_HOME during uninstall~~ fix: stop deleting $CARGO_HOME during uninstall Jun 8, 2026

rami3l approved these changes Jun 8, 2026

View reviewed changes

Comment thread src/cli/self_update.rs Outdated

Comment thread tests/suite/cli_self_upd.rs

Cloud0310 force-pushed the main branch from 60c433c to ca2bba6 Compare June 8, 2026 18:43

Cloud0310 requested a review from rami3l June 9, 2026 15:09

rami3l reviewed Jun 10, 2026

View reviewed changes

Comment thread src/cli/self_update.rs Outdated

Comment thread src/cli/self_update/windows.rs

Comment thread src/cli/self_update.rs

Cloud0310 force-pushed the main branch 2 times, most recently from 0677697 to d5e6935 Compare June 10, 2026 14:29

refactor: route Windows cargo home cleanup through parent helper

8e407d3

Cloud0310 force-pushed the main branch from d5e6935 to c8af677 Compare June 10, 2026 15:37

Cloud0310 added 3 commits June 10, 2026 23:53

refactor: rename Windows uninstall gc helper

297cac5

refactor: pass PATH cleanup to clean_cargo_bin

5f13c0a

doc: add uninstall process overall docstring

d5b237e

Cloud0310 force-pushed the main branch from c8af677 to 31432e4 Compare June 10, 2026 16:07

Cloud0310 requested review from djc and rami3l June 10, 2026 16:07

djc reviewed Jun 11, 2026

View reviewed changes

rami3l reviewed Jun 11, 2026

View reviewed changes

Cloud0310 added 5 commits June 12, 2026 01:17

fix: preserve CARGO_HOME during uninstall

6aa45d2

fix: remove rustup proxy links during uninstall

4a5a86d

test: cover cargo bin removal during uninstall

53a88aa

test: cover cargo bin PATH cleanup during uninstall

07fefef

test: cover Windows cargo bin cleanup during uninstall

8c89d1b

Cloud0310 force-pushed the main branch from 31432e4 to 8c89d1b Compare June 11, 2026 17:20

rami3l reviewed Jun 12, 2026

View reviewed changes

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

djc Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

djc Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

rami3l Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

djc Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rami3l Jun 11, 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 12, 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 12, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Cloud0310 commented May 19, 2026 •

edited

Loading

rami3l left a comment •

edited by rustbot

Loading

djc left a comment •

edited by rustbot

Loading

djc left a comment •

edited by rustbot

Loading

rami3l Jun 11, 2026 •

edited by rustbot

Loading

rami3l Jun 12, 2026 •

edited

Loading

rami3l Jun 12, 2026 •

edited by rustbot

Loading