[Github][CI] Add doc8 for clang-tidy documentation formatting
#168827
Conversation
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-github-workflow Author: mitchell (zeyi2) ChangesThis PR is not ready for review now. Closes #167098 Full diff: https://github.com/llvm/llvm-project/pull/168827.diff 3 Files Affected:
diff --git a/.github/workflows/containers/github-action-ci-tooling/Dockerfile b/.github/workflows/containers/github-action-ci-tooling/Dockerfile
index b78c99efb9be3..8d02baa05f489 100644
--- a/.github/workflows/containers/github-action-ci-tooling/Dockerfile
+++ b/.github/workflows/containers/github-action-ci-tooling/Dockerfile
@@ -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
diff --git a/.github/workflows/pr-code-lint.yml b/.github/workflows/pr-code-lint.yml
index 5444a29c22205..60c1900000e5e 100644
--- a/.github/workflows/pr-code-lint.yml
+++ b/.github/workflows/pr-code-lint.yml
@@ -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
@@ -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
@@ -71,25 +71,38 @@ 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: 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
+
- name: Upload results
uses: actions/upload-artifact@330a01c490aca151604b8cf639adc76d48f6c5d4 # v5.0.0
if: always()
diff --git a/llvm/utils/git/code-lint-helper.py b/llvm/utils/git/code-lint-helper.py
index 1232f3ab0d370..fc2068b438209 100755
--- a/llvm/utils/git/code-lint-helper.py
+++ b/llvm/utils/git/code-lint-helper.py
@@ -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:
@@ -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:
@@ -135,13 +140,22 @@ 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:
+ comment_tag = get_comment_tag(args.linter)
for comment in pr.as_issue().get_comments():
- if COMMENT_TAG in comment.body:
+ if comment_tag in comment.body:
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]:
@@ -150,9 +164,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:
@@ -215,7 +230,126 @@ 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/"
+
+
+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)
@@ -255,6 +389,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"],
+ required=True,
+ help="The linter to run.",
+ )
parser.add_argument(
"--token", type=str, required=True, help="GitHub authentication token"
)
@@ -291,6 +432,12 @@ 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"
)
@@ -298,32 +445,44 @@ def run_linter(changed_files: List[str], args: LintArgs) -> tuple[bool, Optional
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)
|
.github/workflows/containers/github-action-ci-tooling/Dockerfile
Outdated
Show resolved
Hide resolved
zeyi2
force-pushed
the
clang-tidy-doc8-ci
branch
from
260cd88 to
8fc6300
Compare
.github/workflows/pr-code-lint.yml
Outdated
| - name: Run code linter | ||
| - name: Install linter dependencies | ||
| run: pip install doc8 --break-system-packages |
There was a problem hiding this comment.
I understand that this is completely wrong, this purpose of this is to add doc8 package (seems that modifying Dockerfile doesn't work in permerge CI) so I can test whether the modified python script can find the issue in the repo correctly.
There was a problem hiding this comment.
(seems that modifying Dockerfile doesn't work in permerge CI)
Yes, we need to do it in 2 steps:
First make a PR with new dockerfile and push it to main branch. It will become the default of https://github.com/llvm/llvm-project/pkgs/container/ci-ubuntu-24.04-lint.
Then, in this PR you will be able to use Dockerfile with doc8 installed.
There was a problem hiding this comment.
Another approach would be: build CI lint container locally and push it to LLVM github containers via personal token.
I did such thing in #164294.
I have a setup already so can do it myself and then we can test with real container.
There was a problem hiding this comment.
I pushed https://github.com/orgs/llvm/packages/container/package/ci-ubuntu-24.04-lint-doc8, you can now use it in CI
There was a problem hiding this comment.
You can do it locally with podman build, podman login <token>, podman push
There was a problem hiding this comment.
I pushed https://github.com/orgs/llvm/packages/container/package/ci-ubuntu-24.04-lint-doc8, you can now use it in CI
Thanks a lot!
There was a problem hiding this comment.
Seems that the container is private, I'll build from my machine and test it soon.
And I think for the CI jobs after merging to actually use this, we need to push the Dockerfile update to main for the container becomes public, right?
If so, I'll open a new PR for this :)
There was a problem hiding this comment.
Oh I forgot to change package visibility from private to public, now it should work as intended.
And I think for the CI jobs after merging to actually use this, we need to push the Dockerfile update to main for the container becomes public, right?
As soon as we push container to main, this job rebuild and update container, so code-lint:latests will be ready automatically in around 15 mins. There would be a gap when CI won't work, but IMO it's negligible.
I think we can do it in one go.
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the documentation linter. |
zeyi2
force-pushed
the
clang-tidy-doc8-ci
branch
from
9fe22f1 to
27c30ab
Compare
Verbose logging is fine now: |
🐧 Linux x64 Test Results
|
.github/workflows/pr-code-lint.yml
Outdated
| - 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 |
There was a problem hiding this comment.
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
llvm/utils/git/code-lint-helper.py
Outdated
| def get_doc8_instructions() -> str: | ||
| # TODO: use git diff | ||
| return "doc8 ./clang-tools-extra/docs/clang-tidy/checks/" |
There was a problem hiding this comment.
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"
llvm/utils/git/code-lint-helper.py
Outdated
| parser.add_argument( | ||
| "--linter", | ||
| type=str, | ||
| choices=["clang-tidy", "doc8"], |
There was a problem hiding this comment.
For future enhancements: Flake8 and PyLint are great tools for Python linting. But it'll be necessary to tweak configuration.
There was a problem hiding this comment.
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.
zeyi2
force-pushed
the
clang-tidy-doc8-ci
branch
from
8b1206e to
342cae4
Compare
llvm/utils/git/code-lint-helper.py
Outdated
| if args.changed_files: | ||
| self.changed_files = args.changed_files.split(",") | ||
| else: | ||
| self.changed_files = [] |
There was a problem hiding this comment.
Is this needed? If empty changed_files are passed, then self.changed_files would be empty anyway
There was a problem hiding this comment.
Is this needed? If empty
changed_filesare passed, thenself.changed_fileswould be empty anyway
It was added because previously doc8 and clang-tidy used different ways to handle changed_files, this was meant to unify them. But you are right, this is not necessary. Thanks!
llvm/utils/git/code-lint-helper.py
Outdated
| _, ext = os.path.splitext(filepath) | ||
| if ext not in (".rst"): | ||
| continue | ||
| if not filepath.startswith("clang-tools-extra/docs/clang-tidy/checks/"): |
There was a problem hiding this comment.
Are there any obstacles in linting all docs?
| if not filepath.startswith("clang-tools-extra/docs/clang-tidy/checks/"): | |
| if not filepath.startswith("clang-tools-extra/docs/clang-tidy/"): |
llvm/utils/git/code-lint-helper.py
Outdated
| COMMENT_TAG = "<!--LLVM CODE LINT COMMENT: {linter}-->" | ||
| name: str | ||
| friendly_name: str | ||
| comment: dict = None |
There was a problem hiding this comment.
Incomplete type annotation: dict of what?
llvm/utils/git/code-lint-helper.py
Outdated
| failed_linters = [] | ||
| all_comments = [] |
There was a problem hiding this comment.
Missing type annotation. PyLance in Visual Studio Code is great tool to detect such problems.
boomanaiden154
left a comment
boomanaiden154
left a comment
There was a problem hiding this comment.
Can we first make patch refactoring clang-tidy linter into LintHelper and then add doc8 as separate Patch on top of it?
Agreed, splitting would be nice.
I would ideally like us to have a good monorepo wide story for doc formatting/linting, but I guess if we want to try something on clang-tidy first, it doesn't hurt.
|
Hi, I've finished splitting the clang-tidy lintting part :) |
vbvictor
left a comment
vbvictor
left a comment
There was a problem hiding this comment.
Can we adjust pr description/title accordingly
.github/workflows/containers/github-action-ci-tooling/Dockerfile
Outdated
Show resolved
Hide resolved
llvm/utils/git/code-lint-helper.py
Outdated
| repo: str = None | ||
| changed_files: List[str] = [] | ||
| token: str = None | ||
| start_rev: Optional[str] = None |
There was a problem hiding this comment.
Are optional all here needed? AFAIK we expect all these parameters passed into script
There was a problem hiding this comment.
Are optional all here needed? AFAIK we expect all these parameters passed into script
The reason I marked them as Optional is to silence Pyright diagnostics: Type "None" is not assignable to declared type "str"
If we remove Optional, Pyright will continue reporting this. Will it be a problem?
There was a problem hiding this comment.
Can we assign strings to "" then or simply don't write default values?
Semantically these values shouldn't be optional because they always must be provided as script input
llvm/utils/git/code-lint-helper.py
Outdated
| start_rev: Optional[str] = None | ||
| end_rev: Optional[str] = None | ||
| repo: Optional[str] = None | ||
| changed_files: Optional[str] = None |
There was a problem hiding this comment.
can we leave list of str as is or why do we need just string?
llvm/utils/git/code-lint-helper.py
Outdated
| </details> | ||
| """ | ||
|
|
||
| # TODO: Refactor this |
There was a problem hiding this comment.
what is the idea behind refactor?
There was a problem hiding this comment.
Sorry that I forgot to remove this line, fixed in the latest commit.
llvm/utils/git/code-lint-helper.py
Outdated
| return "\n".join(cleaned_lines) | ||
| return None | ||
|
|
||
| def instructions(self, files_to_lint: List[str], args: LintArgs) -> str: |
There was a problem hiding this comment.
Is List really needed? Type with least requirement should be used. Iterable? Collection? Sequence? Same in other places.
There was a problem hiding this comment.
Sorry I forgot the Python requirements again, I'll fix it soon.
There was a problem hiding this comment.
It's not requirements, using most generic interfaces extend set of data types that could be passed in function.
| self.token = args.token | ||
| self.changed_files = args.changed_files | ||
| self.changed_files = ( | ||
| args.changed_files.split(",") if args.changed_files else [] |
There was a problem hiding this comment.
In the original script at L301, it was:
changed_files = []
if args.changed_files:
changed_files = args.changed_files.split(",")
I moved it here, it is basically the same logic.
llvm/utils/git/code-lint-helper.py
Outdated
| start_rev: str | ||
| end_rev: str | ||
| repo: str | ||
| changed_files: Sequence[str] |
There was a problem hiding this comment.
Class data member should be real containers.
llvm/utils/git/code-lint-helper.py
Outdated
|
|
||
| cleaned_lines.append(line) | ||
| class LintHelper: | ||
| COMMENT_TAG = "<!--LLVM CODE LINT COMMENT: {linter}-->" |
There was a problem hiding this comment.
| COMMENT_TAG = "<!--LLVM CODE LINT COMMENT: {linter}-->" | |
| COMMENT_TAG: Final = "<!--LLVM CODE LINT COMMENT: {linter}-->" |
llvm/utils/git/code-lint-helper.py
Outdated
| repo: str = None | ||
| changed_files: List[str] = [] | ||
| token: str = None | ||
| start_rev: str |
There was a problem hiding this comment.
Is it necessary to expose this data member to external world? If not, prefix with two underscores should be uses (private). Same in other places.
| return self.__repo | ||
|
|
||
| @property | ||
| def changed_files(self) -> List[str]: |
There was a problem hiding this comment.
In this particular place it may make sense to return Sequence, if self.__changed_files should be invariant for external users.
5 participants
This PR is not ready for review now.
Closes #167098