feat(linters): Add cli tool for running clang-tidy in parallel. #17
Conversation
WalkthroughThe changes modify the Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant Main as main()
participant Scheduler as Task Scheduler
participant Worker as ClangTidy Task
CLI->>Main: Invoke CLI with arguments
Main->>Scheduler: Start parallel execution (_clang_tidy_parallel_execution_entry)
Scheduler->>Worker: Launch async task with semaphore
Worker-->>Scheduler: Return ClangTidyResult
Scheduler-->>Main: Aggregate and return results
Main->>CLI: Output results and exit
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
LinZhihao-723
changed the title
LinZhihao-723
changed the title
cli/clang-tidy-utils/pyproject.toml
Outdated
| @@ -0,0 +1,45 @@ | |||
| [project] | |||
| name = "yscope-clang-tidy-utils" | |||
There was a problem hiding this comment.
We may drop yscope- prefix
There was a problem hiding this comment.
I don't have a strong preference, but if we ever do upload this to pypi it probably makes more sense to have the prefix. If we never upload it to pypi the name won't ever really matter right?
There was a problem hiding this comment.
I prefer to drop the prefix for now since it's likely this will be an internal tool for the foreseeable future.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
cli/clang-tidy-utils/src/yscope_clang_tidy_utils/cli.py (3)
22-30: Consider validating file existence.While building the argument list, consider verifying that the file exists before passing it to clang-tidy, especially in large codebases where file paths may be stale. This helps provide clearer error messages if the file is missing.
48-50: Handle unresponsive processes more aggressively.Currently, a
process.terminate()signals the process, but clang-tidy or any subprocess may ignore SIGTERM. Consider adding a fallback mechanism (e.g., process.kill()) after a short delay to ensure the process halts cleanly.except asyncio.CancelledError: process.terminate() + try: + await asyncio.wait_for(process.wait(), timeout=2) + except asyncio.TimeoutError: + process.kill() raise
76-109: Consider early cancellation upon the first failure.All tasks continue to run even if one file fails clang-tidy checks. If you'd like to fail early and skip checking subsequent files, you could cancel remaining tasks once an error is detected.
if 0 != result.ret_code: ret_code = 1 + # Optionally cancel remaining tasks to fail early: + for remaining_task in tasks: + if not remaining_task.done(): + remaining_task.cancel() + breakcli/clang-tidy-utils/README.md (3)
3-3: Fix grammar in project introduction.Use "a CLI script" instead of "a CLI scripts" to maintain grammatical correctness.
-This project is a CLI scripts for running [clang-tidy][clang-tidy-home] checks. +This project is a CLI script for running [clang-tidy][clang-tidy-home] checks.🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: The plural noun “scripts” cannot be used with the article “a”. Did you mean “a CLI script”?
Context: ...scope-clang-tidy-utils This project is a CLI scripts for running [clang-tidy][clang-tidy-hom...(A_NNS)
30-30: Remove trailing punctuation from heading.Remove the colon in "## Development:" to align with common Markdown style guidelines.
-## Development: +## Development🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
30-30: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
41-41: Link reference [uv-venv] is unused.The link definition “[uv-venv]” is never referenced in the document. Consider removing it for clarity.
-[uv-venv]: https://docs.astral.sh/uv/pip/compatibility/#virtual-environments-by-default🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
41-41: Link and image reference definitions should be needed
Unused link or image reference definition: "uv-venv"(MD053, link-image-reference-definitions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cli/clang-tidy-utils/.gitignore(1 hunks)cli/clang-tidy-utils/README.md(1 hunks)cli/clang-tidy-utils/pyproject.toml(1 hunks)cli/clang-tidy-utils/src/yscope_clang_tidy_utils/cli.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cli/clang-tidy-utils/.gitignore
🧰 Additional context used
🪛 LanguageTool
cli/clang-tidy-utils/README.md
[grammar] ~3-~3: The plural noun “scripts” cannot be used with the article “a”. Did you mean “a CLI script”?
Context: ...scope-clang-tidy-utils This project is a CLI scripts for running [clang-tidy][clang-tidy-hom...
(A_NNS)
🪛 markdownlint-cli2 (0.17.2)
cli/clang-tidy-utils/README.md
30-30: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
41-41: Link and image reference definitions should be needed
Unused link or image reference definition: "uv-venv"
(MD053, link-image-reference-definitions)
🔇 Additional comments (5)
cli/clang-tidy-utils/src/yscope_clang_tidy_utils/cli.py (2)
61-74: Concurrency strategy looks solid.The approach to wrap each
execute_clang_tidy_taskwith a semaphore is a well-structured way to impose concurrency limits. No immediate procedural concerns here.
111-175: Ensure clang-tidy is installed before calling.If clang-tidy is missing, the CLI may crash. Consider implementing a preliminary check for clang-tidy’s existence to offer a more helpful error message to the user.
cli/clang-tidy-utils/pyproject.toml (3)
9-10: Verify clang-tidy version compatibility.Ensure that
clang-tidy >= 19.1.0is fully supported in your environment. In certain older distributions, clang-tidy’s package name or version might differ.
13-18: Development dependencies are well structured.Declaring formatting, linting, and type-checking tools fosters a consistent development workflow.
20-21: Handy script entry point.Defining
yscope-clang-tidy-utilsin[project.scripts]is a user-friendly approach that enhances accessibility and makes the CLI straightforward to run.
Fixed. |
LinZhihao-723
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
linters/clang-tidy-utils/src/clang_tidy_utils/cli.py (4)
22-30: Consider validating file existence or providing helpful error messages.
If a file path is invalid, clang-tidy will fail silently here. A quick file check or user-facing error message could enhance usability.
33-58: Robust async approach with graceful cancellation handling.
Terminating the process upon cancellation is a good practice to avoid resource leaks. However, you may consider additional handling if clang-tidy is not installed or is unavailable in PATH.
76-109: Optional: Provide a consolidated summary of failures.
Currently, each file's result is printed individually. For large batches, a final summary of failed files or a short report can improve usability.
111-175: Minor docstring clarity.
In the docstring for_clang_tidy_parallel_execution_entry, the parameterclang_tidy_argsis grouped withfilesin the text. Splitting them out can improve clarity. Also, usingsys.exit(ret_code)instead ofexit(ret_code)is a common convention to maintain consistent script termination.linters/clang-tidy-utils/README.md (1)
34-34: Remove trailing punctuation from the heading.
To adhere to markdown style guidelines (MD026), consider removing the colon in “## Development:”.-## Development: +## Development🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
34-34: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
linters/clang-tidy-utils/.gitignore(1 hunks)linters/clang-tidy-utils/README.md(1 hunks)linters/clang-tidy-utils/pyproject.toml(1 hunks)linters/clang-tidy-utils/src/clang_tidy_utils/cli.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- linters/clang-tidy-utils/.gitignore
- linters/clang-tidy-utils/pyproject.toml
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
linters/clang-tidy-utils/README.md
34-34: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (3)
linters/clang-tidy-utils/src/clang_tidy_utils/cli.py (3)
1-9: Imports look good.
No issues detected; these imports are succinct and well-structured.
10-19: Good use of a data class for encapsulating clang-tidy results.
This design cleanly stores essential details such as return code and captured outputs.
61-74: Concurrency control is well-handled.
Using a semaphore to limit simultaneous tasks helps balance CPU usage effectively.
Description
This PR introduces a cli sub-project for running clang-tidy in parallel. For now, it is almost like a simplified script of the llvm version here.
As discussed offline, we decided to use uv to manage our Python cli scripts.
Validation performed
I have tested this cli by using it to check clp-ffi-py. Here's how it works:
Here, we parallel 10 clang-tidy tasks with extra clang-tidy arguments
-p build/compile_commands.json --line-filter="[{\"name\":\"cpp11_zone.hpp\",\"lines\":[[197,197]]}]"to match what we specify in the linting task here. You can useecho $?to check the return value is 0. If we remove the lint filter in the arguments above, clang-tidy will raise a warning on msgpack's source code and thus the return value ofecho $?will be 1 in this case.Summary by CodeRabbit
Documentation
New Features
Chores