diff --git a/cpp_linter/__init__.py b/cpp_linter/__init__.py index a78c3178..0cac86f2 100644 --- a/cpp_linter/__init__.py +++ b/cpp_linter/__init__.py @@ -75,10 +75,10 @@ def main(): ) end_log_group() - capture_clang_tools_output(files=files, args=args) + clang_versions = capture_clang_tools_output(files=files, args=args) start_log_group("Posting comment(s)") - rest_api_client.post_feedback(files=files, args=args) + rest_api_client.post_feedback(files=files, args=args, clang_versions=clang_versions) end_log_group() diff --git a/cpp_linter/clang_tools/__init__.py b/cpp_linter/clang_tools/__init__.py index be7bb339..b762cdf8 100644 --- a/cpp_linter/clang_tools/__init__.py +++ b/cpp_linter/clang_tools/__init__.py @@ -1,9 +1,9 @@ from concurrent.futures import ProcessPoolExecutor, as_completed import json from pathlib import Path +import re import subprocess -from textwrap import indent -from typing import Optional, List, Dict, Tuple +from typing import Optional, List, Dict, Tuple, cast import shutil from ..common_fs import FileObj @@ -77,7 +77,31 @@ def _run_on_single_file( return file.name, log_stream.getvalue(), tidy_note, format_advice -def capture_clang_tools_output(files: List[FileObj], args: Args): +VERSION_PATTERN = re.compile(r"version\s(\d+\.\d+\.\d+)") + + +def _capture_tool_version(cmd: str) -> str: + """Get version number from output for executable used.""" + version_out = subprocess.run( + [cmd, "--version"], capture_output=True, check=True, text=True + ) + matched = VERSION_PATTERN.search(version_out.stdout) + if matched is None: # pragma: no cover + raise RuntimeError( + f"Failed to get version numbers from `{cmd} --version` output" + ) + ver = cast(str, matched.group(1)) + logger.info("`%s --version`: %s", cmd, ver) + return ver + + +class ClangVersions: + def __init__(self) -> None: + self.tidy: Optional[str] = None + self.format: Optional[str] = None + + +def capture_clang_tools_output(files: List[FileObj], args: Args) -> ClangVersions: """Execute and capture all output from clang-tidy and clang-format. This aggregates results in the :attr:`~cpp_linter.Globals.OUTPUT`. @@ -85,30 +109,27 @@ def capture_clang_tools_output(files: List[FileObj], args: Args): :param args: A namespace of parsed args from the :doc:`CLI <../cli_args>`. """ - def show_tool_version_output(cmd: str): # show version output for executable used - version_out = subprocess.run( - [cmd, "--version"], capture_output=True, check=True - ) - logger.info("%s --version\n%s", cmd, indent(version_out.stdout.decode(), "\t")) - tidy_cmd, format_cmd = (None, None) tidy_filter, format_filter = (None, None) + clang_versions = ClangVersions() if args.style: # if style is an empty value, then clang-format is skipped format_cmd = assemble_version_exec("clang-format", args.version) - assert format_cmd is not None, "clang-format executable was not found" - show_tool_version_output(format_cmd) - tidy_filter = TidyFileFilter( + if format_cmd is None: # pragma: no cover + raise FileNotFoundError("clang-format executable was not found") + clang_versions.format = _capture_tool_version(format_cmd) + format_filter = FormatFileFilter( extensions=args.extensions, - ignore_value=args.ignore_tidy, + ignore_value=args.ignore_format, ) if args.tidy_checks != "-*": # if all checks are disabled, then clang-tidy is skipped tidy_cmd = assemble_version_exec("clang-tidy", args.version) - assert tidy_cmd is not None, "clang-tidy executable was not found" - show_tool_version_output(tidy_cmd) - format_filter = FormatFileFilter( + if tidy_cmd is None: # pragma: no cover + raise FileNotFoundError("clang-tidy executable was not found") + clang_versions.tidy = _capture_tool_version(tidy_cmd) + tidy_filter = TidyFileFilter( extensions=args.extensions, - ignore_value=args.ignore_format, + ignore_value=args.ignore_tidy, ) db_json: Optional[List[Dict[str, str]]] = None @@ -155,3 +176,4 @@ def show_tool_version_output(cmd: str): # show version output for executable us break else: # pragma: no cover raise ValueError(f"Failed to find {file_name} in list of files.") + return clang_versions diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py index af4821ba..d5c0f5bd 100644 --- a/cpp_linter/clang_tools/clang_tidy.py +++ b/cpp_linter/clang_tools/clang_tidy.py @@ -133,12 +133,16 @@ def get_suggestions_from_patch( def _has_related_suggestion(suggestion: Suggestion) -> bool: for known in review_comments.suggestions: - if known.line_end >= suggestion.line_end >= known.line_start: + if ( + known.file_name == suggestion.file_name + and known.line_end >= suggestion.line_end >= known.line_start + ): known.comment += f"\n{suggestion.comment}" return True return False # now check for clang-tidy warnings with no fixes applied + assert isinstance(review_comments.tool_total["clang-tidy"], int) for note in self.notes: if not note.applied_fixes: # if no fix was applied line_numb = int(note.line) diff --git a/cpp_linter/clang_tools/patcher.py b/cpp_linter/clang_tools/patcher.py index 7354f773..5b99cee3 100644 --- a/cpp_linter/clang_tools/patcher.py +++ b/cpp_linter/clang_tools/patcher.py @@ -50,12 +50,17 @@ def __init__(self) -> None: #: The list of actual comments self.suggestions: List[Suggestion] = [] - self.tool_total: Dict[str, int] = {"clang-tidy": 0, "clang-format": 0} + self.tool_total: Dict[str, Optional[int]] = { + "clang-tidy": None, + "clang-format": None, + } """The total number of concerns about a specific clang tool. This may not equate to the length of `suggestions` because 1. There is no guarantee that all suggestions will fit within the PR's diff. 2. Suggestions are a combined result of advice from both tools. + + A `None` value means a review was not requested from the corresponding tool. """ self.full_patch: Dict[str, str] = {"clang-tidy": "", "clang-format": ""} @@ -69,17 +74,26 @@ def merge_similar_suggestion(self, suggestion: Suggestion) -> bool: """ for known in self.suggestions: if ( - known.line_end == suggestion.line_end + known.file_name == suggestion.file_name + and known.line_end == suggestion.line_end and known.line_start == suggestion.line_start ): known.comment += f"\n{suggestion.comment}" return True return False - def serialize_to_github_payload(self) -> Tuple[str, List[Dict[str, Any]]]: + def serialize_to_github_payload( + # avoid circular imports by accepting primitive types (instead of ClangVersions) + self, + tidy_version: Optional[str], + format_version: Optional[str], + ) -> Tuple[str, List[Dict[str, Any]]]: """Serialize this object into a summary and list of comments compatible with Github's REST API. + :param tidy_version: The version numbers of the clang-tidy used. + :param format_version: The version numbers of the clang-format used. + :returns: The returned tuple contains a brief summary (at index ``0``) that contains markdown text describing the summary of the review comments. @@ -98,6 +112,12 @@ def serialize_to_github_payload(self) -> Tuple[str, List[Dict[str, Any]]]: posted_tool_advice["clang-tidy"] += 1 for tool_name in ("clang-tidy", "clang-format"): + tool_version = tidy_version + if tool_name == "clang-format": + tool_version = format_version + if tool_version is None or self.tool_total[tool_name] is None: + continue # if tool wasn't used + summary += f"### Used {tool_name} v{tool_version}\n\n" if ( len(comments) and posted_tool_advice[tool_name] != self.tool_total[tool_name] @@ -115,8 +135,7 @@ def serialize_to_github_payload(self) -> Tuple[str, List[Dict[str, Any]]]: ) elif not self.tool_total[tool_name]: summary += f"No concerns from {tool_name}.\n" - result = (summary, comments) - return result + return (summary, comments) class PatchMixin(ABC): @@ -164,8 +183,9 @@ def get_suggestions_from_patch( assert tool_name in review_comments.full_patch review_comments.full_patch[tool_name] += f"{patch.text}" assert tool_name in review_comments.tool_total + tool_total = review_comments.tool_total[tool_name] or 0 for hunk in patch.hunks: - review_comments.tool_total[tool_name] += 1 + tool_total += 1 if summary_only: continue new_hunk_range = file_obj.is_hunk_contained(hunk) @@ -193,3 +213,5 @@ def get_suggestions_from_patch( comment.comment = body if not review_comments.merge_similar_suggestion(comment): review_comments.suggestions.append(comment) + + review_comments.tool_total[tool_name] = tool_total diff --git a/cpp_linter/rest_api/__init__.py b/cpp_linter/rest_api/__init__.py index fb72230f..c6bfe331 100644 --- a/cpp_linter/rest_api/__init__.py +++ b/cpp_linter/rest_api/__init__.py @@ -12,6 +12,7 @@ from ..common_fs.file_filter import FileFilter from ..cli import Args from ..loggers import logger, log_response_msg +from ..clang_tools import ClangVersions USER_OUTREACH = ( @@ -168,6 +169,7 @@ def make_comment( files: List[FileObj], format_checks_failed: int, tidy_checks_failed: int, + clang_versions: ClangVersions, len_limit: Optional[int] = None, ) -> str: """Make an MarkDown comment from the given advice. Also returns a count of @@ -176,6 +178,7 @@ def make_comment( :param files: A list of objects, each describing a file's information. :param format_checks_failed: The amount of clang-format checks that have failed. :param tidy_checks_failed: The amount of clang-tidy checks that have failed. + :param clang_versions: The versions of the clang tools used. :param len_limit: The length limit of the comment generated. :Returns: The markdown comment as a `str` @@ -199,12 +202,14 @@ def adjust_limit(limit: Optional[int], text: str) -> Optional[int]: files=files, checks_failed=format_checks_failed, len_limit=len_limit, + version=clang_versions.format, ) if tidy_checks_failed: comment += RestApiClient._make_tidy_comment( files=files, checks_failed=tidy_checks_failed, len_limit=adjust_limit(limit=len_limit, text=comment), + version=clang_versions.tidy, ) else: prefix = ":heavy_check_mark:\nNo problems need attention." @@ -215,9 +220,12 @@ def _make_format_comment( files: List[FileObj], checks_failed: int, len_limit: Optional[int] = None, + version: Optional[str] = None, ) -> str: """make a comment describing clang-format errors""" - comment = "\n
clang-format reports: " + comment = "\n
clang-format{} reports: ".format( + "" if version is None else f" (v{version})" + ) comment += f"{checks_failed} file(s) not formatted\n\n" closer = "\n
" checks_failed = 0 @@ -238,9 +246,12 @@ def _make_tidy_comment( files: List[FileObj], checks_failed: int, len_limit: Optional[int] = None, + version: Optional[str] = None, ) -> str: """make a comment describing clang-tidy errors""" - comment = "\n
clang-tidy reports: " + comment = "\n
clang-tidy{} reports: ".format( + "" if version is None else f" (v{version})" + ) comment += f"{checks_failed} concern(s)\n\n" closer = "\n
" for file_obj in files: @@ -276,11 +287,13 @@ def post_feedback( self, files: List[FileObj], args: Args, + clang_versions: ClangVersions, ): """Post action's results using REST API. :param files: A list of objects, each describing a file's information. :param args: A namespace of arguments parsed from the :doc:`CLI <../cli_args>`. + :param clang_versions: The version of the clang tools used. """ raise NotImplementedError("Must be defined in the derivative") diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index a6e922b1..b48ddd39 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -25,6 +25,7 @@ ) from ..clang_tools.clang_tidy import tally_tidy_advice from ..clang_tools.patcher import ReviewComments, PatchMixin +from ..clang_tools import ClangVersions from ..cli import Args from ..loggers import logger, log_commander from ..git import parse_diff, get_diff @@ -187,6 +188,7 @@ def post_feedback( self, files: List[FileObj], args: Args, + clang_versions: ClangVersions, ): format_checks_failed = tally_format_advice(files) tidy_checks_failed = tally_tidy_advice(files) @@ -198,6 +200,7 @@ def post_feedback( files=files, format_checks_failed=format_checks_failed, tidy_checks_failed=tidy_checks_failed, + clang_versions=clang_versions, len_limit=None, ) with open(environ["GITHUB_STEP_SUMMARY"], "a", encoding="utf-8") as summary: @@ -225,6 +228,7 @@ def post_feedback( files=files, format_checks_failed=format_checks_failed, tidy_checks_failed=tidy_checks_failed, + clang_versions=clang_versions, len_limit=65535, ) @@ -253,6 +257,7 @@ def post_feedback( format_review=args.format_review, no_lgtm=args.no_lgtm, passive_reviews=args.passive_reviews, + clang_versions=clang_versions, ) def make_annotations( @@ -388,6 +393,7 @@ def post_review( format_review: bool, no_lgtm: bool, passive_reviews: bool, + clang_versions: ClangVersions, ): url = f"{self.api_url}/repos/{self.repo}/pulls/{self.pull_request}" response = self.api_request(url=url) @@ -419,11 +425,15 @@ def post_review( summary_only=summary_only, review_comments=review_comments, ) - (summary, comments) = review_comments.serialize_to_github_payload() + (summary, comments) = review_comments.serialize_to_github_payload( + # avoid circular imports by passing primitive types + tidy_version=clang_versions.tidy, + format_version=clang_versions.format, + ) if not summary_only: payload_comments.extend(comments) body += summary - if sum(review_comments.tool_total.values()): + if sum([x for x in review_comments.tool_total.values() if isinstance(x, int)]): event = "REQUEST_CHANGES" else: if no_lgtm: @@ -457,6 +467,8 @@ def create_review_comments( :param review_comments: An object (passed by reference) that is used to store the results. """ + tool_name = "clang-tidy" if tidy_tool else "clang-format" + review_comments.tool_total[tool_name] = 0 for file_obj in files: tool_advice: Optional[PatchMixin] if tidy_tool: diff --git a/tests/capture_tools_output/test_database_path.py b/tests/capture_tools_output/test_database_path.py index 3a0e685f..c468b015 100644 --- a/tests/capture_tools_output/test_database_path.py +++ b/tests/capture_tools_output/test_database_path.py @@ -11,7 +11,7 @@ from cpp_linter.loggers import logger from cpp_linter.common_fs import FileObj, CACHE_PATH from cpp_linter.rest_api.github_api import GithubApiClient -from cpp_linter.clang_tools import capture_clang_tools_output +from cpp_linter.clang_tools import ClangVersions, capture_clang_tools_output from cpp_linter.clang_tools.clang_format import tally_format_advice from cpp_linter.clang_tools.clang_tidy import tally_tidy_advice from cpp_linter.cli import Args @@ -96,7 +96,7 @@ def test_ninja_database(monkeypatch: pytest.MonkeyPatch, tmp_path: Path): args.lines_changed_only = 0 # analyze complete file # run clang-tidy and verify paths of project files were matched with database paths - capture_clang_tools_output(files, args=args) + clang_versions: ClangVersions = capture_clang_tools_output(files, args=args) found_project_file = False for concern in [a.tidy_advice for a in files if a.tidy_advice]: for note in concern.notes: @@ -112,6 +112,7 @@ def test_ninja_database(monkeypatch: pytest.MonkeyPatch, tmp_path: Path): files=files, tidy_checks_failed=tidy_checks_failed, format_checks_failed=format_checks_failed, + clang_versions=clang_versions, ) assert tidy_checks_failed diff --git a/tests/capture_tools_output/test_tools_output.py b/tests/capture_tools_output/test_tools_output.py index 03efd326..d674088a 100644 --- a/tests/capture_tools_output/test_tools_output.py +++ b/tests/capture_tools_output/test_tools_output.py @@ -16,7 +16,7 @@ from cpp_linter.common_fs import FileObj, CACHE_PATH from cpp_linter.git import parse_diff, get_diff -from cpp_linter.clang_tools import capture_clang_tools_output +from cpp_linter.clang_tools import capture_clang_tools_output, ClangVersions from cpp_linter.clang_tools.clang_format import tally_format_advice from cpp_linter.clang_tools.clang_tidy import tally_tidy_advice from cpp_linter.loggers import log_commander, logger @@ -67,10 +67,14 @@ def make_comment( ): format_checks_failed = tally_format_advice(files) tidy_checks_failed = tally_tidy_advice(files) + clang_versions = ClangVersions() + clang_versions.format = "x.y.z" + clang_versions.tidy = "x.y.z" comment = GithubApiClient.make_comment( files=files, tidy_checks_failed=tidy_checks_failed, format_checks_failed=format_checks_failed, + clang_versions=clang_versions, ) return comment, format_checks_failed, tidy_checks_failed diff --git a/tests/comments/test_comments.py b/tests/comments/test_comments.py index 86cf7954..a8ab65c5 100644 --- a/tests/comments/test_comments.py +++ b/tests/comments/test_comments.py @@ -66,7 +66,7 @@ def test_post_feedback( args.thread_comments = thread_comments args.step_summary = thread_comments == "update" and not no_lgtm args.file_annotations = thread_comments == "update" and no_lgtm - capture_clang_tools_output(files, args=args) + clang_versions = capture_clang_tools_output(files, args=args) # add a non project file to tidy_advice to intentionally cover a log.debug() for file in files: if file.tidy_advice: @@ -169,4 +169,4 @@ def test_post_feedback( # to get debug files saved to test workspace folders: enable logger verbosity caplog.set_level(logging.DEBUG, logger=logger.name) - gh_client.post_feedback(files, args) + gh_client.post_feedback(files, args, clang_versions) diff --git a/tests/reviews/test_pr_review.py b/tests/reviews/test_pr_review.py index 3f7de7aa..1d1aaf19 100644 --- a/tests/reviews/test_pr_review.py +++ b/tests/reviews/test_pr_review.py @@ -13,10 +13,6 @@ TEST_REPO = "cpp-linter/test-cpp-linter-action" TEST_PR = 27 -DEFAULT_TIDY_CHECKS = ( - "boost-*,bugprone-*,performance-*,readability-*,portability-*,modernize-*," - "clang-analyzer-*,cppcoreguidelines-*" -) test_parameters = OrderedDict( is_draft=False, @@ -155,7 +151,8 @@ def test_post_review( files.clear() args = Args() - args.tidy_checks = DEFAULT_TIDY_CHECKS + if not tidy_review: + args.tidy_checks = "-*" args.version = environ.get("CLANG_VERSION", "16") args.style = "file" args.extensions = extensions @@ -170,12 +167,15 @@ def test_post_review( args.file_annotations = False args.passive_reviews = is_passive - capture_clang_tools_output(files, args=args) + clang_versions = capture_clang_tools_output(files, args=args) if not force_approved: format_advice = list(filter(lambda x: x.format_advice is not None, files)) tidy_advice = list(filter(lambda x: x.tidy_advice is not None, files)) - assert tidy_advice and len(tidy_advice) < len(files) - assert format_advice and len(format_advice) < len(files) + if tidy_review: + assert tidy_advice and len(tidy_advice) <= len(files) + else: + assert not tidy_advice + assert format_advice and len(format_advice) <= len(files) # simulate draft PR by changing the request response cache_pr_response = (cache_path / f"pr_{TEST_PR}.json").read_text( @@ -194,7 +194,7 @@ def test_post_review( headers={"Accept": "application/vnd.github.text+json"}, text=cache_pr_response, ) - gh_client.post_feedback(files, args) + gh_client.post_feedback(files, args, clang_versions) # inspect the review payload for correctness last_request = mock.last_request diff --git a/tests/test_comment_length.py b/tests/test_comment_length.py index 47c98521..0d4359c6 100644 --- a/tests/test_comment_length.py +++ b/tests/test_comment_length.py @@ -3,6 +3,7 @@ from cpp_linter.rest_api import USER_OUTREACH from cpp_linter.clang_tools.clang_format import FormatAdvice, FormatReplacementLine from cpp_linter.common_fs import FileObj +from cpp_linter.clang_tools import ClangVersions def test_comment_length_limit(tmp_path: Path): @@ -15,11 +16,14 @@ def test_comment_length_limit(tmp_path: Path): dummy_advice = FormatAdvice(file_name) dummy_advice.replaced_lines = [FormatReplacementLine(line_numb=1)] file.format_advice = dummy_advice + clang_versions = ClangVersions() + clang_versions.format = "x.y.z" files = [file] * format_checks_failed thread_comment = GithubApiClient.make_comment( files=files, format_checks_failed=format_checks_failed, tidy_checks_failed=0, + clang_versions=clang_versions, len_limit=abs_limit, ) assert len(thread_comment) < abs_limit @@ -28,6 +32,7 @@ def test_comment_length_limit(tmp_path: Path): files=files, format_checks_failed=format_checks_failed, tidy_checks_failed=0, + clang_versions=clang_versions, len_limit=None, ) assert len(step_summary) != len(thread_comment)