[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 03:33:48 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/3] [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/3] [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)
>From cbfc3510bdde43f6432c5319ea055b205ea9f1b2 Mon Sep 17 00:00:00 2001
From: Ryan Prichard <rprichard at google.com>
Date: Fri, 20 Oct 2023 02:50:54 -0700
Subject: [PATCH 3/3] Fixes to log output (remove "Formatter" lines, fix order)
* Prefix "clang-format" to the Excluding lines rather than print
"Formatter darker (Python code formatter):" and
"Formatter clang-format (C/C++ code formatter):" Those formatter
lines were being printed even when there were no C++/Python files to
format.
* Capture stderr and then print it out, rather than let the subprocess
directly write it out. This ensures that output appears in the right
order when the script's output is buffered.
---
llvm/utils/git/code-format-helper.py | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/llvm/utils/git/code-format-helper.py b/llvm/utils/git/code-format-helper.py
index 70019320c0775da..c45bb4d935d478a 100644
--- a/llvm/utils/git/code-format-helper.py
+++ b/llvm/utils/git/code-format-helper.py
@@ -86,7 +86,6 @@ def update_pr(
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 is None:
comment_text = f"""
@@ -123,7 +122,7 @@ def libcxx_excluded_files(self) -> list[str]:
def should_be_excluded(self, path: str) -> bool:
if path in self.libcxx_excluded_files:
- print(f"Excluding file {path}")
+ print(f"{self.name}: Excluding file {path}")
return True
return False
@@ -151,7 +150,8 @@ def format_run(
] + cpp_files
print(f"Running: {' '.join(cf_cmd)}")
self.cf_cmd = cf_cmd
- proc = subprocess.run(cf_cmd, stdout=subprocess.PIPE)
+ proc = subprocess.run(cf_cmd, capture_output=True)
+ sys.stdout.write(proc.stderr.decode("utf-8"))
if proc.returncode != 0:
# formatting needed, or the command otherwise failed
@@ -194,7 +194,8 @@ def format_run(
] + py_files
print(f"Running: {' '.join(darker_cmd)}")
self.darker_cmd = darker_cmd
- proc = subprocess.run(darker_cmd, stdout=subprocess.PIPE)
+ proc = subprocess.run(darker_cmd, capture_output=True)
+ sys.stdout.write(proc.stderr.decode("utf-8"))
if proc.returncode != 0:
# formatting needed, or the command otherwise failed
More information about the llvm-commits
mailing list