From a38ad8026f146dcb195b63e0cc28719154d21a0d Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Tue, 29 Aug 2023 22:17:20 +0900 Subject: [PATCH] Cleanup --- CHANGELOG.md | 2 + src/features.rs | 5 +-- src/main.rs | 110 ++++++++++++++++++------------------------------ src/manifest.rs | 30 +++++++++---- src/rustup.rs | 2 +- tests/test.rs | 67 +++++++++++++++-------------- 6 files changed, 104 insertions(+), 112 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21a02d23..c037d9d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ Note: In this file, do not use the hard wrap in the middle of a sentence for com ## [Unreleased] +- Fix bug in `--no-private` flag with virtual workspace. + ## [0.6.3] - 2023-08-28 - Fix bug in `--no-private` flag on Windows. diff --git a/src/features.rs b/src/features.rs index 742f8ccf..a85e0792 100644 --- a/src/features.rs +++ b/src/features.rs @@ -170,6 +170,7 @@ pub(crate) fn feature_powerset<'a>( let deps_map = feature_deps(map); powerset(features, depth) .into_iter() + .skip(1) // The first element of a powerset is `[]` so it should be skipped. .filter(|fs| { !fs.iter().any(|f| { f.as_group().iter().filter_map(|f| deps_map.get(&&**f)).any(|deps| { @@ -274,9 +275,7 @@ mod tests { vec!["a", "b", "c", "d"], ]); let filtered = feature_powerset(list.iter().collect::>(), None, &map); - assert_eq!(filtered, vec![vec![], vec!["a"], vec!["b"], vec!["c"], vec!["d"], vec![ - "c", "d" - ]]); + assert_eq!(filtered, vec![vec!["a"], vec!["b"], vec!["c"], vec!["d"], vec!["c", "d"]]); } #[test] diff --git a/src/main.rs b/src/main.rs index c3f4fbc2..fc8ceba9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,7 +1,12 @@ #![forbid(unsafe_code)] #![warn(rust_2018_idioms, single_use_lifetimes, unreachable_pub)] #![warn(clippy::pedantic)] -#![allow(clippy::cast_lossless, clippy::struct_excessive_bools, clippy::too_many_lines)] +#![allow( + clippy::cast_lossless, + clippy::single_match_else, + clippy::struct_excessive_bools, + clippy::too_many_lines +)] #[macro_use] mod term; @@ -67,52 +72,34 @@ fn try_main() -> Result<()> { } } let line = cmd!("cargo"); - { - // First, generate the lockfile using the oldest cargo specified. - // https://github.com/taiki-e/cargo-hack/issues/105 - let toolchain = &range[0].1; - rustup::install_toolchain(toolchain, &cx.target, true)?; - let mut line = line.clone(); - line.leading_arg(toolchain); - line.arg("generate-lockfile"); - if let Some(pid) = cx.current_package() { - let package = cx.packages(pid); - if !cx.no_manifest_path { - line.arg("--manifest-path"); - line.arg( - package - .manifest_path - .strip_prefix(&cx.current_dir) - .unwrap_or(&package.manifest_path), - ); - } - } - line.run_with_output()?; - } + // First, generate the lockfile using the oldest cargo specified. + // https://github.com/taiki-e/cargo-hack/issues/105 + let mut generate_lockfile = true; + // Workaround for spurious "failed to select a version" error. + // (This does not work around the underlying cargo bug: https://github.com/rust-lang/cargo/issues/10623) let mut regenerate_lockfile_on_51_or_up = false; - range.iter().enumerate().try_for_each(|(i, (cargo_version, toolchain))| { - if i != 0 { - rustup::install_toolchain(toolchain, &cx.target, true)?; - if regenerate_lockfile_on_51_or_up && *cargo_version >= 51 { - let mut line = line.clone(); - line.leading_arg(toolchain); - line.arg("generate-lockfile"); - if let Some(pid) = cx.current_package() { - let package = cx.packages(pid); - if !cx.no_manifest_path { - line.arg("--manifest-path"); - line.arg( - package - .manifest_path - .strip_prefix(&cx.current_dir) - .unwrap_or(&package.manifest_path), - ); - } + for (cargo_version, toolchain) in range { + rustup::install_toolchain(toolchain, &cx.target, true)?; + if generate_lockfile || regenerate_lockfile_on_51_or_up && *cargo_version >= 51 { + let mut line = line.clone(); + line.leading_arg(toolchain); + line.arg("generate-lockfile"); + if let Some(pid) = cx.current_package() { + let package = cx.packages(pid); + if !cx.no_manifest_path { + line.arg("--manifest-path"); + line.arg( + package + .manifest_path + .strip_prefix(&cx.current_dir) + .unwrap_or(&package.manifest_path), + ); } - line.run_with_output()?; - regenerate_lockfile_on_51_or_up = false; } + line.run_with_output()?; + generate_lockfile = false; + regenerate_lockfile_on_51_or_up = false; } if *cargo_version < 51 { regenerate_lockfile_on_51_or_up = true; @@ -132,8 +119,8 @@ fn try_main() -> Result<()> { &mut progress, &mut keep_going, *cargo_version, - ) - })?; + )?; + } } else { let mut line = cx.cargo(); line.apply_context(cx); @@ -259,8 +246,7 @@ fn determine_kind<'a>( progress.total += 1; Kind::Normal } else { - // -1: the first element of a powerset is `[]` - progress.total += features.len() - 1 + progress.total += features.len() + !cx.exclude_no_default_features as usize + (!cx.exclude_all_features && pkg_features.normal().len() + pkg_features.optional_deps().len() > 1) @@ -373,28 +359,15 @@ fn exec_on_package( ); } - exec_actual(cx, id, kind, &mut line, progress, keep_going) -} - -fn exec_actual( - cx: &Context, - id: &PackageId, - kind: &Kind<'_>, - line: &mut ProcessBuilder<'_>, - progress: &mut Progress, - keep_going: &mut KeepGoing, -) -> Result<()> { match kind { - Kind::SkipAsPrivate => unreachable!(), Kind::Normal => { // only run with default features - return exec_cargo(cx, id, line, progress, keep_going); + return exec_cargo(cx, id, &mut line, progress, keep_going); } Kind::Each { .. } | Kind::Powerset { .. } => {} + Kind::SkipAsPrivate => unreachable!(), } - let mut line = line.clone(); - if !cx.no_default_features { line.arg("--no-default-features"); } @@ -411,15 +384,14 @@ fn exec_actual( match kind { Kind::Each { features } => { - features.iter().try_for_each(|f| { - exec_cargo_with_features(cx, id, &line, progress, keep_going, Some(f)) - })?; + for f in features { + exec_cargo_with_features(cx, id, &line, progress, keep_going, Some(f))?; + } } Kind::Powerset { features } => { - // The first element of a powerset is `[]` so it should be skipped. - features.iter().skip(1).try_for_each(|f| { - exec_cargo_with_features(cx, id, &line, progress, keep_going, f) - })?; + for f in features { + exec_cargo_with_features(cx, id, &line, progress, keep_going, f)?; + } } _ => unreachable!(), } diff --git a/src/manifest.rs b/src/manifest.rs index e8bb6e45..0fd4487d 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -111,7 +111,6 @@ pub(crate) fn with(cx: &Context, f: impl FnOnce() -> Result<()>) -> Result<()> { let mut restore_handles = Vec::with_capacity(cx.metadata.workspace_members.len()); let workspace_root = &cx.metadata.workspace_root; let root_manifest = &workspace_root.join("Cargo.toml"); - let mut has_root_crate = false; let mut root_id = None; let mut private_crates = vec![]; for id in &cx.metadata.workspace_members { @@ -121,7 +120,6 @@ pub(crate) fn with(cx: &Context, f: impl FnOnce() -> Result<()>) -> Result<()> { if is_root { root_id = Some(id); } - has_root_crate |= is_root; let is_private = cx.is_private(id); if is_private && no_private { if is_root { @@ -131,7 +129,7 @@ pub(crate) fn with(cx: &Context, f: impl FnOnce() -> Result<()>) -> Result<()> { } private_crates.push(manifest_path); } else if is_root && no_private { - // + // This case is handled in the if block after loop. } else if no_dev_deps { let manifest = cx.manifests(id); let mut doc = manifest.doc.clone(); @@ -143,11 +141,27 @@ pub(crate) fn with(cx: &Context, f: impl FnOnce() -> Result<()>) -> Result<()> { fs::write(manifest_path, doc.to_string())?; } } - if no_private && (no_dev_deps && has_root_crate || !private_crates.is_empty()) { + if no_private && (no_dev_deps && root_id.is_some() || !private_crates.is_empty()) { let manifest_path = root_manifest; - let manifest = cx.manifests(root_id.unwrap()); - let mut doc = manifest.doc.clone(); - if no_dev_deps && has_root_crate { + let (mut doc, orig) = match root_id { + Some(id) => { + let manifest = cx.manifests(id); + (manifest.doc.clone(), manifest.raw.clone()) + } + None => { + let orig = fs::read_to_string(manifest_path)?; + ( + orig.parse().with_context(|| { + format!( + "failed to parse manifest `{}` as toml", + manifest_path.display() + ) + })?, + orig, + ) + } + }; + if no_dev_deps && root_id.is_some() { if term::verbose() { info!("removing dev-dependencies from {}", manifest_path.display()); } @@ -159,7 +173,7 @@ pub(crate) fn with(cx: &Context, f: impl FnOnce() -> Result<()>) -> Result<()> { } remove_private_crates(&mut doc, &cx.metadata, &private_crates)?; } - restore_handles.push(cx.restore.register(&manifest.raw, manifest_path)); + restore_handles.push(cx.restore.register(orig, manifest_path)); fs::write(manifest_path, doc.to_string())?; } if restore_lockfile { diff --git a/src/rustup.rs b/src/rustup.rs index 82898787..b61d2577 100644 --- a/src/rustup.rs +++ b/src/rustup.rs @@ -31,7 +31,7 @@ pub(crate) fn version_range( if let Some(patch) = version.patch { warn!( "--version-range always selects the latest patch release per minor release, \ - not the specified patch release `{patch}`", + not the specified patch release `{patch}`", ); } Ok(()) diff --git a/tests/test.rs b/tests/test.rs index e7cfd59b..74bf4cd3 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -41,6 +41,7 @@ fn multi_arg() { "--feature-powerset", "--no-dev-deps", "--remove-dev-deps", + "--no-private", "--ignore-private", "--ignore-unknown-features", "--optional-deps", @@ -146,7 +147,9 @@ fn virtual_all_in_subcrate() { #[test] fn real_ignore_private() { - cargo_hack(["check", "--ignore-private"]) + // --no-private is not supported yet with workspace with private root crate + let flag = "--ignore-private"; + cargo_hack(["check", flag]) .assert_success("real") .stderr_not_contains( " @@ -159,7 +162,7 @@ fn real_ignore_private() { ) .stderr_contains("skipped running on private package `real`"); - cargo_hack(["check", "--all", "--ignore-private"]) + cargo_hack(["check", "--all", flag]) .assert_success("real") .stderr_contains( " @@ -181,35 +184,37 @@ fn real_ignore_private() { #[test] fn virtual_ignore_private() { - cargo_hack(["check", "--ignore-private"]) - .assert_success("virtual") - .stderr_contains( - " - running `cargo check` on member1 - skipped running on private package `member2` - ", - ) - .stderr_not_contains( - " - skipped running on private package `member1` - running `cargo check` on member2 - ", - ); - - cargo_hack(["check", "--all", "--ignore-private"]) - .assert_success("virtual") - .stderr_contains( - " - running `cargo check` on member1 - skipped running on private package `member2` - ", - ) - .stderr_not_contains( - " - running `cargo check` on member2 - skipped running on private package `member1` - ", - ); + for flag in ["--ignore-private", "--no-private"] { + cargo_hack(["check", flag]) + .assert_success("virtual") + .stderr_contains( + " + running `cargo check` on member1 + skipped running on private package `member2` + ", + ) + .stderr_not_contains( + " + skipped running on private package `member1` + running `cargo check` on member2 + ", + ); + + cargo_hack(["check", "--all", flag]) + .assert_success("virtual") + .stderr_contains( + " + running `cargo check` on member1 + skipped running on private package `member2` + ", + ) + .stderr_not_contains( + " + running `cargo check` on member2 + skipped running on private package `member1` + ", + ); + } } #[test]