diff --git a/llvm/utils/git/code-lint-helper.py b/llvm/utils/git/code-lint-helper.py old mode 100755 new mode 100644 index 1232f3ab0d370..1b3965746faab --- a/llvm/utils/git/code-lint-helper.py +++ b/llvm/utils/git/code-lint-helper.py @@ -18,98 +18,101 @@ """ import argparse +import github +import json import os import subprocess import sys -from typing import List, Optional +from typing import Any, Dict, Final, List, Sequence class LintArgs: - start_rev: str = None - end_rev: str = None - repo: str = None - changed_files: List[str] = [] - token: str = None - verbose: bool = True - issue_number: int = 0 - build_path: str = "build" - clang_tidy_binary: str = "clang-tidy" - - def __init__(self, args: argparse.Namespace = None) -> None: + __start_rev: str + __end_rev: str + __repo: str + __changed_files: List[str] + __token: str + __verbose: bool + __issue_number: int + __build_path: str + __clang_tidy_binary: str + + def __init__(self, args: argparse.Namespace) -> None: if not args is None: - self.start_rev = args.start_rev - self.end_rev = args.end_rev - self.repo = args.repo - self.token = args.token - self.changed_files = args.changed_files - self.issue_number = args.issue_number - self.verbose = args.verbose - self.build_path = args.build_path - self.clang_tidy_binary = args.clang_tidy_binary - + self.__start_rev = args.start_rev + self.__end_rev = args.end_rev + self.__repo = args.repo + self.__token = args.token + self.__changed_files = ( + args.changed_files.split(",") if args.changed_files else [] + ) + self.__issue_number = args.issue_number + self.__verbose = args.verbose + self.__build_path = args.build_path + self.__clang_tidy_binary = args.clang_tidy_binary -COMMENT_TAG = "" + @property + def start_rev(self) -> str: + return self.__start_rev + @property + def end_rev(self) -> str: + return self.__end_rev -def get_instructions(cpp_files: List[str]) -> str: - files_str = " ".join(cpp_files) - return f""" -git diff -U0 origin/main...HEAD -- {files_str} | -python3 clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py \\ - -path build -p1 -quiet""" + @property + def repo(self) -> str: + return self.__repo + @property + def changed_files(self) -> List[str]: + return self.__changed_files -def clean_clang_tidy_output(output: str) -> Optional[str]: - """ - - Remove 'Running clang-tidy in X threads...' line - - Remove 'N warnings generated.' line - - Strip leading workspace path from file paths - """ - if not output or output == "No relevant changes found.": - return None - - lines = output.split("\n") - cleaned_lines = [] + @property + def token(self) -> str: + return self.__token - for line in lines: - if line.startswith("Running clang-tidy in") or line.endswith("generated."): - continue + @property + def verbose(self) -> bool: + return self.__verbose - # Remove everything up to rightmost "llvm-project/" for correct files names - idx = line.rfind("llvm-project/") - if idx != -1: - line = line[idx + len("llvm-project/") :] + @property + def issue_number(self) -> int: + return self.__issue_number - cleaned_lines.append(line) + @property + def build_path(self) -> str: + return self.__build_path - if cleaned_lines: - return "\n".join(cleaned_lines) - return None + @property + def clang_tidy_binary(self) -> str: + return self.__clang_tidy_binary -# TODO: Add more rules when enabling other projects to use clang-tidy in CI. -def should_lint_file(filepath: str) -> bool: - return filepath.startswith("clang-tools-extra/clang-tidy/") +class LintHelper: + COMMENT_TAG: Final = "" + name: str + friendly_name: str + comment: Dict[str, Any] = {} + @property + def comment_tag(self) -> str: + return self.COMMENT_TAG.format(linter=self.name) -def filter_changed_files(changed_files: List[str]) -> List[str]: - filtered_files = [] - for filepath in changed_files: - _, ext = os.path.splitext(filepath) - if ext not in (".cpp", ".c", ".h", ".hpp", ".hxx", ".cxx"): - continue - if not should_lint_file(filepath): - continue - if os.path.exists(filepath): - filtered_files.append(filepath) + def instructions(self, files_to_lint: Sequence[str], args: LintArgs) -> str: + raise NotImplementedError() - return filtered_files + def filter_changed_files(self, changed_files: Sequence[str]) -> Sequence[str]: + raise NotImplementedError() + def run_linter_tool(self, files_to_lint: Sequence[str], args: LintArgs) -> str: + raise NotImplementedError() -def create_comment_text(warning: str, cpp_files: List[str]) -> str: - instructions = get_instructions(cpp_files) - return f""" -:warning: C/C++ code linter clang-tidy found issues in your code. :warning: + def create_comment_text( + self, linter_output: str, files_to_lint: Sequence[str], args: LintArgs + ) -> str: + instructions = self.instructions(files_to_lint, args) + return f""" +:warning: {self.friendly_name}, {self.name} found issues in your code. :warning:
@@ -124,133 +127,206 @@ def create_comment_text(warning: str, cpp_files: List[str]) -> str:
-View the output from clang-tidy here. +View the output from {self.name} here. ``` -{warning} +{linter_output} ```
""" + def find_comment(self, pr: Any) -> Any: + for comment in pr.as_issue().get_comments(): + if comment.body.startswith(self.comment_tag): + return comment + return None -def find_comment(pr: any) -> any: - for comment in pr.as_issue().get_comments(): - if COMMENT_TAG in comment.body: - return comment - return None + def update_pr(self, comment_text: str, args: LintArgs, create_new: bool) -> None: + assert args.repo is not None + repo = github.Github(args.token).get_repo(args.repo) + pr = repo.get_issue(args.issue_number).as_pull_request() + comment_text = f"{self.comment_tag}\n\n{comment_text}" -def create_comment( - comment_text: str, args: LintArgs, create_new: bool -) -> Optional[dict]: - import github + existing_comment = self.find_comment(pr) - repo = github.Github(args.token).get_repo(args.repo) - pr = repo.get_issue(args.issue_number).as_pull_request() + if existing_comment: + self.comment = {"body": comment_text, "id": existing_comment.id} + elif create_new: + self.comment = {"body": comment_text} - comment_text = COMMENT_TAG + "\n\n" + comment_text - existing_comment = find_comment(pr) + def run(self, changed_files: Sequence[str], args: LintArgs) -> bool: + if args.verbose: + print(f"got changed files: {changed_files}") - comment = None - if create_new or existing_comment: - comment = {"body": comment_text} - if existing_comment: - comment["id"] = existing_comment.id - return comment + files_to_lint = self.filter_changed_files(changed_files) + if not files_to_lint and args.verbose: + print("no modified files found") -def run_clang_tidy(changed_files: List[str], args: LintArgs) -> Optional[str]: - if not changed_files: - print("no c/c++ files found") - return None + is_success = True + linter_output = "" - git_diff_cmd = [ - "git", - "diff", - "-U0", - f"{args.start_rev}...{args.end_rev}", - "--", - ] + changed_files - - diff_proc = subprocess.run( - git_diff_cmd, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - text=True, - check=False, - ) + if files_to_lint: + linter_output = self.run_linter_tool(files_to_lint, args) + if linter_output: + is_success = False - if diff_proc.returncode != 0: - print(f"Git diff failed: {diff_proc.stderr}") - return None + should_update_gh = args.token is not None and args.repo is not None - diff_content = diff_proc.stdout - if not diff_content.strip(): - print("No diff content found") - return None + if is_success: + if should_update_gh: + comment_text = ( + ":white_check_mark: With the latest revision " + f"this PR passed the {self.friendly_name}." + ) + self.update_pr(comment_text, args, create_new=False) + return True + else: + if should_update_gh: + if linter_output: + comment_text = self.create_comment_text( + linter_output, files_to_lint, args + ) + self.update_pr(comment_text, args, create_new=True) + else: + # The linter failed but didn't output a result (e.g. some sort of + # infrastructure failure). + comment_text = ( + f":warning: The {self.friendly_name} failed without printing " + "an output. Check the logs for output. :warning:" + ) + self.update_pr(comment_text, args, create_new=False) + else: + if linter_output: + print( + f"Warning: {self.friendly_name}, {self.name} detected " + "some issues with your code..." + ) + print(linter_output) + else: + print(f"Warning: {self.friendly_name}, {self.name} failed to run.") + return False + + +class ClangTidyLintHelper(LintHelper): + name: Final = "clang-tidy" + friendly_name: Final = "C/C++ code linter" + + def instructions(self, files_to_lint: Sequence[str], args: LintArgs) -> str: + files_str = " ".join(files_to_lint) + return f""" +git diff -U0 origin/main...HEAD -- {files_str} | +python3 clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py \ + -path {args.build_path} -p1 -quiet""" + + def filter_changed_files(self, changed_files: Sequence[str]) -> Sequence[str]: + clang_tidy_changed_files = [ + arg for arg in changed_files if "third-party" not in arg + ] + + filtered_files = [] + for filepath in clang_tidy_changed_files: + _, ext = os.path.splitext(filepath) + if ext not in (".c", ".cpp", ".cxx", ".h", ".hpp", ".hxx"): + continue + if not self._should_lint_file(filepath): + continue + if os.path.exists(filepath): + filtered_files.append(filepath) + return filtered_files + + def _should_lint_file(self, filepath: str) -> bool: + # TODO: Add more rules when enabling other projects to use clang-tidy in CI. + return filepath.startswith("clang-tools-extra/clang-tidy/") + + def run_linter_tool(self, files_to_lint: Sequence[str], args: LintArgs) -> str: + if not files_to_lint: + return "" + + git_diff_cmd = [ + "git", + "diff", + "-U0", + f"{args.start_rev}...{args.end_rev}", + "--", + ] + git_diff_cmd.extend(files_to_lint) + + diff_proc = subprocess.run( + git_diff_cmd, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + check=False, + ) - tidy_diff_cmd = [ - "clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py", - "-path", - args.build_path, - "-p1", - "-quiet", - ] - - if args.verbose: - print(f"Running clang-tidy-diff: {' '.join(tidy_diff_cmd)}") - - proc = subprocess.run( - tidy_diff_cmd, - input=diff_content, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - text=True, - check=False, - ) + if diff_proc.returncode != 0: + print(f"Git diff failed: {diff_proc.stderr}") + return "" - return clean_clang_tidy_output(proc.stdout.strip()) + diff_content = diff_proc.stdout + if not diff_content.strip(): + if args.verbose: + print("No diff content found") + return "" + tidy_diff_cmd = [ + "clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py", + "-path", + args.build_path, + "-p1", + "-quiet", + ] -def run_linter(changed_files: List[str], args: LintArgs) -> tuple[bool, Optional[dict]]: - changed_files = [arg for arg in changed_files if "third-party" not in arg] + if args.verbose: + print(f"Running clang-tidy-diff: {' '.join(tidy_diff_cmd)}") + + proc = subprocess.run( + tidy_diff_cmd, + input=diff_content, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + check=False, + ) - cpp_files = filter_changed_files(changed_files) + clean_output = self._clean_clang_tidy_output(proc.stdout.strip()) + return clean_output - tidy_result = run_clang_tidy(cpp_files, args) - should_update_gh = args.token is not None and args.repo is not None + def _clean_clang_tidy_output(self, output: str) -> str: + """ + - Remove 'Running clang-tidy in X threads...' line + - Remove 'N warnings generated.' line + - Strip leading workspace path from file paths + """ + if not output or output == "No relevant changes found.": + return "" - comment = None - if tidy_result is None: - if should_update_gh: - comment_text = ( - ":white_check_mark: With the latest revision " - "this PR passed the C/C++ code linter." - ) - comment = create_comment(comment_text, args, create_new=False) - return True, comment - elif len(tidy_result) > 0: - if should_update_gh: - comment_text = create_comment_text(tidy_result, cpp_files) - comment = create_comment(comment_text, args, create_new=True) - else: - print( - "Warning: C/C++ code linter, clang-tidy detected " - "some issues with your code..." - ) - return False, comment - else: - # The linter failed but didn't output a result (e.g. some sort of - # infrastructure failure). - comment_text = ( - ":warning: The C/C++ code linter failed without printing " - "an output. Check the logs for output. :warning:" - ) - comment = create_comment(comment_text, args, create_new=False) - return False, comment + lines = output.split("\n") + cleaned_lines = [] + + for line in lines: + if line.startswith("Running clang-tidy in") or line.endswith("generated."): + continue + + # Remove everything up to rightmost "llvm-project/" for correct files names + idx = line.rfind("llvm-project/") + if idx != -1: + line = line[idx + len("llvm-project/") :] + + cleaned_lines.append(line) + + if cleaned_lines: + return "\n".join(cleaned_lines) + return "" + + + +ALL_LINTERS = (ClangTidyLintHelper(),) if __name__ == "__main__": @@ -298,32 +374,46 @@ def run_linter(changed_files: List[str], args: LintArgs) -> tuple[bool, Optional parsed_args = parser.parse_args() args = LintArgs(parsed_args) - changed_files = [] - if args.changed_files: - changed_files = args.changed_files.split(",") - - if args.verbose: - print(f"got changed files: {changed_files}") + changed_files = args.changed_files - if args.verbose: - print("running linter clang-tidy") + overall_success = True + failed_linters: List[str] = [] + all_comments: List[Dict[str, Any]] = [] - success, comment = run_linter(changed_files, args) - - if not success: - if args.verbose: - print("linter clang-tidy failed") - - # Write comments file if we have a comment - if comment: + for linter in ALL_LINTERS: if args.verbose: - print(f"linter clang-tidy has comment: {comment}") + print(f"running linter {linter.name}") + + linter_passed = linter.run(changed_files, args) + if not linter_passed: + overall_success = False + failed_linters.append(linter.name) + if args.verbose: + print(f"linter {linter.name} failed") + + # Write comments file if we have a comment + if linter.comment: + if args.verbose: + print(f"linter {linter.name} has comment: {linter.comment}") + all_comments.append(linter.comment) + + if len(all_comments): + existing_comments = [] + if os.path.exists("comments"): + try: + with open("comments", "r") as f: + existing_comments = json.load(f) + if not isinstance(existing_comments, list): + existing_comments = [] + except Exception: + existing_comments = [] + + existing_comments.extend(all_comments) with open("comments", "w") as f: - import json - - json.dump([comment], f) + json.dump(existing_comments, f) - if not success: - print("error: some linters failed: clang-tidy") + if not overall_success: + for name in failed_linters: + print(f"error: linter {name} failed") sys.exit(1)