Fixed uninstall a running binary failed on Windows #13053

linyihai · 2023-11-28T12:09:46Z

What does this PR try to resolve?

The problem reproduce when you try to uninstall a running binary and it will failed on Windows, this is because that cargo had already remove the installed binary tracker information in disk, and next to remove the running binary but it is not allowed in Windows. And to the worst, you can not uninstall the binary already and only to reinstall it by the --force flag.

How to solve the issue?

This PR try to make the uninstall a binary more reasonable that only after remove the binary sucessfully then remove the tracker information on binary and make the remove binary one by one.

How should we test and review this PR?

Add testcase 0fd4fd3

install the foo
run foo in another thread.
try to uninstall running foo and only failed in Windows.
wait the foo finish, uninstall foo
install the foo

Additional information

rustbot · 2023-11-28T12:09:51Z

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

linyihai · 2023-11-28T12:10:17Z

r? @weihanglo

src/cargo/ops/cargo_uninstall.rs

weihanglo

I would say a test to verify the behavior is expected to prevent any regression. Usually you can have one commit to verify the "wrong" behavior, and the next commit fixes the bug and the test.

…ng Bin.

weihanglo

BTW, could you make PR description and title a bit more specific and clearer? It's too general to understand what this is going to fix.

tests/testsuite/install.rs

src/cargo/ops/cargo_uninstall.rs

tests/testsuite/install.rs

weihanglo

We're about to merge this. Let me know if you want to make more change or I'll merge it as-is.

tests/testsuite/install.rs

linyihai · 2023-12-01T14:10:57Z

We're about to merge this. Let me know if you want to make more change or I'll merge it as-is.

let me make some change according you suggestion.

weihanglo

Thank you for the contribution!

weihanglo · 2023-12-01T15:09:47Z

@bors r+

bors · 2023-12-01T15:09:50Z

📌 Commit db144c9 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-12-01T15:10:58Z

⌛ Testing commit db144c9 with merge 8479589...

bors · 2023-12-01T15:44:58Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 8479589 to master...

