feat(taskfiles)!: Split utils taskfile by category; Rename extension from yml to yaml.#18
Conversation
WalkthroughThis pull request introduces several new YAML configuration files and tasks to facilitate various automation operations. New tasks include text replacement and virtual environment creation, checksum computation and validation, remote file downloading with extraction, CMake project configuration/build and installation, as well as C++ linting using clang-format and clang-tidy. Additionally, a main utility configuration file is provided to reference and flatten these individual taskfiles for streamlined management. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant VT as Create-Venv Task
participant FS as Filesystem
participant SHELL as Shell Environment
U->>VT: Initiate virtual environment creation
VT->>FS: Remove existing virtual environment directory (if exists)
VT->>SHELL: Create new Python virtual environment
VT->>FS: Modify activation script (remove calls to hash)
VT->>SHELL: Activate virtual environment
VT->>SHELL: Install Python packages from requirements file
sequenceDiagram
participant U as User
participant DET as Download-&-Extract Task
participant CURL as Curl Task
participant VC as Validate-Checksum Task
participant TAR as Tar Extractor
U->>DET: Start download & extraction process
DET->>CURL: Request file download with retry logic
CURL-->>DET: Return downloaded file (or error if failed)
DET->>VC: Validate downloaded file checksum
VC-->>DET: Return validation result
DET->>TAR: Extract tarball contents with defined patterns
Possibly related PRs
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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-cpp.yaml (1)
49-76: CMake Configuration and Build Task:
This task is well-structured, with variable defaults defined for optional parameters (CONF_ARGSandJOBS) and a descriptive label. One point to consider is the handling of theJOBSparameter: if it is left empty, the--parallelflag might still be passed with an empty string, which could lead to unexpected behaviour in CMake. You might refactor this section to conditionally include the--parallelflag only whenJOBSis provided.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
taskfiles/utils-cpp.yaml(1 hunks)taskfiles/utils-misc.yaml(1 hunks)taskfiles/utils.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- taskfiles/utils.yaml
🔇 Additional comments (7)
taskfiles/utils-cpp.yaml (4)
1-5: Initial Configuration and Shell Options:
The version declaration, shell options (setandshopt), and overall initial configuration are clear and succinct. This foundation helps ensure that the subsequent shell commands execute reliably.
11-27: Clang-Format Task Definition:
The task for clang-format is well-documented with inline comments and clear parameter definitions. The templating for iterating over the source paths is correctly applied, and the command chain (activating the virtual environment, usingfindwith proper file matching, piping toclang-formatwith specified flags and error handling) is structured appropriately.
28-44: Clang-Tidy Task Definition:
The clang-tidy task mirrors the clang-format task effectively, with clear documentation of parameters and a similar command structure. The activation of the virtual environment and file selection logic appears sound. It would be beneficial to double-check that the virtual environment contains the correct tools and that the provided flags work as intended.
77-90: CMake Install Task:
The CMake install task is defined cleanly, with required parameters clearly stated and a straightforward command to install the project from the specified build directory. Everything appears in order with this segment.taskfiles/utils-misc.yaml (3)
205-223: Clarify and Document the Reordering for STRING UTILSThe "STRING UTILS" section (which contains the
replace-texttask) has been relocated within the file. Although no content was altered, this section now appears later in the file due to the new alphabetical ordering. To avoid confusion for future maintainers and reviewers, please consider adding a comment that documents the intent behind this reordering and confirms that the change does not affect task execution order.
224-249: Verify the Reordering of VENV UTILS TasksSimilarly, the "VENV UTILS" section (including the
create-venvtask) has been moved as part of the alphabetical reorganisation. The functionality remains unchanged, but the diff may be confusing without explicit clarification. Please ensure that documentation or commit messages clearly note that this reordering is intentional and that no dependent processes rely on a specific order.
1-249: Overall Verification of Taskfile ReorganisationThe file now presents task categories sorted alphabetically (i.e. "CHECKSUM UTILS," "REMOTE UTILS," "STRING UTILS," and "VENV UTILS"). While this arrangement should improve maintainability, please double-check that no implicit execution dependencies rely on the previous ordering. Updating documentation to note this intentional design change will help prevent future confusion.
kirkrodrigues
left a comment
There was a problem hiding this comment.
Not mandatory, but do you want to split checksum-utils and remote-utils into their own files?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
taskfiles/utils-misc.yaml (1)
7-21: Review of 'replace-text' task:
The task is well-documented and clearly explains the cross-platform nuances of usingsed(e.g. the differences in-isyntax between Linux and macOS). One suggestion is to consider adding error checking after thesedcommand so that if the temporary file isn’t created successfully, the task can fail gracefully.taskfiles/utils-checksum.yaml (2)
11-47: Review of 'compute-checksum' task:
The task makes good use oftarwith fixed owner and timestamp settings to ensure a consistent checksum, which is important for reproducibility. While the use ofignore_errorhelps prevent cascading failures, consider logging errors fromtarto aid in debugging if the checksum computation doesn’t behave as expected.
49-81: Review of 'validate-checksum' task:
This task effectively reuses thecompute-checksumfunction to validate file integrity. The inline shell block is concise; however, ensure that the loop iterating over.INCLUDE_PATTERNSadequately quotes paths—especially if they contain spaces—to avoid potential false negatives. It might also be helpful to provide more verbose output when the checksum validation fails.taskfiles/utils-remote.yaml (2)
27-50: Review of 'curl' command block:
The download loop is robust, featuring a clear maximum attempt limit and a pause between retries. Adding more detailed logging (such as printing the current attempt number) could be useful for diagnosing download issues during execution.
51-121: Review of 'download-and-extract-tar' task:
This comprehensive task successfully combines downloading, extraction, and checksum validation. The use of default parameters for file paths and the delegation to dependent tasks promotes modularity. Ensure that the include and exclude patterns fortarare correctly quoted to handle special characters or spaces, and consider emitting a message upon successful extraction for enhanced clarity during task execution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
taskfiles/utils-checksum.yaml(1 hunks)taskfiles/utils-cpp.yaml(1 hunks)taskfiles/utils-misc.yaml(1 hunks)taskfiles/utils-remote.yaml(1 hunks)taskfiles/utils.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- taskfiles/utils.yaml
- taskfiles/utils-cpp.yaml
🔇 Additional comments (2)
taskfiles/utils-misc.yaml (1)
22-43: Review of 'create-venv' task:
This task efficiently automates the creation of a Python virtual environment and smartly reuses thereplace-textsubtask to modify the activation script for compatibility with thegoshshell. It would be beneficial to verify that the activation script exists before attempting to modify it. Additionally, adding some logging or error handling in case therm -rforvenvcreation commands fail could improve maintainability.taskfiles/utils-remote.yaml (1)
12-26: Review of 'curl' task:
Thecurltask is thoughtfully constructed, including a retry mechanism and checksum verification using process substitution. One point to verify is that process substitution (using<(...)) is supported reliably across all target shell environments, as some shells may not support it.
| cmake | ||
| --install "{{.BUILD_DIR}}" | ||
| --prefix "{{.INSTALL_PREFIX}}" | ||
|
|
| # downloaded tar file. | ||
| # @param {string[]} [EXCLUDE_PATTERNS] Path wildcard patterns that should not be extracted. | ||
| # @param {string[]} [INCLUDE_PATTERNS] Path wildcard patterns to extract. | ||
| # @param {int} [NUM_COMPONENTS_TO_STRIP=1] Number of leading path components to strip from the |
There was a problem hiding this comment.
specifying NUM_COMPONENTS_TO_STRIP=0 always result in 1 being specified because sprig's default treats 0 as an empty value
https://masterminds.github.io/sprig/defaults.html#default
The definition of “empty” depends on type:
Numeric: 0
a mitigation is to write NUM_COMPONENTS_TO_STRIP="0" in the caller instead.
we should either:
- change the default value as 0
- check if the user explicitly specifies the var; only if the var is not specified, we default to 1.
- document the behaviour and mitigation / expected usage explicitly
@coderabbitai create an issue to track
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track the NUM_COMPONENTS_TO_STRIP parameter issue
gh issue create \
--repo y-scope/yscope-dev-utils \
--title "Fix NUM_COMPONENTS_TO_STRIP default behavior when 0 is specified" \
--body "## Issue Description
In \`taskfiles/utils-remote.yaml\`, the \`download-and-extract-tar\` task has a parameter \`NUM_COMPONENTS_TO_STRIP\` with a default value of 1. However, due to sprig's \`default\` function treating numeric 0 as an empty value, explicitly setting \`NUM_COMPONENTS_TO_STRIP=0\` results in 1 being used instead.
Reference: https://masterminds.github.io/sprig/defaults.html#default
## Workaround
Users can specify \`NUM_COMPONENTS_TO_STRIP=\"0\"\` (as a string) in the caller to work around this issue.
## Potential Solutions
1. Change the default value to 0
2. Check if the user explicitly specifies the var; only if the var is not specified, default to 1
3. Document the behavior and mitigation/expected usage explicitly
## Context
- **PR**: https://github.com/y-scope/yscope-dev-utils/pull/18
- **Comment**: https://github.com/y-scope/yscope-dev-utils/pull/18#discussion_r2531515156
- **Reported by**: @junhaoliao" \
--assignee junhaoliao
Length of output: 53
@junhaoliao, excellent catch on the sprig default function behavior! I've created issue #96 to track this.
The issue documents the problem, the workaround you mentioned (using NUM_COMPONENTS_TO_STRIP="0" as a string), and the three potential solutions you outlined. This should help ensure it gets addressed in a future update.
🧠 Learnings used
Learnt from: davidlion
Repo: y-scope/yscope-dev-utils PR: 16
File: taskfiles/utils.yml:296-299
Timestamp: 2024-11-15T02:34:27.126Z
Learning: In `taskfiles/utils.yml`, tasks are configured to fail if any command fails, so adding explicit error handling within commands is unnecessary.
Learnt from: Bill-hbrhbr
Repo: y-scope/yscope-dev-utils PR: 91
File: exports/taskfiles/utils/misc.yaml:70-90
Timestamp: 2025-10-13T19:09:44.404Z
Learning: In the yscope-dev-utils repository Taskfiles, prefer minimal guardrails that only protect against benevolent mistakes (e.g., enforcing absolute paths where required) rather than defensive checks like rejecting root paths, empty paths, or validating file vs directory types.
Description
Previously, all taskfile utility tasks were in a single file,
utils.yml. This PR breaks up that file and changes the extension to.yamlto follow guidelines. Additionally,utils.yamlprovides a "meta" taskfile that includes all categorized files. Includingutils.yamlwill behave asutils.ymldid previously.There are no functional changes to any of the actual content. There is one small alphabetization fix for the arguments in a
compute-checksumcall.Validation performed
Tested manually with
clp-ffi-gotasks.Summary by CodeRabbit
Summary by CodeRabbit