refactor(taskfiles)!: Require NAME to be set if CMAKE_SETTINGS_DIR is set in utils:cmake:install (fixes #50).
#57
Conversation
…s:cmake` by requiring explicit definition and checking it with a precondition.
|
""" WalkthroughThe changes unify the parameter naming by replacing Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InstallTask
User->>InstallTask: Invoke install with parameters
alt CMAKE_SETTINGS_DIR is set
InstallTask->>InstallTask: Check CMAKE_PACKAGE_NAME is non-empty (precondition)
InstallTask-->>User: Error if CMAKE_PACKAGE_NAME is empty
else CMAKE_SETTINGS_DIR is not set
InstallTask->>InstallTask: Proceed without CMAKE_PACKAGE_NAME
end
InstallTask-->>User: Complete installation
Possibly related issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (9)
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. 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 (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
exports/taskfiles/utils/cmake.yaml(3 hunks)
🔇 Additional comments (2)
exports/taskfiles/utils/cmake.yaml (2)
110-110: Removal ofNAMEfrom required vars aligns with new optional behaviour.The prerequisite variables for
installno longer includeNAME, matching the conditional requirement introduced below. This change looks correct.
111-119: Conditional precondition enforcesNAMEwhenCMAKE_SETTINGS_DIRis set.The
preconditionsblock correctly checks that.NAMEis non-empty when.CMAKE_SETTINGS_DIRis provided. Consider trimming whitespace or validating the variable further if needed, but the current check is sound.
davidlion
changed the title
CMAKE_SETTINGS_DIR behaviour in utils:cmake by requiring explicit definition and checking it with a precondition (fixes #50).NAME to be set if CMAKE_SETTINGS_DIR is set in utils:cmake:install (fixes #50).
| # @param {string} [CMAKE_SETTINGS_DIR] If set, the directory where the project's CMake settings | ||
| # file should be stored. | ||
| # @param {string[]} [EXTRA_ARGS] Any additional arguments to pass to the install command. | ||
| # @param {string} [CMAKE_PACKAGE_NAME] CMake project name (used in directory names and the CMake |
There was a problem hiding this comment.
Do we want to re-alphabetize this now that the var name has changed?
| # @param {string} [CMAKE_SETTINGS_DIR] If set, the directory where the project's CMake settings | ||
| # file should be stored. | ||
| # @param {string[]} [EXTRA_ARGS] Any additional arguments to pass to the install command. | ||
| # @param {string} [CMAKE_PACKAGE_NAME] CMake project name (used in directory names and the CMake | ||
| # settings file). Required if `CMAKE_SETTINGS_DIR` is set. |
There was a problem hiding this comment.
| # settings file). Required if `CMAKE_SETTINGS_DIR` is set. | |
| # settings file). Required if `CMAKE_SETTINGS_DIR` is set. |
| @@ -201,7 +210,7 @@ tasks: | |||
| EXTRA_ARGS: | |||
| ref: ".CMAKE_INSTALL_ARGS" | |||
| INSTALL_PREFIX: "{{.INSTALL_PREFIX}}" | |||
| NAME: "{{.NAME}}" | |||
| CMAKE_PACKAGE_NAME: "{{.CMAKE_PACKAGE_NAME}}" | |||
Description
As title says.
Additional minor changes:
NAMEis renamed toCMAKE_PACKAGE_NAMEto avoid issues with WSL where theNAMEenvironment variable is set by default.CMAKE_SETTINGS_DIRno longer has a default value inutils:cmake:install-remote-tarto ensure it is set explicitly and intentionally.Checklist
breaking change.
Validation performed
Tested locally with log-surgeon.
Summary by CodeRabbit
Documentation
NAMEtoCMAKE_PACKAGE_NAMEand clarify its requirement whenCMAKE_SETTINGS_DIRis set.New Features
CMAKE_PACKAGE_NAMEonly whenCMAKE_SETTINGS_DIRis specified.Chores