Migration: From Clang Tools Binaries to Python Wheels #87
Conversation
github-actions
bot
added
the
documentation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
==========================================
+ Coverage 95.12% 97.01% +1.89%
==========================================
Files 3 3
Lines 82 134 +52
==========================================
+ Hits 78 130 +52
Misses 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #87 will degrade performances by 78.54%Comparing Summary
Benchmarks breakdown
|
|
Warning Rate limit exceeded@shenxianpeng has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis update migrates the project from using a single Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HookScript
participant UtilModule
participant Pip
participant SystemPATH
User->>HookScript: Run clang-format or clang-tidy hook
HookScript->>UtilModule: ensure_installed(tool, version)
UtilModule->>SystemPATH: Check if tool is installed
alt Tool not installed or version mismatch
UtilModule->>Pip: Install tool via pip
Pip-->>UtilModule: Installation result
UtilModule->>SystemPATH: Verify installation
end
UtilModule-->>HookScript: Return tool name
HookScript->>SystemPATH: Execute tool
Possibly related PRs
Suggested labels
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
cpp_linter_hooks/clang_tidy.py (1)
14-29: Capture stderr to avoid losing diagnostics
clang-tidyprints most findings to stderr, but the current call only captures stdout, so warnings/errors are silently dropped. Merge both streams and preserve exit status:- sp = subprocess.run(command, stdout=subprocess.PIPE, encoding="utf-8") - retval = sp.returncode - output = sp.stdout + sp = subprocess.run( + command, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + encoding="utf-8", + ) + retval = sp.returncode + output = (sp.stdout or "") + (sp.stderr or "")You can then keep the
"warning:" / "error:"heuristic unchanged.
🧹 Nitpick comments (2)
pyproject.toml (1)
35-37: Consider pinning minimum versions for the new wheel depsUn-versioned
clang-format/clang-tidywheels may pull in breaking changes unexpectedly. Recommend declaring at least a compatible floor, e.g.:- "clang-format", - "clang-tidy", + "clang-format >= 18.0.0", + "clang-tidy >= 18.0.0",(or whichever major you have CI for).
MIGRATION.md (1)
28-28: Consider more concise wording.The phrase "exactly the same" could be simplified to "unchanged" or "identical" for better readability.
-The pre-commit hook configuration remains exactly the same +The pre-commit hook configuration remains unchanged
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.gitignore(1 hunks)MIGRATION.md(1 hunks)README.md(2 hunks)cpp_linter_hooks/clang_format.py(1 hunks)cpp_linter_hooks/clang_tidy.py(1 hunks)cpp_linter_hooks/util.py(1 hunks)pyproject.toml(2 hunks)testing/main.c(1 hunks)tests/test_util.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
cpp_linter_hooks/clang_tidy.py (1)
cpp_linter_hooks/util.py (1)
ensure_installed(44-64)
cpp_linter_hooks/clang_format.py (1)
cpp_linter_hooks/util.py (1)
ensure_installed(44-64)
tests/test_util.py (1)
cpp_linter_hooks/util.py (2)
ensure_installed(44-64)is_installed(12-41)
🪛 GitHub Actions: Test
testing/main.c
[warning] 4-5: clang-tidy: statement should be inside braces [readability-braces-around-statements]
[error] clang-format hook failed: files were modified by this hook. Run 'clang-format -i --style=file' to fix formatting issues.
[error] clang-tidy hook failed with exit code 1 and 552 warnings generated (551 suppressed). See logs for details.
🪛 LanguageTool
MIGRATION.md
[style] ~28-~28: ‘exactly the same’ might be wordy. Consider a shorter alternative.
Context: ...e pre-commit hook configuration remains exactly the same - All existing `.pre-commit-config.yaml...
(EN_WORDINESS_PREMIUM_EXACTLY_THE_SAME)
🪛 GitHub Actions: pre-commit
MIGRATION.md
[error] 50-50: pre-commit hook 'trailing-whitespace' failed and fixed trailing whitespace issues in this file. Run 'pre-commit run --all-files' locally to reproduce.
🪛 GitHub Check: codecov/patch
cpp_linter_hooks/util.py
[warning] 28-28: cpp_linter_hooks/util.py#L28
Added line #L28 was not covered by tests
[warning] 33-35: cpp_linter_hooks/util.py#L33-L35
Added lines #L33 - L35 were not covered by tests
[warning] 37-39: cpp_linter_hooks/util.py#L37-L39
Added lines #L37 - L39 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run benchmarks
🔇 Additional comments (13)
pyproject.toml (1)
9-9: Description update looks good
The wording now clarifies that wheels are used—nice catch..gitignore (1)
16-16: Comment wording aligns with migration—LGTM
No behavioural change; just clearer documentation.cpp_linter_hooks/clang_format.py (1)
18-19: Variable rename correctly reflects util change
The switch frompathtotool_namematches the newensure_installedcontract; no issues spotted.README.md (2)
57-57: Clear documentation update for wheel package approach.The updated wording clearly indicates the transition to Python wheel packages, which aligns with the migration objectives.
70-71: Excellent migration notice for users.The NOTE block provides clear information about the version where the migration takes effect and the benefits of using wheel packages. This will help users understand the change and its advantages.
MIGRATION.md (1)
1-112: Comprehensive migration guide with excellent coverage.The migration guide is well-structured and covers all essential aspects:
- Clear overview of the changes
- Detailed explanation of benefits
- Explicit migration steps for both users and developers
- Troubleshooting section
- References to relevant resources
This will be very helpful for users transitioning to the new wheel package approach.
tests/test_util.py (5)
5-5: Good addition of mock import for wheel package testing.The unittest.mock import is necessary for the new testing approach that mocks shutil.which.
16-36: Well-simplified test for the new wheel package approach.The test correctly verifies that
ensure_installedreturns the tool name string and logs appropriately. The mocking ofshutil.whichproperly simulates the wheel package behavior.
38-46: Good test coverage for successful tool detection.This test properly verifies the
is_installedfunction when the tool is found viashutil.which.
48-55: Appropriate test for tool not found scenario.The test correctly verifies that
is_installedreturnsNonewhen the tool is not found anywhere.
58-73: Excellent test for graceful handling of missing tools.The test verifies that
ensure_installedstill returns the tool name even when not found and logs an appropriate warning. This aligns with the new approach of letting subprocess handle the error.cpp_linter_hooks/util.py (2)
2-2: Appropriate import for wheel package tool detection.The shutil import is necessary for the new
shutil.which()based tool detection approach.
44-64: Excellent refactoring to support wheel package approach.The
ensure_installedfunction has been properly refactored:
- No longer attempts installation (appropriate for wheel packages)
- Returns tool name string instead of Path (correct for direct execution)
- Logs helpful warnings when tools aren't found
- Lets subprocess handle execution errors gracefully
This aligns perfectly with the migration to wheel packages where tools are pre-installed dependencies.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
testing/run.sh (3)
1-4: Addset -euo pipefailfor safer scriptingThe script keeps running even if intermediate commands fail (aside from the explicit
|| true).
Fail-fast semantics plus unset-variable protection will make diagnostics easier and avoid silent mis-behaviour.#!/bin/bash +set -euo pipefail
24-26:result.txtshould be truncated before first useBecause
tee -aappends, an existingresult.txtfrom a previous run inflates the failure count and yields false positives/negatives.
Explicitly truncate (or delete) the file up-front.echo "==================================================================================" echo "print result.txt" +# ensure a clean slate +: > result.txt cat result.txt
32-38: Hard-coded failure count is brittle – externalise or derive itChanging the magic number from 10 → 9 fixes the test, but the value is still an opaque constant that will break again with every tooling update.
Either:
- Infer the expected failures dynamically (e.g. by counting “Checking …” blocks and subtracting “Passed”), or
- Surface it as a single
EXPECTED_FAILURESvariable at the top so future updates touch one line.-# expected number of failing hooks -if [ $failed_cases -eq 9 ]; then +# expected number of failing hooks (update when tool versions change) +EXPECTED_FAILURES=9 + +if [ "$failed_cases" -eq "$EXPECTED_FAILURES" ]; then
🧹 Nitpick comments (1)
testing/run.sh (1)
28-28: Prefer modern command substitutionBack-ticks are harder to nest/quote and are considered legacy.
Use$( … )instead of`…`.-failed_cases=`grep -c "Failed" result.txt` +failed_cases=$(grep -c "Failed" result.txt)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.pre-commit-hooks.yaml(1 hunks)pyproject.toml(1 hunks)testing/run.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .pre-commit-hooks.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run benchmarks
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/test_util.py (1)
17-26: Combine nestedwithstatements for better readability.The test logic is correct, but the nested
withstatements can be combined as suggested by static analysis.Apply this diff to improve code style:
- with monkeypatch.context(): - # Mock shutil.which to simulate the tool being available - with patch("shutil.which", return_value=str(tmp_path / tool)): - # Mock _get_runtime_version to return a matching version - mock_version = "20.1.7" if tool == "clang-format" else "20.1.0" - with patch( - "cpp_linter_hooks.util._get_runtime_version", return_value=mock_version - ): + with ( + monkeypatch.context(), + patch("shutil.which", return_value=str(tmp_path / tool)), + patch( + "cpp_linter_hooks.util._get_runtime_version", + return_value="20.1.7" if tool == "clang-format" else "20.1.0" + ), + ):MIGRATION.md (1)
28-29: Consider simplifying wording for conciseness.The documentation is comprehensive and well-structured. Minor suggestion to improve readability:
-- **No breaking changes**: The pre-commit hook configuration remains exactly the same +- **No breaking changes**: The pre-commit hook configuration remains the samecpp_linter_hooks/util.py (4)
13-23: Consider adding test coverage for edge cases.The function correctly extracts versions from dependencies, but static analysis indicates lines 16 and 22 lack test coverage. These handle the cases where pyproject.toml doesn't exist or doesn't contain the expected dependency.
Consider adding a test to cover these edge cases:
def test_get_version_from_dependency_no_file(monkeypatch): """Test when pyproject.toml doesn't exist.""" monkeypatch.setattr(Path, "exists", lambda self: False) assert get_version_from_dependency("clang-format") is None def test_get_version_from_dependency_not_found(): """Test when dependency is not in the list.""" with patch("toml.load", return_value={"project": {"dependencies": ["other==1.0"]}}): assert get_version_from_dependency("clang-format") is None
110-131: Add test coverage for version parsing edge cases.The version resolution logic is well-designed with support for partial version matching. However, some branches lack test coverage.
Consider adding tests for:
- Invalid version strings (lines 115-116)
- Major.minor version matching (line 122)
def test_resolve_version_invalid(): """Test with invalid version string.""" assert _resolve_version(["1.0.0"], "invalid") is None def test_resolve_version_major_minor(): """Test major.minor version matching.""" versions = ["1.0.0", "1.0.1", "1.1.0", "2.0.0"] assert _resolve_version(versions, "1.0") == "1.0.1"
133-144: Make return statement explicit for consistency.The function correctly extracts versions but Pylint flagged inconsistent return statements. Add an explicit return for clarity.
def _get_runtime_version(tool: str) -> Optional[str]: try: output = subprocess.check_output([tool, "--version"], text=True) if tool == "clang-tidy": lines = output.strip().splitlines() if len(lines) > 1: return lines[1].split()[-1] elif tool == "clang-format": return output.strip().split()[-1] except Exception: return None + return None
146-155: Add test coverage for installation failure.The installation logic is correct, but the error handling path (lines 152-154) lacks test coverage.
Consider adding a test for installation failure:
def test_install_tool_failure(caplog): """Test when pip install fails.""" with patch("subprocess.check_call", side_effect=subprocess.CalledProcessError(1, "pip")): result = _install_tool("clang-format", "20.1.7") assert result is None assert "Failed to install" in caplog.text
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
MIGRATION.md(1 hunks)cpp_linter_hooks/clang_format.py(1 hunks)cpp_linter_hooks/clang_tidy.py(1 hunks)cpp_linter_hooks/util.py(1 hunks)pyproject.toml(1 hunks)tests/test_util.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pyproject.toml
- cpp_linter_hooks/clang_tidy.py
- cpp_linter_hooks/clang_format.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_util.py (1)
cpp_linter_hooks/util.py (2)
ensure_installed(193-200)is_installed(185-190)
🪛 Ruff (0.11.9)
tests/test_util.py
17-19: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
🪛 GitHub Check: codecov/patch
cpp_linter_hooks/util.py
[warning] 16-16: cpp_linter_hooks/util.py#L16
Added line #L16 was not covered by tests
[warning] 22-22: cpp_linter_hooks/util.py#L22
Added line #L22 was not covered by tests
[warning] 115-116: cpp_linter_hooks/util.py#L115-L116
Added lines #L115 - L116 were not covered by tests
[warning] 122-122: cpp_linter_hooks/util.py#L122
Added line #L122 was not covered by tests
[warning] 152-154: cpp_linter_hooks/util.py#L152-L154
Added lines #L152 - L154 were not covered by tests
🪛 Pylint (3.3.7)
cpp_linter_hooks/util.py
[refactor] 133-133: Either all return statements in a function should return an expression, or none of them should.
(R1710)
🪛 LanguageTool
MIGRATION.md
[style] ~28-~28: ‘exactly the same’ might be wordy. Consider a shorter alternative.
Context: ...e pre-commit hook configuration remains exactly the same - All existing `.pre-commit-config.yaml...
(EN_WORDINESS_PREMIUM_EXACTLY_THE_SAME)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run benchmarks
🔇 Additional comments (7)
tests/test_util.py (2)
1-11: LGTM! Import changes align with the new testing approach.The addition of
unittest.mock.patchand the focused imports of onlyensure_installedandis_installedproperly reflect the new API design.
40-79: Well-structured tests covering key scenarios.The three new test functions provide good coverage:
test_is_installed_with_shutil_which: Validates successful tool detectiontest_is_installed_not_found: Handles the negative casetest_ensure_installed_tool_not_found: Tests behavior when installation failsThe tests properly mock external dependencies and verify both return values and logging output.
MIGRATION.md (1)
1-112: Excellent migration guide!The documentation thoroughly covers:
- Clear explanation of the migration rationale
- No breaking changes for end users
- Detailed developer migration steps with code examples
- Comprehensive troubleshooting section
- Helpful external references
This will greatly assist users in understanding and adopting the new wheel-based approach.
cpp_linter_hooks/util.py (4)
25-107: Comprehensive version support!The extensive version lists and dynamic default version extraction from pyproject.toml provide excellent flexibility. The fallback values ensure robustness if the TOML parsing fails.
157-183: Well-designed installation orchestration!The function effectively:
- Resolves user-specified versions to available versions
- Falls back to default versions when needed
- Detects version mismatches and triggers reinstallation
- Handles both new installations and updates
The logic flow is clear and handles all necessary scenarios.
185-191: Clean simplification of tool detection!The new implementation correctly relies on
shutil.which()for tool detection, which is appropriate for Python wheel packages that properly register executables in PATH.
193-201: Updated interface aligns well with wheel package approach!The function now returns the tool name string instead of a Path object, which is appropriate for wheel-installed executables that can be invoked directly by name. The logging provides good visibility into the installation process.
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the project’s clang-format and clang-tidy support from a single clang-tools binary package to dedicated Python wheel packages, updating install logic, docs, and tests.
- Replaced binary-based install with pip-based wheel install using
shutil.whichand version resolution. - Updated documentation (
README.md,MIGRATION.md) and pre-commit config to reflect wheel usage. - Expanded tests and adjusted dependencies in
pyproject.toml; tweaked test runner script’s expected failure count.
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_util.py | Refactored tests to exercise new ensure_installed and is_installed behaviors |
| testing/run.sh | Adjusted expected failure count from 10 to 9 |
| pyproject.toml | Swapped clang-tools for separate clang-format/clang-tidy wheels; added deps |
| cpp_linter_hooks/util.py | Rewrote installation/version logic to use pip wheels; added version-resolution |
| cpp_linter_hooks/clang_tidy.py | Updated default version and command to use returned tool name |
| cpp_linter_hooks/clang_format.py | Updated default version and command to use returned tool name |
| README.md | Added note and instructions for wheel-based clang tools |
| MIGRATION.md | New migration guide from clang-tools to wheel packages |
| .pre-commit-hooks.yaml | Changed require_serial to false for parallel hook execution |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
MIGRATION.md (2)
52-61: Addlanguage_versionor environment constraints to the sample config.Many CI environments run system Pythons that default to 3.8 or earlier. When wheels require a newer interpreter, users will encounter opaque
ERROR: wheel is not supportedmessages. Explicitly documenting a minimal Python version (or addinglanguage_version: python3.Xin the snippet) can save troubleshooting time.
67-72: Complement cache-clean instructions withautoupdate.After clearing the cache, users still need to refresh hook revisions in existing repos. Suggest adding:
pre-commit autoupdatebefore
pre-commit installso the new wheels are fetched automatically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MIGRATION.md(1 hunks)README.md(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🪛 LanguageTool
MIGRATION.md
[misspelling] ~35-~35: Did you mean “you're” (short for ‘you are’)?
Context: ...No breaking changes for end users* - Your existing .pre-commit-config.yaml file...
(YOUR)
[duplication] ~88-~88: Possible typo: you repeated a word.
Context: ...perating system and version - Python version - cpp-linter-hooks version - Complete error message/stack trace...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test (3.9)
- GitHub Check: test (3.10)
- GitHub Check: test (3.14)
- GitHub Check: test (3.11)
- GitHub Check: Run benchmarks
🔇 Additional comments (1)
MIGRATION.md (1)
25-28: Pinned wheel versions verified as correctThe versions
clang-format==20.1.7andclang-tidy==20.1.0are available on PyPI and match the latest releases. No updates to the migration guide are needed.
|
shenxianpeng
changed the title
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests