From 2dd47ac6ad5adcfee34f2060e4020739787ff29a Mon Sep 17 00:00:00 2001 From: Erik Zivkovic Date: Sat, 17 Dec 2022 17:37:17 +0100 Subject: [PATCH] Make the recursive option of cargo-sweep recurse past Cargo directories When passing the -r (recursive) option to cargo-sweep, it is supposed to be recursive over the subdirectories of the root path it is given (whether implicit or explicit). cargo-sweep erronously assumed that a Cargo project could not contain unrelated subprojects. This patch lets cargo-sweep keep recursing past cargo roots. The drawback of this patch is that cargo-sweep will need to recurse more, and as such becomes a bit slower to run. Fixes https://github.com/holmgr/cargo-sweep/issues/66 --- src/main.rs | 3 +- tests/integration.rs | 149 ++++++++++++++---- tests/nested-root-workspace/Cargo.toml | 7 + .../bin-crate/Cargo.toml | 8 + .../bin-crate/src/main.rs | 17 ++ .../crates/crate-one/Cargo.toml | 8 + .../crates/crate-one/src/lib.rs | 13 ++ .../crates/crate-two/Cargo.toml | 8 + .../crates/crate-two/src/lib.rs | 13 ++ 9 files changed, 197 insertions(+), 29 deletions(-) create mode 100644 tests/nested-root-workspace/Cargo.toml create mode 100644 tests/nested-root-workspace/bin-crate/Cargo.toml create mode 100644 tests/nested-root-workspace/bin-crate/src/main.rs create mode 100644 tests/nested-root-workspace/crates/crate-one/Cargo.toml create mode 100644 tests/nested-root-workspace/crates/crate-one/src/lib.rs create mode 100644 tests/nested-root-workspace/crates/crate-two/Cargo.toml create mode 100644 tests/nested-root-workspace/crates/crate-two/src/lib.rs diff --git a/src/main.rs b/src/main.rs index 091b828..7d79e76 100644 --- a/src/main.rs +++ b/src/main.rs @@ -110,7 +110,8 @@ fn find_cargo_projects(root: &Path, include_hidden: bool) -> Vec { } if let Some(target_directory) = is_cargo_root(entry.path()) { target_paths.insert(target_directory); - iter.skip_current_dir(); // no reason to look at the src and such + // Previously cargo-sweep skipped subdirectories here, but it is valid for + // subdirectories to contain cargo roots. } } } diff --git a/tests/integration.rs b/tests/integration.rs index 06e7456..3c0378d 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -8,7 +8,7 @@ use std::{ use anyhow::{Context, Result}; use assert_cmd::Command; use assert_cmd::{assert::Assert, cargo::cargo_bin}; -use fs_extra::dir::get_size; +use fs_extra::dir::{get_size, CopyOptions}; use predicates::{ prelude::PredicateBooleanExt, str::{contains, is_empty}, @@ -18,16 +18,19 @@ use pretty_assertions::{assert_eq, assert_ne}; use tempfile::{tempdir, TempDir}; struct AnyhowWithContext(anyhow::Error); + impl Debug for AnyhowWithContext { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{:#?}", self.0) } } + impl> From for AnyhowWithContext { fn from(err: T) -> Self { Self(err.into()) } } + type TestResult = Result<(), AnyhowWithContext>; fn test_dir() -> PathBuf { @@ -61,7 +64,10 @@ fn run(mut cmd: impl BorrowMut) -> Assert { assert } -/// Returns the size of the build directory. +/// Runs a cargo build of the named project (which is expected to exist as a direct +/// child of the `tests` directory. +/// Returns the size of the build directory, as well as the [TempDir] of the unique +/// temporary build target directory. fn build(project: &str) -> Result<(u64, TempDir)> { let target = tempdir()?; let old_size = get_size(target.path())?; @@ -73,7 +79,14 @@ fn build(project: &str) -> Result<(u64, TempDir)> { Ok((new_size, target)) } -fn clean_and_parse(target: &TempDir, args: &[&str]) -> Result { +/// Run the sweep command, cleaning up target files, and then parse the results and +/// return the number of cleaned files. This function takes a `cmd_modifier` which can +/// be used to modify the `sweep` [Command], e.g. adding or changing environment variables. +fn clean_and_parse( + args: &[&str], + // Use this to modify the Command generated by the sweep function. + cmd_modifier: impl Fn(&mut Command) -> &mut Command, +) -> Result { let dry_run = args.iter().any(|&f| f == "-d" || f == "--dry-run"); let (remove_msg, clean_msg) = if dry_run { @@ -81,51 +94,70 @@ fn clean_and_parse(target: &TempDir, args: &[&str]) -> Result { } else { ("Successfully removed", "Cleaned ") }; - let assertion = run(sweep(args).env("CARGO_TARGET_DIR", target.path())) - .stdout(contains(remove_msg).and(contains(clean_msg))); + let assertion = + run(cmd_modifier(&mut sweep(args))).stdout(contains(remove_msg).and(contains(clean_msg))); let output = assertion.get_output(); assert!(output.stderr.is_empty()); - let amount = std::str::from_utf8(&output.stdout)? + + // Collect all the lines that contain the "clean message". + let amount_lines: Vec<&str> = std::str::from_utf8(&output.stdout)? .lines() - .last() - .unwrap() - .split(clean_msg) - .nth(1) - .unwrap(); - let cleaned = amount - .parse::() - .context(format!("failed to parse amount {amount}"))? - .to_bytes(); + .filter(|line| line.contains(clean_msg)) + .map(|line| line.split(clean_msg).nth(1).unwrap()) + .collect(); + + // Turn all those lines into a collected sum of bytes called `clean`. + let cleaned: u64 = amount_lines + .iter() + .map(|amount| { + amount + .parse::() + .context(format!("failed to parse amount {amount}")) + .unwrap() + .to_bytes() + }) + .sum(); Ok(cleaned) } fn count_cleaned(target: &TempDir, args: &[&str], old_size: u64) -> Result { - let cleaned = clean_and_parse(target, args)?; + let cleaned = clean_and_parse(args, |cmd| cmd.env("CARGO_TARGET_DIR", target.path()))?; + assert_sweeped_size(target.path(), cleaned, old_size)?; + Ok(cleaned) +} - // Make sure this is accurate. - let new_size = get_size(target.path())?; - // Due to rounding and truncation we might have an inexact result. Make sure these are within 1% of each other, - // but don't require an exact match. - let calculated_size = old_size - cleaned; +/// Assert that the size of the target directory has the expected size +/// (within a small margin of error). +fn assert_sweeped_size( + path: impl AsRef, + // The number of bytes that were sweeped ("cleaned"). + sweep_size: u64, + // The size of the target directory (in bytes) before the sweep. + pre_sweep_size: u64, +) -> Result<()> { + // Make sure that the expected target directory size is accurate. + let new_size = get_size(path)?; + // Due to rounding and truncation we might have an inexact result. Make sure + // these are within 1% of each other, but don't require an exact match. + let calculated_size = pre_sweep_size - sweep_size; let diff = new_size.abs_diff(calculated_size); - let one_percent = old_size as f64 * 0.01; + let one_percent = pre_sweep_size as f64 * 0.01; assert!( diff <= one_percent as u64, - "new_size={}, old_size={}, cleaned={}, diff={diff}, 1%={one_percent}", + "new_size={}, pre_sweep_size={}, sweep_size={}, diff={diff}, 1%={one_percent}", new_size, - old_size, - cleaned + pre_sweep_size, + sweep_size, ); - - Ok(cleaned) + Ok(()) } fn count_cleaned_dry_run(target: &TempDir, args: &[&str], old_size: u64) -> Result { let mut args = args.to_vec(); args.push("--dry-run"); - let cleaned = clean_and_parse(target, &args)?; + let cleaned = clean_and_parse(&args, |cmd| cmd.env("CARGO_TARGET_DIR", target.path()))?; let new_size = get_size(target.path())?; assert_eq!(old_size, new_size); @@ -282,3 +314,64 @@ fn subcommand_usage() -> TestResult { fn standalone_usage() -> TestResult { golden_reference(&["-h"], "tests/standalone-usage.txt") } + +/// This test is a bit more verbose than the other tests because it uses a slightly +/// different mechanism for setting up and sweeping / cleaning. +/// +/// Step-by-step: +/// +/// * Create a new temporary directory. Other tests only output the built files into +/// the target directory, but this test copies the whole template project into the +/// target directory. +/// * Build the two different (nested) projects using cargo build, but without incremental +/// compilation, because cargo-sweep doesn't remove files built by incremental compilation, +/// which affects this test since this test doesn't use a separate target folder for just +/// the target files. +/// * Do a dry run sweep, asserting that nothing was actually cleaned. +/// * Do a proper sweep, asserting that the target directory size is back down to its +/// expected size. +#[test] +fn recursive_multiple_root_workspaces() -> TestResult { + let target = tempdir()?; + + let nested_workspace_dir = test_dir().join("nested-root-workspace"); + let options = CopyOptions::default(); + + fs_extra::copy_items(&[&nested_workspace_dir], target.path(), &options)?; + + let old_size = get_size(target.path())?; + + // Build bin-crate + run(Command::new(env!("CARGO")) + .env("CARGO_INCREMENTAL", "0") + .arg("build") + .current_dir(target.path().join("nested-root-workspace/bin-crate"))); + + let intermediate_build_size = get_size(target.path())?; + assert!(intermediate_build_size > old_size); + + // Build workspace crates + run(Command::new(env!("CARGO")) + .env("CARGO_INCREMENTAL", "0") + .arg("build") + .current_dir(target.path().join("nested-root-workspace"))); + + let final_build_size = get_size(target.path())?; + assert!(final_build_size > intermediate_build_size); + + // Run a dry-run of cargo-sweep ("clean") in the target directory (recursive) + let args = &["-r", "--time", "0", "--dry-run"]; + let expected_cleaned = clean_and_parse(args, |cmd| cmd.current_dir(target.path()))?; + assert!(expected_cleaned > 0); + let size_after_dry_run_clean = get_size(target.path())?; + // Make sure that nothing was actually cleaned + assert_eq!(final_build_size, size_after_dry_run_clean); + + // Run a proper cargo-sweep ("clean) in the target directory (recursive) + let args = &["-r", "--time", "0"]; + let actual_cleaned = clean_and_parse(args, |cmd| cmd.current_dir(target.path()))?; + assert_sweeped_size(target.path(), actual_cleaned, final_build_size)?; + assert_eq!(actual_cleaned, expected_cleaned); + + Ok(()) +} diff --git a/tests/nested-root-workspace/Cargo.toml b/tests/nested-root-workspace/Cargo.toml new file mode 100644 index 0000000..b4d859d --- /dev/null +++ b/tests/nested-root-workspace/Cargo.toml @@ -0,0 +1,7 @@ +[workspace] +members = [ + "crates/*", +] +exclude = [ + "bin-crate", +] diff --git a/tests/nested-root-workspace/bin-crate/Cargo.toml b/tests/nested-root-workspace/bin-crate/Cargo.toml new file mode 100644 index 0000000..c5f9791 --- /dev/null +++ b/tests/nested-root-workspace/bin-crate/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "bin-crate" +version = "0.1.0" +edition = "2021" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] diff --git a/tests/nested-root-workspace/bin-crate/src/main.rs b/tests/nested-root-workspace/bin-crate/src/main.rs new file mode 100644 index 0000000..93efce5 --- /dev/null +++ b/tests/nested-root-workspace/bin-crate/src/main.rs @@ -0,0 +1,17 @@ +fn main() { + println!("{}", bin_string()); +} + +fn bin_string() -> String { + "This is bin-crate".to_string() +} + +#[cfg(test)] +mod test { + use super::bin_string; + + #[test] + fn test() { + assert_eq!("This is bin-crate".to_string(), bin_string()); + } +} diff --git a/tests/nested-root-workspace/crates/crate-one/Cargo.toml b/tests/nested-root-workspace/crates/crate-one/Cargo.toml new file mode 100644 index 0000000..df8e0fc --- /dev/null +++ b/tests/nested-root-workspace/crates/crate-one/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "crate-one" +version = "0.1.0" +edition = "2021" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] diff --git a/tests/nested-root-workspace/crates/crate-one/src/lib.rs b/tests/nested-root-workspace/crates/crate-one/src/lib.rs new file mode 100644 index 0000000..8f3bba6 --- /dev/null +++ b/tests/nested-root-workspace/crates/crate-one/src/lib.rs @@ -0,0 +1,13 @@ +pub fn crate_one_string() -> String { + "This is crate-one".to_string() +} + +#[cfg(test)] +mod test { + use super::crate_one_string; + + #[test] + fn test() { + assert_eq!("This is crate-one".to_string(), crate_one_string()); + } +} diff --git a/tests/nested-root-workspace/crates/crate-two/Cargo.toml b/tests/nested-root-workspace/crates/crate-two/Cargo.toml new file mode 100644 index 0000000..df1daae --- /dev/null +++ b/tests/nested-root-workspace/crates/crate-two/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "crate-two" +version = "0.1.0" +edition = "2021" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] diff --git a/tests/nested-root-workspace/crates/crate-two/src/lib.rs b/tests/nested-root-workspace/crates/crate-two/src/lib.rs new file mode 100644 index 0000000..f9aaf7e --- /dev/null +++ b/tests/nested-root-workspace/crates/crate-two/src/lib.rs @@ -0,0 +1,13 @@ +pub fn crate_two_string() -> String { + "This is crate-two".to_string() +} + +#[cfg(test)] +mod test { + use super::crate_two_string; + + #[test] + fn test() { + assert_eq!("This is crate-two".to_string(), crate_two_string()); + } +}