-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[Workflow] Add new code format helper. #66684
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
name: "Check code formatting" | ||
on: pull_request | ||
permissions: | ||
pull-requests: write | ||
|
||
jobs: | ||
code_formatter: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Fetch LLVM sources | ||
uses: actions/checkout@v4 | ||
with: | ||
persist-credentials: false | ||
fetch-depth: 2 | ||
|
||
- name: Get changed files | ||
id: changed-files | ||
uses: tj-actions/changed-files@v39 | ||
with: | ||
separator: "," | ||
|
||
- name: "Listed files" | ||
run: | | ||
echo "Formatting files:" | ||
echo "${{ steps.changed-files.outputs.all_changed_files }}" | ||
|
||
- name: Install clang-format | ||
uses: aminya/setup-cpp@v1 | ||
with: | ||
clangformat: 16.0.6 | ||
|
||
- name: Setup Python env | ||
uses: actions/setup-python@v4 | ||
with: | ||
python-version: '3.11' | ||
cache: 'pip' | ||
cache-dependency-path: 'llvm/utils/git/requirements_formatting.txt' | ||
|
||
- name: Install python dependencies | ||
run: pip install -r llvm/utils/git/requirements_formatting.txt | ||
|
||
- name: Run code formatter | ||
env: | ||
GITHUB_PR_NUMBER: ${{ github.event.pull_request.number }} | ||
START_REV: ${{ github.event.pull_request.base.sha }} | ||
END_REV: ${{ github.event.pull_request.head.sha }} | ||
CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }} | ||
run: | | ||
python llvm/utils/git/code-format-helper.py \ | ||
--token ${{ secrets.GITHUB_TOKEN }} \ | ||
--issue-number $GITHUB_PR_NUMBER \ | ||
--start-rev $START_REV \ | ||
--end-rev $END_REV \ | ||
--changed-files "$CHANGED_FILES" |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,233 @@ | ||
#!/usr/bin/env python3 | ||
# | ||
# ====- code-format-helper, runs code formatters from the ci --*- python -*--==# | ||
# | ||
# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
# See https://llvm.org/LICENSE.txt for license information. | ||
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
# | ||
# ==-------------------------------------------------------------------------==# | ||
|
||
import argparse | ||
import os | ||
import subprocess | ||
import sys | ||
from functools import cached_property | ||
|
||
import github | ||
from github import IssueComment, PullRequest | ||
|
||
|
||
class FormatHelper: | ||
COMMENT_TAG = "<!--LLVM CODE FORMAT COMMENT: {fmt}-->" | ||
name = "unknown" | ||
|
||
@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 | ||
|
||
def pr_comment_text(self, diff: str) -> str: | ||
return f""" | ||
{self.comment_tag} | ||
|
||
:warning: {self.friendly_name}, {self.name} found issues in your code. :warning: | ||
|
||
<details> | ||
<summary> | ||
You can test this locally with the following command: | ||
</summary> | ||
|
||
``````````bash | ||
{self.instructions} | ||
`````````` | ||
|
||
</details> | ||
|
||
<details> | ||
<summary> | ||
View the diff from {self.name} here. | ||
</summary> | ||
|
||
``````````diff | ||
{diff} | ||
`````````` | ||
|
||
</details> | ||
""" | ||
|
||
def find_comment( | ||
self, pr: PullRequest.PullRequest | ||
) -> IssueComment.IssueComment | None: | ||
for comment in pr.as_issue().get_comments(): | ||
if self.comment_tag in comment.body: | ||
return comment | ||
return None | ||
|
||
def update_pr(self, diff: str, args: argparse.Namespace): | ||
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() | ||
|
||
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}. | ||
""" | ||
) | ||
|
||
def run(self, changed_files: [str], args: argparse.Namespace): | ||
diff = self.format_run(changed_files, args) | ||
if diff: | ||
self.update_pr(diff, args) | ||
return False | ||
else: | ||
self.update_pr_success(args) | ||
return True | ||
|
||
|
||
class ClangFormatHelper(FormatHelper): | ||
name = "clang-format" | ||
friendly_name = "C/C++ code formatter" | ||
|
||
@property | ||
def instructions(self): | ||
return " ".join(self.cf_cmd) | ||
|
||
Comment on lines
+107
to
+111
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe that's good as a good iteration but i think as a user I want to know how to fix it rather than how to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that make sense. Let me just remove the diff then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually - I tried this - and it becomes a bit complicated, if you want to pass two SHA's as we do you also have to pass So, if we want to reformat the local files we need more instructions to ensure that you are in the right branch on the right commit. Are you ok with landing this as it is right now and we can iterate on the instructions later or make sure we link to external documentation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, of course! |
||
@cached_property | ||
def libcxx_excluded_files(self): | ||
with open("libcxx/utils/data/ignore_format.txt", "r") as ifd: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to have in tree config files that tell clang-format what to ignore? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I asked something similar in discord the other day: https://discord.com/channels/636084430946959380/636732894974312448/1152588493902589962 |
||
return [excl.strip() for excl in ifd.readlines()] | ||
|
||
def should_be_excluded(self, path: str) -> bool: | ||
if path in self.libcxx_excluded_files: | ||
print(f"Excluding file {path}") | ||
return True | ||
return False | ||
|
||
def filter_changed_files(self, changed_files: [str]) -> [str]: | ||
filtered_files = [] | ||
for path in changed_files: | ||
_, ext = os.path.splitext(path) | ||
if ext in (".cpp", ".c", ".h", ".hpp", ".hxx", ".cxx"): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Libc++ has a bunch of headers without any extension (e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, but this method can be expanded to include all files in that subdir. Happy to do so if you can outline the rules. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need rules at all? ie, clang-format should already handle that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rule is just "all files inside I know it's a real pain, but we don't have a choice here because the name of these headers is mandated by the Standard. Sometimes I wish the Standard had used an extension like everybody else, but they don't and it creates these small (but annoying) complexities. |
||
if not self.should_be_excluded(path): | ||
filtered_files.append(path) | ||
return filtered_files | ||
|
||
def format_run(self, changed_files: [str], args: argparse.Namespace) -> str | None: | ||
cpp_files = self.filter_changed_files(changed_files) | ||
if not cpp_files: | ||
return | ||
cf_cmd = [ | ||
"git-clang-format", | ||
"--diff", | ||
args.start_rev, | ||
args.end_rev, | ||
"--", | ||
] + cpp_files | ||
print(f"Running: {' '.join(cf_cmd)}") | ||
self.cf_cmd = cf_cmd | ||
proc = subprocess.run(cf_cmd, capture_output=True) | ||
|
||
# formatting needed | ||
if proc.returncode == 1: | ||
return proc.stdout.decode("utf-8") | ||
|
||
return None | ||
|
||
|
||
class DarkerFormatHelper(FormatHelper): | ||
name = "darker" | ||
friendly_name = "Python code formatter" | ||
|
||
@property | ||
def instructions(self): | ||
return " ".join(self.darker_cmd) | ||
|
||
def filter_changed_files(self, changed_files: [str]) -> [str]: | ||
filtered_files = [] | ||
for path in changed_files: | ||
name, ext = os.path.splitext(path) | ||
if ext == ".py": | ||
filtered_files.append(path) | ||
|
||
return filtered_files | ||
|
||
def format_run(self, changed_files: [str], args: argparse.Namespace) -> str | None: | ||
py_files = self.filter_changed_files(changed_files) | ||
if not py_files: | ||
return | ||
darker_cmd = [ | ||
"darker", | ||
"--check", | ||
"--diff", | ||
"-r", | ||
f"{args.start_rev}..{args.end_rev}", | ||
] + py_files | ||
print(f"Running: {' '.join(darker_cmd)}") | ||
self.darker_cmd = darker_cmd | ||
proc = subprocess.run(darker_cmd, capture_output=True) | ||
|
||
# formatting needed | ||
if proc.returncode == 1: | ||
return proc.stdout.decode("utf-8") | ||
|
||
return None | ||
|
||
|
||
ALL_FORMATTERS = (DarkerFormatHelper(), ClangFormatHelper()) | ||
|
||
if __name__ == "__main__": | ||
parser = argparse.ArgumentParser() | ||
parser.add_argument( | ||
"--token", type=str, required=True, help="GitHub authentiation token" | ||
) | ||
parser.add_argument( | ||
"--repo", | ||
type=str, | ||
default=os.getenv("GITHUB_REPOSITORY", "llvm/llvm-project"), | ||
help="The GitHub repository that we are working with in the form of <owner>/<repo> (e.g. llvm/llvm-project)", | ||
) | ||
parser.add_argument("--issue-number", type=int, required=True) | ||
parser.add_argument( | ||
"--start-rev", | ||
type=str, | ||
required=True, | ||
help="Compute changes from this revision.", | ||
) | ||
parser.add_argument( | ||
"--end-rev", type=str, required=True, help="Compute changes to this revision" | ||
) | ||
parser.add_argument( | ||
"--changed-files", | ||
type=str, | ||
help="Comma separated list of files that has been changed", | ||
) | ||
|
||
args = parser.parse_args() | ||
|
||
changed_files = [] | ||
if args.changed_files: | ||
changed_files = args.changed_files.split(",") | ||
|
||
exit_code = 0 | ||
for fmt in ALL_FORMATTERS: | ||
if not fmt.run(changed_files, args): | ||
exit_code = 1 | ||
|
||
sys.exit(exit_code) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
# | ||
# This file is autogenerated by pip-compile with Python 3.11 | ||
# by the following command: | ||
# | ||
# pip-compile --output-file=llvm/utils/git/requirements_formatting.txt llvm/utils/git/requirements_formatting.txt.in | ||
# | ||
black==23.9.1 | ||
# via | ||
# -r llvm/utils/git/requirements_formatting.txt.in | ||
# darker | ||
certifi==2023.7.22 | ||
# via requests | ||
cffi==1.15.1 | ||
# via | ||
# cryptography | ||
# pynacl | ||
charset-normalizer==3.2.0 | ||
# via requests | ||
click==8.1.7 | ||
# via black | ||
cryptography==41.0.3 | ||
# via pyjwt | ||
darker==1.7.2 | ||
# via -r llvm/utils/git/requirements_formatting.txt.in | ||
deprecated==1.2.14 | ||
# via pygithub | ||
idna==3.4 | ||
# via requests | ||
mypy-extensions==1.0.0 | ||
# via black | ||
packaging==23.1 | ||
# via black | ||
pathspec==0.11.2 | ||
# via black | ||
platformdirs==3.10.0 | ||
# via black | ||
pycparser==2.21 | ||
# via cffi | ||
pygithub==1.59.1 | ||
# via -r llvm/utils/git/requirements_formatting.txt.in | ||
pyjwt[crypto]==2.8.0 | ||
# via pygithub | ||
pynacl==1.5.0 | ||
# via pygithub | ||
requests==2.31.0 | ||
# via pygithub | ||
toml==0.10.2 | ||
# via darker | ||
urllib3==2.0.4 | ||
# via requests | ||
wrapt==1.15.0 | ||
# via deprecated |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
black~=23.0 | ||
darker==1.7.2 | ||
PyGithub==1.59.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to give ourselves more control by installing from https://apt.llvm.org/ ?
Although we probably don't want to use an experimental build so as long as the action keeps up, this is fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually uses apt.llvm.org already which is why I thought it was fine :)