feat(lint): Add tasks for linting CMake scripts with gersemi; Apply gersemi to CMake scripts. #147
Conversation
…ples files excluded.
WalkthroughThis update introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant TaskRunner
participant LintVenv
participant Gersemi
Developer->>TaskRunner: Run lint check or fix
TaskRunner->>TaskRunner: Invoke check-cmake/fix-cmake subtask
TaskRunner->>LintVenv: Activate lint virtual environment
TaskRunner->>Gersemi: Run gersemi on CMake files (with flags)
Gersemi-->>TaskRunner: Report lint results or apply fixes
TaskRunner-->>Developer: Output lint status
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
davidlion
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.gersemirc(1 hunks)CMakeLists.txt(3 hunks)examples/CMakeLists.txt(2 hunks)lint-requirements.txt(1 hunks)taskfiles/lint.yaml(5 hunks)tests/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2024-11-13T22:25:54.168Z
Learnt from: SharafMohamed
PR: y-scope/log-surgeon#48
File: tests/test-tag.cpp:10-10
Timestamp: 2024-11-13T22:25:54.168Z
Learning: In the log-surgeon codebase (C++), particularly in the finite automata components involving the `Tag` class (`src/log_surgeon/finite_automata/Tag.hpp`), it's important to ensure that `Tag*` pointers in other objects cannot be `nullptr`. Test cases should focus on validating that these `Tag*` pointers are not null where they are used, and handle `nullptr` appropriately.
Applied to files:
tests/CMakeLists.txt
📚 Learning: 2025-07-22T02:00:47.620Z
Learnt from: davidlion
PR: y-scope/log-surgeon#142
File: taskfiles/examples.yaml:63-76
Timestamp: 2025-07-22T02:00:47.620Z
Learning: CMake's find_package() command automatically honors <PackageName>_ROOT variables (e.g., LOG_SURGEON_ROOT) during its search process. The CMakeLists.txt file does not need to explicitly reference these _ROOT variables - they are built into CMake's package discovery mechanism.
Applied to files:
.gersemircCMakeLists.txt
📚 Learning: 2024-11-27T21:56:13.425Z
Learnt from: SharafMohamed
PR: y-scope/log-surgeon#56
File: src/log_surgeon/finite_automata/RegisterHandler.hpp:0-0
Timestamp: 2024-11-27T21:56:13.425Z
Learning: In the `log_surgeon` project, header guards in C++ header files should include `_HPP` at the end to match the filename. For example, in `RegisterHandler.hpp`, the header guard should be `LOG_SURGEON_FINITE_AUTOMATA_REGISTER_HANDLER_HPP`.
Applied to files:
CMakeLists.txtexamples/CMakeLists.txt
📚 Learning: 2024-10-10T18:05:34.072Z
Learnt from: SharafMohamed
PR: y-scope/log-surgeon#42
File: src/log_surgeon/finite_automata/RegexAST.hpp:106-112
Timestamp: 2024-10-10T18:05:34.072Z
Learning: In the C++ codebase for `log_surgeon`, the convention is to use `false == condition` instead of `!condition` when checking conditions, for clarity.
Applied to files:
CMakeLists.txt
📚 Learning: 2025-05-01T14:47:57.016Z
Learnt from: davidlion
PR: y-scope/log-surgeon#106
File: src/log_surgeon/Lexer.hpp:114-114
Timestamp: 2025-05-01T14:47:57.016Z
Learning: When handling error cases in log-surgeon, prefer using the `Result<T, ErrorCode>` type from ystdlib-cpp (https://github.com/y-scope/ystdlib-cpp/blob/main/src/ystdlib/error_handling/Result.hpp) instead of `std::pair<ErrorCode, T>` for better type safety and clearer semantics.
Applied to files:
CMakeLists.txt
📚 Learning: 2025-05-05T14:55:34.455Z
Learnt from: SharafMohamed
PR: y-scope/log-surgeon#106
File: src/log_surgeon/Lalr1Parser.tpp:661-665
Timestamp: 2025-05-05T14:55:34.455Z
Learning: The log-surgeon codebase follows a design approach where function contracts (like `ErrorCode::Success` guaranteeing a valid token) are trusted, and contract violations are allowed to throw exceptions rather than being explicitly checked at every call site.
Applied to files:
CMakeLists.txt
⏰ 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). (4)
- GitHub Check: build (ubuntu-24.04, release)
- GitHub Check: build (ubuntu-22.04, debug)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: lint (ubuntu-24.04)
🔇 Additional comments (4)
tests/CMakeLists.txt (1)
2-20: Formatting only – looks goodNo functional changes detected; the multiline style matches the rest of the PR.
.gersemirc (1)
1-9: Validate paths & redundant lint directives
definitionspoints tobuild/deps/Catch2-extracted/extras/Catch.cmake, which is generated after a configure step and may not exist in a clean checkout. This will causegersemito error when developers runtask lint:check-cmakebefore generating thebuild/tree. Consider pointing to the source-tree copy ofCatch.cmakeor using a glob that tolerates absence.Line 1 disables yamllint’s line-length rule, yet
line_length: 100is set below. The first directive is now redundant and can be removed to let yamllint enforce the same limit as gersemi.taskfiles/lint.yaml (2)
4-6:deps.yamlcorrectly includedAdding the internal
depsTaskfile resolves the previously-flagged dependency gap and allowsgersemito reuse the existingdeps:install-alllogic.
49-50: New CMake check wired into the mainchecktask
check-cmakeis now invoked alongside the existing C++ and YAML checks, addressing the earlier review note that it should be part of the default lint flow.
| target_compile_options( | ||
| log_surgeon | ||
| PRIVATE | ||
| $<$<CXX_COMPILER_ID:MSVC>:/W4 | ||
| /WX> | ||
| $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wall | ||
| -Wextra | ||
| -Wpedantic | ||
| -Werror> | ||
| ) |
There was a problem hiding this comment.
Same generator-expression split here – will break the top-level build
The target_compile_options(log_surgeon …) block contains the identical line-break issue as flagged in examples/CMakeLists.txt. Merge the arguments so each $<…> expression stays contiguous:
- $<$<CXX_COMPILER_ID:MSVC>:/W4
- /WX>
- $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wall
- -Wextra
- -Wpedantic
- -Werror>
+ $<$<CXX_COMPILER_ID:MSVC>:/W4 /WX>
+ $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wall -Wextra -Wpedantic -Werror>Without this fix, cmake .. fails with “Error evaluating generator expression”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| target_compile_options( | |
| log_surgeon | |
| PRIVATE | |
| $<$<CXX_COMPILER_ID:MSVC>:/W4 | |
| /WX> | |
| $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wall | |
| -Wextra | |
| -Wpedantic | |
| -Werror> | |
| ) | |
| target_compile_options( | |
| log_surgeon | |
| PRIVATE | |
| $<$<CXX_COMPILER_ID:MSVC>:/W4 /WX> | |
| $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wall -Wextra -Wpedantic -Werror> | |
| ) |
🤖 Prompt for AI Agents
In CMakeLists.txt lines 143 to 152, the target_compile_options call splits
generator expressions across multiple lines, causing CMake to fail with an
"Error evaluating generator expression." Fix this by merging each generator
expression into a single contiguous line without line breaks inside the $<...>
expressions, ensuring the entire expression is continuous and properly parsed by
CMake.
| target_compile_options( | ||
| ${target} | ||
| PRIVATE | ||
| $<$<CXX_COMPILER_ID:MSVC>:/W4 | ||
| /WX> | ||
| $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wall | ||
| -Wextra | ||
| -Wpedantic | ||
| -Werror> | ||
| ) |
There was a problem hiding this comment.
Broken generator expression – will fail CMake configure
target_compile_options splits each generator expression across two arguments:
$<$<CXX_COMPILER_ID:MSVC>:/W4
/WX>
CMake treats a generator expression as a single, unbroken argument; splitting it inserts an unintended line break that terminates the expression early and produces a “syntax error in generator expression” at configure time.
- target_compile_options(
- ${target}
- PRIVATE
- $<$<CXX_COMPILER_ID:MSVC>:/W4
- /WX>
- $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wall
- -Wextra
- -Wpedantic
- -Werror>
- )
+ target_compile_options(
+ ${target}
+ PRIVATE
+ $<$<CXX_COMPILER_ID:MSVC>:/W4 /WX>
+ $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wall -Wextra -Wpedantic -Werror>
+ )The same pattern recurs in the root CMakeLists.txt; please fix there as well.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| target_compile_options( | |
| ${target} | |
| PRIVATE | |
| $<$<CXX_COMPILER_ID:MSVC>:/W4 | |
| /WX> | |
| $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wall | |
| -Wextra | |
| -Wpedantic | |
| -Werror> | |
| ) | |
| target_compile_options( | |
| ${target} | |
| PRIVATE | |
| $<$<CXX_COMPILER_ID:MSVC>:/W4 /WX> | |
| $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wall -Wextra -Wpedantic -Werror> | |
| ) |
🤖 Prompt for AI Agents
In examples/CMakeLists.txt around lines 30 to 39, the generator expressions in
target_compile_options are split across multiple lines, causing CMake to treat
them as separate arguments and resulting in a syntax error. To fix this, rewrite
each generator expression as a single continuous string without line breaks,
ensuring the entire expression is passed as one argument. Also, apply the same
fix to the root CMakeLists.txt where this pattern occurs.
2 participants
Description
Depends on #125.
This PR adds the tool, gersemi, to link CMake files through tasks and in CI.
To pass CI it applies the refactoring done by gersemi.
Checklist
breaking change.
Validation performed
gersemi passing in CI and locally. Existing CI still passing.
Summary by CodeRabbit
New Features
Chores
Style