feat: integrate ctcache for clang-tidy result caching in CI #7071
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughAdds ctcache support for caching clang-tidy results in CI. The workflow (.github/workflows/build-src.yml) restores and saves a ctcache cache on linux64_multiprocess runs (save only on pushes to the default branch). ci/dash/lint-tidy.sh initializes and validates ctcache and clang-tidy, zeroes and reports per-run ctcache stats, invokes clang-tidy via a ctcache wrapper, enforces size/age-based cleanup, and expands iwyu_tool.py inputs. contrib/containers/ci/ci.Dockerfile fetches ctcache helpers at a pinned commit and creates /cache/ctcache. ci/test/00_setup_env.sh adds CTCACHE_DIR and CTCACHE_MAXSIZE_MB defaults. Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
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 |
![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 (1)
ci/dash/lint-tidy.sh (1)
26-26: LGTM with observation on hardcoded path.The clang-tidy invocation correctly uses the ctcache wrapper via
-clang-tidy-binary=/usr/local/bin/clang-tidy-cache. The hardcoded path matches the Dockerfile installation, but consider whether this should be more flexible (e.g., using a variable or which command).If you want to make the path more maintainable, consider:
+CLANG_TIDY_CACHE="/usr/local/bin/clang-tidy-cache" + cd "${BASE_ROOT_DIR}/build-ci/dashcore-${BUILD_TARGET}/src" -if ! ( run-clang-tidy -clang-tidy-binary=/usr/local/bin/clang-tidy-cache -quiet "${MAKEJOBS}" | tee tmp.tidy-out.txt ); then +if ! ( run-clang-tidy -clang-tidy-binary="${CLANG_TIDY_CACHE}" -quiet "${MAKEJOBS}" | tee tmp.tidy-out.txt ); then
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build-src.yml(2 hunks)ci/dash/lint-tidy.sh(1 hunks)contrib/containers/ci/ci.Dockerfile(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{guix-build*,releases,**/guix-build*,releases/**,.github/**,depends/**,ci/**,contrib/**,doc/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not make changes to build system files (guix-build*), release artifacts, or avoid changes to .github, depends, ci, contrib, and doc directories unless specifically prompted
Files:
ci/dash/lint-tidy.sh.github/workflows/build-src.ymlcontrib/containers/ci/ci.Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: Lint / Run linters
🔇 Additional comments (9)
.github/workflows/build-src.yml (2)
114-123: LGTM!The ctcache restore logic is correctly structured, following the same pattern as ccache. The cache key incorporates the Dockerfile and dependencies hashes to ensure proper invalidation when the build environment changes, and the restore-keys allow efficient cache reuse across commits.
132-142: LGTM!The ctcache save step correctly restricts cache persistence to pushes on the default branch, preventing cache pollution from PR branches. The path and key structure are consistent with the restore step.
contrib/containers/ci/ci.Dockerfile (2)
83-89: LGTM!The ctcache directory is created consistently with the existing cache directory structure, and ownership is properly assigned to the non-privileged user.
68-81: The pinned commit and patches are still necessary.The Dockerfile correctly pins ctcache to commit
e33c063ca6e52b48bcefbc45d46d8c150ffa81caand applies two necessary patches:
- Line 77-78: Fixes
return shlex.split(command["arguments"][0])to return the list directly, sincecommand["arguments"]is already a list- Line 79-80: Changes digest format from
{i:x}to{i:02x}for zero-padded hex formattingBoth patches remain necessary—the upstream code in both the pinned commit and main branch still contains the unpatched code. No fixes have been integrated upstream as of the commit date (2025-09-02). The pinning strategy is appropriate given these unresolved issues.
ci/dash/lint-tidy.sh (5)
14-19: LGTM!The ctcache environment setup correctly exports the necessary variables, and the cache directory path is consistent with the workflow configuration and Dockerfile. The
mkdir -psafely handles the case where the directory already exists.
21-22: LGTM with minor observation.The zero-stats call correctly resets statistics before the run. The
|| trueprevents script failure if the statistics feature is unavailable, which is appropriate for a non-critical operation.
32-36: LGTM!The ctcache statistics display provides useful diagnostics with appropriate error handling. The combination of
du -shand--show-statsgives both size and hit rate information.
53-53: LGTM!The divider line properly closes the ctcache statistics section, maintaining clean output formatting.
37-52: Add defensive checks for cache size retrieval.The cache management logic is generally sound, but if
du -smfails (e.g., permissions issue, directory not found),CACHE_SIZE_MBwill be empty, causing the comparison on Line 40 to fail silently. This could allow unbounded cache growth.Apply this diff to add defensive checks:
# Limit cache size (ctcache has no built-in size management) CTCACHE_MAXSIZE_MB=50 CACHE_SIZE_MB=$(du -sm "${CTCACHE_DIR}" 2>/dev/null | cut -f1) +if [ -z "${CACHE_SIZE_MB}" ]; then + echo "Warning: Unable to determine cache size, skipping cleanup" + CACHE_SIZE_MB=0 +fi if [ "${CACHE_SIZE_MB}" -gt "${CTCACHE_MAXSIZE_MB}" ]; then echo "Cache size ${CACHE_SIZE_MB}MB exceeds limit ${CTCACHE_MAXSIZE_MB}MB, cleaning old entries..." # Remove files older than 7 days, or if still too large, oldest 20% of files find "${CTCACHE_DIR}" -type f -mtime +7 -delete 2>/dev/null || true CACHE_SIZE_MB=$(du -sm "${CTCACHE_DIR}" 2>/dev/null | cut -f1) + if [ -z "${CACHE_SIZE_MB}" ]; then + CACHE_SIZE_MB=0 + fi if [ "${CACHE_SIZE_MB}" -gt "${CTCACHE_MAXSIZE_MB}" ]; thenLikely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ci/dash/lint-tidy.sh (3)
14-19: Consider verifying clang-tidy availability explicitly.The
which clang-tidycommand will cause the script to exit (due toset -e) if clang-tidy is not in PATH, but without a clear error message. While the fail-fast behavior is acceptable for CI, consider adding an explicit check with a descriptive error message for easier debugging.Example improvement:
+# Verify clang-tidy is available +if ! command -v clang-tidy >/dev/null 2>&1; then + echo "Error: clang-tidy not found in PATH" + exit 1 +fi + # Setup ctcache for clang-tidy result caching export CTCACHE_DIR="/cache/ctcache" export CTCACHE_SAVE_OUTPUT=1 CTCACHE_CLANG_TIDY=$(which clang-tidy) export CTCACHE_CLANG_TIDY mkdir -p "${CTCACHE_DIR}"
21-25: Verify ctcache installation before use.The hardcoded paths to ctcache binaries are not verified before use. If ctcache is not properly installed (e.g., missing from the Docker image), the script will fail at Line 29 with an unclear error when
run-clang-tidycannot find the binary.Add verification:
CLANG_TIDY_CACHE="/usr/local/bin/clang-tidy-cache" CLANG_TIDY_CACHE_PY="/usr/local/bin/src/ctcache/clang_tidy_cache.py" + +# Verify ctcache is installed +if [ ! -x "${CLANG_TIDY_CACHE}" ]; then + echo "Error: ctcache binary not found at ${CLANG_TIDY_CACHE}" + exit 1 +fi +if [ ! -f "${CLANG_TIDY_CACHE_PY}" ]; then + echo "Warning: ctcache Python script not found at ${CLANG_TIDY_CACHE_PY}, statistics will be unavailable" +fi # Zero stats before run to get accurate statistics for this run only python3 "${CLANG_TIDY_CACHE_PY}" --zero-stats 2>&1 || true
49-52: Consider using null-terminated strings for maximum robustness.The current file removal pipeline works correctly, but using null-terminated strings would be more robust against unusual filenames (newlines, etc.) and is a best practice for shell scripts processing file paths.
Optional improvement:
FILE_COUNT=$(find "${CTCACHE_DIR}" -type f | wc -l) REMOVE_COUNT=$((FILE_COUNT / 5)) - find "${CTCACHE_DIR}" -type f -printf '%T+ %p\n' | sort | head -n "${REMOVE_COUNT}" | cut -d' ' -f2- | xargs rm -f 2>/dev/null || true + find "${CTCACHE_DIR}" -type f -printf '%T+ %p\0' | sort -z | head -z -n "${REMOVE_COUNT}" | cut -z -d' ' -f2- | xargs -0 rm -f 2>/dev/null || true echo "Removed ${REMOVE_COUNT} oldest cache entries"Note: This uses null-terminated strings throughout the pipeline for safer file path handling.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ci/dash/lint-tidy.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{guix-build*,releases,**/guix-build*,releases/**,.github/**,depends/**,ci/**,contrib/**,doc/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not make changes to build system files (guix-build*), release artifacts, or avoid changes to .github, depends, ci, contrib, and doc directories unless specifically prompted
Files:
ci/dash/lint-tidy.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (4)
ci/dash/lint-tidy.sh (4)
29-29: LGTM: ctcache integration in run-clang-tidy.The modification correctly integrates ctcache by using the
-clang-tidy-binaryflag while preserving the existing error handling and output capture logic.
35-38: LGTM: Statistics display.The statistics display provides useful debugging information and correctly handles potential failures with fallback messages.
40-55: Note: GNU find dependency.The cache cleanup logic uses
find -printf, which is specific to GNU findutils and may not work on BSD systems. Since this script runs in a controlled Docker CI environment (likely GNU/Linux), this is acceptable. However, if portability becomes a concern, consider usingfind -exec statas an alternative.The cleanup logic assumes GNU find. Verify that the CI Docker image uses GNU coreutils:
#!/bin/bash # Verify GNU find is available in the CI environment # Expected: GNU find is installed and -printf is supported # Check if we can inspect the Dockerfile if [ -f "contrib/containers/ci/ci.Dockerfile" ]; then echo "=== Checking CI Dockerfile for base image ===" grep -E "^FROM" contrib/containers/ci/ci.Dockerfile echo "" fi # Check if there's documentation about the CI environment echo "=== Checking for CI documentation ===" fd -t f "README" ci/ contrib/containers/ 2>/dev/null | head -5
42-43: Add safety check for cache size calculation.If the cache directory doesn't exist or
dufails,CACHE_SIZE_MBwill be an empty string, causing the integer comparison at Line 43 to fail with "integer expression expected" error.Apply this diff to add a safety check:
# Limit cache size (ctcache has no built-in size management) CTCACHE_MAXSIZE_MB=50 CACHE_SIZE_MB=$(du -sm "${CTCACHE_DIR}" 2>/dev/null | cut -f1) +# Default to 0 if du fails or returns empty +CACHE_SIZE_MB=${CACHE_SIZE_MB:-0} if [ "${CACHE_SIZE_MB}" -gt "${CTCACHE_MAXSIZE_MB}" ]; thenLikely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ci/dash/lint-tidy.sh (1)
20-25: Consider usingcommand -vfor consistency.Line 23 uses
which clang-tidywhile line 15 usescommand -v clang-tidy. For consistency and modern shell best practices, consider usingcommand -vhere as well.Apply this diff to improve consistency:
-CTCACHE_CLANG_TIDY=$(which clang-tidy) +CTCACHE_CLANG_TIDY=$(command -v clang-tidy)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between e176501 and f9d5ccce4b84ec1d206a664ce0471bf73269cbfb.
📒 Files selected for processing (1)
ci/dash/lint-tidy.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{guix-build*,releases,**/guix-build*,releases/**,.github/**,depends/**,ci/**,contrib/**,doc/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not make changes to build system files (guix-build*), release artifacts, or avoid changes to .github, depends, ci, contrib, and doc directories unless specifically prompted
Files:
ci/dash/lint-tidy.sh
🔇 Additional comments (4)
ci/dash/lint-tidy.sh (4)
14-18: LGTM!The clang-tidy availability check is correctly implemented using
command -vwith proper error handling.
27-38: LGTM!The ctcache path definitions and validation checks are properly implemented with appropriate error handling.
40-41: LGTM!Appropriate use of
|| trueto prevent stats initialization failures from blocking the build.
45-45: LGTM!The clang-tidy invocation correctly uses the ctcache binary while preserving existing flags and parallel execution.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ci/dash/lint-tidy.sh (1)
64-67: Optional: File-count-based cleanup may not optimize cache size.The cleanup removes 20% of files by count, but cache files can vary significantly in size. If the cache contains many small files and a few large ones, removing 20% by count might not free sufficient space to bring the cache under the size limit.
Consider a size-aware cleanup strategy that removes files until the target size is reached:
# Alternative: Remove oldest files until size target is met TARGET_SIZE_MB=$((CTCACHE_MAXSIZE_MB * 80 / 100)) # Target 80% of max find "${CTCACHE_DIR}" -type f -printf '%T+ %s %p\0' | sort -z | while IFS=' ' read -r -d $'\0' timestamp size path; do if [ "${CACHE_SIZE_MB}" -le "${TARGET_SIZE_MB}" ]; then break fi rm -f "$path" 2>/dev/null || true CACHE_SIZE_MB=$(du -sm "${CTCACHE_DIR}" 2>/dev/null | cut -f1) doneHowever, the current count-based approach is simpler and may be sufficient for relatively uniform cache file sizes.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between f9d5ccce4b84ec1d206a664ce0471bf73269cbfb and e08360b.
📒 Files selected for processing (1)
ci/dash/lint-tidy.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{guix-build*,releases,**/guix-build*,releases/**,.github/**,depends/**,ci/**,contrib/**,doc/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not make changes to build system files (guix-build*), release artifacts, or avoid changes to .github, depends, ci, contrib, and doc directories unless specifically prompted
Files:
ci/dash/lint-tidy.sh
🔇 Additional comments (8)
ci/dash/lint-tidy.sh (8)
14-18: LGTM! Clear error handling for missing clang-tidy.The availability check provides a clear, early failure message if clang-tidy is not found. Using
command -vis the standard approach for checking command availability in shell scripts.
20-26: LGTM! ctcache environment setup is appropriate for CI.The hardcoded
/cache/ctcachepath is acceptable in this CI-specific context and matches the Docker container structure defined in the Dockerfile. The environment variables are correctly exported for ctcache to use.
27-28: LGTM! Binary paths match Dockerfile installation.The hardcoded paths correspond to the ctcache installation locations defined in contrib/containers/ci/ci.Dockerfile. While tightly coupled, this is appropriate for a CI-specific script.
30-38: LGTM! Proper validation of ctcache installation.The checks ensure that the ctcache components installed by the Dockerfile are present before attempting to use them, providing clear error messages for debugging if the installation is incomplete.
40-41: LGTM! Appropriate handling of statistics reset.Zeroing statistics before the run ensures accurate per-run metrics. The
|| trueprevents the script from failing if stats reset fails, which is acceptable for non-critical cache statistics.
45-45: LGTM! Correct integration of ctcache wrapper.The
-clang-tidy-binaryflag correctly redirects run-clang-tidy to use the ctcache wrapper, enabling result caching while preserving all existing behavior including error handling and output redirection.
51-55: LGTM! Clear reporting of ctcache statistics.The statistics reporting provides useful visibility into cache performance. Using
|| truefor error suppression is appropriate here since cache statistics are non-critical to the linting process.
69-72: LGTM! Appropriate final reporting of cache state.The final cache size display provides useful feedback on the effectiveness of the cleanup operations.
Adds ctcache to cache clang-tidy analysis results, significantly reducing CI time for subsequent runs. Unchanged files serve results from cache while only modified files get re-analyzed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Move CTCACHE_DIR and CTCACHE_MAXSIZE_MB to ci/test/00_setup_env.sh - Use command -v instead of hardcoded paths for clang-tidy and clang-tidy-cache - Store clang-tidy location in CLANG_TIDY_BIN variable for reuse - Extract cache cleanup logic into cleanup_ctcache() function - Convert CTCACHE_COMMIT to ARG variable in Dockerfile 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ci/dash/lint-tidy.sh (2)
14-35: Consider refining the cache cleanup strategy.The current cleanup logic removes files older than 7 days, then removes the oldest 20% if still over the limit. This approach has some limitations:
- Size-agnostic removal: Removes 20% of files by count, not by size. A few large, rarely-used entries might be more suitable for eviction than many small, frequently-used entries.
- No post-cleanup verification: Doesn't verify that cleanup actually brought the cache under the limit.
- Aggressive eviction: Removing 20% at once might evict recently useful entries, reducing hit rates.
Consider a more granular approach: remove files in smaller batches (e.g., 5-10% at a time) until under the limit, or prioritize removal by size-to-age ratio.
🔎 Example: Iterative cleanup until under limit
cleanup_ctcache() { local cache_dir=$1 local max_size_mb=$2 local cache_size_mb cache_size_mb=$(du -sm "${cache_dir}" 2>/dev/null | cut -f1) if [ "${cache_size_mb}" -gt "${max_size_mb}" ]; then echo "Cache size ${cache_size_mb}MB exceeds limit ${max_size_mb}MB, cleaning old entries..." # Remove files older than 7 days, or if still too large, oldest 20% of files find "${cache_dir}" -type f -mtime +7 -delete 2>/dev/null || true cache_size_mb=$(du -sm "${cache_dir}" 2>/dev/null | cut -f1) - if [ "${cache_size_mb}" -gt "${max_size_mb}" ]; then + while [ "${cache_size_mb}" -gt "${max_size_mb}" ]; do local file_count remove_count file_count=$(find "${cache_dir}" -type f | wc -l) - remove_count=$((file_count / 5)) + remove_count=$((file_count / 10)) # Remove 10% at a time instead of 20% + if [ "${remove_count}" -eq 0 ]; then + echo "Warning: Cannot reduce cache size further" + break + fi find "${cache_dir}" -type f -printf '%T+ %p\0' | sort -z | head -z -n "${remove_count}" | cut -z -d' ' -f2- | xargs -0 rm -f 2>/dev/null || true echo "Attempted to remove ${remove_count} oldest cache entries" - fi + cache_size_mb=$(du -sm "${cache_dir}" 2>/dev/null | cut -f1) + done echo "Cache size after cleanup: $(du -sh "${cache_dir}" 2>/dev/null | cut -f1)" fi }
73-80: Consider logging suppressed errors for debugging.The statistics and cleanup commands suppress errors with
|| trueand2>&1 || true, which prevents CI failures but may hide real issues (e.g., permission problems, corrupted cache). Consider capturing and logging these errors for debugging while still allowing the script to continue.🔎 Example: Log errors while continuing
echo "=== ctcache statistics ===" -du -sh "${CTCACHE_DIR}" 2>/dev/null || echo "Cache directory not found" -python3 "${CLANG_TIDY_CACHE_PY}" --show-stats 2>&1 || true +if ! du -sh "${CTCACHE_DIR}" 2>&1; then + echo "Warning: Failed to check cache directory size" +fi +if ! python3 "${CLANG_TIDY_CACHE_PY}" --show-stats 2>&1; then + echo "Warning: Failed to retrieve cache statistics" +fi
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build-src.yml(1 hunks)ci/dash/lint-tidy.sh(1 hunks)ci/test/00_setup_env.sh(1 hunks)contrib/containers/ci/ci.Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contrib/containers/ci/ci.Dockerfile
- .github/workflows/build-src.yml
🧰 Additional context used
📓 Path-based instructions (1)
{guix-build*,releases,**/guix-build*,releases/**,.github/**,depends/**,ci/**,contrib/**,doc/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not make changes to build system files (guix-build*), release artifacts, or avoid changes to .github, depends, ci, contrib, and doc directories unless specifically prompted
Files:
ci/test/00_setup_env.shci/dash/lint-tidy.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: Lint / Run linters
🔇 Additional comments (3)
ci/dash/lint-tidy.sh (2)
37-42: LGTM!The clang-tidy verification correctly uses
command -v, stores the result in a variable for reuse (addressing previous feedback), and provides clear error handling.
67-67: LGTM!The clang-tidy invocation correctly uses the ctcache wrapper via
-clang-tidy-binary="${CLANG_TIDY_CACHE}"while preserving the quiet mode and parallelism settings.ci/test/00_setup_env.sh (1)
64-66: The review comment references PR objectives and specific cache hit metrics that cannot be verified in the codebase or git history. Additionally, the underlying assumption about cache size requirements is contradicted by ctcache documentation, which states that storage needs are minimal since ctcache only stores hashes by default. The codebase already implements automatic cache cleanup logic (removing files older than 7 days, or oldest 20% if size exceeds limit), indicating the maintainers are aware of size constraints.Likely an incorrect or invalid review comment.
Instead of hardcoding the Python script path, derive it from the clang-tidy-cache wrapper location using the same logic the wrapper itself uses. This improves portability if ctcache is installed in a different location. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ci/dash/lint-tidy.sh (1)
20-21: Consider defensive handling for empty cache size.If
du -smfails (e.g., directory doesn't exist or is inaccessible),cache_size_mbwill be an empty string. Line 21's integer comparison will then fail with "integer expression expected" and cause the script to exit due toset -eo pipefail.While the directory is created on line 47 before
cleanup_ctcacheis called, adding a default value improves robustness.🔎 Optional defensive fix
- cache_size_mb=$(du -sm "${cache_dir}" 2>/dev/null | cut -f1) + cache_size_mb=$(du -sm "${cache_dir}" 2>/dev/null | cut -f1) + cache_size_mb=${cache_size_mb:-0} if [ "${cache_size_mb}" -gt "${max_size_mb}" ]; thenThis ensures
cache_size_mbdefaults to 0 if empty, preventing comparison errors.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ci/dash/lint-tidy.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{guix-build*,releases,**/guix-build*,releases/**,.github/**,depends/**,ci/**,contrib/**,doc/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not make changes to build system files (guix-build*), release artifacts, or avoid changes to .github, depends, ci, contrib, and doc directories unless specifically prompted
Files:
ci/dash/lint-tidy.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (3)
ci/dash/lint-tidy.sh (3)
37-62: LGTM: Robust validation and setup.The clang-tidy and ctcache verification logic is well-implemented:
- Uses
command -vfor binary discovery- Stores binary paths in variables for reuse
- Provides clear error messages when required tools are missing
- Derives the Python script path from the wrapper location (matching ctcache's own logic)
- Creates cache directory with
mkdir -pfor safetyAll past review concerns have been addressed.
64-73: LGTM: Correct ctcache integration.The stats zeroing and clang-tidy execution are properly implemented:
- Stats are zeroed before the run to isolate per-run metrics (with
|| truefor resilience)run-clang-tidycorrectly uses the ctcache wrapper via-clang-tidy-binary- Output is captured for error reporting
- Error handling shows context with
grep -C5
75-82: LGTM: Comprehensive reporting and cleanup.The statistics reporting and cache management are well-implemented:
- Clear section delimiters for CI log readability
- Defensive error handling (
|| true,|| echo) for resilience- Cache cleanup enforces size limits (ctcache lacks built-in size management, as noted)
- Uses environment variables (
CTCACHE_DIR,CTCACHE_MAXSIZE_MB) configured inci/test/00_setup_env.sh
|
Both my fixes were merged into upstream, we no longer have to patch it manually. Added 040ac27. CI: https://github.com/UdjinM6/dash/actions/runs/20409759062 |
3 participants
Issue being fixed or feature implemented
Add ctcache to cache clang-tidy analysis results, reducing time spent in "Run linters" step from 33-34 minutes to 3-4 minutes in the best case scenarios.
What was done?
contrib/containers/ci/ci.Dockerfileand patch it to work correctly.github/workflows/build-src.ymlto save/restore ctcache cache (forlinux64_multiprocess, save on pushes to default branch only)ci/dash/lint-tidy.shto configure ctcache, forcerun-clang-tidyto use itci/dash/lint-tidy.shsince ctcache doesn't have a way to set internal limits like ccache doesHow Has This Been Tested?
Run, check "Run linters" step in
linux64_multiprocess-buildjob.develop, no ctcache: 31m 43s
develop*, empty cache: 0% hits, 33m 19s
develop*, second run, using cache: 100% hits, 3m 27s
New branch*, merged #6947 (chainlock, evo): 96.5% hits, 5m 13s
New branch*, merged #7056 (chainlock, instantsend, llmq, etc): 59.3%, 19m 50s
New branch*, merged #7005 (wallet, utils): 40.9% hits, 25m 49s
* with this patch merged into develop
Breaking Changes
n/a
Checklist: