fix(cmake): Split CMake settings generation from library install tasks to support multi-project dependency builds. #88
Conversation
WalkthroughUpdates Boost taskfile paths to capitalized “Boost” directories and removes CMAKE_SETTINGS_DIR usage. Introduces misc helpers to reset dirs/files. Adds CMake tasks to generate and aggregate settings, reworks install flows to delegate settings generation via install-deps-and-generate-settings and generate-cmake-settings, and standardizes run: when_changed on relevant tasks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant CI as CI/Invoker
participant CMake as utils/cmake:install-deps-and-generate-settings
participant MiscD as utils/misc:reset-dir
participant MiscF as utils/misc:reset-file
participant Dep as DEP_TASK (per dependency)
participant Gen as utils/cmake:generate-cmake-settings
participant FS as Settings FS
Dev->>CI: Run install-deps-and-generate-settings<br/>(CMAKE_SETTINGS_DIR, CMAKE_SETTINGS_FILE)
CI->>CMake: Invoke with DEP_TASK, PACKAGE_INSTALL_PREFIX, etc.
CMake->>MiscD: Reset CMAKE_SETTINGS_DIR
MiscD-->>CMake: Dir ready
CMake->>MiscF: Reset CMAKE_SETTINGS_FILE
MiscF-->>CMake: File ready
loop For each dependency
CMake->>Dep: Run with CMAKE_SETTINGS_DIR + CMAKE_SETTINGS_FILE
Dep->>Gen: generate-cmake-settings(CMAKE_PACKAGE_NAME, DIR, PREFIX)
Gen->>FS: Append CMAKE_PACKAGE_NAME_ROOT entry
end
sequenceDiagram
autonumber
participant Inv as Task Runner
participant Boost as utils/boost tasks
participant DL as download-and-extract-tar
participant Build as build-and-install
Inv->>Boost: download-and-install (no CMAKE_SETTINGS_DIR)
Boost->>DL: download-and-extract-tar
Note right of DL: run: when_changed<br/>tracks up-to-date by labels
DL-->>Boost: Sources ready
Boost->>Build: build-and-install (uses Boost-* dirs)
Build-->>Inv: Installed to Boost-install
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
Bill-hbrhbr
changed the title
Bill-hbrhbr
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
exports/taskfiles/utils/cmake.yaml (2)
35-44: Restore the conditional emission of--parallel
buildnow always passes--parallel ""when.JOBSis unset. CMake treats the empty string as an invalid jobs count and aborts. Please gate the flag so it is only emitted when a numeric value is provided.- --parallel "{{.JOBS}}" + {{- if .JOBS}} + --parallel "{{.JOBS}}" + {{- end}}
257-262: Skip the include loop when no package settings existIf
DEP_TASKproduces no*.cmakefiles (for example, a dependency only installs artefacts), the glob remains literal and you end up appendinginclude(".../*.cmake"), which later causes CMake to fail. Guard the loop withcompgen(or enablenullglob) so you only emit includes for real files.- - |- - for file in {{.CMAKE_SETTINGS_DIR}}/*.cmake; do - if [[ "$file" != "{{.CMAKE_SETTINGS_FILE}}" ]]; then - echo "include(\"$file\")" >> "{{.CMAKE_SETTINGS_FILE}}"; - fi - done + - |- + if compgen -G "{{.CMAKE_SETTINGS_DIR}}/"'*.cmake' > /dev/null; then + for file in {{.CMAKE_SETTINGS_DIR}}/*.cmake; do + if [[ "$file" != "{{.CMAKE_SETTINGS_FILE}}" ]]; then + echo "include(\"$file\")" >> "{{.CMAKE_SETTINGS_FILE}}"; + fi + done + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
exports/taskfiles/utils/boost.yaml(3 hunks)exports/taskfiles/utils/cmake.yaml(7 hunks)exports/taskfiles/utils/misc.yaml(1 hunks)
| reset-dir: | ||
| desc: "Removes the given directory and create it empty. Runs once per directory path." | ||
| internal: true | ||
| label: "{{.TASK}}:{{.DIR_PATH}}" | ||
| requires: | ||
| vars: ["DIR_PATH"] | ||
| run: "when_changed" | ||
| cmds: | ||
| - "rm -rf {{.DIR_PATH}}" | ||
| - "mkdir -p {{.DIR_PATH}}" | ||
|
|
||
| reset-file: | ||
| desc: "Removes the given file. Runs once per file path." | ||
| internal: true | ||
| label: "{{.TASK}}:{{.FILE_PATH}}" | ||
| requires: | ||
| vars: ["FILE_PATH"] | ||
| run: "when_changed" | ||
| cmds: | ||
| - "rm -rf {{.FILE_PATH}}" | ||
| - "mkdir -p $(dirname -- {{.FILE_PATH}})" |
There was a problem hiding this comment.
Quote the reset commands to avoid clobbering the wrong paths
Both reset-dir and reset-file call rm -rf/mkdir -p without quoting the expanded variables. If the caller passes a path containing spaces or glob tokens, the shell will split the argument and you could delete or recreate unintended locations. Please add the usual quoting around the templated paths and around the dirname call.
- - "rm -rf {{.DIR_PATH}}"
- - "mkdir -p {{.DIR_PATH}}"
+ - 'rm -rf "{{.DIR_PATH}}"'
+ - 'mkdir -p "{{.DIR_PATH}}"'
...
- - "rm -rf {{.FILE_PATH}}"
- - "mkdir -p $(dirname -- {{.FILE_PATH}})"
+ - 'rm -rf "{{.FILE_PATH}}"'
+ - 'mkdir -p "$(dirname -- "{{.FILE_PATH}}")"'Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In exports/taskfiles/utils/misc.yaml around lines 45 to 65 the rm -rf and mkdir
-p commands use unquoted templated paths which allows word-splitting and glob
expansion (dangerous for paths with spaces or glob chars); fix by quoting the
templated path variables in both commands (e.g. quote {{.DIR_PATH}} and
{{.FILE_PATH}}) and also quote the dirname expression used for the file case,
and include the standard "--" sentinel to dirname to safely handle names
beginning with "-" so the shell receives a single, quoted path argument.
There was a problem hiding this comment.
@Bill-hbrhbr This makes sense to me. Could you fix this by applying what create-venv is doing?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| ref: ".CMAKE_INSTALL_ARGS" | ||
| INSTALL_PREFIX: "{{.INSTALL_PREFIX}}" | ||
|
|
||
| # Create a settings file in CMAKE_SETTINGS_DIR, containing a `{{.CMAKE_PACKAGE_NAME}}_ROOT` CMake |
There was a problem hiding this comment.
| # Create a settings file in CMAKE_SETTINGS_DIR, containing a `{{.CMAKE_PACKAGE_NAME}}_ROOT` CMake | |
| # Creates a settings file in `CMAKE_SETTINGS_DIR`, containing a `{{.CMAKE_PACKAGE_NAME}}_ROOT` CMake |
| # - The task must not require any arguments (to use a task with arguments create a new task that | ||
| # calls the original with any arguments set). |
There was a problem hiding this comment.
Removing this is a little misleading. We should say that the task must take two arguments CMAKE_SETTINGS_DIRS and CMAKE_SETTINGS_FILE, and cannot have other required arguments.
| # - The task name must be qualified from the root of the project. | ||
| # - The task must not require any arguments (to use a task with arguments create a new task that | ||
| # calls the original with any arguments set). | ||
| # - The task takes in CMAKE_SETTINGS_DIR and CMAKE_SETTINGS_FILE as optional arguments. |
There was a problem hiding this comment.
From previous part of the docstring and the task's requires, it looks like CMAKE_SETTINGS_DIR is not optional.
| reset-dir: | ||
| desc: "Removes the given directory and create it empty. Runs once per directory path." | ||
| internal: true | ||
| label: "{{.TASK}}:{{.DIR_PATH}}" | ||
| requires: | ||
| vars: ["DIR_PATH"] | ||
| run: "when_changed" | ||
| cmds: | ||
| - "rm -rf {{.DIR_PATH}}" | ||
| - "mkdir -p {{.DIR_PATH}}" | ||
|
|
||
| reset-file: | ||
| desc: "Removes the given file. Runs once per file path." | ||
| internal: true | ||
| label: "{{.TASK}}:{{.FILE_PATH}}" | ||
| requires: | ||
| vars: ["FILE_PATH"] | ||
| run: "when_changed" | ||
| cmds: | ||
| - "rm -rf {{.FILE_PATH}}" | ||
| - "mkdir -p $(dirname -- {{.FILE_PATH}})" |
There was a problem hiding this comment.
@Bill-hbrhbr This makes sense to me. Could you fix this by applying what create-venv is doing?
Description
When multiple projects in the same repository (e.g.,
coreandspider) share dependencies, they may each need to generate their own CMake settings files while installing shared libraries only once.Currently, the
install-deps-and-generate-settingstask couples installation and CMake settings generation. This means:CMAKE_SETTINGS_DIRconcurrently.This PR decouples CMake settings generation from the installation process:
clp,spider) to decide how to bundle installation and settings generation.Checklist
breaking change.
Validation performed
Works with the example usage case here: y-scope/clp#1384
Summary by CodeRabbit