Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ COPY --from=llvm-downloader /llvm-extract/LLVM-${LLVM_VERSION}-Linux-X64/bin/cla
COPY clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py ${LLVM_SYSROOT}/bin/clang-tidy-diff.py

# Install dependencies for 'pr-code-lint.yml' job
RUN apt-get update && \
DEBIAN_FRONTEND=noninteractive apt-get install -y python3-doc8 && \
apt-get clean && \
rm -rf /var/lib/apt/lists/*
COPY llvm/utils/git/requirements_linting.txt requirements_linting.txt
RUN pip install -r requirements_linting.txt --break-system-packages && \
rm requirements_linting.txt
Expand Down
30 changes: 24 additions & 6 deletions .github/workflows/pr-code-lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ jobs:
uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
with:
fetch-depth: 2

- name: Get changed files
id: changed-files
uses: tj-actions/changed-files@24d32ffd492484c1d75e0c0b894501ddb9d30d62 # v47.0.0
Expand All @@ -39,14 +39,14 @@ jobs:
skip_initial_fetch: true
base_sha: 'HEAD~1'
sha: 'HEAD'

- name: Listed files
env:
CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }}
run: |
echo "Changed files:"
echo "$CHANGED_FILES"

# TODO: create special mapping for 'codegen' targets, for now build predefined set
# TODO: add entrypoint in 'compute_projects.py' that only adds a project and its direct dependencies
- name: Configure and CodeGen
Expand All @@ -71,25 +71,43 @@ jobs:
-DLLVM_INCLUDE_TESTS=OFF \
-DCLANG_INCLUDE_TESTS=OFF \
-DCMAKE_BUILD_TYPE=Release

ninja -C build \
clang-tablegen-targets \
genconfusable # for "ConfusableIdentifierCheck.h"

- name: Run code linter
- name: Install linter dependencies
run: |
pip install doc8 --break-system-packages
echo "$HOME/.local/bin" >> $GITHUB_PATH

- name: Run clang-tidy linter
env:
GITHUB_PR_NUMBER: ${{ github.event.pull_request.number }}
CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }}
run: |
echo "[]" > comments &&
python3 llvm/utils/git/code-lint-helper.py \
--linter clang-tidy \
--token ${{ secrets.GITHUB_TOKEN }} \
--issue-number $GITHUB_PR_NUMBER \
--start-rev HEAD~1 \
--end-rev HEAD \
--verbose \
--changed-files "$CHANGED_FILES"


- name: Run doc8 linter
env:
GITHUB_PR_NUMBER: ${{ github.event.pull_request.number }}
run: |
python3 llvm/utils/git/code-lint-helper.py \
--linter doc8 \
--token ${{ secrets.GITHUB_TOKEN }} \
--issue-number $GITHUB_PR_NUMBER \
--start-rev HEAD~1 \
--end-rev HEAD \
--verbose
Copy link
Contributor

Choose a reason for hiding this comment

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

We should do it in one step inside code-lint-helper.py. It should be a generic helper to accumulate all linters.
Look at formatting job that run multiple linters under one code-format-helper.py invokation


- name: Upload results
uses: actions/upload-artifact@330a01c490aca151604b8cf639adc76d48f6c5d4 # v5.0.0
if: always()
Expand Down
205 changes: 184 additions & 21 deletions llvm/utils/git/code-lint-helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ class LintArgs:
issue_number: int = 0
build_path: str = "build"
clang_tidy_binary: str = "clang-tidy"
doc8_binary: str = "doc8"
linter: str = None

def __init__(self, args: argparse.Namespace = None) -> None:
if not args is None:
Expand All @@ -46,9 +48,12 @@ def __init__(self, args: argparse.Namespace = None) -> None:
self.verbose = args.verbose
self.build_path = args.build_path
self.clang_tidy_binary = args.clang_tidy_binary
self.doc8_binary = args.doc8_binary
self.linter = args.linter


COMMENT_TAG = "<!--LLVM CODE LINT COMMENT: clang-tidy-->"
COMMENT_TAG_CLANG_TIDY = "<!--LLVM CODE LINT COMMENT: clang-tidy-->"
COMMENT_TAG_DOC8 = "<!--LLVM CODE LINT COMMENT: doc8-->"


def get_instructions(cpp_files: List[str]) -> str:
Expand Down Expand Up @@ -135,13 +140,27 @@ def create_comment_text(warning: str, cpp_files: List[str]) -> str:
"""


def find_comment(pr: any) -> any:
def find_comment(pr: any, args: LintArgs) -> any:
linter_tag = get_comment_tag(args.linter)
other_linter = "doc8" if args.linter == "clang-tidy" else "clang-tidy"
other_tag = get_comment_tag(other_linter)

for comment in pr.as_issue().get_comments():
if COMMENT_TAG in comment.body:
body = comment.body
if linter_tag in body and other_tag not in body:
# Found a comment that is exclusively for this linter.
return comment
return None


def get_comment_tag(linter: str) -> str:
if linter == "clang-tidy":
return COMMENT_TAG_CLANG_TIDY
elif linter == "doc8":
return COMMENT_TAG_DOC8
raise ValueError(f"Unknown linter: {linter}")


def create_comment(
comment_text: str, args: LintArgs, create_new: bool
) -> Optional[dict]:
Expand All @@ -150,9 +169,10 @@ def create_comment(
repo = github.Github(args.token).get_repo(args.repo)
pr = repo.get_issue(args.issue_number).as_pull_request()

comment_text = COMMENT_TAG + "\n\n" + comment_text
comment_tag = get_comment_tag(args.linter)
comment_text = comment_tag + "\n\n" + comment_text

existing_comment = find_comment(pr)
existing_comment = find_comment(pr, args)

comment = None
if create_new or existing_comment:
Expand Down Expand Up @@ -215,7 +235,125 @@ def run_clang_tidy(changed_files: List[str], args: LintArgs) -> Optional[str]:
return clean_clang_tidy_output(proc.stdout.strip())


def run_linter(changed_files: List[str], args: LintArgs) -> tuple[bool, Optional[dict]]:
def clean_doc8_output(output: str) -> Optional[str]:
if not output:
return None

lines = output.split("\n")
cleaned_lines = []
in_summary = False

for line in lines:
if line.startswith("Scanning...") or line.startswith("Validating..."):
continue
if line.startswith("========"):
in_summary = True
continue
if in_summary:
continue
if line.strip():
cleaned_lines.append(line)

if cleaned_lines:
return "\n".join(cleaned_lines)
return None


def get_doc8_instructions() -> str:
# TODO: use git diff
return "doc8 ./clang-tools-extra/docs/clang-tidy/checks/"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should implement it in the end, it should be easy because we have changed_files already passed as argument - just check if one of the file there starts with "/clang-tools-extra/docs/clang-tidy/checks/".
Look at how clang-tidy handle it: --changed-files "$CHANGED_FILES"



def create_doc8_comment_text(doc8_output: str) -> str:
instructions = get_doc8_instructions()
return f"""
:warning: Documentation linter doc8 found issues in your code. :warning:

<details>
<summary>
You can test this locally with the following command:
</summary>

```bash
{instructions}
```

</details>

<details>
<summary>
View the output from doc8 here.
</summary>

```
{doc8_output}
```

</details>
"""


def run_doc8(args: LintArgs) -> tuple[int, Optional[str]]:
doc8_cmd = [args.doc8_binary, "./clang-tools-extra/docs/clang-tidy/checks/"]

if args.verbose:
print(f"Running doc8: {' '.join(doc8_cmd)}")

proc = subprocess.run(
doc8_cmd,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
check=False,
)

cleaned_output = clean_doc8_output(proc.stdout.strip())
if proc.returncode != 0 and cleaned_output is None:
# Infrastructure failure
return proc.returncode, proc.stderr.strip()

return proc.returncode, cleaned_output


def run_doc8_linter(args: LintArgs) -> tuple[bool, Optional[dict]]:
returncode, result = run_doc8(args)
should_update_gh = args.token is not None and args.repo is not None
comment = None

if returncode == 0:
if should_update_gh:
comment_text = (
":white_check_mark: With the latest revision "
"this PR passed the documentation linter."
)
comment = create_comment(comment_text, args, create_new=False)
return True, comment
else:
if should_update_gh:
if result:
comment_text = create_doc8_comment_text(result)
comment = create_comment(comment_text, args, create_new=True)
else:
comment_text = (
":warning: The documentation linter failed without printing "
"an output. Check the logs for output. :warning:"
)
comment = create_comment(comment_text, args, create_new=False)
else:
if result:
print(
"Warning: Documentation linter, doc8 detected "
"some issues with your code..."
)
print(result)
else:
print("Warning: Documentation linter, doc8 failed to run.")
return False, comment


def run_clang_tidy_linter(
changed_files: List[str], args: LintArgs
) -> tuple[bool, Optional[dict]]:
changed_files = [arg for arg in changed_files if "third-party" not in arg]

cpp_files = filter_changed_files(changed_files)
Expand Down Expand Up @@ -255,6 +393,13 @@ def run_linter(changed_files: List[str], args: LintArgs) -> tuple[bool, Optional

if __name__ == "__main__":
parser = argparse.ArgumentParser()
parser.add_argument(
"--linter",
type=str,
choices=["clang-tidy", "doc8"],
Copy link
Contributor

Choose a reason for hiding this comment

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

For future enhancements: Flake8 and PyLint are great tools for Python linting. But it'll be necessary to tweak configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't really need it now because clang-tidy has very little python files that don't change frequently.
If we are to add python linter for whole LLVM - it should be an RFC first.

required=True,
help="The linter to run.",
)
parser.add_argument(
"--token", type=str, required=True, help="GitHub authentication token"
)
Expand Down Expand Up @@ -291,39 +436,57 @@ def run_linter(changed_files: List[str], args: LintArgs) -> tuple[bool, Optional
default="clang-tidy",
help="Path to clang-tidy binary",
)
parser.add_argument(
"--doc8-binary",
type=str,
default="doc8",
help="Path to doc8 binary",
)
parser.add_argument(
"--verbose", action="store_true", default=True, help="Verbose output"
)

parsed_args = parser.parse_args()
args = LintArgs(parsed_args)

changed_files = []
if args.changed_files:
changed_files = args.changed_files.split(",")

if args.verbose:
print(f"got changed files: {changed_files}")

if args.verbose:
print("running linter clang-tidy")
print(f"running linter {args.linter}")

success, comment = run_linter(changed_files, args)
success, comment = False, None
if args.linter == "clang-tidy":
changed_files = []
if args.changed_files:
changed_files = args.changed_files.split(",")
if args.verbose:
print(f"got changed files: {changed_files}")
success, comment = run_clang_tidy_linter(changed_files, args)
elif args.linter == "doc8":
success, comment = run_doc8_linter(args)

if not success:
if args.verbose:
print("linter clang-tidy failed")
print(f"linter {args.linter} failed")

# Write comments file if we have a comment
if comment:
import json
if args.verbose:
print(f"linter clang-tidy has comment: {comment}")
print(f"linter {args.linter} has comment: {comment}")

with open("comments", "w") as f:
import json
existing_comments = []
if os.path.exists("comments"):
with open("comments", "r") as f:
try:
existing_comments = json.load(f)
except json.JSONDecodeError:
# File might be empty or invalid, start fresh
pass

json.dump([comment], f)
existing_comments.append(comment)

with open("comments", "w") as f:
json.dump(existing_comments, f)

if not success:
print("error: some linters failed: clang-tidy")
print(f"error: linter {args.linter} failed")
sys.exit(1)
Loading