Auto merge of #10621 - LovecraftianHorror:more-precise-artifact-cleanup, r=weihanglo

Only remove fingerprints and build script artifacts of the requested package

Fixes #10069

This is my first PR to cargo. Let me know if you want me to add tests, or if there are any other changes you would like to see :)

This PR changes the globs used to remove fingerprints and build artifacts when running `cargo clean -p <pkid>`. The glob used was `<package_name>-*` which would match artifacts for packages that are prefixed by `<package_name>-` (e.g. `cargo clean -p sqlx` would also remove artifacts for `sqlx-{core,macros,etc.}`). This problem is not seen with other artifacts since those use the crate name instead of package name which normalize hyphens to underscores.

The changes follow the naive approach mentioned in #10069 where some basic string manipulation is done to strip off the last hyphen, hash, and potential extension to get the original package name which can be used to determine if the artifact is actually for the desired package. This means that this **does not** handle trying to resolve the package to determine the artifacts, so it still ignores the url and version that may be passed in the pkgid

Author: bors

src/cargo/ops/cargo_clean.rs

@@ -141,15 +141,25 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> {
         // Clean fingerprints.
         for (_, layout) in &layouts_with_host {
             let dir = escape_glob_path(layout.fingerprint())?;
-            rm_rf_glob(&Path::new(&dir).join(&pkg_dir), config, &mut progress)?;
+            rm_rf_package_glob_containing_hash(
+                &pkg.name(),
+                &Path::new(&dir).join(&pkg_dir),
+                config,
+                &mut progress,
+            )?;
         }
 
         for target in pkg.targets() {
             if target.is_custom_build() {
                 // Get both the build_script_build and the output directory.
                 for (_, layout) in &layouts_with_host {
                     let dir = escape_glob_path(layout.build())?;
-                    rm_rf_glob(&Path::new(&dir).join(&pkg_dir), config, &mut progress)?;
+                    rm_rf_package_glob_containing_hash(
+                        &pkg.name(),
+                        &Path::new(&dir).join(&pkg_dir),
+                        config,
+                        &mut progress,
+                    )?;
                 }
                 continue;
             }
@@ -222,6 +232,40 @@ fn escape_glob_path(pattern: &Path) -> CargoResult<String> {
     Ok(glob::Pattern::escape(pattern))
 }
 
+/// Glob remove artifacts for the provided `package`
+///
+/// Make sure the artifact is for `package` and not another crate that is prefixed by
+/// `package` by getting the original name stripped of the trailing hash and possible
+/// extension
+fn rm_rf_package_glob_containing_hash(
+    package: &str,
+    pattern: &Path,
+    config: &Config,
+    progress: &mut dyn CleaningProgressBar,
+) -> CargoResult<()> {
+    // TODO: Display utf8 warning to user?  Or switch to globset?
+    let pattern = pattern
+        .to_str()
+        .ok_or_else(|| anyhow::anyhow!("expected utf-8 path"))?;
+    for path in glob::glob(pattern)? {
+        let path = path?;
+
+        let pkg_name = path
+            .file_name()
+            .and_then(std::ffi::OsStr::to_str)
+            .and_then(|artifact| artifact.rsplit_once('-'))
+            .ok_or_else(|| anyhow::anyhow!("expected utf-8 path"))?
+            .0;
+
+        if pkg_name != package {
+            continue;
+        }
+
+        rm_rf(&path, config, progress)?;
+    }
+    Ok(())
+}
+
 fn rm_rf_glob(
     pattern: &Path,
     config: &Config,

tests/testsuite/clean.rs

@@ -96,33 +96,113 @@ fn clean_multiple_packages_in_glob_char_path() {
         .build();
     let foo_path = &p.build_dir().join("debug").join("deps");
 
+    #[cfg(not(target_env = "msvc"))]
+    let file_glob = "foo-*";
+
+    #[cfg(target_env = "msvc")]
+    let file_glob = "foo.pdb";
+
     // Assert that build artifacts are produced
     p.cargo("build").run();
-    assert_ne!(get_build_artifacts(foo_path).len(), 0);
+    assert_ne!(get_build_artifacts(foo_path, file_glob).len(), 0);
 
     // Assert that build artifacts are destroyed
     p.cargo("clean -p foo").run();
-    assert_eq!(get_build_artifacts(foo_path).len(), 0);
+    assert_eq!(get_build_artifacts(foo_path, file_glob).len(), 0);
 }
 
-fn get_build_artifacts(path: &PathBuf) -> Vec<Result<PathBuf, GlobError>> {
+fn get_build_artifacts(path: &PathBuf, file_glob: &str) -> Vec<Result<PathBuf, GlobError>> {
     let pattern = path.to_str().expect("expected utf-8 path");
     let pattern = glob::Pattern::escape(pattern);
 
-    #[cfg(not(target_env = "msvc"))]
-    const FILE: &str = "foo-*";
-
-    #[cfg(target_env = "msvc")]
-    const FILE: &str = "foo.pdb";
-
-    let path = PathBuf::from(pattern).join(FILE);
+    let path = PathBuf::from(pattern).join(file_glob);
     let path = path.to_str().expect("expected utf-8 path");
     glob::glob(path)
         .expect("expected glob to run")
         .into_iter()
         .collect::<Vec<Result<PathBuf, GlobError>>>()
 }
 
+#[cargo_test]
+fn clean_p_only_cleans_specified_package() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [workspace]
+                members = [
+                    "foo",
+                    "foo_core",
+                    "foo-base",
+                ]
+            "#,
+        )
+        .file("foo/Cargo.toml", &basic_manifest("foo", "0.1.0"))
+        .file("foo/src/lib.rs", "//! foo")
+        .file("foo_core/Cargo.toml", &basic_manifest("foo_core", "0.1.0"))
+        .file("foo_core/src/lib.rs", "//! foo_core")
+        .file("foo-base/Cargo.toml", &basic_manifest("foo-base", "0.1.0"))
+        .file("foo-base/src/lib.rs", "//! foo-base")
+        .build();
+
+    let fingerprint_path = &p.build_dir().join("debug").join(".fingerprint");
+
+    p.cargo("build -p foo -p foo_core -p foo-base").run();
+
+    let mut fingerprint_names = get_fingerprints_without_hashes(fingerprint_path);
+
+    // Artifacts present for all after building
+    assert!(fingerprint_names.iter().any(|e| e == "foo"));
+    let num_foo_core_artifacts = fingerprint_names
+        .iter()
+        .filter(|&e| e == "foo_core")
+        .count();
+    assert_ne!(num_foo_core_artifacts, 0);
+    let num_foo_base_artifacts = fingerprint_names
+        .iter()
+        .filter(|&e| e == "foo-base")
+        .count();
+    assert_ne!(num_foo_base_artifacts, 0);
+
+    p.cargo("clean -p foo").run();
+
+    fingerprint_names = get_fingerprints_without_hashes(fingerprint_path);
+
+    // Cleaning `foo` leaves artifacts for the others
+    assert!(!fingerprint_names.iter().any(|e| e == "foo"));
+    assert_eq!(
+        fingerprint_names
+            .iter()
+            .filter(|&e| e == "foo_core")
+            .count(),
+        num_foo_core_artifacts,
+    );
+    assert_eq!(
+        fingerprint_names
+            .iter()
+            .filter(|&e| e == "foo-base")
+            .count(),
+        num_foo_core_artifacts,
+    );
+}
+
+fn get_fingerprints_without_hashes(fingerprint_path: &Path) -> Vec<String> {
+    std::fs::read_dir(fingerprint_path)
+        .expect("Build dir should be readable")
+        .filter_map(|entry| entry.ok())
+        .map(|entry| {
+            let name = entry.file_name();
+            let name = name
+                .into_string()
+                .expect("fingerprint name should be UTF-8");
+            name.rsplit_once('-')
+                .expect("Name should contain at least one hyphen")
+                .0
+                .to_owned()
+        })
+        .collect()
+}
+
 #[cargo_test]
 fn clean_release() {
     let p = project()