Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: capture and output clang tool's version number #54

Merged
merged 3 commits into from
Nov 23, 2024
Merged

feat: capture and output clang tool's version number #54

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4c4f20a
capture and output clang tool's version number
2bndy5 Nov 23, 2024
ddccd00
improve regex pattern that captures version number
2bndy5 Nov 23, 2024
391ffa9
final review
2bndy5 Nov 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions cpp-linter/src/clang_tools/clang_tidy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ impl MakeSuggestions for TidyAdvice {
fn parse_tidy_output(
tidy_stdout: &[u8],
database_json: &Option<Vec<CompilationUnit>>,
) -> Result<Option<TidyAdvice>> {
) -> Result<TidyAdvice> {
let note_header = Regex::new(r"^(.+):(\d+):(\d+):\s(\w+):(.*)\[([a-zA-Z\d\-\.]+)\]$").unwrap();
let fixed_note =
Regex::new(r"^.+:(\d+):\d+:\snote: FIX-IT applied suggested code changes$").unwrap();
Expand Down Expand Up @@ -218,14 +218,10 @@ fn parse_tidy_output(
if let Some(note) = notification {
result.push(note);
}
if result.is_empty() {
Ok(None)
} else {
Ok(Some(TidyAdvice {
notes: result,
patched: None,
}))
}
Ok(TidyAdvice {
notes: result,
patched: None,
})
}

/// Get a total count of clang-tidy advice from the given list of [FileObj]s.
Expand Down Expand Up @@ -316,7 +312,10 @@ pub fn run_clang_tidy(
),
));
}
file.tidy_advice = parse_tidy_output(&output.stdout, &clang_params.database_json)?;
file.tidy_advice = Some(parse_tidy_output(
&output.stdout,
&clang_params.database_json,
)?);
if clang_params.tidy_review {
if let Some(tidy_advice) = &mut file.tidy_advice {
// cache file changes in a buffer and restore the original contents for further analysis by clang-format
Expand Down
97 changes: 70 additions & 27 deletions cpp-linter/src/clang_tools/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use anyhow::{anyhow, Context, Result};
use git2::{DiffOptions, Patch};
// non-std crates
use lenient_semver;
use regex::Regex;
use semver::Version;
use tokio::task::JoinSet;
use which::{which, which_in};
Expand Down Expand Up @@ -135,6 +136,28 @@ fn analyze_single_file(
Ok((file.name.clone(), logs))
}

/// A struct to contain the version numbers of the clang-tools used
#[derive(Default)]
pub struct ClangVersions {
/// The clang-format version used.
pub format_version: Option<String>,

/// The clang-tidy version used.
pub tidy_version: Option<String>,
}

/// Run `clang-tool --version`, the extract and return the version number.
fn capture_clang_version(clang_tool: &PathBuf) -> Result<String> {
let output = Command::new(clang_tool).arg("--version").output()?;
let stdout = String::from_utf8_lossy(&output.stdout);
let version_pattern = Regex::new(r"version\s*(\d+\.\d+\.\d+)").unwrap();
let captures = version_pattern.captures(&stdout).ok_or(anyhow!(
"Failed to find version number in `{} --version` output",
clang_tool.to_string_lossy()
))?;
Ok(captures.get(1).unwrap().as_str().to_string())
}
2bndy5 marked this conversation as resolved.
Show resolved Hide resolved

/// Runs clang-tidy and/or clang-format and returns the parsed output from each.
///
/// If `tidy_checks` is `"-*"` then clang-tidy is not executed.
Expand All @@ -144,30 +167,29 @@ pub async fn capture_clang_tools_output(
version: &str,
clang_params: &mut ClangParams,
rest_api_client: &impl RestApiClient,
) -> Result<()> {
) -> Result<ClangVersions> {
let mut clang_versions = ClangVersions::default();
// find the executable paths for clang-tidy and/or clang-format and show version
// info as debugging output.
if clang_params.tidy_checks != "-*" {
clang_params.clang_tidy_command = {
let cmd = get_clang_tool_exe("clang-tidy", version)?;
log::debug!(
"{} --version\n{}",
&cmd.to_string_lossy(),
String::from_utf8_lossy(&Command::new(&cmd).arg("--version").output()?.stdout)
);
Some(cmd)
}
let exe_path = get_clang_tool_exe("clang-tidy", version)?;
let version_found = capture_clang_version(&exe_path)?;
log::debug!(
"{} --version: v{version_found}",
&exe_path.to_string_lossy()
);
clang_versions.tidy_version = Some(version_found);
clang_params.clang_tidy_command = Some(exe_path);
};
if !clang_params.style.is_empty() {
clang_params.clang_format_command = {
let cmd = get_clang_tool_exe("clang-format", version)?;
log::debug!(
"{} --version\n{}",
&cmd.to_string_lossy(),
String::from_utf8_lossy(&Command::new(&cmd).arg("--version").output()?.stdout)
);
Some(cmd)
}
let exe_path = get_clang_tool_exe("clang-format", version)?;
let version_found = capture_clang_version(&exe_path)?;
log::debug!(
"{} --version: v{version_found}",
&exe_path.to_string_lossy()
);
clang_versions.format_version = Some(version_found);
clang_params.clang_format_command = Some(exe_path);
};

// parse database (if provided) to match filenames when parsing clang-tidy's stdout
Expand Down Expand Up @@ -199,7 +221,7 @@ pub async fn capture_clang_tools_output(
rest_api_client.end_log_group();
}
}
Ok(())
Ok(clang_versions)
}

/// A struct to describe a single suggestion in a pull_request review.
Expand All @@ -221,7 +243,7 @@ pub struct ReviewComments {
///
/// This differs from `comments.len()` because some suggestions may
/// not fit within the file's diff.
pub tool_total: [u32; 2],
pub tool_total: [Option<u32>; 2],
/// A list of comment suggestions to be posted.
///
/// These suggestions are guaranteed to fit in the file's diff.
Expand All @@ -234,11 +256,26 @@ pub struct ReviewComments {
}

impl ReviewComments {
pub fn summarize(&self) -> String {
pub fn summarize(&self, clang_versions: &ClangVersions) -> String {
let mut body = format!("{COMMENT_MARKER}## Cpp-linter Review\n");
for t in 0u8..=1 {
let mut total = 0;
let tool_name = if t == 0 { "clang-format" } else { "clang-tidy" };
let (tool_name, tool_version) = if t == 0 {
("clang-format", clang_versions.format_version.as_ref())
} else {
("clang-tidy", clang_versions.tidy_version.as_ref())
};

if self.tool_total[t as usize].is_none() {
// review was not requested from this tool or the tool was not used at all
continue;
}

// If the tool's version is unknown, then we don't need to output this line.
// NOTE: If the tool was invoked at all, then the tool's version shall be known.
if let Some(ver_str) = tool_version {
body.push_str(format!("### Used {tool_name} {ver_str}\n").as_str());
}
for comment in &self.comments {
if comment
.suggestion
Expand All @@ -248,11 +285,12 @@ impl ReviewComments {
}
}

if total != self.tool_total[t as usize] {
// at this point tool_total is Some() value; unwrap() is safe here
let tool_total = self.tool_total[t as usize].unwrap();
if total != tool_total {
body.push_str(
format!(
"\nOnly {} out of {} {tool_name} concerns fit within this pull request's diff.\n",
self.tool_total[t as usize], total
"\nOnly {total} out of {tool_total} {tool_name} concerns fit within this pull request's diff.\n",
)
.as_str(),
);
Expand Down Expand Up @@ -351,6 +389,9 @@ pub trait MakeSuggestions {
.with_context(|| format!("Failed to convert patch to string: {file_name}"))?
.as_str(),
);
if review_comments.tool_total[is_tidy_tool as usize].is_none() {
review_comments.tool_total[is_tidy_tool as usize] = Some(0);
}
if summary_only {
return Ok(());
}
Expand Down Expand Up @@ -408,7 +449,9 @@ pub trait MakeSuggestions {
review_comments.comments.push(comment);
}
}
review_comments.tool_total[is_tidy_tool as usize] += hunks_in_patch;
review_comments.tool_total[is_tidy_tool as usize] = Some(
review_comments.tool_total[is_tidy_tool as usize].unwrap_or_default() + hunks_in_patch,
);
Ok(())
}
}
Expand Down
16 changes: 9 additions & 7 deletions cpp-linter/src/common_fs/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,12 +158,12 @@ impl FileObj {
.unwrap_or_default()
.to_str()
.unwrap_or_default();
let mut total = 0;
for note in &advice.notes {
if note.fixed_lines.is_empty() {
// notification had no suggestion applied in `patched`
let mut suggestion = format!(
"### clang-tidy diagnostic\n**{}:{}:{}** {}: [{}]\n> {}",
file_name,
"### clang-tidy diagnostic\n**{file_name}:{}:{}** {}: [{}]\n> {}",
&note.line,
&note.cols,
&note.severity,
Expand All @@ -172,10 +172,10 @@ impl FileObj {
);
if !note.suggestion.is_empty() {
suggestion.push_str(
format!("```{}\n{}```", file_ext, &note.suggestion.join("\n")).as_str(),
format!("```{file_ext}\n{}```", &note.suggestion.join("\n")).as_str(),
);
}
review_comments.tool_total[1] += 1;
total += 1;
let mut is_merged = false;
for s in &mut review_comments.comments {
if s.path == file_name
Expand All @@ -197,6 +197,8 @@ impl FileObj {
}
}
}
review_comments.tool_total[1] =
Some(review_comments.tool_total[1].unwrap_or_default() + total);
}
Ok(())
}
Expand Down Expand Up @@ -308,14 +310,14 @@ mod test {
// *********************** tests for FileObj::get_ranges()

#[test]
fn get_ranges_0() {
fn get_ranges_none() {
let file_obj = FileObj::new(PathBuf::from("tests/demo/demo.cpp"));
let ranges = file_obj.get_ranges(&LinesChangedOnly::Off);
assert!(ranges.is_empty());
}

#[test]
fn get_ranges_2() {
fn get_ranges_diff() {
let diff_chunks = vec![1..=10];
let added_lines = vec![4, 5, 9];
let file_obj = FileObj::from(
Expand All @@ -328,7 +330,7 @@ mod test {
}

#[test]
fn get_ranges_1() {
fn get_ranges_added() {
let diff_chunks = vec![1..=10];
let added_lines = vec![4, 5, 9];
let file_obj = FileObj::from(
Expand Down
22 changes: 18 additions & 4 deletions cpp-linter/src/rest_api/github/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use serde_json;
use super::{RestApiClient, RestApiRateLimitHeaders};
use crate::clang_tools::clang_format::tally_format_advice;
use crate::clang_tools::clang_tidy::tally_tidy_advice;
use crate::clang_tools::ClangVersions;
use crate::cli::{FeedbackInput, ThreadComments};
use crate::common_fs::{FileFilter, FileObj};
use crate::git::{get_diff, open_repo, parse_diff, parse_diff_from_buf};
Expand Down Expand Up @@ -230,6 +231,7 @@ impl RestApiClient for GithubApiClient {
&self,
files: &[Arc<Mutex<FileObj>>],
feedback_inputs: FeedbackInput,
clang_versions: ClangVersions,
) -> Result<u64> {
let tidy_checks_failed = tally_tidy_advice(files);
let format_checks_failed = tally_format_advice(files);
Expand All @@ -239,8 +241,13 @@ impl RestApiClient for GithubApiClient {
self.post_annotations(files, feedback_inputs.style.as_str());
}
if feedback_inputs.step_summary {
comment =
Some(self.make_comment(files, format_checks_failed, tidy_checks_failed, None));
comment = Some(self.make_comment(
files,
format_checks_failed,
tidy_checks_failed,
&clang_versions,
None,
));
self.post_step_summary(comment.as_ref().unwrap());
}
self.set_exit_code(
Expand All @@ -256,6 +263,7 @@ impl RestApiClient for GithubApiClient {
files,
format_checks_failed,
tidy_checks_failed,
&clang_versions,
Some(65535),
));
}
Expand Down Expand Up @@ -284,7 +292,8 @@ impl RestApiClient for GithubApiClient {
if self.event_name == "pull_request"
&& (feedback_inputs.tidy_review || feedback_inputs.format_review)
{
self.post_review(files, &feedback_inputs).await?;
self.post_review(files, &feedback_inputs, &clang_versions)
.await?;
}
Ok(format_checks_failed + tidy_checks_failed)
}
Expand All @@ -308,6 +317,7 @@ mod test {
clang_tools::{
clang_format::{FormatAdvice, Replacement},
clang_tidy::{TidyAdvice, TidyNotification},
ClangVersions,
},
cli::FeedbackInput,
common_fs::{FileFilter, FileObj},
Expand Down Expand Up @@ -389,8 +399,12 @@ mod test {
gh_out_path.path()
},
);
let clang_versions = ClangVersions {
format_version: Some("x.y.z".to_string()),
tidy_version: Some("x.y.z".to_string()),
};
rest_api_client
.post_feedback(&files, feedback_inputs)
.post_feedback(&files, feedback_inputs, clang_versions)
.await
.unwrap();
let mut step_summary_content = String::new();
Expand Down
8 changes: 5 additions & 3 deletions cpp-linter/src/rest_api/github/specific_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use anyhow::{anyhow, Context, Result};
use reqwest::{Client, Method, Url};

use crate::{
clang_tools::{clang_format::summarize_style, ReviewComments},
clang_tools::{clang_format::summarize_style, ClangVersions, ReviewComments},
cli::FeedbackInput,
common_fs::FileObj,
rest_api::{RestApiRateLimitHeaders, COMMENT_MARKER, USER_AGENT},
Expand Down Expand Up @@ -305,6 +305,7 @@ impl GithubApiClient {
&self,
files: &[Arc<Mutex<FileObj>>],
feedback_input: &FeedbackInput,
clang_versions: &ClangVersions,
) -> Result<()> {
let url = self
.api_url
Expand Down Expand Up @@ -370,15 +371,16 @@ impl GithubApiClient {
let mut payload = FullReview {
event: if feedback_input.passive_reviews {
String::from("COMMENT")
} else if has_no_changes {
} else if has_no_changes && review_comments.comments.is_empty() {
// if patches have no changes AND there are no comments about clang-tidy diagnostics
String::from("APPROVE")
} else {
String::from("REQUEST_CHANGES")
},
body: String::new(),
comments: vec![],
};
payload.body = review_comments.summarize();
payload.body = review_comments.summarize(clang_versions);
if !summary_only {
payload.comments = {
let mut comments = vec![];
Expand Down
Loading
Loading