feat: support for clang-include-cleaner and clang-apply-replacements and switch to dynamic PyPI version resolution #185
Conversation
…eplacements - Add 'clang-include-cleaner' and 'clang-apply-replacements' to WHEEL_TOOLS set in main.py, enabling installation via 'clang-tools install <tool> --wheel' - Add tests for wheel install, auto-detect, and unsupported-tool validation covering the new tools
|
Warning Review limit reached
More reviews will be available in 20 minutes and 14 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughA new ChangesWheel installation system expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #185 +/- ##
===========================================
+ Coverage 99.87% 100.00% +0.12%
===========================================
Files 7 9 +2
Lines 787 1047 +260
===========================================
+ Hits 786 1047 +261
+ Misses 1 0 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Use resolve_install_with_diagnostics() instead of resolve_install() to get rich error messages from PyPI API - Update tests to mock new tuple-returning API - Bump cpp-linter-hooks minimum version to >=1.2.0 - Update uv.lock
Remove the cpp-linter-hooks dependency. Wheel installation is now handled by clang_tools/wheel_install.py using only the Python standard library (urllib + json for PyPI API, subprocess for pip). - Add clang_tools/wheel_install.py: resolve_wheel_install() with PyPI JSON API version resolution, LRU caching, and fallback to locally installed versions when PyPI is unreachable - Remove clang_tools/wheel.py: legacy cpp-linter-hooks wrapper - Remove cpp-linter-hooks from pyproject.toml dependencies - Update tests to mock the new clang_tools.wheel_install module
for more information, see https://pre-commit.ci
shenxianpeng
changed the title
clang-include-cleaner and clang-apply-replacements and switch to dynamic PyPI version resolution
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clang_tools/wheel_install.py`:
- Around line 179-182: The current return statement always returns None as the
second element of the tuple, even when _install_tool() fails. This breaks the
resolver contract that expects a non-None error when installation or
post-install lookup fails. Modify the return statement to capture any error that
occurs when _install_tool(tool, user_version) is called and return that error as
the second element of the tuple instead of hardcoding None, ensuring the
contract is maintained and error handling in clang_tools/main.py works
correctly.
- Around line 134-142: The _is_version_installed function uses a substring check
(if version in result.stdout) which can incorrectly match when the installed
version merely contains the requested version as a substring. Replace this with
exact version parsing and equality comparison. Additionally, add the timeout and
OSError handling to the subprocess.run call that you already implemented in
_detect_installed_version to make the code consistent and more robust. Parse the
version token from the stdout output and compare it for equality rather than
substring containment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5afa1947-4bea-4c5e-b1aa-ce0c17d03005
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
clang_tools/main.pyclang_tools/wheel.pyclang_tools/wheel_install.pypyproject.tomltests/test_main.py
💤 Files with no reviewable changes (2)
- pyproject.toml
- clang_tools/wheel.py
🚧 Files skipped from review as they are similar to previous changes (1)
- clang_tools/main.py
| def _is_version_installed(tool: str, version: str) -> Optional[Path]: | ||
| """Return the tool path if the installed version matches, otherwise None.""" | ||
| existing = shutil.which(tool) | ||
| if not existing: | ||
| return None | ||
| result = subprocess.run([existing, "--version"], capture_output=True, text=True) | ||
| if version in result.stdout: | ||
| return Path(existing) | ||
| return None |
There was a problem hiding this comment.
Use an exact parsed version match here, not a raw substring check.
if version in result.stdout can reuse the wrong local binary for an explicit request when the installed version merely contains that string, and this path also drops the timeout/OSError handling you already added in _detect_installed_version(). Parse the version token and compare for equality instead.
Proposed fix
def _is_version_installed(tool: str, version: str) -> Optional[Path]:
"""Return the tool path if the installed version matches, otherwise None."""
existing = shutil.which(tool)
if not existing:
return None
- result = subprocess.run([existing, "--version"], capture_output=True, text=True)
- if version in result.stdout:
- return Path(existing)
+ try:
+ result = subprocess.run(
+ [existing, "--version"], capture_output=True, text=True, timeout=10
+ )
+ except (OSError, subprocess.TimeoutExpired):
+ return None
+ match = re.search(r"(\d+\.\d+\.\d+(?:\.\d+)?)", result.stdout)
+ if match and match.group(1) == version:
+ return Path(existing)
return None🧰 Tools
🪛 ast-grep (0.43.0)
[error] 138-138: Use of unsanitized data to create processes
Context: subprocess.run([existing, "--version"], capture_output=True, text=True)
Note: [CWE-78].
(os-system-unsanitized-data)
[error] 138-138: Command coming from incoming request
Context: subprocess.run([existing, "--version"], capture_output=True, text=True)
Note: [CWE-20].
(subprocess-from-request)
🪛 Ruff (0.15.15)
[error] 139-139: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clang_tools/wheel_install.py` around lines 134 - 142, The
_is_version_installed function uses a substring check (if version in
result.stdout) which can incorrectly match when the installed version merely
contains the requested version as a substring. Replace this with exact version
parsing and equality comparison. Additionally, add the timeout and OSError
handling to the subprocess.run call that you already implemented in
_detect_installed_version to make the code consistent and more robust. Parse the
version token from the stdout output and compare it for equality rather than
substring containment.
| return ( | ||
| _is_version_installed(tool, user_version) or _install_tool(tool, user_version), | ||
| None, | ||
| ) |
There was a problem hiding this comment.
Don't let install failures escape as (None, None).
If _install_tool() fails, this returns (None, None), which breaks the resolver contract documented in this module and drops the per-tool error handling that clang_tools/main.py now expects. Return a non-None error whenever the install or post-install lookup fails.
Proposed fix
def resolve_wheel_install(
tool: str, version: Optional[str]
) -> Tuple[Optional[Path], Optional[str]]:
@@
user_version, error = _resolve_version(tool, version)
if error is not None:
return None, error
- return (
- _is_version_installed(tool, user_version) or _install_tool(tool, user_version),
- None,
- )
+ installed = _is_version_installed(tool, user_version)
+ if installed is not None:
+ return installed, None
+
+ installed = _install_tool(tool, user_version)
+ if installed is not None:
+ return installed, None
+
+ return None, f"Failed to install {tool} {user_version}."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clang_tools/wheel_install.py` around lines 179 - 182, The current return
statement always returns None as the second element of the tuple, even when
_install_tool() fails. This breaks the resolver contract that expects a non-None
error when installation or post-install lookup fails. Modify the return
statement to capture any error that occurs when _install_tool(tool,
user_version) is called and return that error as the second element of the tuple
instead of hardcoding None, ensuring the contract is maintained and error
handling in clang_tools/main.py works correctly.
- Add tests/test_wheel_install.py: comprehensive tests for PyPI version resolution, pip installation, and all edge cases (23 tests) - Add test for (None, None) fallback in _wheel_install - Add OS-forcing tests for non-macos and non-linux branches - Restructure OS-dependent test branches for full coverage - Production code: 100% (5 modules, 365 statements) - All tests: 151 passed
for more information, see https://pre-commit.ci
Co-authored-by: Xianpeng Shen <xianpeng.shen@gmail.com>
This reverts commit 98fb20e.
…ply-replacements clang-include-cleaner tested for version >= 16 (introduced in LLVM 16) clang-apply-replacements tested for version >= 13 (matching clang-tidy)
PyPI only has clang-apply-replacements wheels starting at 16.0.0
…lity clang-include-cleaner: only v22 on PyPI clang-apply-replacements: only v16 and v17 on PyPI
|
shenxianpeng
deleted the
feature/support-include-cleaner-apply-replacements
branch
1 participant
Summary
Add
clang-include-cleanerandclang-apply-replacementsto theWHEEL_TOOLSset inclang-tools-pip, enabling:Changes
clang_tools/main.py"clang-include-cleaner"and"clang-apply-replacements"toWHEEL_TOOLStests/test_main.pytest_main_install_wheel_include_cleaner— basic wheel installtest_main_install_wheel_apply_replacements— basic wheel installtest_main_install_auto_detect_new_wheel_tools— auto-detect treats both as wheel toolsNote
These tools currently have limited version availability on PyPI:
clang-include-cleaner: only 22.1.7clang-apply-replacements: 16.0.0, 17.0.6Requires: cpp-linter/cpp-linter-hooks#242
Summary by CodeRabbit
clang-include-cleanerandclang-apply-replacementsto support installation via the--wheeloption.cpp-linter-hooksdependency to>=1.2.0.