Skip to content
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

[Workflow] Add new code format helper. #66684

Merged
merged 3 commits into from
Sep 20, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions .github/workflows/pr-code-format.yml
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
Comment on lines +27 to +30
Copy link
Contributor

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

Copy link
Collaborator Author

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 :)


- 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"
39 changes: 0 additions & 39 deletions .github/workflows/pr-python-format.yml

This file was deleted.

236 changes: 236 additions & 0 deletions llvm/utils/git/code-format-helper.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,236 @@
#!/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
Copy link
Contributor

Choose a reason for hiding this comment

The 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
run the bot locally.
Ie I guess for clang-format I want the command without the --diff
" ".join(self.cf_cmd.remove("--diff")) is probably workable for clang-format

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that make sense. Let me just remove the diff then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 --diff

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course!
Actually, i could not find docs to use git-clang-format in LLVM so if we don't have that maybe we can improve that too later

@cached_property
def libcxx_excluded_files(self):
with open("libcxx/utils/data/ignore_format.txt", "r") as ifd:
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return [excl.strip() for excl in ifd.readlines()]

def should_be_excluded(self, path: str) -> bool:
for excl in self.libcxx_excluded_files:
if path == excl:
print(f"Excluding file {path}")
return True
return False
tru marked this conversation as resolved.
Show resolved Hide resolved

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"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Libc++ has a bunch of headers without any extension (e.g. libcxx/include/algorithm), will those be handled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule is just "all files inside libcxx/include".

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:
self.args = args
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that ever used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope it's not. Will fix!

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:
self.args = args
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)
52 changes: 52 additions & 0 deletions llvm/utils/git/requirements_formatting.txt
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
3 changes: 3 additions & 0 deletions llvm/utils/git/requirements_formatting.txt.in
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing new lines