Auto merge of #13053 - linyihai:cargo-uninstall-fixed, r=weihanglo

Fixed uninstall a running binary failed on Windows

### What does this PR try to resolve?
 Fixes #3364

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

Author: bors

src/cargo/ops/cargo_uninstall.rs

@@ -6,7 +6,6 @@ use crate::util::errors::CargoResult;
 use crate::util::Config;
 use crate::util::Filesystem;
 use anyhow::bail;
-use cargo_util::paths;
 use std::collections::BTreeSet;
 use std::env;
 
@@ -103,7 +102,6 @@ fn uninstall_pkgid(
     bins: &[String],
     config: &Config,
 ) -> CargoResult<()> {
-    let mut to_remove = Vec::new();
     let installed = match tracker.installed_bins(pkgid) {
         Some(bins) => bins.clone(),
         None => bail!("package `{}` is not installed", pkgid),
@@ -137,19 +135,18 @@ fn uninstall_pkgid(
         }
     }
 
-    if bins.is_empty() {
-        to_remove.extend(installed.iter().map(|b| dst.join(b)));
-        tracker.remove(pkgid, &installed);
-    } else {
-        for bin in bins.iter() {
-            to_remove.push(dst.join(bin));
+    let to_remove = {
+        if bins.is_empty() {
+            installed
+        } else {
+            bins
         }
-        tracker.remove(pkgid, &bins);
-    }
-    tracker.save()?;
+    };
+
     for bin in to_remove {
-        config.shell().status("Removing", bin.display())?;
-        paths::remove_file(bin)?;
+        let bin_path = dst.join(&bin);
+        config.shell().status("Removing", bin_path.display())?;
+        tracker.remove_bin_then_save(pkgid, &bin, &bin_path)?;
     }
 
     Ok(())

src/cargo/ops/common_for_install_and_uninstall.rs

@@ -7,6 +7,7 @@ use std::rc::Rc;
 use std::task::Poll;
 
 use anyhow::{bail, format_err, Context as _};
+use cargo_util::paths;
 use ops::FilterRule;
 use serde::{Deserialize, Serialize};
 
@@ -319,6 +320,20 @@ impl InstallTracker {
         self.v1.remove(pkg_id, bins);
         self.v2.remove(pkg_id, bins);
     }
+
+    /// Remove a bin after it successfully had been removed in disk and then save the tracker at last.
+    pub fn remove_bin_then_save(
+        &mut self,
+        pkg_id: PackageId,
+        bin: &str,
+        bin_path: &PathBuf,
+    ) -> CargoResult<()> {
+        paths::remove_file(bin_path)?;
+        self.v1.remove_bin(pkg_id, bin);
+        self.v2.remove_bin(pkg_id, bin);
+        self.save()?;
+        Ok(())
+    }
 }
 
 impl CrateListingV1 {
@@ -359,6 +374,17 @@ impl CrateListingV1 {
         }
     }
 
+    fn remove_bin(&mut self, pkg_id: PackageId, bin: &str) {
+        let mut installed = match self.v1.entry(pkg_id) {
+            btree_map::Entry::Occupied(e) => e,
+            btree_map::Entry::Vacant(..) => panic!("v1 unexpected missing `{}`", pkg_id),
+        };
+        installed.get_mut().remove(bin);
+        if installed.get().is_empty() {
+            installed.remove();
+        }
+    }
+
     fn save(&self, lock: &FileLock) -> CargoResult<()> {
         let mut file = lock.file();
         file.seek(SeekFrom::Start(0))?;
@@ -468,6 +494,17 @@ impl CrateListingV2 {
         }
     }
 
+    fn remove_bin(&mut self, pkg_id: PackageId, bin: &str) {
+        let mut info_entry = match self.installs.entry(pkg_id) {
+            btree_map::Entry::Occupied(e) => e,
+            btree_map::Entry::Vacant(..) => panic!("v1 unexpected missing `{}`", pkg_id),
+        };
+        info_entry.get_mut().bins.remove(bin);
+        if info_entry.get().bins.is_empty() {
+            info_entry.remove();
+        }
+    }
+
     fn save(&self, lock: &FileLock) -> CargoResult<()> {
         let mut file = lock.file();
         file.seek(SeekFrom::Start(0))?;

tests/testsuite/install.rs

@@ -3,6 +3,7 @@
 use std::fs::{self, OpenOptions};
 use std::io::prelude::*;
 use std::path::Path;
+use std::thread;
 
 use cargo_test_support::compare;
 use cargo_test_support::cross_compile;
@@ -11,10 +12,10 @@ use cargo_test_support::registry::{self, registry_path, Package};
 use cargo_test_support::{
     basic_manifest, cargo_process, no_such_file_err_msg, project, project_in, symlink_supported, t,
 };
-use cargo_util::ProcessError;
+use cargo_util::{ProcessBuilder, ProcessError};
 
 use cargo_test_support::install::{
-    assert_has_installed_exe, assert_has_not_installed_exe, cargo_home,
+    assert_has_installed_exe, assert_has_not_installed_exe, cargo_home, exe,
 };
 use cargo_test_support::paths::{self, CargoPathExt};
 use std::env;
@@ -2507,3 +2508,100 @@ fn install_incompat_msrv() {
 ")
         .with_status(101).run();
 }
+
+fn assert_tracker_noexistence(key: &str) {
+    let v1_data: toml::Value =
+        toml::from_str(&fs::read_to_string(cargo_home().join(".crates.toml")).unwrap()).unwrap();
+    let v2_data: serde_json::Value =
+        serde_json::from_str(&fs::read_to_string(cargo_home().join(".crates2.json")).unwrap())
+            .unwrap();
+
+    assert!(v1_data["v1"].get(key).is_none());
+    assert!(v2_data["installs"][key].is_null());
+}
+
+#[cargo_test]
+fn uninstall_running_binary() {
+    Package::new("foo", "0.0.1")
+        .file("src/lib.rs", "")
+        .file(
+            "Cargo.toml",
+            r#"
+    [package]
+    name = "foo"
+    version = "0.0.1"
+    
+    [[bin]]
+    name = "foo"
+    path = "src/main.rs"
+"#,
+        )
+        .file(
+            "src/main.rs",
+            r#"
+        use std::{{thread, time}}; 
+        fn main() {
+            thread::sleep(time::Duration::from_secs(3));
+        }
+"#,
+        )
+        .publish();
+
+    cargo_process("install foo")
+        .with_stderr(
+            "\
+[UPDATING] `[..]` index
+[DOWNLOADING] crates ...
+[DOWNLOADED] foo v0.0.1 (registry [..])
+[INSTALLING] foo v0.0.1
+[COMPILING] foo v0.0.1
+[FINISHED] release [optimized] target(s) in [..]
+[INSTALLING] [CWD]/home/.cargo/bin/foo[EXE]
+[INSTALLED] package `foo v0.0.1` (executable `foo[EXE]`)
+[WARNING] be sure to add `[..]` to your PATH to be able to run the installed binaries
+",
+        )
+        .run();
+    assert_has_installed_exe(cargo_home(), "foo");
+
+    let foo_bin = cargo_home().join("bin").join(exe("foo"));
+    let t = thread::spawn(|| ProcessBuilder::new(foo_bin).exec().unwrap());
+    let key = "foo 0.0.1 (registry+https://github.com/rust-lang/crates.io-index)";
+
+    #[cfg(windows)]
+    {
+        cargo_process("uninstall foo")
+            .with_status(101)
+            .with_stderr_contains("[ERROR] failed to remove file `[CWD]/home/.cargo/bin/foo[EXE]`")
+            .run();
+        t.join().unwrap();
+        cargo_process("uninstall foo")
+            .with_stderr("[REMOVING] [CWD]/home/.cargo/bin/foo[EXE]")
+            .run();
+    };
+
+    #[cfg(not(windows))]
+    {
+        cargo_process("uninstall foo")
+            .with_stderr("[REMOVING] [CWD]/home/.cargo/bin/foo[EXE]")
+            .run();
+        t.join().unwrap();
+    };
+
+    assert_has_not_installed_exe(cargo_home(), "foo");
+    assert_tracker_noexistence(key);
+
+    cargo_process("install foo")
+        .with_stderr(
+            "\
+[UPDATING] `[..]` index
+[INSTALLING] foo v0.0.1
+[COMPILING] foo v0.0.1
+[FINISHED] release [optimized] target(s) in [..]
+[INSTALLING] [CWD]/home/.cargo/bin/foo[EXE]
+[INSTALLED] package `foo v0.0.1` (executable `foo[EXE]`)
+[WARNING] be sure to add `[..]` to your PATH to be able to run the installed binaries
+",
+        )
+        .run();
+}