feat(lint): Cache necessary files for lint:check-cpp-static-full to skip unnecessary re-runs.#815
Conversation
…kip unnecessary re-runs.
WalkthroughThe changes update two GitHub Actions workflow files to introduce caching for the Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as GitHub Action Runner
participant Restore as Restore Cache Step
participant Update as Update Cache Step
Runner->>Restore: Check if event is not scheduled
Restore-->>Runner: Return restored cache (OS-specific key)
Runner->>Update: Check if branch is 'main' and not a pull request
Update-->>Runner: Save updated cache using key from restore step
Possibly related PRs
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/clp-core-build-macos.yaml (1)
92-105: Efficient Cache Restoration for clang-tidy Results
The added step to restore the lint cache is implemented correctly. The conditional check ('schedule' != github.event_name) ensures that on scheduled events the cache restoration is skipped—this is in line with the intent to update cache only on specific events. Additionally, using a per-OS cache key (main-branch-${{matrix.os}}-lint:check-cpp-tidy-static-full) is a smart approach to separate caches for different operating systems.Suggestion: Consider adding an inline comment explaining why scheduled events are excluded from the cache restore to help future maintainers understand this decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/clp-core-build-macos.yaml(1 hunks).github/workflows/clp-core-build.yaml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build (macos-latest)
🔇 Additional comments (3)
.github/workflows/clp-core-build-macos.yaml (1)
112-123: Robust Cache Update Implementation
The update step correctly uses a conditional ('pull_request' != github.event_name && 'refs/heads/main' == github.ref) to ensure that the cache is only updated after successful runs on the main branch. This assures that only valid and current outputs (logs and checksums) are saved, reducing redundant lint re-runs. The use of the output from the previous restoration step (${{steps.cache-restore-lint-check-cpp-static-full.outputs.cache-primary-key}}) maintains consistency in cache key usage.Make sure that the logged outputs in the specified paths (checksums and
build/lint-clang-tidy) are always generated by the lint task to avoid stale cache issues..github/workflows/clp-core-build.yaml (2)
330-343: Proper Cache Restoration for Ubuntu Lint Job
The restoration step for theubuntu-jammy-lintjob mirrors the macOS implementation well. Using the condition ('schedule' != github.event_name) and a unique per-OS cache key (main-branch-ubuntu-jammy-lint:check-cpp-tidy-static-full) ensures that different OS environments maintain independent caches. This strategy will help skip unnecessary clang-tidy checks when no relevant changes have occurred.Ensure that excluding scheduled events from this step aligns with your broader testing strategy, as scheduled runs might require a full lint pass.
358-369: Consistent Cache Update Mechanism
The update step is set up to save the computed lint results only on the main branch and not during pull requests, which is critical for maintaining cache integrity. Using the cache key output from the restore step (${{steps.cache-restore-lint-check-cpp-static-full.outputs.cache-primary-key}}) is an effective way to tie both steps together.No further changes are needed here; this implementation should effectively reduce redundant lint executions and improve workflow speed.
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
.github/workflows/clp-core-build.yaml:344
- [nitpick] Consider renaming the cache key to reflect 'static' as used in other parts of the task (e.g., 'lint:check-cpp-static-full') to ensure consistency across the configuration.
key: "main-branch-ubuntu-jammy-lint:check-cpp-tidy-static-full"
.github/workflows/clp-core-build-macos.yaml:106
- [nitpick] Consider aligning the cache key naming with the rest of the workflow by using 'lint:check-cpp-static-full' instead of 'lint:check-cpp-tidy-static-full' to avoid potential confusion.
key: "main-branch-${{matrix.os}}-lint:check-cpp-tidy-static-full"
…kip unnecessary re-runs. (y-scope#815)
…kip unnecessary re-runs. (y-scope#815)
Description
The lint:check-cpp-static-full task in the clp-core-build and clp-core-build-macos workflows currently takes ~10m to run. As we fix clang-tidy violations in more files, this time will likely increase steadily. However, since a pull request might only change a few C++ source files, the time spent re-checking the other source files is wasted.
This PR uses the actions/cache action to cache the lint:check-cpp-static-full task from the main branch, so that when run in a pull request, it will skip re-running clang-tidy on the files which haven't changed. This could miss issues in files that haven't changed but that depend on the files which have changed, but those will still be caught by the nightly scheduled run which doesn't use the cache.
Note that we cache both:
The cache is only updated when it's run successfully on main, so the generated files should all contain a success message.
Overall, this reduces the time for the lint:check-cpp-static-full task in the workflows to less than a minute when the C++ files haven't been changed or only small changes have been made.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Summary by CodeRabbit