build(taskfile): Add tasks to run clang-tidy and to run non-C++ linting; Move C++ linting tasks into the clp-core build GH workflows. #753
Conversation
![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)
lint-tasks.yml (1)
171-763: Comprehensivecheck-cpp-static-fulltask configuration.
This task invokes the full static analysis via the:utils:cpp-lint:clang-tidy-findcommand and comes with a very detailed configuration that includes an extensive exclusion list along with inline comments explaining the rationale (e.g., exclusion of.incfiles due to circular inclusion issues).
- Maintenance note: The long
EXCLUDE_PATTERNSlist may become unwieldy over time. Consider managing these exclusions in an external configuration file if the list grows further.- All flags, include patterns, and directory settings (such as
OUTPUT_DIRandROOT_PATHS) appear to be correctly specified.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lint-tasks.yml(3 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, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (11)
lint-tasks.yml (11)
4-4: Added clang-tidy output directory variable.
The new variableG_LINT_CLANG_TIDY_DIRis introduced to designate where clang‐tidy outputs are stored. Please ensure this variable is used consistently throughout the lint tasks and that it points to the correct build directory.
10-15: Refactoredchecktask with granular language-specific checks.
Thechecktask now sequentially runscheck-cpp-full,check-js,check-py, andcheck-yaml. This segmentation aligns with the PR objectives by splitting the C++ checks from those for other languages, thereby enhancing clarity. Please double-check that all downstream CI configurations reference the updated task names where needed.
16-20: Newcheck-cpp-difftask for incremental C++ checks.
This task chains the formatting check (check-cpp-format) with a static diff analysis (check-cpp-static-diff). Verify that the referenced tasks exist and are invoked properly for incremental checking scenarios.
21-25: Newcheck-cpp-fulltask for comprehensive C++ analysis.
By combining formatting and full static analysis (check-cpp-static-full), this task facilitates a complete verification of the C++ code. It is important to ensure that the full analysis does not adversely affect build time or CI stability.
26-30: Introduction of thecheck-no-cpptask.
This task deliberately excludes C++ linting while retaining checks for JavaScript, Python, and YAML. This design supports workflows where C++ checks might otherwise cause failures. Ensure that its usage is clearly documented for contributors and CI configurations.
32-38: Updatedfixtask to include both C++ and non-C++ fixes.
Thefixtask now triggersfix-cpp-full,fix-js,fix-py, andfix-yaml. This update consolidates the fixing routines and matches the overall task refactoring goals.
39-43: Addedfix-cpp-difftask for incremental C++ fixes.
This task runs bothfix-cpp-formatandfix-cpp-static-diffto address smaller code issues. Ensure that this incremental fix mode integrates well with your overall development workflow.
44-48: Introducedfix-cpp-fulltask for complete C++ fixes.
By combining fixes for formatting and static issues, this task enables a thorough perfection of C++ source files. Make sure that the automated fixes are tested to avoid unexpected side effects.
49-53:fix-no-cpptask to manage non-C++ fixes separately.
This new task focuses on the non-C++ parts (i.e., JavaScript, Python, YAML), which helps isolate and address language-specific issues without running unnecessary C++ fix routines.
156-170:check-cpp-static-difftask configuration for incremental static analysis.
The task is configured with proper dependencies and flags—for example, the-p '{{.G_CORE_COMPONENT_BUILD_DIR}}'flag is used to specify the build directory. Please verify that the build directory variable indeed reflects the intended compile environment.
828-839:venvtask configured to run only once.
The addition ofrun: "once"ensures that the virtual environment set up for linting is executed a single time per workflow run, which should improve overall efficiency. Confirm that this approach meets the project’s requirements in all deployment scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lint-tasks.yml (2)
4-4: New Variable for clang-tidy Output Directory
The addition ofG_LINT_CLANG_TIDY_DIR: "{{.G_BUILD_DIR}}/lint-clang-tidy"is well considered and fits neatly with the other build directory variables. Please ensure that its purpose and usage are documented in the project’s contribution guidelines.
171-764: Extensive Configuration in "check-cpp-static-full"
This task provides a comprehensive setup for running:utils:cpp-lint:clang-tidy-findon the full C++ codebase. The use of a detailed EXCLUDE_PATTERNS list—with comments explaining the rationale for excluding certain.incfiles and addressing circular include issues—is thorough. However, the exclusion list is very long; it might be beneficial to move these patterns to an external configuration file or define them as a reusable variable/anchor to improve maintainability in the long term. Also, the inline YAML comment to disable the line-length rule is practical; just be sure that it does not inadvertently suppress important format warnings elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lint-tasks.yml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: centos-stream-9-deps-image
🔇 Additional comments (10)
lint-tasks.yml (10)
11-14: Updated "check" Task to Invoke Full C++ Linting
Thechecktask now calls"check-cpp-full"(alongside the JS, Python, and YAML checks) instead of a previous variant. This change supports the transition to full C++ analysis. Confirm that this meets the desired workflow, as diff-based checks are handled separately.
16-20: New Task "check-cpp-diff" for Incremental C++ Linting
The newly addedcheck-cpp-difftask correctly combinescheck-cpp-formatandcheck-cpp-static-diff, which should help in analysing only modified files. Please verify that the underlying variables (such as the git reference for changes) are defined elsewhere, and that the command order produces the intended output.
21-25: New Task "check-cpp-full" for Comprehensive C++ Linting
This task neatly groups the full C++ linting actions by callingcheck-cpp-formatandcheck-cpp-static-full. It is important to ensure that full static analysis is performed in a timely manner, especially in CI.
26-30: New Task "check-no-cpp" Excluding C++ Checks
Thecheck-no-cpptask now isolates non-C++ checks (JS, Python, YAML). This separation aids in environments where C++ linting with clang-tidy might be undesirable.
32-38: "fix" Task Updated to Use the Full C++ Fix Variant
Thefixtask now callsfix-cpp-full(along with tasks for JS, Python, and YAML). Ensure that users are aware that C++ fixes will always apply the full set of changes, as opposed to a diff-based fix, and that this behaviour aligns with their expectations.
39-43: New Task "fix-cpp-diff" for Incremental Fixes in C++
The newly introducedfix-cpp-difftask utilises bothfix-cpp-formatandfix-cpp-static-diff. This should allow for applying fixes only to changed portions of the code. Confirm that its integration with the corresponding check task is consistent.
44-48: New Task "fix-cpp-full" for Comprehensive C++ Fixes
This task correctly pairsfix-cpp-formatwithfix-cpp-static-fullto address all C++ style and static issues. Its configuration looks consistent with the corresponding linting task.
49-54: New Task "fix-no-cpp" Excluding C++ Auto-fixes
Thefix-no-cpptask is designed to run fixes only for JS, Python, and YAML files, thereby excluding C++ fixes. This separation is logical given the diff/full split for C++ handling.
156-170: New Task "check-cpp-static-diff" for Diff-Based Static Analysis
This task calls:utils:cpp-lint:clang-tidy-diffwith the appropriate variables including FLAGS, OUTPUT_DIR, and VENV_DIR. Ensure that{{.G_CORE_COMPONENT_BUILD_DIR}}is correctly defined in the overall config, as it is crucial for clang-tidy to locate the build directory.
839-839: "venv" Task Now Runs Only Once
The addition ofrun: "once"in thevenvtask should prevent unnecessary re-creation of the virtual environment, which is a positive change for efficiency. Please verify in CI and local environments that this behaviour is as intended.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lint-tasks.yml (1)
171-765: Comprehensive Configuration forcheck-cpp-static-full
This block provides a detailed setup for performing a full clang-tidy static analysis on C++ code. The configuration includes:
- Documentation Comments: The inline notes explain why certain file types (e.g.
.incfiles) are excluded due to circular includes and false positives.- EXCLUDE_PATTERNS Array: The exclusion list is extensive to cover many specific files. While the detail is valuable, the list is quite long and might become challenging to maintain over time. Consider one of the following improvements:
- External Configuration: Move the exclusion patterns to an external file or a separate YAML anchor that can be updated independently.
- Advanced Glob Patterns: If applicable, explore using glob patterns to simplify the list and reduce redundancy.
- FLAGS and Other Variables: The
FLAGSarray includes correct references to the clang-tidy configuration and build directory, which is good practice.- Overall Maintainability: The current configuration is precise and well-documented; however, ensure that the long list of exclusions is periodically reviewed to remove files that have been updated, as indicated in the comments.
This block is functionally comprehensive but may benefit from future refactoring aimed at enhancing maintainability and reducing redundancy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lint-requirements.txt(1 hunks)lint-tasks.yml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lint-requirements.txt
⏰ 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: lint-check (macos-latest)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-14, false)
🔇 Additional comments (11)
lint-tasks.yml (11)
4-4: New Variable for Clang-Tidy Output Directory
The addition of the variableG_LINT_CLANG_TIDY_DIR(set to"{{.G_BUILD_DIR}}/lint-clang-tidy") is clear and consistent with the other build directory variables. This change should help centralise the output location for clang‐tidy results.
11-14: Update to the Global Check Task
Thechecktask now sequentially runscheck-cpp-full,check-js,check-py, andcheck-yaml. This refactoring consolidates the C++ linting into thecheck-cpp-fulltask, which seems to be the desired approach. Ensure that replacing the old C++ check withcheck-cpp-fullmeets all workflow requirements.
16-20: Introduction ofcheck-cpp-diffTask
This new task executes bothcheck-cpp-formatandcheck-cpp-static-diff. The naming is consistent and it provides a clear separation for differential static analysis. Verify that the behaviour ofcheck-cpp-diffaligns with other diff tasks in the workflow.
21-25: Introduction ofcheck-cpp-fullTask
This task combinescheck-cpp-formatandcheck-cpp-static-fullto perform a comprehensive C++ analysis. The structure is clear and mirrors the differential task but for a full scan. Confirm that downstream consumers are updated to utilise this new task.
26-31: Creation ofcheck-no-cppTask
The newcheck-no-cpptask is designed to run checks for JavaScript, Python, and YAML only, explicitly omitting C++ checks. This should be useful in contexts where C++ linting is not required (e.g. in CI when clang‐tidy is expected to fail). Ensure that all relevant documentation reflects this change.
34-34: Addition in thefixTask for C++
By addingfix-cpp-fullunder thefixtask, the refactoring now matches the split tasks for checking and fixing C++ code. This maintains consistency with the new check tasks.
39-43: Newfix-cpp-diffTask
Thefix-cpp-difftask now runs bothfix-cpp-formatandfix-cpp-static-diff. This mirrors the check counterpart and properly segments differential fixes for C++ issues.
44-48: Newfix-cpp-fullTask
Similarly, thefix-cpp-fulltask is set up to runfix-cpp-formatandfix-cpp-static-full. This ensures that a full fix is applied when needed. Double-check the integration of these commands with the overall CI workflow.
49-54: Newfix-no-cppTask
The addition of thefix-no-cpptask to handle JavaScript, Python, and YAML fixes only is straightforward and consistent with the approach used for checking. It clearly isolates non‑C++ fixes from those intended for C++.
840-840: Venv Task: Single Run Directive
The update in thevenvtask addingrun: "once"ensures that the virtual environment is created only once. This can improve efficiency by preventing redundant setup steps. Verify that this change does not interfere with scenarios that might require multiple venv initialisations.
156-170:check-cpp-static-diffTask Configuration
This task is configured to perform clang‐tidy analysis on files modified relative to a base reference. The commands and variables (e.g.FLAGS,OUTPUT_DIR, andVENV_DIR) are appropriately set. Please confirm that the git reference used (through dependency on:core-generate) and the build directory variable (G_CORE_COMPONENT_BUILD_DIR) are correctly defined elsewhere in the project.
davidlion
changed the title
| # - src/clp/ir/EncodedTextAst.cpp | ||
| # - tests/test-ffi_IrUnitHandlerInterface.cpp | ||
| # yamllint disable rule:line-length | ||
| EXCLUDE_PATTERNS: |
There was a problem hiding this comment.
Sorry, forgot to mention in the first review, but can we alphabetize this list?
|
For the PR title, how about: |
davidlion
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lint-tasks.yml (1)
171-765: Comprehensive Full Static Analysis for C++The
check-cpp-static-fulltask encompasses a complete clang-tidy run:
- Setup: It references all C++ source files (
*cpp_source_files) and depends on:core-generate,cpp-lint-configs, andvenv.- Command: Uses
:utils:cpp-lint:clang-tidy-findwith detailed variables.- Exclusion Patterns:
An extensiveEXCLUDE_PATTERNSlist is defined to skip files with known issues (e.g., circular includes in.incfiles), which is crucial given the recurring errors in these files.- Additional Variables:
The task specifiesFLAGS,INCLUDE_PATTERNS,OUTPUT_DIR,ROOT_PATHS, andVENV_DIRto tie the analysis together.Suggestions:
- Maintainability: Consider alphabetising or categorising the
EXCLUDE_PATTERNSlist. This will ease future maintenance and readability.- Ongoing Review: As files are updated and underlying issues are resolved, update the exclusion list accordingly to gradually remove blacklisted files.
Overall, this segment is integral to realizing a robust full static analysis and appears comprehensive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lint-tasks.yml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (11)
lint-tasks.yml (11)
4-4: New Variable Definition for Clang-Tidy Output DirectoryThe added variable
G_LINT_CLANG_TIDY_DIRis well-defined as"{{.G_BUILD_DIR}}/lint-clang-tidy". This clearly designates the directory for clang-tidy outputs.
11-14: Refinement of the Unified Check TaskThe
checktask now sequentially calls:
check-cpp-fullcheck-jscheck-pycheck-yamlThis modularisation aligns with the PR objectives to split and organise the linting workflows. Please verify that all upstream consumers of the old
check-cpptask have been updated accordingly.
16-20: Implementation of the Diff-Based C++ CheckThe
check-cpp-difftask now integrates:
check-cpp-formatcheck-cpp-static-diffThis setup focuses on analysing only the changed C++ files. Ensure that the diff comparison uses the intended git reference (defaulting to
origin/HEADas per standard practice).
21-25: Full C++ Check Task UpdateThe
check-cpp-fulltask now performs a complete check by sequentially running:
check-cpp-formatcheck-cpp-static-fullThis change is consistent with the objective to perform a full analysis across all C++ files. Verify that the increased scope does not adversely impact performance in CI.
26-30: Separation of Non-C++ Linting TasksThe introduction of the
check-no-cpptask cleanly segregates JavaScript, Python, and YAML linting from C++ checks. This enhances clarity and avoids unintended invocation of clang-tidy when running non-C++ linting.
32-37: Updated Fix Task CompositionThe
fixtask has been updated to call:
fix-cpp-fullfix-jsfix-pyfix-yamlThis redesign reflects the new structure of the linting workflows. It simplifies maintenance by unifying fixes across different languages.
39-43: Diff Fix Task for C++The
fix-cpp-difftask now correctly chains:
fix-cpp-formatfix-cpp-static-diffThis provides a targeted fix for only those files that have changed. The setup appears solid; please ensure that its integration with the corresponding diff-check task is verified.
44-48: Full Fix Task for C++The
fix-cpp-fulltask now executes:
fix-cpp-formatfix-cpp-static-fullThis comprehensively addresses formatting and static analysis fixes across the entire C++ codebase. Verify that this approach meets the expected remediation in your workflows.
49-53: Non-C++ Fix Task UpdateThe
fix-no-cpptask now delegates fixing to:
fix-jsfix-pyfix-yamlThis clear separation excludes C++ from non-related fixes, reducing the likelihood of cross-language side effects.
156-170: Static Diff Analysis for C++The
check-cpp-static-difftask is configured as follows:
- Sources: Uses
*cpp_source_files- Dependencies:
:core-generate,cpp-lint-configs, andvenv- Commands: Invokes
:utils:cpp-lint:clang-tidy-diffwith:
FLAGS(including--config-fileand-poptions),OUTPUT_DIRset to{{.G_LINT_CLANG_TIDY_DIR}},VENV_DIRdefined as{{.G_LINT_VENV_DIR}}.This configuration should accurately analyse only the changed files. Please double-check that the build directory specified in
-p '{{.G_CORE_COMPONENT_BUILD_DIR}}'aligns with your environment settings.
840-840: Single Execution of Virtual Environment SetupThe directive
run: "once"in thevenvtask guarantees that the environment setup is executed only a single time, preventing unnecessary redundancy in the build process.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lint-tasks.yml (1)
171-765: Comprehensive “check-cpp-static-full” Task for Full Static AnalysisThis newly added task is a major enhancement which:
- Invokes a full clang‐tidy static analysis using the
:utils:cpp-lint:clang-tidy-findtask.- Specifies a detailed list of exclusion patterns to prevent false positives from files (e.g. due to circular includes or known problematic files).
- Uses a consistent configuration with flags (pointing to
{{.ROOT_DIR}}/.clang-tidyand using the build directory) and defines inclusion patterns for source and test files.Recommendations:
- The extensive blacklist under
EXCLUDE_PATTERNSis crucial for avoiding known issues but may become unwieldy over time. Consider maintaining this list in a separate file or variable to improve readability and ease future updates.- Keep track of files removed from the exclusion list once issues are resolved.
Overall, this task is well configured to perform a robust static analysis across the entire C++ codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lint-tasks.yml(3 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-13, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (11)
lint-tasks.yml (11)
4-4: New Clang-Tidy Output Directory VariableA new variable,
G_LINT_CLANG_TIDY_DIR, is defined to designate the directory where clang‐tidy results will be stored. This clarifies the output location for static analysis results and aligns with the integration of clang‐tidy in the lint tasks.
11-14: Updated “check” Task CommandsThe
checktask now sequentially triggers the tasks:
•check-cpp-full
•check-js
•check-py
•check-yamlThis change refactors the previous C++ lint task to use the comprehensive C++ full check. The ordering appears alphabetical, which was previously suggested. Just ensure that any future additions continue to follow this convention.
16-19: Introduction of the “check-cpp-diff” TaskThe newly added
check-cpp-difftask combines both formatting and static analysis for C++ files that have changed compared to a given base reference. Please verify that the underlying tasks (check-cpp-formatandcheck-cpp-static-diff) are correctly configured to operate on diffs.
21-24: New “check-cpp-full” Task ConfigurationThe
check-cpp-fulltask invokes both C++ formatting and a full static analysis by runningcheck-cpp-formatandcheck-cpp-static-full. This appropriately encapsulates a complete C++ linting routine. Confirm that the full analysis provides all the diagnostics needed.
26-27: 'check-no-cpp' Task for Non‑C++ LintingThis task intentionally excludes C++ linting and runs only the JavaScript, Python, and YAML checks. This meets the PR’s objective to separate non‑C++ checks from the C++ linting tasks.
34-34: Updated “fix” TaskWithin the
fixtask, the command now callsfix-cpp-full(instead of a deprecated or combined C++ fix) along with the other fix tasks. This change standardises the fix workflow to mirror the check tasks split.
39-43: New “fix-cpp-diff” Task SetupThe introduction of the
fix-cpp-difftask, which runs bothfix-cpp-formatandfix-cpp-static-diff, aids in applying fixes only on changed C++ code. Be sure to test that the diff‑targeted fixes behave as expected.
44-48: Configuration for “fix-cpp-full” TaskThe
fix-cpp-fulltask now correctly sequences the C++ formatting and static full fixes by runningfix-cpp-formatfollowed byfix-cpp-static-full. This symmetry with the corresponding check task is beneficial for consistency; ensure that they remain in sync over time.
49-53: ‘fix-no-cpp’ Task for Non‑C++ FixesThe
fix-no-cpptask is designed to run fixes only for JavaScript, Python, and YAML. This separation follows the new design that omits C++ fixes from non‑C++ lint workflows.
156-170: New “check-cpp-static-diff” Task for Diff-Based Static AnalysisThis task targets only the changed C++ files and applies clang‐tidy in diff mode. The flags and the output directory (
G_LINT_CLANG_TIDY_DIR) are specified, and the build directory is passed using the-pflag. It is recommended to verify that the diff mode analysis produces the desired diagnostic output.
840-840: Optimisation in the “venv” TaskThe addition of
run: "once"ensures that the virtual environment setup happens only once per execution. This is a useful optimisation that should reduce unnecessary repeated initialisation.
2 participants
Description
This PR adds support for running clang-tidy through taskfiles and adds it to CI.
For now, clang-tidy will be fully run on all files using an exclude list (in
lint-task.yaml) to skip files with known issues. Once existing issues are fixed we will switch to running clang-tidy only on files changed in a PR to save time. A nightly full run will be used to pick up other errors.To support clang-tidy we've added/changed some tasks. Going from low-level to high-level:
check-cpp-static-diff: Run clang-tidy on files changed compared to a git reference (defaults toorigin/HEAD)check-cpp-static-full: Run clang-tidy against all files in CLP using a blacklist to avoid files with known errors.check-cpp-diff: split ofcheck-cppthat callscheck-cpp-static-diffwithcheck-cpp-formatcheck-cpp-full: split ofcheck-cppthat callscheck-cpp-static-fullwithcheck-cpp-formatcheck-no-cpp: split ofcheckthat calls all linting without any C++check: now callscheck-cpp-diffwith all other lintingCI has been changed such that
clp-lint.yamlcallscheck-no-cppas clang-tidy must be run in an environment that can build clp. Therefore, theclp-core-build.yamlandclp-core-build-macos.yamlhave been updated to also runcheck-cpp-full.Checklist
breaking change.
Validation performed
Tested tasks locally and CI through fork repo.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Chores
clang-tidy.python3-pipandpython3-venv) for enhanced functionality.