[llvm] [Workflow] Expand code-format-helper.py error reporting (PR #69686)
Ryan Prichard via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 20 00:48:47 PDT 2023
https://github.com/rprichard created https://github.com/llvm/llvm-project/pull/69686
* Consider the darker/clang-format command to have failed if the exit code is non-zero, regardless of the stdout/stderr output.
* Allow stderr from the formatter command to propagate to the script's caller (and into the GitHub log).
* On success, dump stdout to the caller, so it ends up in GitHub's log. I'm not sure what this would ever be, but if it exists, it should be preserved.
* Just before the script exits, if any formatter failed, print a line showing which formatters failed.
Replace [str] with list[str] to satisfy mypy/pytype.
>From acb8511112b083e6cc4094b7cd693651154187b9 Mon Sep 17 00:00:00 2001
From: Ryan Prichard <rprichard at google.com>
Date: Thu, 19 Oct 2023 23:21:09 -0700
Subject: [PATCH] [Workflow] Expand code-format-helper.py error reporting
* Consider the darker/clang-format command to have failed if the
exit code is non-zero, regardless of the stdout/stderr output.
* Allow stderr from the formatter command to propagate to the script's
caller (and into the GitHub log).
* On success, dump stdout to the caller, so it ends up in GitHub's
log. I'm not sure what this would ever be, but if it exists, it
should be preserved.
* Just before the script exits, if any formatter failed, print a line
showing which formatters failed.
Replace [str] with list[str] to satisfy mypy/pytype.
---
llvm/utils/git/code-format-helper.py | 115 +++++++++++++++------------
1 file changed, 66 insertions(+), 49 deletions(-)
diff --git a/llvm/utils/git/code-format-helper.py b/llvm/utils/git/code-format-helper.py
index 8d3c30b309d015d..3d08fdc352d55c4 100644
--- a/llvm/utils/git/code-format-helper.py
+++ b/llvm/utils/git/code-format-helper.py
@@ -20,19 +20,24 @@
class FormatHelper:
COMMENT_TAG = "<!--LLVM CODE FORMAT COMMENT: {fmt}-->"
- name = "unknown"
+ name: str
+ friendly_name: str
@property
def comment_tag(self) -> str:
return self.COMMENT_TAG.replace("fmt", self.name)
- def format_run(self, changed_files: [str], args: argparse.Namespace) -> str | None:
- pass
+ @property
+ def instructions(self) -> str:
+ raise NotImplementedError()
- def pr_comment_text(self, diff: str) -> str:
- return f"""
-{self.comment_tag}
+ def format_run(
+ self, changed_files: list[str], args: argparse.Namespace
+ ) -> str | None:
+ raise NotImplementedError()
+ def pr_comment_text_for_diff(self, diff: str) -> str:
+ return f"""
:warning: {self.friendly_name}, {self.name} found issues in your code. :warning:
<details>
@@ -66,39 +71,41 @@ def find_comment(
return comment
return None
- def update_pr(self, diff: str, args: argparse.Namespace):
+ def update_pr(
+ self, comment_text: str, args: argparse.Namespace, create_new: bool
+ ) -> None:
repo = github.Github(args.token).get_repo(args.repo)
pr = repo.get_issue(args.issue_number).as_pull_request()
- existing_comment = self.find_comment(pr)
- pr_text = self.pr_comment_text(diff)
-
- if existing_comment:
- existing_comment.edit(pr_text)
- else:
- pr.as_issue().create_comment(pr_text)
-
- def update_pr_success(self, args: argparse.Namespace):
- repo = github.Github(args.token).get_repo(args.repo)
- pr = repo.get_issue(args.issue_number).as_pull_request()
+ comment_text = self.comment_tag + "\n\n" + comment_text
existing_comment = self.find_comment(pr)
if existing_comment:
- existing_comment.edit(
- f"""
-{self.comment_tag}
-:white_check_mark: With the latest revision this PR passed the {self.friendly_name}.
-"""
- )
+ existing_comment.edit(comment_text)
+ elif create_new:
+ pr.as_issue().create_comment(comment_text)
- def run(self, changed_files: [str], args: argparse.Namespace):
+ def run(self, changed_files: list[str], args: argparse.Namespace) -> bool:
+ print(f"Formatter {self.name} ({self.friendly_name}):")
diff = self.format_run(changed_files, args)
- if diff:
- self.update_pr(diff, args)
+ if diff is None:
+ comment_text = f"""
+:white_check_mark: With the latest revision this PR passed the {self.friendly_name}.
+"""
+ self.update_pr(comment_text, args, create_new=False)
+ return True
+ elif len(diff) > 0:
+ comment_text = self.pr_comment_text_for_diff(diff)
+ self.update_pr(comment_text, args, create_new=True)
return False
else:
- self.update_pr_success(args)
- return True
+ # The formatter failed but didn't output a diff (e.g. some sort of
+ # infrastructure failure).
+ comment_text = f"""
+:warning: The {self.friendly_name} failed without printing a diff. Check the logs for stderr output. :warning:
+"""
+ self.update_pr(comment_text, args, create_new=False)
+ return False
class ClangFormatHelper(FormatHelper):
@@ -120,7 +127,7 @@ def should_be_excluded(self, path: str) -> bool:
return True
return False
- def filter_changed_files(self, changed_files: [str]) -> [str]:
+ def filter_changed_files(self, changed_files: list[str]) -> list[str]:
filtered_files = []
for path in changed_files:
_, ext = os.path.splitext(path)
@@ -129,10 +136,12 @@ def filter_changed_files(self, changed_files: [str]) -> [str]:
filtered_files.append(path)
return filtered_files
- def format_run(self, changed_files: [str], args: argparse.Namespace) -> str | None:
+ def format_run(
+ self, changed_files: list[str], args: argparse.Namespace
+ ) -> str | None:
cpp_files = self.filter_changed_files(changed_files)
if not cpp_files:
- return
+ return None
cf_cmd = [
"git-clang-format",
"--diff",
@@ -142,13 +151,15 @@ def format_run(self, changed_files: [str], args: argparse.Namespace) -> str | No
] + cpp_files
print(f"Running: {' '.join(cf_cmd)}")
self.cf_cmd = cf_cmd
- proc = subprocess.run(cf_cmd, capture_output=True)
+ proc = subprocess.run(cf_cmd, stdout=subprocess.PIPE)
- # formatting needed
- if proc.returncode == 1:
+ if proc.returncode != 0:
+ # formatting needed, or the command otherwise failed
+ print(f"error: {self.name} exited with code {proc.returncode}")
return proc.stdout.decode("utf-8")
-
- return None
+ else:
+ sys.stdout.write(proc.stdout.decode("utf-8"))
+ return None
class DarkerFormatHelper(FormatHelper):
@@ -156,10 +167,10 @@ class DarkerFormatHelper(FormatHelper):
friendly_name = "Python code formatter"
@property
- def instructions(self):
+ def instructions(self) -> str:
return " ".join(self.darker_cmd)
- def filter_changed_files(self, changed_files: [str]) -> [str]:
+ def filter_changed_files(self, changed_files: list[str]) -> list[str]:
filtered_files = []
for path in changed_files:
name, ext = os.path.splitext(path)
@@ -168,10 +179,12 @@ def filter_changed_files(self, changed_files: [str]) -> [str]:
return filtered_files
- def format_run(self, changed_files: [str], args: argparse.Namespace) -> str | None:
+ def format_run(
+ self, changed_files: list[str], args: argparse.Namespace
+ ) -> str | None:
py_files = self.filter_changed_files(changed_files)
if not py_files:
- return
+ return None
darker_cmd = [
"darker",
"--check",
@@ -181,13 +194,15 @@ def format_run(self, changed_files: [str], args: argparse.Namespace) -> str | No
] + py_files
print(f"Running: {' '.join(darker_cmd)}")
self.darker_cmd = darker_cmd
- proc = subprocess.run(darker_cmd, capture_output=True)
+ proc = subprocess.run(darker_cmd, stdout=subprocess.PIPE)
- # formatting needed
- if proc.returncode == 1:
+ if proc.returncode != 0:
+ # formatting needed, or the command otherwise failed
+ print(f"error: {self.name} exited with code {proc.returncode}")
return proc.stdout.decode("utf-8")
-
- return None
+ else:
+ sys.stdout.write(proc.stdout.decode("utf-8"))
+ return None
ALL_FORMATTERS = (DarkerFormatHelper(), ClangFormatHelper())
@@ -225,9 +240,11 @@ def format_run(self, changed_files: [str], args: argparse.Namespace) -> str | No
if args.changed_files:
changed_files = args.changed_files.split(",")
- exit_code = 0
+ failed_formatters = []
for fmt in ALL_FORMATTERS:
if not fmt.run(changed_files, args):
- exit_code = 1
+ failed_formatters.append(fmt.name)
- sys.exit(exit_code)
+ if len(failed_formatters) > 0:
+ print(f"error: some formatters failed: {' '.join(failed_formatters)}")
+ sys.exit(1)
More information about the llvm-commits
mailing list