From df2e12122b01447d8beb7eb5ed41e85b59ab588d Mon Sep 17 00:00:00 2001 From: Sandy Carter Date: Thu, 23 Nov 2023 12:10:03 -0500 Subject: [PATCH 1/5] Fix review command line to run on windows Use list of args when calling subprocess Use Backslashes in line ranges Use posix slashes when looking up files from PR changeset --- .../clang_tidy_review/__init__.py | 28 ++++++++++++------- tests/test_review.py | 4 +-- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/post/clang_tidy_review/clang_tidy_review/__init__.py b/post/clang_tidy_review/clang_tidy_review/__init__.py index 3ab4283..4f1a616 100644 --- a/post/clang_tidy_review/clang_tidy_review/__init__.py +++ b/post/clang_tidy_review/clang_tidy_review/__init__.py @@ -68,14 +68,18 @@ def build_clang_tidy_warnings( print(f"Using config: {config}") - command = f"{clang_tidy_binary} -p={build_dir} {config} -line-filter={line_filter} {files} --export-fixes={FIXES_FILE}" + args = [ + clang_tidy_binary, + f"-p={build_dir}", + config, + f"-line-filter={line_filter}", + f"--export-fixes={FIXES_FILE}", + ] + files start = datetime.datetime.now() try: - with message_group(f"Running:\n\t{command}"): - subprocess.run( - command, capture_output=True, shell=True, check=True, encoding="utf-8" - ) + with message_group(f"Running:\n\t{args}"): + subprocess.run(args, capture_output=True, check=True, encoding="utf-8") except subprocess.CalledProcessError as e: print( f"\n\nclang-tidy failed with return code {e.returncode} and error:\n{e.stderr}\nOutput was:\n{e.stdout}" @@ -118,7 +122,7 @@ def config_file_or_checks( return "" if version >= 12: - return f'--config-file="{config_file}"' + return f"--config-file={config_file}" if config_file != ".clang-tidy": print( @@ -540,7 +544,7 @@ def format_diff_line(diagnostic, offset_lookup, source_line, line_offset, line_n return code_blocks, end_line -def try_relative(path): +def try_relative(path) -> pathlib.Path: """Try making `path` relative to current directory, otherwise make it an absolute path""" try: here = pathlib.Path.cwd() @@ -678,7 +682,9 @@ def create_review_file( notes=diagnostic.get("Notes", []), ) - rel_path = str(try_relative(get_diagnostic_file_path(diagnostic, build_dir))) + rel_path = try_relative( + get_diagnostic_file_path(diagnostic, build_dir) + ).as_posix() # diff lines are 1-indexed source_line = 1 + find_line_number_from_offset( offset_lookup, @@ -764,12 +770,12 @@ def create_review( # Run clang-tidy with the configured parameters and produce the CLANG_TIDY_FIXES file build_clang_tidy_warnings( - shlex.quote(line_ranges), + line_ranges, build_dir, clang_tidy_checks, clang_tidy_binary, config_file, - shlex.join(files), + files, ) # Read and parse the CLANG_TIDY_FIXES file @@ -888,6 +894,8 @@ def get_line_ranges(diff, files): line_filter_json = [] for name, lines in lines_by_file.items(): + # On windows, unidiff has forward slashes but clang-tidy expects backslashes + name = os.path.join(*name.split("/")) line_filter_json.append({"name": name, "lines": lines}) return json.dumps(line_filter_json, separators=(",", ":")) diff --git a/tests/test_review.py b/tests/test_review.py index 6acd7c6..c3b3916 100644 --- a/tests/test_review.py +++ b/tests/test_review.py @@ -408,7 +408,7 @@ def test_config_file(monkeypatch, tmp_path): flag = ctr.config_file_or_checks( "not-clang-tidy", clang_tidy_checks="readability", config_file=str(config_file) ) - assert flag == f'--config-file="{config_file}"' + assert flag == f"--config-file={config_file}" # If you set clang_tidy_checks and config_file to an empty string, neither are sent to the clang-tidy. flag = ctr.config_file_or_checks( @@ -420,7 +420,7 @@ def test_config_file(monkeypatch, tmp_path): flag = ctr.config_file_or_checks( "not-clang-tidy", clang_tidy_checks="", config_file=str(config_file) ) - assert flag == f'--config-file="{config_file}"' + assert flag == f"--config-file={config_file}" # If you get clang_tidy_checks to something and config_file to nothing, clang_tidy_checks is sent to clang-tidy. flag = ctr.config_file_or_checks( From a09d6ab33b3fb2f84c40f8d67a5dfd1b4e2a5a67 Mon Sep 17 00:00:00 2001 From: Sandy Carter Date: Fri, 24 Nov 2023 15:59:01 -0500 Subject: [PATCH 2/5] Split workflow: return the number of comments --- post/clang_tidy_review/clang_tidy_review/__init__.py | 3 ++- post/clang_tidy_review/clang_tidy_review/post.py | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/post/clang_tidy_review/clang_tidy_review/__init__.py b/post/clang_tidy_review/clang_tidy_review/__init__.py index 4f1a616..4b9749a 100644 --- a/post/clang_tidy_review/clang_tidy_review/__init__.py +++ b/post/clang_tidy_review/clang_tidy_review/__init__.py @@ -1003,7 +1003,7 @@ def convert_comment_to_annotations(comment): } -def post_annotations(pull_request: PullRequest, review: Optional[PRReview]): +def post_annotations(pull_request: PullRequest, review: Optional[PRReview]) -> int: """Post the first 10 comments in the review as annotations""" body = { @@ -1041,6 +1041,7 @@ def post_annotations(pull_request: PullRequest, review: Optional[PRReview]): } pull_request.post_annotations(body) + return total_comments def bool_argument(user_input) -> bool: diff --git a/post/clang_tidy_review/clang_tidy_review/post.py b/post/clang_tidy_review/clang_tidy_review/post.py index cc671a5..79e0e96 100755 --- a/post/clang_tidy_review/clang_tidy_review/post.py +++ b/post/clang_tidy_review/clang_tidy_review/post.py @@ -20,7 +20,7 @@ ) -def main(): +def main() -> int: parser = argparse.ArgumentParser( description="Post a review based on feedback generated by the clang-tidy-review action" ) @@ -79,13 +79,13 @@ def main(): ) if args.annotations: - post_annotations(pull_request, review) + return post_annotations(pull_request, review) else: lgtm_comment_body = strip_enclosing_quotes(args.lgtm_comment_body) - post_review( + return post_review( pull_request, review, args.max_comments, lgtm_comment_body, args.dry_run ) if __name__ == "__main__": - main() + exit(main()) From e78c0b171597e2ca74b44011e4f5c830682639c9 Mon Sep 17 00:00:00 2001 From: Sandy Carter Date: Thu, 23 Nov 2023 16:55:39 -0500 Subject: [PATCH 3/5] Add an option to merge multiple comments --- .../clang_tidy_review/__init__.py | 63 ++++++++++++++++--- .../clang_tidy_review/post.py | 14 ++++- 2 files changed, 65 insertions(+), 12 deletions(-) diff --git a/post/clang_tidy_review/clang_tidy_review/__init__.py b/post/clang_tidy_review/clang_tidy_review/__init__.py index 4b9749a..6d8ac7e 100644 --- a/post/clang_tidy_review/clang_tidy_review/__init__.py +++ b/post/clang_tidy_review/clang_tidy_review/__init__.py @@ -20,7 +20,6 @@ import re import io import zipfile -import shlex from github import Github from github.Requester import Requester from github.PaginatedList import PaginatedList @@ -853,21 +852,65 @@ def save_metadata(pr_number: int) -> None: json.dump(metadata, metadata_file) -def load_review() -> Optional[PRReview]: - """Load review output from the standard REVIEW_FILE path. - This file contains +def load_review(review_file: pathlib.Path) -> Optional[PRReview]: + """Load review output""" - """ - - if not pathlib.Path(REVIEW_FILE).exists(): - print(f"WARNING: Could not find review file ('{REVIEW_FILE}')", flush=True) + if not review_file.exists(): + print(f"WARNING: Could not find review file ('{review_file}')", flush=True) return None - with open(REVIEW_FILE, "r") as review_file: - payload = json.load(review_file) + with open(review_file, "r") as review_file_handle: + payload = json.load(review_file_handle) return payload or None +def load_and_merge_reviews(review_files: List[pathlib.Path]) -> Optional[PRReview]: + reviews = [] + for file in review_files: + review = load_review(file) + if review is not None: + reviews.append(review) + + if not reviews: + return None + + result = reviews[0] + + class Comment: + def __init__(self, data): + self.data = data + def __hash__(self): + return hash( + ( + self.data["body"], + self.data["line"], + self.data["path"], + self.data["side"], + ) + ) + def __eq__(self, other): + return type(other) is Comment and self.data == other.data + + def __lt__(self, other): + if self.data["path"] != other.data["path"]: + return self.data["path"] < other.data["path"] + if self.data["line"] != other.data["line"]: + return self.data["line"] < other.data["line"] + if self.data["side"] != other.data["side"]: + return self.data["side"] < other.data["side"] + if self.data["body"] != other.data["body"]: + return self.data["body"] < other.data["body"] + return hash(self) < hash(other) + + comments = set() + for review in reviews: + comments.update(map(Comment, review["comments"])) + + result["comments"] = [c.data for c in sorted(comments)] + + return result + + def get_line_ranges(diff, files): """Return the line ranges of added lines in diff, suitable for the line-filter argument of clang-tidy diff --git a/post/clang_tidy_review/clang_tidy_review/post.py b/post/clang_tidy_review/clang_tidy_review/post.py index 79e0e96..78d306e 100755 --- a/post/clang_tidy_review/clang_tidy_review/post.py +++ b/post/clang_tidy_review/clang_tidy_review/post.py @@ -6,17 +6,19 @@ # See LICENSE for more information import argparse +import pathlib import pprint from clang_tidy_review import ( PullRequest, - load_review, + load_and_merge_reviews, post_review, load_metadata, strip_enclosing_quotes, download_artifacts, post_annotations, bool_argument, + REVIEW_FILE, ) @@ -53,6 +55,14 @@ def main() -> int: type=bool_argument, default=False, ) + parser.add_argument( + "reviews", + metavar="REVIEW_FILES", + type=pathlib.Path, + nargs="*", + default=[REVIEW_FILE], + help="Split workflow review results", + ) args = parser.parse_args() @@ -60,7 +70,7 @@ def main() -> int: # Try to read the review artifacts if they're already present metadata = load_metadata() - review = load_review() + review = load_and_merge_reviews(args.reviews) # If not, try to download them automatically if metadata is None and args.workflow_id is not None: From e2610c9474fddc1ee2e6b7f343a0d9c0a0a06e8b Mon Sep 17 00:00:00 2001 From: Sandy Date: Wed, 10 Jan 2024 10:10:41 -0500 Subject: [PATCH 4/5] Use pathlib for clang_tidy_binary and args list for version This fixes getting the version on windows because the forward slashes are not properly interpreted in `subprocess.run` in shell mode. --- .../clang_tidy_review/__init__.py | 16 ++++++++++------ .../clang_tidy_review/review.py | 3 ++- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/post/clang_tidy_review/clang_tidy_review/__init__.py b/post/clang_tidy_review/clang_tidy_review/__init__.py index 6d8ac7e..f873753 100644 --- a/post/clang_tidy_review/clang_tidy_review/__init__.py +++ b/post/clang_tidy_review/clang_tidy_review/__init__.py @@ -59,7 +59,12 @@ class PRReview(TypedDict): def build_clang_tidy_warnings( - line_filter, build_dir, clang_tidy_checks, clang_tidy_binary, config_file, files + line_filter, + build_dir, + clang_tidy_checks, + clang_tidy_binary: pathlib.Path, + config_file, + files, ) -> None: """Run clang-tidy on the given files and save output into FIXES_FILE""" @@ -88,12 +93,11 @@ def build_clang_tidy_warnings( print(f"Took: {end - start}") -def clang_tidy_version(clang_tidy_binary: str): +def clang_tidy_version(clang_tidy_binary: pathlib.Path): try: version_out = subprocess.run( - f"{clang_tidy_binary} --version", + [clang_tidy_binary, "--version"], capture_output=True, - shell=True, check=True, text=True, ).stdout @@ -111,7 +115,7 @@ def clang_tidy_version(clang_tidy_binary: str): def config_file_or_checks( - clang_tidy_binary: str, clang_tidy_checks: str, config_file: str + clang_tidy_binary: pathlib.Path, clang_tidy_checks: str, config_file: str ): version = clang_tidy_version(clang_tidy_binary) @@ -739,7 +743,7 @@ def create_review( pull_request: PullRequest, build_dir: str, clang_tidy_checks: str, - clang_tidy_binary: str, + clang_tidy_binary: pathlib.Path, config_file: str, include: List[str], exclude: List[str], diff --git a/post/clang_tidy_review/clang_tidy_review/review.py b/post/clang_tidy_review/clang_tidy_review/review.py index 556b2e9..6df5cd7 100755 --- a/post/clang_tidy_review/clang_tidy_review/review.py +++ b/post/clang_tidy_review/clang_tidy_review/review.py @@ -7,6 +7,7 @@ import argparse import os +import pathlib import re import subprocess @@ -34,7 +35,7 @@ def main(): parser.add_argument("--repo", help="Repo name in form 'owner/repo'") parser.add_argument("--pr", help="PR number", type=int) parser.add_argument( - "--clang_tidy_binary", help="clang-tidy binary", default="clang-tidy-14" + "--clang_tidy_binary", help="clang-tidy binary", default="clang-tidy-14", type=pathlib.Path ) parser.add_argument( "--build_dir", help="Directory with compile_commands.json", default="." From 406fab7d45c5f8c72c1f02d8f8a78fd9211e8d96 Mon Sep 17 00:00:00 2001 From: bwrsandman Date: Thu, 11 Jan 2024 15:31:05 +0000 Subject: [PATCH 5/5] Apply black changes --- post/clang_tidy_review/clang_tidy_review/__init__.py | 2 ++ post/clang_tidy_review/clang_tidy_review/review.py | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/post/clang_tidy_review/clang_tidy_review/__init__.py b/post/clang_tidy_review/clang_tidy_review/__init__.py index f873753..b797b5e 100644 --- a/post/clang_tidy_review/clang_tidy_review/__init__.py +++ b/post/clang_tidy_review/clang_tidy_review/__init__.py @@ -883,6 +883,7 @@ def load_and_merge_reviews(review_files: List[pathlib.Path]) -> Optional[PRRevie class Comment: def __init__(self, data): self.data = data + def __hash__(self): return hash( ( @@ -892,6 +893,7 @@ def __hash__(self): self.data["side"], ) ) + def __eq__(self, other): return type(other) is Comment and self.data == other.data diff --git a/post/clang_tidy_review/clang_tidy_review/review.py b/post/clang_tidy_review/clang_tidy_review/review.py index 6df5cd7..a3d3357 100755 --- a/post/clang_tidy_review/clang_tidy_review/review.py +++ b/post/clang_tidy_review/clang_tidy_review/review.py @@ -35,7 +35,10 @@ def main(): parser.add_argument("--repo", help="Repo name in form 'owner/repo'") parser.add_argument("--pr", help="PR number", type=int) parser.add_argument( - "--clang_tidy_binary", help="clang-tidy binary", default="clang-tidy-14", type=pathlib.Path + "--clang_tidy_binary", + help="clang-tidy binary", + default="clang-tidy-14", + type=pathlib.Path, ) parser.add_argument( "--build_dir", help="Directory with compile_commands.json", default="."