chore(taskfiles)!: Split up cmake-config-and-build; Add cmake-clean.#20
Conversation
WalkthroughThe changes update the CMake task definitions in the configuration file. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant T as Task Runner
U->>T: Execute cmake-generate
U->>T: Execute cmake-build (requires prior cmake-generate)
U->>T: Execute cmake-install (requires prior cmake-build)
alt Performing Clean Operation
U->>T: Execute cmake-clean (requires prior cmake-generate, leverages cmake-build)
end
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (2)
taskfiles/utils-cmake.yaml (2)
18-18: Consider simplifying the task label.The label includes all parameters which might show empty values when optional parameters aren't provided. Consider omitting optional parameters from the label or using conditional templating.
- label: "{{.TASK}}:{{.BUILD_DIR}}-{{.TARGETS}}-{{.EXTRA_ARGS}}" + label: "{{.TASK}}:{{.BUILD_DIR}}{{if .TARGETS}}-{{.TARGETS}}{{end}}{{if .EXTRA_ARGS}}-{{.EXTRA_ARGS}}{{end}}"
71-73: Enhance prerequisite documentation.While the documentation mentions the requirement for
cmake-build, it could be more explicit about whether a successful build is required or if just running the build step is sufficient.- # Runs the CMake install step for the given build directory. The caller must have previously - # called `cmake-build` on `BUILD_DIR` for this task to succeed. We purposely omit `sources` and - # `generates` as we defer to `cmake` to decide whether it should perform any actions. + # Runs the CMake install step for the given build directory. The caller must have previously + # successfully completed the `cmake-build` task on `BUILD_DIR` for this task to succeed. + # We purposely omit `sources` and `generates` as we defer to `cmake` to decide whether it + # should perform any actions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
taskfiles/utils-cmake.yaml(1 hunks)
🔇 Additional comments (2)
taskfiles/utils-cmake.yaml (2)
17-17: Verify if this task should be internal.Since this is a key task in the CMake workflow (as indicated in the PR objectives), consider whether marking it as
internal: truemight make it harder for users to discover and use directly.
47-47: Verify the ref syntax for default value.The syntax
ref: "default (list) .EXTRA_ARGS"seems unusual. Please verify if this is the correct way to reference default values in Task.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
taskfiles/utils-cmake.yaml (3)
18-18: Consider improving the label format to handle missing optional parameters.The current label format
{{.TASK}}:{{.BUILD_DIR}}-{{.TARGETS}}-{{.EXTRA_ARGS}}might produce awkward labels whenTARGETSorEXTRA_ARGSare not provided (e.g., resulting in double hyphens).Consider using conditional formatting:
- label: "{{.TASK}}:{{.BUILD_DIR}}-{{.TARGETS}}-{{.EXTRA_ARGS}}" + label: >- + {{.TASK}}:{{.BUILD_DIR}} + {{- if .TARGETS}}-{{.TARGETS}}{{end}} + {{- if .EXTRA_ARGS}}-{{.EXTRA_ARGS}}{{end}}
47-47: Clarify the reference syntax for EXTRA_ARGS.The syntax
ref: "default (list) .EXTRA_ARGS"might be unclear to users unfamiliar with the task runner's reference system.Consider adding a comment explaining the reference syntax or using a more explicit form:
- ref: "default (list) .EXTRA_ARGS" + # Forward any EXTRA_ARGS provided to this task to cmake-build + ref: "default (list) .EXTRA_ARGS"
71-90: Consider adding install prefix validation.While the implementation is correct, it might be helpful to verify that the install prefix is absolute and exists before proceeding with the installation.
Consider adding a validation step:
cmds: + - test -d "{{.INSTALL_PREFIX}}" || mkdir -p "{{.INSTALL_PREFIX}}" - >- cmake
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
taskfiles/utils-cmake.yaml(1 hunks)
🔇 Additional comments (1)
taskfiles/utils-cmake.yaml (1)
50-69: Well-structured implementation with clear documentation.The task effectively handles the new EXTRA_ARGS parameter while maintaining clear documentation about the purposeful omission of
sourcesandgenerates.
|
Changes lgtm. My only concern is that, although not time-consuming, CMake build and install will dump a lot of useless info when source files have not been changed. |
Description
This PR breaks up
cmake-config-and-buildintocmake-generateandcmake-build. Additionally, it addscmake-cleanand improves the docstrings for each task.The notable decisions in our design are:
sourcesorgeneratesand defer decision making tocmaketo ensure expected and reliable behaviour.Checklist
breaking change.
Validation performed
Tested with various tasks in
clp.Summary by CodeRabbit
New Features
cmake-cleancommand to enhance cleaning operations.EXTRA_ARGSandTARGETSfor more flexible build configurations.Chores