Add --log-group option to opt-out/force-enable log grouping

taiki-e · Sep 9, 2023 · f57aae1 · f57aae1
1 parent dd5481a
commit f57aae1
Show file tree

Hide file tree

Showing 7 changed files with 115 additions and 17 deletions.
diff --git a/README.md b/README.md
@@ -202,6 +202,11 @@ OPTIONS:
         --keep-going
             Keep going on failure.
 
+        --log-group <KIND>
+            Log grouping: none, github-actions.
+
+            If this option is not used, the environment will be automatically detected.
+
         --print-command-list
             Print commands without run (Unstable).
 

diff --git a/src/cli.rs b/src/cli.rs
@@ -11,7 +11,7 @@ use lexopt::{
     ValueExt,
 };
 
-use crate::{term, version::VersionRange, Feature, Rustup};
+use crate::{term, version::VersionRange, Feature, LogGroup, Rustup};
 
 pub(crate) struct Args {
     pub(crate) leading_args: Vec<String>,
@@ -51,10 +51,12 @@ pub(crate) struct Args {
     pub(crate) keep_going: bool,
     /// --print-command-list
     pub(crate) print_command_list: bool,
-    /// --version-range
+    /// --version-range/--rust-version
     pub(crate) version_range: Option<VersionRange>,
     /// --version-step
     pub(crate) version_step: u16,
+    /// --log-group
+    pub(crate) log_group: LogGroup,
 
     // options for --each-feature and --feature-powerset
     /// --optional-deps [DEPS]...
@@ -152,6 +154,7 @@ impl Args {
         let mut rust_version = false;
         let mut version_range = None;
         let mut version_step = None;
+        let mut log_group: Option<String> = None;
 
         let mut optional_deps = None;
         let mut include_features = vec![];
@@ -240,6 +243,7 @@ impl Args {
                 Long("rust-version") => parse_flag!(rust_version),
                 Long("version-range") => parse_opt!(version_range, false),
                 Long("version-step") => parse_opt!(version_step, false),
+                Long("log-group") => parse_opt!(log_group, false),
 
                 Short('p') | Long("package") => package.push(parser.value()?.parse()?),
                 Long("exclude") => exclude.push(parser.value()?.parse()?),
@@ -544,6 +548,11 @@ impl Args {
             bail!("--version-step cannot be zero");
         }
 
+        let log_group = match log_group {
+            Some(v) => v.parse()?,
+            None => LogGroup::auto(),
+        };
+
         if no_dev_deps {
             info!(
                 "--no-dev-deps removes dev-dependencies from real `Cargo.toml` while cargo-hack is running and restores it when finished"
@@ -593,6 +602,7 @@ impl Args {
             include_deps_features,
             version_range,
             version_step,
+            log_group,
 
             depth,
             group_features,
@@ -789,6 +799,9 @@ const HELP: &[HelpText<'_>] = &[
         "This flag can only be used together with --version-range flag.",
     ]),
     ("", "--keep-going", "", "Keep going on failure", &[]),
+    ("", "--log-group", "<KIND>", "Log grouping: none, github-actions", &[
+        "If this option is not used, the environment will be automatically detected."
+    ]),
     ("", "--print-command-list", "", "Print commands without run (Unstable)", &[]),
     ("", "--no-manifest-path", "", "Do not pass --manifest-path option to cargo (Unstable)", &[]),
     ("-v", "--verbose", "", "Use verbose output", &[]),

diff --git a/src/context.rs b/src/context.rs
@@ -27,7 +27,6 @@ pub(crate) struct Context {
     pub(crate) cargo_version: u32,
     pub(crate) restore: restore::Manager,
     pub(crate) current_dir: PathBuf,
-    pub(crate) use_github_action_grouping: bool,
 }
 
 impl Context {
@@ -74,8 +73,6 @@ impl Context {
             cargo_version,
             restore,
             current_dir: env::current_dir()?,
-            // TODO: should be optional?
-            use_github_action_grouping: env::var_os("GITHUB_ACTIONS").is_some(),
         };
 
         // TODO: Ideally, we should do this, but for now, we allow it as cargo-hack

diff --git a/src/main.rs b/src/main.rs
@@ -29,9 +29,10 @@ use std::{
     collections::BTreeMap,
     env,
     fmt::{self, Write},
+    str::FromStr,
 };
 
-use anyhow::{bail, format_err, Result};
+use anyhow::{bail, format_err, Error, Result};
 
 use crate::{
     context::Context,
@@ -510,6 +511,36 @@ impl fmt::Display for KeepGoing {
     }
 }
 
+#[derive(Clone, Copy, PartialEq)]
+enum LogGroup {
+    None,
+    GithubActions,
+}
+
+impl LogGroup {
+    fn auto() -> Self {
+        if env::var_os("GITHUB_ACTIONS").is_some() {
+            Self::GithubActions
+        } else {
+            Self::None
+        }
+    }
+}
+
+impl FromStr for LogGroup {
+    type Err = Error;
+
+    fn from_str(s: &str) -> std::result::Result<Self, Self::Err> {
+        match s {
+            "none" => Ok(Self::None),
+            "github-actions" => Ok(Self::GithubActions),
+            other => bail!(
+                "argument for --log-group must be none or github-actions, but found `{other}`"
+            ),
+        }
+    }
+}
+
 fn exec_cargo(
     cx: &Context,
     id: &PackageId,
@@ -540,7 +571,7 @@ fn exec_cargo_inner(
     line: &mut ProcessBuilder<'_>,
     progress: &mut Progress,
 ) -> Result<()> {
-    if progress.count != 0 && !cx.print_command_list && !cx.use_github_action_grouping {
+    if progress.count != 0 && !cx.print_command_list && cx.log_group == LogGroup::None {
         eprintln!();
     }
     progress.count += 1;
@@ -562,18 +593,21 @@ fn exec_cargo_inner(
         write!(msg, "running {line} on {}", cx.packages(id).name).unwrap();
     }
     write!(msg, " ({}/{})", progress.count, progress.total).unwrap();
-    let _guard = if cx.use_github_action_grouping {
-        struct Guard;
-        impl Drop for Guard {
-            fn drop(&mut self) {
-                println!("::endgroup::");
+    let _guard = match cx.log_group {
+        LogGroup::GithubActions => {
+            struct Guard;
+            impl Drop for Guard {
+                fn drop(&mut self) {
+                    println!("::endgroup::");
+                }
             }
+            println!("::group::{msg}");
+            Some(Guard)
+        }
+        LogGroup::None => {
+            info!("{msg}");
+            None
         }
-        println!("::group::{msg}");
-        Some(Guard)
-    } else {
-        info!("{msg}");
-        None
     };
 
     line.run()

diff --git a/tests/long-help.txt b/tests/long-help.txt
@@ -173,6 +173,11 @@ OPTIONS:
         --keep-going
             Keep going on failure.
 
+        --log-group <KIND>
+            Log grouping: none, github-actions.
+
+            If this option is not used, the environment will be automatically detected.
+
         --print-command-list
             Print commands without run (Unstable).
 

diff --git a/tests/short-help.txt b/tests/short-help.txt
@@ -43,6 +43,7 @@ OPTIONS:
                                          command
         --clean-per-version              Remove artifacts per Rust version
         --keep-going                     Keep going on failure
+        --log-group <KIND>               Log grouping: none, github-actions
         --print-command-list             Print commands without run (Unstable)
         --no-manifest-path               Do not pass --manifest-path option to cargo (Unstable)
     -v, --verbose                        Use verbose output

diff --git a/tests/test.rs b/tests/test.rs
@@ -299,6 +299,49 @@ fn exclude_failure() {
         .stderr_contains("--exclude can only be used together with --workspace");
 }
 
+#[test]
+fn log_group() {
+    cargo_hack(["check", "--all", "--log-group", "none"])
+        .assert_success("virtual")
+        .stderr_contains(
+            "
+            running `cargo check` on member1
+            running `cargo check` on member2
+            ",
+        )
+        .stdout_not_contains(
+            "
+            ::endgroup::
+            ::group::
+            ",
+        )
+        .stderr_not_contains(
+            "
+            ::endgroup::
+            ::group::
+            ",
+        );
+
+    cargo_hack(["check", "--all", "--log-group", "github-actions"])
+        .assert_success("virtual")
+        .stdout_contains(
+            "
+            ::group::running `cargo check` on member1
+            ::endgroup::
+            ::group::running `cargo check` on member2
+            ::endgroup::
+            ",
+        )
+        .stderr_not_contains(
+            "
+            ::group::running `cargo check` on member1
+            ::endgroup::
+            ::group::running `cargo check` on member2
+            ::endgroup::
+            ",
+        );
+}
+
 #[test]
 fn no_dev_deps() {
     cargo_hack(["check", "--no-dev-deps"]).assert_success("real").stderr_contains(