feat(taskfiles): Add cmake-install-remote-tar; Fix optional lists to properly default to an empty list.#22
Conversation
|
Warning Rate limit exceeded@davidlion has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request modifies the Changes
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 (1)
taskfiles/utils-cmake.yaml (1)
125-164: Consider adding URL validation and cleanup.Two potential improvements:
- Add URL format validation before downloading
- Consider adding an optional cleanup step to remove temporary directories after installation
Example implementation for URL validation:
cmake-install-remote-tar: internal: true label: "{{.TASK}}:{{.NAME}}-{{.URL}}-{{.INSTALL_PREFIX}}" + preconditions: + - sh: >- + [[ "{{.URL}}" =~ ^https?:// ]] || (echo "URL must start with http:// or https://" && exit 1) + - msg: "Invalid URL format" vars: BUILD_DIR: >- {{default (printf "%s/%s-build" .WORK_DIR .NAME) .BUILD_DIR}}Would you like me to propose a complete implementation for the cleanup functionality as well?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
taskfiles/utils-cmake.yaml(5 hunks)
🔇 Additional comments (2)
taskfiles/utils-cmake.yaml (2)
19-23: LGTM! Consistent handling of optional list variables.The changes standardize how optional list variables are handled across all CMake tasks, ensuring they properly default to an empty list when not provided. The implementation is consistent and well-structured.
Also applies to: 51-52, 65-67, 89-91
103-124: LGTM! Well-documented and secure implementation.The task is thoroughly documented and implements proper security measures by requiring SHA256 verification. The directory structure is logical, and the task effectively reuses existing functionality.
Bill-hbrhbr
left a comment
There was a problem hiding this comment.
Quick scan through of the code.
| WORK_DIR: >- | ||
| {{default .TASK_DIR .WORK_DIR}} | ||
| requires: | ||
| vars: ["NAME", "FILE_SHA256", "URL", "WORK_DIR"] |
There was a problem hiding this comment.
I think WORK_DIR is not required since it has a default value based on the task description
There was a problem hiding this comment.
Right, I missed this when I made it optional.
| BUILD_DIR: >- | ||
| {{default (printf "%s/%s-build" .WORK_DIR .NAME) .BUILD_DIR}} | ||
| INSTALL_PREFIX: >- | ||
| {{default (printf "%s/%s-install" .WORK_DIR .NAME) .INSTALL_PREFIX}} | ||
| SOURCE_DIR: >- | ||
| {{default (printf "%s/%s-src" .WORK_DIR .NAME) .SOURCE_DIR}} | ||
| WORK_DIR: >- | ||
| {{default .TASK_DIR .WORK_DIR}} |
There was a problem hiding this comment.
Not sure if the order matters here when WORK_DIR is not specified and defaulted to TASK_DIR. Taskfile may treat WORK_DIR as an empty string when trying to assign default values to BUILD_DIR, INSTALL_PREFIX and SOURCE_DIR.
Taskfile is very weird in these situations.
There was a problem hiding this comment.
Good point. Let me check.
| # project. We purposely omit `sources` and `generates` as we defer to `cmake` to decide whether it | ||
| # should perform any actions. However, the download and extraction will be skipped if unnecessary. | ||
| # | ||
| # @param {string} NAME Project name used for directory names. |
There was a problem hiding this comment.
nit: if you change this to DEP or DEP_NAME it will keep the required args alphabetical with DEP* being the first arg.
But this is really unnecessary
There was a problem hiding this comment.
Technically, the cmake project doesn't need to be a dependency so I think it is better to leave it as NAME.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
taskfiles/utils-cmake.yaml (1)
108-182: Consider adding validation and cleanupThe task implementation looks good, but consider these improvements:
- Add validation for the FILE_SHA256 format to prevent invalid hashes
- Consider adding an optional cleanup flag to remove temporary directories after successful installation
Would you like me to propose an implementation for these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
taskfiles/utils-cmake.yaml(5 hunks)
🔇 Additional comments (4)
taskfiles/utils-cmake.yaml (4)
19-25: LGTM! Improved variable handling in cmake-build taskThe addition of explicit variable defaults enhances reliability by ensuring proper initialization of optional parameters.
47-49: LGTM! Simplified variable handling in cmake-clean taskThe changes maintain consistency with other tasks while reducing redundancy in variable references.
Also applies to: 57-57
70-72: LGTM! Enhanced variable handling in cmake-generate taskThe addition of explicit variable defaults aligns with the improvements made to other tasks.
94-96: LGTM! Improved variable handling in cmake-install taskThe changes maintain consistency with the variable handling improvements across all tasks.
| WORK_DIR: >- | ||
| {{default .ROOT_DIR .WORK_DIR}} |
There was a problem hiding this comment.
Fix WORK_DIR default value
The default value uses .ROOT_DIR but the documentation states it should default to .TASK_DIR.
Apply this diff to fix the default value:
- {{default .ROOT_DIR .WORK_DIR}}
+ {{default .TASK_DIR .WORK_DIR}}📝 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.
| WORK_DIR: >- | |
| {{default .ROOT_DIR .WORK_DIR}} | |
| WORK_DIR: >- | |
| {{default .TASK_DIR .WORK_DIR}} |
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
taskfiles/utils-cmake.yaml (1)
134-135:⚠️ Potential issueFix WORK_DIR default value.
The default value uses
.ROOT_DIRbut the documentation states it should default to.TASK_DIR.Apply this diff to fix the default value:
- {{default .ROOT_DIR .WORK_DIR}} + {{default .TASK_DIR .WORK_DIR}}
🧹 Nitpick comments (1)
taskfiles/utils-cmake.yaml (1)
130-130: Consider making this task public.This task appears to be a utility for users to install CMake projects from remote tarballs. Consider removing the
internal: trueflag to make it accessible as a public utility.Apply this diff:
- internal: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
taskfiles/utils-cmake.yaml(5 hunks)
🔇 Additional comments (2)
taskfiles/utils-cmake.yaml (2)
19-25: LGTM! Improved handling of optional list arguments.The changes enhance reliability by explicitly defaulting optional list arguments and using consistent reference syntax across all tasks.
Also applies to: 47-49, 70-72, 94-96
108-182: LGTM! Well-structured implementation.The new task is well-designed with:
- Clear separation of paths and optional arguments
- Proper reuse of existing tasks
- Thorough documentation
Description
This PR adds a task that downloads a tar file containing a CMake project and then generates, builds, and installs the project. This task is useful when handling dependencies in C++ projects.
The original idea/design is from @Bill-hbrhbr.
Additionally, it adds small fixes to ensure optional list variables default to an empty list. This protects against the case where an optional list ends up defaulting to
nilinstead of[]by accident, which leads to an error from task/go.Here is some pseudo code to help explain the issue:
Checklist
breaking change.
Validation performed
The new task was tested in ffi-go, but the list handling changes were also verified to cause no issues in clp.
Summary by CodeRabbit
Summary by CodeRabbit