From c19edad9ac98e3b2f19822a3cdb6df612dcac428 Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Wed, 24 Jan 2024 23:06:07 +0900 Subject: [PATCH] Fix handling of glob members in --no-private --- src/cli.rs | 11 +++++++-- src/manifest.rs | 59 +++++++++++++++++++++++++++++++++++-------------- tests/test.rs | 8 +++---- 3 files changed, 56 insertions(+), 22 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index be00f01a..33c26c85 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -561,9 +561,16 @@ impl Args { None => LogGroup::auto(), }; - if no_dev_deps { + if no_dev_deps || no_private { + let flag = if no_dev_deps && no_private { + "--no-dev-deps and --no-private modify" + } else if no_dev_deps { + "--no-dev-deps modifies" + } else { + "--no-private modifies" + }; info!( - "--no-dev-deps removes dev-dependencies from real `Cargo.toml` while cargo-hack is running and restores it when finished" + "{flag} real `Cargo.toml` while cargo-hack is running and restores it when finished" ); } diff --git a/src/manifest.rs b/src/manifest.rs index 5b6e073d..98acf460 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -1,6 +1,9 @@ // SPDX-License-Identifier: Apache-2.0 OR MIT -use std::{collections::BTreeMap, path::Path}; +use std::{ + collections::{BTreeMap, BTreeSet}, + path::Path, +}; use anyhow::{bail, format_err, Context as _, Result}; @@ -106,7 +109,7 @@ pub(crate) fn with(cx: &Context, f: impl FnOnce() -> Result<()>) -> Result<()> { let workspace_root = &cx.metadata.workspace_root; let root_manifest = &workspace_root.join("Cargo.toml"); let mut root_id = None; - let mut private_crates = vec![]; + let mut private_crates = BTreeSet::new(); for id in &cx.metadata.workspace_members { let package = cx.packages(id); let manifest_path = &*package.manifest_path; @@ -121,7 +124,7 @@ pub(crate) fn with(cx: &Context, f: impl FnOnce() -> Result<()>) -> Result<()> { "--no-private is not supported yet with workspace with private root crate" ); } - private_crates.push(manifest_path); + private_crates.insert(manifest_path); } else if is_root && no_private { // This case is handled in the if block after loop. } else if no_dev_deps { @@ -165,7 +168,7 @@ pub(crate) fn with(cx: &Context, f: impl FnOnce() -> Result<()>) -> Result<()> { if term::verbose() { info!("removing private crates from {}", manifest_path.display()); } - remove_private_crates(&mut doc, workspace_root, &private_crates)?; + remove_private_crates(&mut doc, workspace_root, private_crates); } restore_handles.push(cx.restore.register(orig, manifest_path)); fs::write(manifest_path, doc.to_string())?; @@ -205,8 +208,8 @@ fn remove_dev_deps(doc: &mut toml_edit::Document) { fn remove_private_crates( doc: &mut toml_edit::Document, workspace_root: &Path, - private_crates: &[&Path], -) -> Result<()> { + mut private_crates: BTreeSet<&Path>, +) { let table = doc.as_table_mut(); if let Some(workspace) = table.get_mut("workspace").and_then(toml_edit::Item::as_table_like_mut) { @@ -216,25 +219,49 @@ fn remove_private_crates( while i < members.len() { if let Some(member) = members.get(i).and_then(toml_edit::Value::as_str) { let manifest_path = workspace_root.join(member).join("Cargo.toml"); - if private_crates - .iter() - .find_map(|p| { - same_file::is_same_file(p, &manifest_path) - .map(|v| if v { Some(()) } else { None }) - .transpose() + if let Some(p) = private_crates.iter().find_map(|p| { + same_file::is_same_file(p, &manifest_path).ok().and_then(|v| { + if v { + Some(*p) + } else { + None + } }) - .transpose()? - .is_some() - { + }) { members.remove(i); + private_crates.remove(p); continue; } } i += 1; } } + if private_crates.is_empty() { + return; + } + // Handles the case that the members field contains glob. + // TODO: test that it also works when public and private crates are nested. + if let Some(exclude) = workspace.get_mut("exclude").and_then(toml_edit::Item::as_array_mut) + { + for private_crate in private_crates { + exclude.push(private_crate.parent().unwrap().to_str().unwrap()); + } + } else { + workspace.insert( + "exclude", + toml_edit::Item::Value(toml_edit::Value::Array( + private_crates + .iter() + .map(|p| { + toml_edit::Value::String(toml_edit::Formatted::new( + p.parent().unwrap().to_str().unwrap().to_owned(), + )) + }) + .collect::(), + )), + ); + } } - Ok(()) } #[cfg(test)] diff --git a/tests/test.rs b/tests/test.rs index ecd96981..7bc8823a 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -348,16 +348,16 @@ fn no_dev_deps() { cargo_hack(["check", "--no-dev-deps"]).assert_success("real").stderr_contains( " running `cargo check` on real - --no-dev-deps removes dev-dependencies from real `Cargo.toml` while cargo-hack is \ - running and restores it when finished + --no-dev-deps modifies real `Cargo.toml` while cargo-hack is running and \ + restores it when finished ", ); // with --all cargo_hack(["check", "--no-dev-deps", "--all"]).assert_success("real").stderr_contains( " - --no-dev-deps removes dev-dependencies from real `Cargo.toml` while cargo-hack is \ - running and restores it when finished + --no-dev-deps modifies real `Cargo.toml` while cargo-hack is running and \ + restores it when finished ", ); }