From 1f2d5ef0ef56801c90cfd7d29b87df40b81962ea Mon Sep 17 00:00:00 2001 From: Lovecraftian Horror Date: Sun, 1 May 2022 20:24:39 -0500 Subject: [PATCH 1/4] Only remove fingerprints and build script artifacts of the requested package --- src/cargo/ops/cargo_clean.rs | 52 +++++++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index 08fb3ac181b..b55c2726e31 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -141,7 +141,12 @@ 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() { @@ -149,7 +154,12 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { // 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,17 +232,53 @@ fn escape_glob_path(pattern: &Path) -> CargoResult { Ok(glob::Pattern::escape(pattern)) } +fn rm_rf_package_glob_containing_hash( + package: &str, + pattern: &Path, + config: &Config, + progress: &mut dyn CleaningProgressBar, +) -> CargoResult<()> { + rm_rf_glob_helper(Some(package), pattern, config, progress) +} + fn rm_rf_glob( pattern: &Path, config: &Config, progress: &mut dyn CleaningProgressBar, +) -> CargoResult<()> { + rm_rf_glob_helper(None, pattern, config, progress) +} + +fn rm_rf_glob_helper( + package: Option<&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)? { - rm_rf(&path?, config, progress)?; + let path = path?; + + // Make sure the artifact is for `package` and not another crate that is prefixed by + // `package` by getting the original name stripped of the trialing hash and possible + // extension + if let Some(package) = package { + let pkg_name = path + .file_name() + .and_then(std::ffi::OsStr::to_str) + .and_then(|artifact| artifact.rsplit_once('-')) + .expect("artifact name is valid UTF-8 and contains at least one hyphen") + .0; + + if pkg_name != package { + continue; + } + } + + rm_rf(&path, config, progress)?; } Ok(()) } From a6d0e967984fc0ed0ea2d04510656e8759a46102 Mon Sep 17 00:00:00 2001 From: Lovecraftian Horror Date: Sat, 29 Oct 2022 22:28:49 -0600 Subject: [PATCH 2/4] Add test to verify functionality --- tests/testsuite/clean.rs | 63 +++++++++++++++++++++++++++++++++------- 1 file changed, 52 insertions(+), 11 deletions(-) diff --git a/tests/testsuite/clean.rs b/tests/testsuite/clean.rs index 98c1e54dded..06494440b1d 100644 --- a/tests/testsuite/clean.rs +++ b/tests/testsuite/clean.rs @@ -2,7 +2,8 @@ use cargo_test_support::registry::Package; use cargo_test_support::{ - basic_bin_manifest, basic_manifest, git, main_file, project, project_in, rustc_host, + basic_bin_manifest, basic_lib_manifest, basic_manifest, git, main_file, project, project_in, + rustc_host, }; use glob::GlobError; use std::env; @@ -96,26 +97,66 @@ 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> { - let pattern = path.to_str().expect("expected utf-8 path"); - let pattern = glob::Pattern::escape(pattern); +#[cargo_test] +fn clean_p_only_cleans_specified_package() { + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = [ + "foo", + "foo_core", + ] + "#, + ) + .file("foo/Cargo.toml", &basic_lib_manifest("foo")) + .file("foo/src/lib.rs", "//! foo") + .file("foo_core/Cargo.toml", &basic_lib_manifest("foo_core")) + .file("foo_core/src/lib.rs", "//! foo_core") + .build(); - #[cfg(not(target_env = "msvc"))] - const FILE: &str = "foo-*"; + let deps_path = &p.build_dir().join("debug").join("deps"); + let foo_glob = "foo-*"; + let foo_core_glob = "foo_core-*"; - #[cfg(target_env = "msvc")] - const FILE: &str = "foo.pdb"; + p.cargo("build -p foo -p foo_core").run(); + + // Artifacts present for both after building + assert!(!get_build_artifacts(deps_path, foo_glob).is_empty()); + let num_foo_core_artifacts = get_build_artifacts(deps_path, foo_core_glob).len(); + assert_ne!(num_foo_core_artifacts, 0); + + p.cargo("clean -p foo").run(); + + // Cleaning `foo` leaves artifacts for `foo_core` + assert!(get_build_artifacts(deps_path, foo_glob).is_empty()); + assert_eq!( + num_foo_core_artifacts, + get_build_artifacts(deps_path, foo_core_glob).len() + ); +} + +fn get_build_artifacts(path: &PathBuf, file_glob: &str) -> Vec> { + let pattern = path.to_str().expect("expected utf-8 path"); + let pattern = glob::Pattern::escape(pattern); - 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") From 604aeb5c24c44eebca2e5f09c88538466326c3b3 Mon Sep 17 00:00:00 2001 From: Lovecraftian Horror Date: Sat, 29 Oct 2022 23:05:19 -0600 Subject: [PATCH 3/4] Address review comments --- src/cargo/ops/cargo_clean.rs | 56 +++++++++++++++++------------------- 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index b55c2726e31..507c2d89b1c 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -232,25 +232,41 @@ fn escape_glob_path(pattern: &Path) -> CargoResult { 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<()> { - rm_rf_glob_helper(Some(package), pattern, config, progress) -} + // 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?; -fn rm_rf_glob( - pattern: &Path, - config: &Config, - progress: &mut dyn CleaningProgressBar, -) -> CargoResult<()> { - rm_rf_glob_helper(None, pattern, config, progress) + 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_helper( - package: Option<&str>, +fn rm_rf_glob( pattern: &Path, config: &Config, progress: &mut dyn CleaningProgressBar, @@ -260,25 +276,7 @@ fn rm_rf_glob_helper( .to_str() .ok_or_else(|| anyhow::anyhow!("expected utf-8 path"))?; for path in glob::glob(pattern)? { - let path = path?; - - // Make sure the artifact is for `package` and not another crate that is prefixed by - // `package` by getting the original name stripped of the trialing hash and possible - // extension - if let Some(package) = package { - let pkg_name = path - .file_name() - .and_then(std::ffi::OsStr::to_str) - .and_then(|artifact| artifact.rsplit_once('-')) - .expect("artifact name is valid UTF-8 and contains at least one hyphen") - .0; - - if pkg_name != package { - continue; - } - } - - rm_rf(&path, config, progress)?; + rm_rf(&path?, config, progress)?; } Ok(()) } From 2ebcc75bf4c72b8dba38dd108738d2540da294bb Mon Sep 17 00:00:00 2001 From: Lovecraftian Horror Date: Mon, 31 Oct 2022 20:43:20 -0600 Subject: [PATCH 4/4] Actually make test effective --- tests/testsuite/clean.rs | 87 +++++++++++++++++++++++++++++----------- 1 file changed, 63 insertions(+), 24 deletions(-) diff --git a/tests/testsuite/clean.rs b/tests/testsuite/clean.rs index 06494440b1d..8aa633381d4 100644 --- a/tests/testsuite/clean.rs +++ b/tests/testsuite/clean.rs @@ -2,8 +2,7 @@ use cargo_test_support::registry::Package; use cargo_test_support::{ - basic_bin_manifest, basic_lib_manifest, basic_manifest, git, main_file, project, project_in, - rustc_host, + basic_bin_manifest, basic_manifest, git, main_file, project, project_in, rustc_host, }; use glob::GlobError; use std::env; @@ -112,6 +111,18 @@ fn clean_multiple_packages_in_glob_char_path() { assert_eq!(get_build_artifacts(foo_path, file_glob).len(), 0); } +fn get_build_artifacts(path: &PathBuf, file_glob: &str) -> Vec> { + let pattern = path.to_str().expect("expected utf-8 path"); + let pattern = glob::Pattern::escape(pattern); + + 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::>>() +} + #[cargo_test] fn clean_p_only_cleans_specified_package() { let p = project() @@ -122,46 +133,74 @@ fn clean_p_only_cleans_specified_package() { members = [ "foo", "foo_core", + "foo-base", ] "#, ) - .file("foo/Cargo.toml", &basic_lib_manifest("foo")) + .file("foo/Cargo.toml", &basic_manifest("foo", "0.1.0")) .file("foo/src/lib.rs", "//! foo") - .file("foo_core/Cargo.toml", &basic_lib_manifest("foo_core")) + .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 deps_path = &p.build_dir().join("debug").join("deps"); - let foo_glob = "foo-*"; - let foo_core_glob = "foo_core-*"; + let fingerprint_path = &p.build_dir().join("debug").join(".fingerprint"); + + p.cargo("build -p foo -p foo_core -p foo-base").run(); - p.cargo("build -p foo -p foo_core").run(); + let mut fingerprint_names = get_fingerprints_without_hashes(fingerprint_path); - // Artifacts present for both after building - assert!(!get_build_artifacts(deps_path, foo_glob).is_empty()); - let num_foo_core_artifacts = get_build_artifacts(deps_path, foo_core_glob).len(); + // 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(); - // Cleaning `foo` leaves artifacts for `foo_core` - assert!(get_build_artifacts(deps_path, foo_glob).is_empty()); + 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, - get_build_artifacts(deps_path, foo_core_glob).len() ); } -fn get_build_artifacts(path: &PathBuf, file_glob: &str) -> Vec> { - let pattern = path.to_str().expect("expected utf-8 path"); - let pattern = glob::Pattern::escape(pattern); - - 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::>>() +fn get_fingerprints_without_hashes(fingerprint_path: &Path) -> Vec { + 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]