feat(taskfiles)!: Add cmake:install-deps-and-generate-settings to install all CMake-based dependencies and write their settings files.
#41
Conversation
…d write their settings files.
## Walkthrough
The changes introduce enhancements to CMake-related task automation. The `install` task now requires a `NAME` parameter and optionally accepts a `CMAKE_SETTINGS_DIR` parameter; when provided, it writes a CMake cache variable file defining `<NAME>_ROOT` in that directory. The `install-remote-tar` task is updated to default its `CMAKE_SETTINGS_DIR` and to pass both `NAME` and `CMAKE_SETTINGS_DIR` to the `install` task. A new internal task `install-deps-and-generate-settings` is added to manage CMake dependencies by recreating the settings directory, running a specified dependency installation task, and generating an aggregated CMake settings file including all `.cmake` files except itself.
## Changes
| File(s) | Change Summary |
|----------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| exports/taskfiles/utils/cmake.yaml | - `install` task: Added required `NAME` and optional `CMAKE_SETTINGS_DIR` parameters; writes `<NAME>.cmake` file with root path variable.<br>- `install-remote-tar` task: Defaults `CMAKE_SETTINGS_DIR` to `${WORK_DIR}/cmake-settings`; passes `NAME` and `CMAKE_SETTINGS_DIR` to `install`.<br>- Added new internal `install-deps-and-generate-settings` task: Recreates settings directory, runs dependency installation task (`DEP_TASK`), and generates combined CMake settings file excluding itself. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant TaskRunner
participant InstallTask
participant InstallDepsTask
User->>TaskRunner: Run install-deps-and-generate-settings (CMAKE_SETTINGS_DIR, DEP_TASK)
TaskRunner->>InstallDepsTask: Remove & recreate CMAKE_SETTINGS_DIR
InstallDepsTask->>TaskRunner: Run DEP_TASK (dependency installation)
InstallDepsTask->>InstallDepsTask: Generate combined settings.cmake including all .cmake files except itself
InstallDepsTask->>User: Done
User->>TaskRunner: Run install (NAME, CMAKE_SETTINGS_DIR)
TaskRunner->>InstallTask: install
InstallTask->>InstallTask: Write <NAME>.cmake with <NAME>_ROOT variable in CMAKE_SETTINGS_DIR
InstallTask->>User: DonePossibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
exports/taskfiles/utils/cmake.yaml (1)
242-251: 🛠️ Refactor suggestionHarden
setup-depsscript executionCurrently, if there are no
.cmakefiles, the glob will remain literal (dir/*.cmake), and errors in the dependency task won’t abort the loop. Consider enablingnullglobanderrexit:cmds: - - "rm -rf {{.CMAKE_SETTINGS_DIR}}" - - "mkdir -p {{.CMAKE_SETTINGS_DIR}}" + - "set -euo pipefail; shopt -s nullglob" + - "rm -rf {{.CMAKE_SETTINGS_DIR}}" + - "mkdir -p {{.CMAKE_SETTINGS_DIR}}" - task: "::{{.DEP_TASK}}" - >- for file in {{.CMAKE_SETTINGS_DIR}}/*.cmake; doThis ensures the script stops on failures and skips the loop when no files are present.
🧹 Nitpick comments (3)
exports/taskfiles/utils/cmake.yaml (3)
89-96: Refine documentation forinstalltask parametersThe descriptions for
NAMEand[CMAKE_SETTINGS_DIR]should align with the style of other tasks (use backticks for code elements, include punctuation). Consider reordering parameters to match therequires.varsblock and add a brief note on the conditional behaviour whenCMAKE_SETTINGS_DIRis empty.
148-152: Fixinstall-remote-tarparameter docsThe
@param [CMAKE_SETTINGS_DIR]entry is slightly misaligned and should explicitly mention its default. For consistency:# @param {string} [CMAKE_SETTINGS_DIR={{.WORK_DIR}}/cmake-settings] # Directory to write each project's CMake settings file.
219-232: Add description for optionalCMAKE_SETTINGS_FILEThe
@param {string} [CMAKE_SETTINGS_FILE]tag is declared but has no description. Please document its purpose (e.g., “Path to the combined settings file, defaults to${CMAKE_SETTINGS_DIR}/settings.cmake”).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
exports/taskfiles/utils/cmake.yaml(5 hunks)
🔇 Additional comments (2)
exports/taskfiles/utils/cmake.yaml (2)
181-187: PassingNAMEandCMAKE_SETTINGS_DIRtoinstallThis block correctly propagates the new
NAMEandCMAKE_SETTINGS_DIRvariables to theinstalltask, enabling the generation of project-specific CMake settings files as intended.
233-241: Initializesetup-depstask correctlyThe
setup-depstask is well-defined withinternal, a clearlabel, defaultingCMAKE_SETTINGS_FILE, and requiring the proper variables. This aligns with the new dependency aggregation feature.
| CMAKE_SETTINGS_DIR: >- | ||
| {{default "" .CMAKE_SETTINGS_DIR}} | ||
| EXTRA_ARGS: | ||
| ref: "default (list) .EXTRA_ARGS" | ||
| requires: | ||
| vars: ["BUILD_DIR", "INSTALL_PREFIX"] | ||
| vars: ["BUILD_DIR", "INSTALL_PREFIX", "NAME"] | ||
| cmds: |
There was a problem hiding this comment.
Requiring NAME is a breaking change
Adding NAME to the required variables for the install task will break existing invocations that didn’t supply this value. You may want to:
- Make
NAMEoptional (with a default or empty fallback). - Or require
NAMEonly whenCMAKE_SETTINGS_DIRis set (using templating logic).
davidlion
changed the title
cmake:setup-deps to install all dependencies and write their settings files.cmake:setup-deps to install all dependencies and write their settings files.
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
exports/taskfiles/utils/cmake.yaml (2)
110-111: RequiringNAMEis a breaking change.This enforces a new mandatory
NAMEparameter. Existing workflows withoutNAMEwill break. Consider makingNAMEoptional (with a default) or conditionally requiring it only whenCMAKE_SETTINGS_DIRis set.
119-126: Refactor echo block to a here‑doc.Chaining multiple
echolines in YAML can quickly become hard to maintain. A here‑doc preserves formatting and is clearer:- echo "set({{.NAME}}_ROOT - \"{{.INSTALL_PREFIX}}\" - CACHE PATH - \"Path to {{.NAME}} settings.\" - )" >> "{{.CMAKE_SETTINGS_DIR}}/{{.NAME}}.cmake" + cat <<EOF >> "{{.CMAKE_SETTINGS_DIR}}/{{.NAME}}.cmake" + set({{.NAME}}_ROOT "{{.INSTALL_PREFIX}}" CACHE PATH "Path to {{.NAME}} settings.") + EOF
🧹 Nitpick comments (5)
exports/taskfiles/utils/cmake.yaml (5)
88-90: Clarify the install task description formatting.The newly added lines explain CMAKE_SETTINGS_DIR behaviour well, but consider converting these into a bullet list under the existing paragraph to improve readability and avoid long wrapped comment lines.
97-100: Document default values in parameter docs.The
@paramentries forNAMEandCMAKE_SETTINGS_DIRdon’t show their defaults. Please update them to mention thatCMAKE_SETTINGS_DIRdefaults to empty (or a specific path when used) or point to thevarssection below.
223-238: Enhancesetup-depsdocumentation.The new task’s docstring is detailed, but:
- Change “Setup” (noun) to “set up” (verb) in the header.
- Format the 1., 2., 3. steps as Markdown list items without leading capitals on continuation lines.
- Clarify that
CMAKE_SETTINGS_FILEdefaults tosettings.cmakeunder the directory.
243-245: Consider renaming the default combined file toall.cmake.Most consumers expect a name like
all.cmaketo aggregate multiple includes, matching common conventions. You could defaultCMAKE_SETTINGS_FILEto{{.CMAKE_SETTINGS_DIR}}/all.cmake.
255-259: Increase portability of the include‑loop.Currently using Bash‑specific
[[ ]]and unquoted globs. To make this POSIX‑compliant and safer with spaces, consider:- for file in {{.CMAKE_SETTINGS_DIR}}/*.cmake; do - if [[ "$file" != "{{.CMAKE_SETTINGS_FILE}}" ]]; then - echo "include(\"$file\")" >> "{{.CMAKE_SETTINGS_FILE}}"; - fi - done + for file in "{{.CMAKE_SETTINGS_DIR}}"/*.cmake; do + [ "$file" != "{{.CMAKE_SETTINGS_FILE}}" ] && \ + printf 'include("%s")\n' "$file" >> "{{.CMAKE_SETTINGS_FILE}}" + done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
exports/taskfiles/utils/cmake.yaml(5 hunks)
🔇 Additional comments (5)
exports/taskfiles/utils/cmake.yaml (5)
105-106: Valid default for CMAKE_SETTINGS_DIR.Using an empty default for
CMAKE_SETTINGS_DIRis correct to make the feature opt‑in. It cleanly disables the settings‑file generation when unspecified.
185-186: Default for remote‑tar settings directory is appropriate.Using
{{printf "%s/cmake-settings" .WORK_DIR}}ensures all projects share a predictable default. This aligns with the install task’s option and the examples.
217-221: Pass through CMAKE_SETTINGS_DIR and NAME correctly.These lines correctly inject the settings directory and project name into the
installtask. This ensures downstream tasks will generate the proper<NAME>.cmakefiles.
248-249: Clean and recreate settings directory.The
rm -rfthenmkdir -psequence reliably resets the directory. This pattern is clear and intentional for idempotent task runs.
253-253: Qualified DEP_TASK invocation is correct.Prefixing with
::ensures the dependency task is resolved from the project root, avoiding ambiguity.
exports/taskfiles/utils/cmake.yaml
Outdated
| # included in CMAKE_SETTINGS_FILE. | ||
| # @param {string} [CMAKE_SETTINGS_FILE={{.CMAKE_SETTINGS_DIR}}/all.cmake] The file in which to | ||
| # combine each dependency's settings file. | ||
| setup-deps: |
There was a problem hiding this comment.
How about install-deps-and-generate-settings?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
exports/taskfiles/utils/cmake.yaml (2)
109-111: Requiring NAME is a breaking change
This mirrors an earlier concern: makingNAMEmandatory will break existinginstallinvocations that didn’t supply it. Consider makingNAMEoptional or only required whenCMAKE_SETTINGS_DIRis non‑empty.
119-126: 🛠️ Refactor suggestionImprove CMake settings file generation block
The current multi‑lineechoinside a folded>-block risks formatting issues and is hard to maintain. Using a literal block withcat <<EOFor a here‑doc preserves newlines and indentation more clearly. For example:- - >- - {{- if .CMAKE_SETTINGS_DIR}} - echo "set({{.NAME}}_ROOT - \"{{.INSTALL_PREFIX}}\" - CACHE PATH - \"Package root for {{.NAME}}.\" - )" >> "{{.CMAKE_SETTINGS_DIR}}/{{.NAME}}.cmake" - {{- end}} + - |- + {{- if .CMAKE_SETTINGS_DIR}} + cat <<-EOF >> "{{.CMAKE_SETTINGS_DIR}}/{{.NAME}}.cmake" + set({{.NAME}}_ROOT "{{.INSTALL_PREFIX}}" CACHE PATH "Package root for {{.NAME}}.") + EOF + {{- end}}
🧹 Nitpick comments (4)
exports/taskfiles/utils/cmake.yaml (4)
88-93: Clarify install task documentation
The new comments explainingCMAKE_SETTINGS_DIRbehaviour could be streamlined and aligned with the style of other tasks. For example, start with a concise summary sentence and use consistent Markdown‑style bullets for optional features.
97-99: Document default behaviour for CMAKE_SETTINGS_DIR
It would help users to explicitly note that omittingCMAKE_SETTINGS_DIR(defaulting to"") disables settings file generation. Consider enhancing the@paramdescription:- # @param {string} [CMAKE_SETTINGS_DIR] If set, the directory where the project's CMake settings - # file should be stored. + # @param {string} [CMAKE_SETTINGS_DIR] If set, the directory where the project's CMake settings + # file should be stored. Defaults to `""`, which skips writing any settings file.
223-238: Refine install-deps-and-generate-settings doc comments
The new task documentation could be more concise and consistent with other taskfiles. Suggestions:
- Use “Set up” instead of “Setup” for the verb in the summary.
- Format lists as Markdown bullets (
-) without extra indentation.- Keep the
@paramdefaults in sync with the code.
254-259: Enhance portability of settings file inclusion loop
Thefor file in *.cmakeloop may expand the literal pattern if no files exist. Consider guarding against missing matches and using portable syntax, for example:for file in {{.CMAKE_SETTINGS_DIR}}/*.cmake; do [ -e "$file" ] || continue if [ "$file" != "{{.CMAKE_SETTINGS_FILE}}" ]; then printf 'include("%s")\n' "$file" >> "{{.CMAKE_SETTINGS_FILE}}" fi done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
exports/taskfiles/utils/cmake.yaml(5 hunks)
🔇 Additional comments (4)
exports/taskfiles/utils/cmake.yaml (4)
105-106: Default CMAKE_SETTINGS_DIR fallback
Using{{default "" .CMAKE_SETTINGS_DIR}}cleanly disables file generation when unset. This aligns nicely with the conditional block below.
185-186: Default CMAKE_SETTINGS_DIR in install-remote-tar
Providing${WORK_DIR}/cmake-settingsas the default aligns with the overall workflow and user expectations. This change is sensible.
217-222: Pass CMAKE_SETTINGS_DIR and NAME to install task
The updatedinstall-remote-tarinvocation correctly wires bothCMAKE_SETTINGS_DIRandNAMEthrough to theinstalltask.
248-253: Clean and prepare settings directory then run DEP_TASK
Usingrm -rffollowed bymkdir -pand a fully qualified::{{.DEP_TASK}}invocation is clear and effective for resetting the environment.
davidlion
changed the title
cmake:setup-deps to install all dependencies and write their settings files.cmake:install-deps-and-generate-settings to install all CMake-based dependencies and write their settings files.
Description
Adds support for writing CMake settings files to be included by CMake projects when installing dependencies. The original source of the idea and tasks is from y-scope/ystdlib-cpp#39.
Example usage:
Checklist
breaking change.
Validation performed
Tested with local changes in log-surgeon.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Chores