Auto merge of #7246 - ehuss:install-remove-orphan, r=alexcrichton

`cargo install`: Remove orphaned executables.

When a new version of a package is installed that no longer contains an executable from a previous version, this change causes those orphaned executables to also be removed.

I can place this new behavior behind the `install-upgrade` feature gate if anyone is uncomfortable with changing the behavior now.

cc #6797

Author: bors

src/cargo/ops/cargo_compile.rs

@@ -443,6 +443,9 @@ impl CompileFilter {
         all_bens: bool,
         all_targets: bool,
     ) -> CompileFilter {
+        if all_targets {
+            return CompileFilter::new_all_targets();
+        }
         let rule_lib = if lib_only {
             LibRule::True
         } else {
@@ -453,18 +456,7 @@ impl CompileFilter {
         let rule_exms = FilterRule::new(exms, all_exms);
         let rule_bens = FilterRule::new(bens, all_bens);
 
-        if all_targets {
-            CompileFilter::Only {
-                all_targets: true,
-                lib: LibRule::Default,
-                bins: FilterRule::All,
-                examples: FilterRule::All,
-                benches: FilterRule::All,
-                tests: FilterRule::All,
-            }
-        } else {
-            CompileFilter::new(rule_lib, rule_bins, rule_tsts, rule_exms, rule_bens)
-        }
+        CompileFilter::new(rule_lib, rule_bins, rule_tsts, rule_exms, rule_bens)
     }
 
     /// Construct a CompileFilter from underlying primitives.
@@ -496,6 +488,17 @@ impl CompileFilter {
         }
     }
 
+    pub fn new_all_targets() -> CompileFilter {
+        CompileFilter::Only {
+            all_targets: true,
+            lib: LibRule::Default,
+            bins: FilterRule::All,
+            examples: FilterRule::All,
+            benches: FilterRule::All,
+            tests: FilterRule::All,
+        }
+    }
+
     pub fn need_dev_deps(&self, mode: CompileMode) -> bool {
         match mode {
             CompileMode::Test | CompileMode::Doctest | CompileMode::Bench => true,

src/cargo/ops/cargo_install.rs

@@ -1,4 +1,4 @@
-use std::collections::{BTreeMap, BTreeSet, HashSet};
+use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
 use std::{env, fs};
@@ -9,7 +9,7 @@ use tempfile::Builder as TempFileBuilder;
 use crate::core::compiler::Freshness;
 use crate::core::compiler::{DefaultExecutor, Executor};
 use crate::core::resolver::ResolveOpts;
-use crate::core::{Edition, PackageId, PackageIdSpec, Source, SourceId, Workspace};
+use crate::core::{Edition, Package, PackageId, PackageIdSpec, Source, SourceId, Workspace};
 use crate::ops;
 use crate::ops::common_for_install_and_uninstall::*;
 use crate::sources::{GitSource, SourceConfigMap};
@@ -414,6 +414,13 @@ fn install_one(
             rustc.verbose_version,
         );
 
+        if let Err(e) = remove_orphaned_bins(&ws, &mut tracker, &duplicates, pkg, &dst) {
+            // Don't hard error on remove.
+            config
+                .shell()
+                .warn(format!("failed to remove orphan: {:?}", e))?;
+        }
+
         match tracker.save() {
             Err(err) => replace_result.chain_err(|| err)?,
             Ok(_) => replace_result?,
@@ -521,3 +528,58 @@ pub fn install_list(dst: Option<&str>, config: &Config) -> CargoResult<()> {
     }
     Ok(())
 }
+
+/// Removes executables that are no longer part of a package that was
+/// previously installed.
+fn remove_orphaned_bins(
+    ws: &Workspace<'_>,
+    tracker: &mut InstallTracker,
+    duplicates: &BTreeMap<String, Option<PackageId>>,
+    pkg: &Package,
+    dst: &Path,
+) -> CargoResult<()> {
+    let filter = ops::CompileFilter::new_all_targets();
+    let all_self_names = exe_names(pkg, &filter);
+    let mut to_remove: HashMap<PackageId, BTreeSet<String>> = HashMap::new();
+    // For each package that we stomped on.
+    for other_pkg in duplicates.values() {
+        // Only for packages with the same name.
+        if let Some(other_pkg) = other_pkg {
+            if other_pkg.name() == pkg.name() {
+                // Check what the old package had installed.
+                if let Some(installed) = tracker.installed_bins(*other_pkg) {
+                    // If the old install has any names that no longer exist,
+                    // add them to the list to remove.
+                    for installed_name in installed {
+                        if !all_self_names.contains(installed_name.as_str()) {
+                            to_remove
+                                .entry(*other_pkg)
+                                .or_default()
+                                .insert(installed_name.clone());
+                        }
+                    }
+                }
+            }
+        }
+    }
+
+    for (old_pkg, bins) in to_remove {
+        tracker.remove(old_pkg, &bins);
+        for bin in bins {
+            let full_path = dst.join(bin);
+            if full_path.exists() {
+                ws.config().shell().status(
+                    "Removing",
+                    format!(
+                        "executable `{}` from previous version {}",
+                        full_path.display(),
+                        old_pkg
+                    ),
+                )?;
+                paths::remove_file(&full_path)
+                    .chain_err(|| format!("failed to remove {:?}", full_path))?;
+            }
+        }
+    }
+    Ok(())
+}

src/cargo/ops/common_for_install_and_uninstall.rs

@@ -743,6 +743,14 @@ pub fn exe_names(pkg: &Package, filter: &ops::CompileFilter) -> BTreeSet<String>
             .filter(|t| t.is_bin())
             .map(|t| to_exe(t.name()))
             .collect(),
+        CompileFilter::Only {
+            all_targets: true, ..
+        } => pkg
+            .targets()
+            .iter()
+            .filter(|target| target.is_executable())
+            .map(|target| to_exe(target.name()))
+            .collect(),
         CompileFilter::Only {
             ref bins,
             ref examples,

tests/testsuite/install.rs

@@ -531,6 +531,7 @@ fn install_force_partial_overlap() {
 [FINISHED] release [optimized] target(s) in [..]
 [INSTALLING] [CWD]/home/.cargo/bin/foo-bin3[EXE]
 [REPLACING] [CWD]/home/.cargo/bin/foo-bin2[EXE]
+[REMOVING] executable `[..]/bin/foo-bin1[EXE]` from previous version foo v0.0.1 [..]
 [INSTALLED] package `foo v0.2.0 ([..]/foo2)` (executable `foo-bin3[EXE]`)
 [REPLACED] package `foo v0.0.1 ([..]/foo)` with `foo v0.2.0 ([..]/foo2)` (executable `foo-bin2[EXE]`)
 [WARNING] be sure to add `[..]` to your PATH to be able to run the installed binaries
@@ -541,8 +542,6 @@ fn install_force_partial_overlap() {
     cargo_process("install --list")
         .with_stdout(
             "\
-foo v0.0.1 ([..]):
-    foo-bin1[..]
 foo v0.2.0 ([..]):
     foo-bin2[..]
     foo-bin3[..]

tests/testsuite/install_upgrade.rs

@@ -65,6 +65,7 @@ fn installed_process(name: &str) -> Execs {
 
 /// Check that the given package name/version has the following bins listed in
 /// the trackers. Also verifies that both trackers are in sync and valid.
+/// Pass in an empty `bins` list to assert that the package is *not* installed.
 fn validate_trackers(name: &str, version: &str, bins: &[&str]) {
     let v1 = load_crates1();
     let v1_table = v1.get("v1").unwrap().as_table().unwrap();
@@ -88,7 +89,14 @@ fn validate_trackers(name: &str, version: &str, bins: &[&str]) {
             .map(|b| b.as_str().unwrap().to_string())
             .collect();
         if pkg_id.name().as_str() == name && pkg_id.version().to_string() == version {
-            assert_eq!(bins, v1_bins);
+            if bins.is_empty() {
+                panic!(
+                    "Expected {} to not be installed, but found: {:?}",
+                    name, v1_bins
+                );
+            } else {
+                assert_eq!(bins, v1_bins);
+            }
         }
         let pkg_id_value = serde_json::to_value(&pkg_id).unwrap();
         let pkg_id_str = pkg_id_value.as_str().unwrap();
@@ -782,3 +790,58 @@ Add --force to overwrite
         .with_status(101)
         .run();
 }
+
+#[cargo_test]
+fn deletes_orphaned() {
+    // When an executable is removed from a project, upgrading should remove it.
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+            "#,
+        )
+        .file("src/main.rs", "fn main() {}")
+        .file("src/bin/other.rs", "fn main() {}")
+        .file("examples/ex1.rs", "fn main() {}")
+        .build();
+    p.cargo("install -Z install-upgrade --path . --bins --examples")
+        .masquerade_as_nightly_cargo()
+        .run();
+    assert!(installed_exe("other").exists());
+
+    // Remove a binary, add a new one, and bump the version.
+    fs::remove_file(p.root().join("src/bin/other.rs")).unwrap();
+    p.change_file("examples/ex2.rs", "fn main() {}");
+    p.change_file(
+        "Cargo.toml",
+        r#"
+        [package]
+        name = "foo"
+        version = "0.2.0"
+        "#,
+    );
+    p.cargo("install -Z install-upgrade --path . --bins --examples")
+        .masquerade_as_nightly_cargo()
+        .with_stderr(
+            "\
+[INSTALLING] foo v0.2.0 [..]
+[COMPILING] foo v0.2.0 [..]
+[FINISHED] release [..]
+[INSTALLING] [..]/.cargo/bin/ex2[EXE]
+[REPLACING] [..]/.cargo/bin/ex1[EXE]
+[REPLACING] [..]/.cargo/bin/foo[EXE]
+[REMOVING] executable `[..]/.cargo/bin/other[EXE]` from previous version foo v0.1.0 [..]
+[INSTALLED] package `foo v0.2.0 [..]` (executable `ex2[EXE]`)
+[REPLACED] package `foo v0.1.0 [..]` with `foo v0.2.0 [..]` (executables `ex1[EXE]`, `foo[EXE]`)
+[WARNING] be sure to add [..]
+",
+        )
+        .run();
+    assert!(!installed_exe("other").exists());
+    validate_trackers("foo", "0.2.0", &["foo", "ex1", "ex2"]);
+    // 0.1.0 should not have any entries.
+    validate_trackers("foo", "0.1.0", &[]);
+}