[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 01:24:09 PDT 2023
https://github.com/rprichard updated https://github.com/llvm/llvm-project/pull/69686
>From 8a96d1d654e7fd8825eade605936c6cb6385b93e Mon Sep 17 00:00:00 2001
From: Ryan Prichard <rprichard at google.com>
Date: Fri, 20 Oct 2023 01:15:02 -0700
Subject: [PATCH 1/2] [Workflow] make code-format-helper.py mypy-safe (NFC)
Fix type errors that mypy reports with code-format-helper.py.
Add a few return type annotations and change `param: [str]` to
`param: list[str]`.
Leave a few required FormatHelper members missing instead of defining a
placeholder:
- FormatHelper.name
- FormatHelper.friendly_name
- FormatHelper.format_run: NotImplementedError() instead of `pass`
---
llvm/utils/git/code-format-helper.py | 39 ++++++++++++++++++----------
1 file changed, 25 insertions(+), 14 deletions(-)
diff --git a/llvm/utils/git/code-format-helper.py b/llvm/utils/git/code-format-helper.py
index 8d3c30b309d015d..f02415bfbe0d201 100644
--- a/llvm/utils/git/code-format-helper.py
+++ b/llvm/utils/git/code-format-helper.py
@@ -20,14 +20,21 @@
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 format_run(
+ self, changed_files: list[str], args: argparse.Namespace
+ ) -> str | None:
+ raise NotImplementedError()
def pr_comment_text(self, diff: str) -> str:
return f"""
@@ -66,7 +73,7 @@ def find_comment(
return comment
return None
- def update_pr(self, diff: str, args: argparse.Namespace):
+ def update_pr(self, diff: str, args: argparse.Namespace) -> None:
repo = github.Github(args.token).get_repo(args.repo)
pr = repo.get_issue(args.issue_number).as_pull_request()
@@ -78,7 +85,7 @@ def update_pr(self, diff: str, args: argparse.Namespace):
else:
pr.as_issue().create_comment(pr_text)
- def update_pr_success(self, args: argparse.Namespace):
+ def update_pr_success(self, args: argparse.Namespace) -> None:
repo = github.Github(args.token).get_repo(args.repo)
pr = repo.get_issue(args.issue_number).as_pull_request()
@@ -91,7 +98,7 @@ def update_pr_success(self, args: argparse.Namespace):
"""
)
- def run(self, changed_files: [str], args: argparse.Namespace):
+ def run(self, changed_files: list[str], args: argparse.Namespace) -> bool:
diff = self.format_run(changed_files, args)
if diff:
self.update_pr(diff, args)
@@ -106,11 +113,11 @@ class ClangFormatHelper(FormatHelper):
friendly_name = "C/C++ code formatter"
@property
- def instructions(self):
+ def instructions(self) -> str:
return " ".join(self.cf_cmd)
@cached_property
- def libcxx_excluded_files(self):
+ def libcxx_excluded_files(self) -> list[str]:
with open("libcxx/utils/data/ignore_format.txt", "r") as ifd:
return [excl.strip() for excl in ifd.readlines()]
@@ -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",
@@ -159,7 +168,7 @@ class DarkerFormatHelper(FormatHelper):
def instructions(self):
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 +177,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",
>From ac2b8dceb9ad01d19595f84323bce2e6d8346f4b Mon Sep 17 00:00:00 2001
From: Ryan Prichard <rprichard at google.com>
Date: Fri, 20 Oct 2023 01:21:39 -0700
Subject: [PATCH 2/2] [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 | 84 +++++++++++++++-------------
1 file changed, 45 insertions(+), 39 deletions(-)
diff --git a/llvm/utils/git/code-format-helper.py b/llvm/utils/git/code-format-helper.py
index f02415bfbe0d201..70019320c0775da 100644
--- a/llvm/utils/git/code-format-helper.py
+++ b/llvm/utils/git/code-format-helper.py
@@ -36,10 +36,8 @@ def format_run(
) -> str | None:
raise NotImplementedError()
- def pr_comment_text(self, diff: str) -> str:
+ def pr_comment_text_for_diff(self, diff: str) -> str:
return f"""
-{self.comment_tag}
-
:warning: {self.friendly_name}, {self.name} found issues in your code. :warning:
<details>
@@ -73,39 +71,41 @@ def find_comment(
return comment
return None
- def update_pr(self, diff: str, args: argparse.Namespace) -> None:
+ 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) -> None:
- 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: 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):
@@ -151,13 +151,15 @@ def format_run(
] + 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):
@@ -165,7 +167,7 @@ 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: list[str]) -> list[str]:
@@ -192,13 +194,15 @@ def format_run(
] + 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())
@@ -236,9 +240,11 @@ def format_run(
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