Skip to content

Commit

Permalink
Cleanup
Browse files Browse the repository at this point in the history
  • Loading branch information
@taiki-e
taiki-e committed Aug 29, 2023
1 parent 13a9f17 commit a38ad80
Show file tree
Hide file tree
Showing 6 changed files with 104 additions and 112 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
5 changes: 2 additions & 3 deletions src/features.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Expand Down Expand Up @@ -274,9 +275,7 @@ mod tests {
vec!["a", "b", "c", "d"],
]);
let filtered = feature_powerset(list.iter().collect::<Vec<_>>(), 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]
Expand Down
110 changes: 41 additions & 69 deletions src/main.rs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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");
}
Expand All @@ -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!(),
}
Expand Down
30 changes: 22 additions & 8 deletions src/manifest.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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();
Expand All @@ -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());
}
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion src/rustup.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
Expand Down
67 changes: 36 additions & 31 deletions tests/test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ fn multi_arg() {
"--feature-powerset",
"--no-dev-deps",
"--remove-dev-deps",
"--no-private",
"--ignore-private",
"--ignore-unknown-features",
"--optional-deps",
Expand Down Expand Up @@ -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(
"
Expand All @@ -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(
"
Expand All @@ -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]
Expand Down

0 comments on commit a38ad80

Please sign in to comment.