Update cargo 25 commits in 26333c732095d207aa05932ce863d850fb309386..58fb23140972092a12f7011d17a7db1d99e3eacf 2023-11-28 20:07:39 +0000 to 2023-12-02 14:15:16 +0000 - test(install): use TCP connection instead of thread sleep (rust-lang/cargo#13099) - test(mdman): Switch to snapbox (rust-lang/cargo#13098) - Include declared list of features in fingerprint for `-Zcheck-cfg` (rust-lang/cargo#13012) - chore(deps): update compatible (rust-lang/cargo#13083) - chore(ci): Always update gix packages together (rust-lang/cargo#13093) - chore(deps): update rust crate windows-sys to 0.52 (rust-lang/cargo#13089) - refactor(toml): Decouple logic from schema (rust-lang/cargo#13080) - Have cargo add --optional <dep> create a <dep> = "dep:<dep> feature (rust-lang/cargo#13071) - Add `--public` for `cargo add` (rust-lang/cargo#13046) - chore(deps): update rust crate toml_edit to 0.21.0 (rust-lang/cargo#13088) - chore(deps): update rust crate rusqlite to 0.30.0 (rust-lang/cargo#13087) - test(trim-paths): exercise with real world debugger (rust-lang/cargo#13091) - Fixed uninstall a running binary failed on Windows (rust-lang/cargo#13053) - chore(deps): update rust crate itertools to 0.12.0 (rust-lang/cargo#13086) - Add more options to registry test support. (rust-lang/cargo#13085) - Don't filter on workspace members when scraping doc examples (rust-lang/cargo#13077) - Remove the outdated comment (rust-lang/cargo#13076) - fix(resolver): Remove unused public-deps error handling (rust-lang/cargo#13036) - Fixes error count display is different when there's only one error left (rust-lang/cargo#12484) - fix: reorder `--remap-path-prefix` flags for `-Zbuild-std` (rust-lang/cargo#13065) - remove jobserver env var in some tests (rust-lang/cargo#13072) - doc: clarify different target has different set of `CARGO_CFG_*` values (rust-lang/cargo#13069) - docs: remove review capacity notice in PR template (rust-lang/cargo#13070) - chore(deps): update rust crate openssl to 0.10.60 [security] (rust-lang/cargo#13068) - fix(resolver): De-prioritize no-rust-version in MSRV resolver (rust-lang/cargo#13066) r? ghost

Update cargo 27 commits in 26333c732095d207aa05932ce863d850fb309386..623b788496b3e51dc2f9282373cf0f6971a229b5 2023-11-28 20:07:39 +0000 to 2023-12-02 18:10:03 +0000 - docs(book): make old title anchorable (rust-lang/cargo#13102) - Revert "chore(deps): update rust crate openssl to 0.10.60 [security]" (rust-lang/cargo#13101) - test(install): use TCP connection instead of thread sleep (rust-lang/cargo#13099) - test(mdman): Switch to snapbox (rust-lang/cargo#13098) - Include declared list of features in fingerprint for `-Zcheck-cfg` (rust-lang/cargo#13012) - chore(deps): update compatible (rust-lang/cargo#13083) - chore(ci): Always update gix packages together (rust-lang/cargo#13093) - chore(deps): update rust crate windows-sys to 0.52 (rust-lang/cargo#13089) - refactor(toml): Decouple logic from schema (rust-lang/cargo#13080) - Have cargo add --optional <dep> create a <dep> = "dep:<dep> feature (rust-lang/cargo#13071) - Add `--public` for `cargo add` (rust-lang/cargo#13046) - chore(deps): update rust crate toml_edit to 0.21.0 (rust-lang/cargo#13088) - chore(deps): update rust crate rusqlite to 0.30.0 (rust-lang/cargo#13087) - test(trim-paths): exercise with real world debugger (rust-lang/cargo#13091) - Fixed uninstall a running binary failed on Windows (rust-lang/cargo#13053) - chore(deps): update rust crate itertools to 0.12.0 (rust-lang/cargo#13086) - Add more options to registry test support. (rust-lang/cargo#13085) - Don't filter on workspace members when scraping doc examples (rust-lang/cargo#13077) - Remove the outdated comment (rust-lang/cargo#13076) - fix(resolver): Remove unused public-deps error handling (rust-lang/cargo#13036) - Fixes error count display is different when there's only one error left (rust-lang/cargo#12484) - fix: reorder `--remap-path-prefix` flags for `-Zbuild-std` (rust-lang/cargo#13065) - remove jobserver env var in some tests (rust-lang/cargo#13072) - doc: clarify different target has different set of `CARGO_CFG_*` values (rust-lang/cargo#13069) - docs: remove review capacity notice in PR template (rust-lang/cargo#13070) - chore(deps): update rust crate openssl to 0.10.60 [security] (rust-lang/cargo#13068) - fix(resolver): De-prioritize no-rust-version in MSRV resolver (rust-lang/cargo#13066)

Fixed cargo uninstall behavior in Windows

09be0ee

rustbot assigned ehuss Nov 28, 2023

rustbot added Command-uninstall S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2023

rustbot assigned weihanglo and unassigned ehuss Nov 28, 2023

epage reviewed Nov 28, 2023

View reviewed changes

src/cargo/ops/cargo_uninstall.rs Outdated Show resolved Hide resolved

weihanglo reviewed Nov 28, 2023

View reviewed changes

linyihai force-pushed the cargo-uninstall-fixed branch from 7ac6873 to 30efd2b Compare November 29, 2023 09:39

Add test case for cargo uninstall while try to uninstalling a runni…

0fd4fd3

…ng Bin.

linyihai force-pushed the cargo-uninstall-fixed branch from 30efd2b to 0fd4fd3 Compare November 29, 2023 09:54

epage marked this pull request as draft November 29, 2023 17:43

linyihai added 2 commits November 30, 2023 15:29

Combine the behavior for cargo uninstall

26cf274

chore: fixed clippy

1e3b423

linyihai marked this pull request as ready for review November 30, 2023 08:22

weihanglo reviewed Nov 30, 2023

View reviewed changes

tests/testsuite/install.rs Outdated Show resolved Hide resolved

src/cargo/ops/cargo_uninstall.rs Outdated Show resolved Hide resolved

tests/testsuite/install.rs Show resolved Hide resolved

linyihai changed the title ~~Fixed cargo uninstall behavior in Windows~~ Fixed uninstall a running binary failed on Windows Dec 1, 2023

linyihai force-pushed the cargo-uninstall-fixed branch from f381571 to 1584fad Compare December 1, 2023 11:54

linyihai marked this pull request as draft December 1, 2023 11:56

linyihai force-pushed the cargo-uninstall-fixed branch 2 times, most recently from 511f417 to ed9c4b7 Compare December 1, 2023 13:52

weihanglo reviewed Dec 1, 2023

View reviewed changes

tests/testsuite/install.rs Outdated Show resolved Hide resolved

tests/testsuite/install.rs Outdated Show resolved Hide resolved

tests/testsuite/install.rs Outdated Show resolved Hide resolved

weihanglo marked this pull request as ready for review December 1, 2023 14:07

Add tracker verification for test case uninstall_running_binary

db144c9

linyihai force-pushed the cargo-uninstall-fixed branch from ed9c4b7 to db144c9 Compare December 1, 2023 14:28

weihanglo approved these changes Dec 1, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 1, 2023

bors merged commit 8479589 into rust-lang:master Dec 1, 2023
20 checks passed

weihanglo mentioned this pull request Dec 2, 2023

Update cargo rust-lang/rust#118541

Merged

ehuss added this to the 1.76.0 milestone Dec 6, 2023

linyihai deleted the cargo-uninstall-fixed branch December 9, 2023 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed uninstall a running binary failed on Windows #13053

Fixed uninstall a running binary failed on Windows #13053

linyihai commented Nov 28, 2023 •

edited

Loading

rustbot commented Nov 28, 2023

linyihai commented Nov 28, 2023

weihanglo left a comment

weihanglo left a comment

weihanglo left a comment

linyihai commented Dec 1, 2023

weihanglo left a comment

weihanglo commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023

Fixed uninstall a running binary failed on Windows #13053

Fixed uninstall a running binary failed on Windows #13053

Conversation

linyihai commented Nov 28, 2023 • edited Loading

What does this PR try to resolve?

How to solve the issue?

How should we test and review this PR?

Additional information

rustbot commented Nov 28, 2023

linyihai commented Nov 28, 2023

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

linyihai commented Dec 1, 2023

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023

linyihai commented Nov 28, 2023 •

edited

Loading