Skip to content

fix: stop remove $CARGO_HOME on uninstallation#4861

Open
Cloud0310 wants to merge 3 commits into
rust-lang:mainfrom
Cloud0310:main
Open

fix: stop remove $CARGO_HOME on uninstallation#4861
Cloud0310 wants to merge 3 commits into
rust-lang:mainfrom
Cloud0310:main

Conversation

@Cloud0310
Copy link
Copy Markdown
Contributor

@Cloud0310 Cloud0310 commented May 18, 2026

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.

FranciscoTGouveia
FranciscoTGouveia reviewed May 19, 2026
View reviewed changes
Comment thread tests/suite/cli_self_upd.rs
@Cloud0310 Cloud0310 marked this pull request as ready for review May 19, 2026 10:37
@Cloud0310
Copy link
Copy Markdown
Contributor Author

Cloud0310 commented May 19, 2026 •
edited
Loading

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 .

rami3l
rami3l reviewed May 19, 2026
View reviewed changes
Comment thread src/cli/self_update/unix.rs Outdated
Comment thread src/cli/self_update/unix.rs Outdated
Comment thread tests/suite/cli_self_upd.rs Outdated
Comment thread src/cli/self_update.rs Outdated
Comment thread src/cli/self_update/unix.rs Outdated
Comment thread src/cli/self_update/windows.rs Outdated
rami3l
rami3l reviewed May 19, 2026
View reviewed changes
Comment thread src/cli/self_update.rs Outdated
djc
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
Copy link
Copy Markdown
Contributor Author

Cloud0310 commented May 20, 2026

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

rami3l
rami3l reviewed May 21, 2026
View reviewed changes
Comment thread src/cli/self_update/unix.rs
rami3l
rami3l approved these changes May 21, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@rami3l rami3l left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo some tiny nits, nice work :D

View changes since this review

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
}

#[tokio::test]
async fn uninstall_deletes_cargo_home() {
Copy link
Copy Markdown
Member

@rami3l rami3l May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just squash all commits into one since all of them seems to be focusing on a single topic.

@Cloud0310 Cloud0310 force-pushed the main branch 2 times, most recently from 4a4880a to 971dffa Compare May 22, 2026 08:13
Cloud0310 added a commit to Cloud0310/rustup that referenced this pull request May 22, 2026
@Cloud0310
fix: stop deleting $CARGO_HOME during uninstall
b7bddd9 
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 added a commit to Cloud0310/rustup that referenced this pull request May 22, 2026
@Cloud0310
fix: stop deleting $CARGO_HOME during uninstall
01cfd07 
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
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 22, 2026

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.

Cloud0310 added a commit to Cloud0310/rustup that referenced this pull request May 22, 2026
@Cloud0310
fix: stop deleting $CARGO_HOME during uninstall
61b3e49 
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
djc reviewed May 22, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@djc djc left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
}

/// Run by rustup-gc-$num.exe to delete CARGO_HOME
/// Run by rustup-gc-$num.exe to delete rustup binary
Copy link
Copy Markdown
Contributor

@djc djc May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated, maybe move it into a separate commit?

Copy link
Copy Markdown
Member

@rami3l rami3l May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djc I think this is relevant though; the new behavior is to remove traces of rustup in the bindir rather than nuking CARGO_HOME.

@Cloud0310 Cloud0310 mentioned this pull request May 22, 2026
Open
@rami3l rami3l mentioned this pull request May 22, 2026
Open
Cloud0310 added a commit to Cloud0310/rustup that referenced this pull request May 22, 2026
@Cloud0310
fix: stop deleting $CARGO_HOME during uninstall
a80abc9 
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 added a commit to Cloud0310/rustup that referenced this pull request May 22, 2026
@Cloud0310
fix: stop deleting $CARGO_HOME during uninstall
cdcc902 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@djc djc djc left review comments

@rami3l rami3l rami3l approved these changes

@FranciscoTGouveia FranciscoTGouveia Awaiting requested review from FranciscoTGouveia

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Stop nuking $CARGO_HOME (~/.cargo) on rustup self uninstall

5 participants

@Cloud0310 @rustbot @djc @rami3l @FranciscoTGouveia