diff --git a/Cargo.lock b/Cargo.lock index 17400205..7a561c63 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -406,6 +406,7 @@ dependencies = [ "chrono", "clang-tools-manager", "clap", + "colored", "fast-glob", "futures", "git-bot-feedback", diff --git a/Cargo.toml b/Cargo.toml index 21f93187..1bb9f4ca 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,6 +22,7 @@ strip = "symbols" [workspace.dependencies] anyhow = "1.0.102" clap = { version = "4.6.1", features = ["derive"] } +colored = "3.1.1" log = { version = "0.4.31", features = ["std"] } mockito = "1.7.2" pyo3 = {version = "0.29.0", features = ["extension-module"] } diff --git a/clang-tools-manager/Cargo.toml b/clang-tools-manager/Cargo.toml index 1c570066..4de24a46 100644 --- a/clang-tools-manager/Cargo.toml +++ b/clang-tools-manager/Cargo.toml @@ -25,7 +25,7 @@ required-features = ["bin"] anyhow = { workspace = true, optional = true } blake2 = "0.11.0" clap = { workspace = true, optional = true } -colored = { version = "3.1.1", optional = true } +colored = { workspace = true, optional = true } directories = "6.0.0" log = { workspace = true } regex = { workspace = true } diff --git a/cpp-linter/Cargo.toml b/cpp-linter/Cargo.toml index fe0b3dff..9124da90 100644 --- a/cpp-linter/Cargo.toml +++ b/cpp-linter/Cargo.toml @@ -19,6 +19,7 @@ async-trait = "0.1.89" chrono = "0.4.44" clang-tools-manager = { path = "../clang-tools-manager", features = ["bin"], version = "0.3.1" } clap = { workspace = true, optional = true } +colored = { workspace = true } fast-glob = "1.0.1" futures = "0.3.32" gix-imara-diff = { version = "0.2.2", features = ["unified_diff"] } diff --git a/cpp-linter/src/error.rs b/cpp-linter/src/error.rs index d934ff7b..b01343e6 100644 --- a/cpp-linter/src/error.rs +++ b/cpp-linter/src/error.rs @@ -70,6 +70,14 @@ pub enum ClientError { #[source] source: std::io::Error, }, + + /// Error when failing to write to stdout. + #[error("Failed to write to stdout: {0}")] + StdoutWriteFailed(#[source] std::io::Error), + + /// Error when failing to read the generated patch file. + #[error("Failed to read the generated patch file: {0}")] + PatchReadFailed(#[source] std::io::Error), } /// Errors related to invoking clang tools and processing their output. diff --git a/cpp-linter/src/rest_client.rs b/cpp-linter/src/rest_client.rs index 64bd012d..8b457687 100644 --- a/cpp-linter/src/rest_client.rs +++ b/cpp-linter/src/rest_client.rs @@ -2,10 +2,12 @@ use std::{ env, + io::{BufRead, Write, stdout}, path::PathBuf, sync::{Arc, Mutex}, }; +use colored::Colorize; use git_bot_feedback::{ AnnotationLevel, CommentKind, CommentPolicy, FileAnnotation, FileFilter, LinesChangedOnly, OutputVariable, RestApiClient, ReviewAction, ReviewOptions, ThreadCommentOptions, @@ -145,11 +147,11 @@ impl RestClient { if feedback_inputs.step_summary { self.client.append_step_summary(&summary)?; } - if let Some(summary_output_file) = feedback_inputs.summary_output_file { + if let Some(summary_output_file) = &feedback_inputs.summary_output_file { let output_file = if summary_output_file.is_absolute() { summary_output_file } else { - feedback_inputs.repo_root.join(&summary_output_file) + &feedback_inputs.repo_root.join(summary_output_file) }; if let Some(parent) = output_file.parent() { std::fs::create_dir_all(parent).map_err(|e| ClientError::MkDirFailed { @@ -157,9 +159,9 @@ impl RestClient { source: e, })?; } - std::fs::write(&output_file, &summary).map_err(|e| { + std::fs::write(output_file, &summary).map_err(|e| { ClientError::SummaryOutputFileWriteFailed { - file_path: output_file, + file_path: output_file.to_owned(), source: e, } })?; @@ -182,35 +184,6 @@ impl RestClient { ]; self.client.write_output_variables(&output_vars)?; - if feedback_inputs.thread_comments != ThreadComments::Off { - // post thread comment for PR or push event - if comment.as_ref().is_none_or(|c| c.len() > u16::MAX as usize) { - comment = Some(Self::make_comment( - files, - format_checks_failed, - tidy_checks_failed, - &clang_versions, - Some(u16::MAX as u64), - )?); - } - let options = ThreadCommentOptions { - policy: if feedback_inputs.thread_comments == ThreadComments::Update { - CommentPolicy::Update - } else { - // feedback_inputs.thread_comments is not Off and not Update, so it must be just On. - CommentPolicy::Anew - }, - comment: comment.unwrap_or_default(), - kind: if format_checks_failed == 0 && tidy_checks_failed == 0 { - CommentKind::Lgtm - } else { - CommentKind::Concerns - }, - marker: COMMENT_MARKER.to_string(), - no_lgtm: feedback_inputs.no_lgtm, - }; - self.client.post_thread_comment(options).await?; - } if self.client.is_pr_event() && feedback_inputs.pr_review { let summary_only = ["true", "on", "1"].contains( &env::var("CPP_LINTER_PR_REVIEW_SUMMARY_ONLY") @@ -228,6 +201,7 @@ impl RestClient { &feedback_inputs.repo_root, )?; } + self.show_patch(&feedback_inputs)?; let mut options = ReviewOptions { marker: COMMENT_MARKER.to_string(), @@ -265,16 +239,37 @@ impl RestClient { .map_err(|e| ClientError::MutexPoisoned(e.to_string()))?; file.maybe_append_patch(&feedback_inputs.repo_root)?; } + self.show_patch(&feedback_inputs)?; } - let auto_fix_patch_path = feedback_inputs - .repo_root - .join(ClangParams::CACHE_DIR) - .join(ClangParams::AUTO_FIX_PATCH); - if auto_fix_patch_path.exists() { - self.client.write_output_variables(&[OutputVariable { - name: "fix-patch-path".to_string(), - value: auto_fix_patch_path.to_string_lossy().replace("\\", "/"), - }])?; + + if feedback_inputs.thread_comments != ThreadComments::Off { + // post thread comment for PR or push event + if comment.as_ref().is_none_or(|c| c.len() > u16::MAX as usize) { + comment = Some(Self::make_comment( + files, + format_checks_failed, + tidy_checks_failed, + &clang_versions, + Some(u16::MAX as u64), + )?); + } + let options = ThreadCommentOptions { + policy: if feedback_inputs.thread_comments == ThreadComments::Update { + CommentPolicy::Update + } else { + // feedback_inputs.thread_comments is not Off and not Update, so it must be just On. + CommentPolicy::Anew + }, + comment: comment.unwrap_or_default(), + kind: if format_checks_failed == 0 && tidy_checks_failed == 0 { + CommentKind::Lgtm + } else { + CommentKind::Concerns + }, + marker: COMMENT_MARKER.to_string(), + no_lgtm: feedback_inputs.no_lgtm, + }; + self.client.post_thread_comment(options).await?; } Ok(format_checks_failed + tidy_checks_failed) } @@ -409,6 +404,52 @@ impl RestClient { comment.push_str(USER_OUTREACH); Ok(comment) } + + /// Show the generated patch content in the console output. + /// + /// And also write the patch path to the output variable `fix-patch-path` for CI platforms. + fn show_patch(&self, feedback_inputs: &FeedbackInput) -> Result<(), ClientError> { + let auto_fix_patch_path = feedback_inputs + .repo_root + .join(ClangParams::CACHE_DIR) + .join(ClangParams::AUTO_FIX_PATCH); + if let Ok(patch_file) = std::fs::File::open(&auto_fix_patch_path) { + self.client.write_output_variables(&[OutputVariable { + name: "fix-patch-path".to_string(), + value: auto_fix_patch_path.to_string_lossy().replace("\\", "/"), + }])?; + + log::info!("Generated patch content:"); + let stdout_handle = stdout(); + let mut reader = std::io::BufReader::new(patch_file); + let mut line_buf = String::new(); + let mut stdout_lock = stdout_handle.lock(); + while let bytes_read = reader + .read_line(&mut line_buf) + .map_err(ClientError::PatchReadFailed)? + && bytes_read > 0 + { + if line_buf.starts_with('+') { + // Green for additions + stdout_lock + .write_all(line_buf.green().to_string().as_bytes()) + .map_err(ClientError::StdoutWriteFailed)?; + } else if line_buf.starts_with('-') { + // Red for deletions + stdout_lock + .write_all(line_buf.red().to_string().as_bytes()) + .map_err(ClientError::StdoutWriteFailed)?; + } else { + // Default style for context lines + stdout_lock + .write_all(line_buf.as_bytes()) + .map_err(ClientError::StdoutWriteFailed)?; + } + line_buf.clear(); + } + } + Ok(()) + } } /// A closing tag for details blocks in markdown comments. diff --git a/cpp-linter/tests/comments.rs b/cpp-linter/tests/comments.rs index 53705bf8..70439616 100644 --- a/cpp-linter/tests/comments.rs +++ b/cpp-linter/tests/comments.rs @@ -1,6 +1,5 @@ #![cfg(feature = "bin")] use chrono::Utc; -use clap::builder::styling::{AnsiColor, Color, Style}; use cpp_linter::rest_client::USER_OUTREACH; use cpp_linter::run::run_main; use cpp_linter::{cli::ThreadComments, rest_client::COMMENT_MARKER}; @@ -309,19 +308,6 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { patch_path.exists(), "Patch file does not exist at expected path." ); - let patch_content = - std::fs::read_to_string(patch_path).expect("Failed to read generated patch file."); - println!("Generated patch content:"); - for l in patch_content.lines() { - let style = if l.starts_with('+') { - Style::new().fg_color(Some(Color::Ansi(AnsiColor::Green))) // Green for additions - } else if l.starts_with('-') { - Style::new().fg_color(Some(Color::Ansi(AnsiColor::Red))) // Red for deletions - } else { - Style::new() // Default style for context lines - }; - println!("{style}{l}{style:#}"); - } let apply_patch_status = std::process::Command::new("git") .arg("apply")