Taskefile: Add cmake and remote utils; update checksum data path to an array. #16
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Learnings (1)taskfiles/utils.yml (1)🔇 Additional comments (6)taskfiles/utils.yml (6)
The addition of
The change from single directory to multiple wildcard patterns provides more flexibility while maintaining backwards compatibility. Also applies to: 21-21, 32-37
The separation of configuration/build from installation is a good practice. The use of parallel builds and proper CMake commands is excellent. Also applies to: 186-199
The task properly handles:
Fix path existence check for wildcard patterns. The current implementation using Apply this diff to fix the path existence check: - ( {{- range .DATA_PATTERNS}}
- for path in {{.}}; do
- test -e "$path"
- done
- {{- end}}
+ ( {{- range .DATA_PATTERNS}}
+ if ! compgen -G "{{.}}" > /dev/null; then
+ exit 1
+ fi
+ {{- end}}
Add security headers to curl command. While the retry mechanism is good, the curl command should include security headers to protect against MITM attacks. Apply this diff to add security headers: if curl \
--fail \
--location \
--show-error \
--connect-timeout 10 \
--max-time 300 \
+ --proto '=https' \
+ --tlsv1.2 \
+ --ciphers HIGH \
"{{.URL}}" \
--output "{{.OUTPUT_FILE}}";
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
taskfiles/utils.yml (4)
Line range hint
16-18: Update required vars to match new parameter nameThe task still requires "DATA_DIR" but the parameter has been renamed to "DATA_PATHS". This inconsistency could cause runtime errors.
Apply this diff:
requires: vars: - - "DATA_DIR" + - "DATA_PATHS" - "OUTPUT_FILE"
Line range hint
49-51: Update required vars to match new parameter nameSimilar to the compute-checksum task, the required vars need to be updated here as well.
Apply this diff:
requires: vars: - - "CHECKSUM_FILE" - - "DATA_DIR" + - "CHECKSUM_FILE" + - "DATA_PATHS"
154-195: Consider exposing CMAKE_BUILD_TYPE as a parameterThe cmake-build task is well-implemented, but it might be beneficial to expose CMAKE_BUILD_TYPE as an optional parameter for better control over the build configuration.
Consider adding this parameter to the documentation and implementation:
# @param {string}[optional] CMAKE_ARGS Any additional arguments to pass to cmake configure. + # @param {string}[optional] CMAKE_BUILD_TYPE Build type (Debug, Release, etc.). Defaults to Release. cmake-build: label: "cmake-build: {{.SOURCE_DIR}} {{.BUILD_DIR}}" internal: true vars: CHECKSUM_FILE: '{{default (printf "%s.md5" .BUILD_DIR) .CHECKSUM_FILE}}' CMAKE_ARGS: "{{default nil .CMAKE_ARGS}}" + CMAKE_BUILD_TYPE: "{{default "Release" .CMAKE_BUILD_TYPE}}"Then update the cmake command:
cmake -S "{{.SOURCE_DIR}}" -B "{{.BUILD_DIR}}" + -DCMAKE_BUILD_TYPE="{{.CMAKE_BUILD_TYPE}}" {{.CMAKE_ARGS}}
241-265: Consider adding retry mechanism for curlNetwork operations can be flaky. Adding a retry mechanism would improve reliability.
Consider updating the curl command:
curl: # ... existing configuration ... cmds: - |- mkdir -p "{{dir .OUTPUT_FILE}}" - curl -L "{{.URL}}" -o "{{.OUTPUT_FILE}}" + curl --retry 3 --retry-delay 5 -L "{{.URL}}" -o "{{.OUTPUT_FILE}}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
taskfiles/utils.yml(4 hunks)
🔇 Additional comments (3)
taskfiles/utils.yml (3)
29-32: LGTM: Elegant implementation of multiple paths handling
The range-based iteration over DATA_PATHS is a clean and efficient way to handle multiple directory paths.
196-236: LGTM: Well-structured cmake-install task
The implementation includes:
- Clear parameter documentation
- Proper checksum validation
- Flexible DATA_PATHS override capability
- Clean installation prefix handling
266-304: LGTM: Robust implementation of fetch-src task
The implementation includes:
- Proper dependency on curl task
- Clean cleanup of output directory
- Flexible strip components handling
- Proper checksum validation
Co-authored-by: Junhao Liao <[email protected]>
Co-authored-by: Junhao Liao <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
taskfiles/utils.yml (3)
Line range hint
16-17: Update requires section to match new parameter nameThe task still requires "DATA_DIR" but the parameter has been changed to "DATA_PATHS".
Apply this diff:
requires: - vars: ["DATA_DIR", "OUTPUT_FILE"] + vars: ["DATA_PATHS", "OUTPUT_FILE"]
179-189: Add error handling for CMake commandsThe CMake commands should handle potential failures gracefully.
Consider adding error checking:
cmds: - >- + set -e cmake -S "{{.SOURCE_DIR}}" -B "{{.BUILD_DIR}}" {{.CMAKE_ARGS}} - >- + set -e cmake --build "{{.BUILD_DIR}}" --parallel
296-299: Add error handling for tar extractionThe tar extraction should verify successful completion.
Consider adding error checking:
- |- + set -e rm -rf "{{.OUTPUT_DIR}}" mkdir -p "{{.OUTPUT_DIR}}" tar -x --strip-components="{{.STRIP}}" -C "{{.OUTPUT_DIR}}" -f "{{.TAR_FILE}}" + # Verify directory is not empty after extraction + if [ -z "$(ls -A {{.OUTPUT_DIR}})" ]; then + echo "Error: Extraction resulted in empty directory" + exit 1 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
taskfiles/utils.yml(4 hunks)
🔇 Additional comments (1)
taskfiles/utils.yml (1)
50-51:
Update requires section to match new parameter name
Similar to the compute-checksum task, the requires section needs to be updated.
Apply this diff:
requires:
- vars: ["CHECKSUM_FILE", "DATA_DIR"]
+ vars: ["CHECKSUM_FILE", "DATA_PATHS"]Likely invalid or redundant comment.
There was a problem hiding this comment.
Some quick high-level comments before a detailed review:
- The Rabbit has some suggestions that I think are valid.
- Can
fetch-srcreplace CLP's download-and-extract-tar? If so, can we renamefetch-srctodownload-and-extract-tar? - What docstring format are you basing things on?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
taskfiles/utils.yml (2)
154-195: Consider adding BUILD_TYPE parameter for CMake configurationThe cmake-build task could benefit from a BUILD_TYPE parameter to explicitly set Release/Debug modes.
vars: CHECKSUM_FILE: '{{default (printf "%s.md5" .BUILD_DIR) .CHECKSUM_FILE}}' CMAKE_ARGS: "{{default "" .CMAKE_ARGS}}" + BUILD_TYPE: "{{default "Release" .BUILD_TYPE}}" requires: vars: ["BUILD_DIR", "SOURCE_DIR"] # ... later in cmds: - >- cmake -S "{{.SOURCE_DIR}}" -B "{{.BUILD_DIR}}" + -DCMAKE_BUILD_TYPE="{{.BUILD_TYPE}}" {{.CMAKE_ARGS}}🧰 Tools
🪛 yamllint
[error] 165-165: syntax error: expected , but found ''
(syntax)
196-236: Consider adding COMPONENT parameter for selective installationThe cmake-install task could support selective component installation for more granular control.
vars: CHECKSUM_FILE: '{{default (printf "%s.md5" .INSTALL_PREFIX) .CHECKSUM_FILE}}' + COMPONENT: "{{default "" .COMPONENT}}" # ... later in cmds: - >- cmake --install "{{.BUILD_DIR}}" --prefix "{{.INSTALL_PREFIX}}" + {{if .COMPONENT}}--component "{{.COMPONENT}}"{{end}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
taskfiles/utils.yml(4 hunks)
🧰 Additional context used
🪛 yamllint
taskfiles/utils.yml
[error] 165-165: syntax error: expected , but found ''
(syntax)
🔇 Additional comments (3)
taskfiles/utils.yml (3)
29-32: LGTM: Proper iteration over multiple paths
The implementation correctly handles multiple paths using the range construct.
262-264: Add timeout and retry mechanism for curl
This issue was previously identified. The curl command should implement timeouts and retries for better reliability.
8-11: Consider adding validation for empty DATA_PATHS array
The parameter description should clarify whether empty arrays are allowed and how they're handled.
…xtract-tar needs to handle filtering the tar ball.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
taskfiles/utils.yml (3)
13-13: Update task description to reflect multiple pathsThe description still mentions "given directory" but the task now supports multiple paths.
- desc: "Tries to compute a checksum for the given directory and output it to a file." + desc: "Tries to compute a checksum for the given paths and output it to a file."
154-197: Consider adding build type and generator configurationThe cmake-build task implementation is solid, but could benefit from additional configuration options.
Consider adding these common CMake configuration options:
vars: CHECKSUM_FILE: >- {{default (printf "%s.md5" .BUILD_DIR) .CHECKSUM_FILE}} CMAKE_ARGS: >- {{default "" .CMAKE_ARGS}} + BUILD_TYPE: >- + {{default "Release" .BUILD_TYPE}} + GENERATOR: >- + {{default "Ninja" .GENERATOR}}Then update the cmake command:
cmake -S "{{.SOURCE_DIR}}" -B "{{.BUILD_DIR}}" +-G "{{.GENERATOR}}" +-DCMAKE_BUILD_TYPE="{{.BUILD_TYPE}}" {{.CMAKE_ARGS}}
259-263: Improve SHA256 validation reliabilityThe current status check might fail if the file doesn't exist. Consider making it more resilient.
- - >- - diff - <(echo "{{.URL_SHA256}}") - <(openssl dgst -sha256 "{{.OUTPUT_FILE}}" - | awk '{print $2}') + - |- + if [ -f "{{.OUTPUT_FILE}}" ]; then + diff <(echo "{{.URL_SHA256}}") \ + <(openssl dgst -sha256 "{{.OUTPUT_FILE}}" | awk '{print $2}') + else + exit 1 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
taskfiles/utils.yml(7 hunks)
🧰 Additional context used
📓 Learnings (1)
taskfiles/utils.yml (1)
Learnt from: davidlion
PR: y-scope/yscope-dev-utils#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.
🔇 Additional comments (2)
taskfiles/utils.yml (2)
29-31: LGTM! Proper handling of multiple paths
The implementation correctly iterates over multiple paths while maintaining the existing tar functionality.
270-271: LGTM! Robust curl implementation
The curl command includes all necessary flags for reliable downloads:
--fail: Fail on HTTP errors--location: Follow redirects--show-error: Show error messages--connect-timeoutand--max-time: Prevent hanging
New commits have added in Rabbit's suggestions, updated |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
taskfiles/utils.yml (2)
154-197: Consider adding common CMake configuration optionsThe cmake-build task could benefit from additional configuration options that are commonly used:
- Build type (Debug/Release)
- Compiler selection
- Platform-specific options
Consider adding these options through additional parameters:
# @param {string} SOURCE_DIR Project source directory containing the CMakeLists.txt file. # @param {string={{.BUILD_DIR}}.md5} [CHECKSUM_FILE] Path to store the checksum of built files. # @param {string=""} [CMAKE_ARGS] Any additional arguments to pass to cmake configure. +# @param {string="Release"} [BUILD_TYPE] CMake build type (Debug/Release). +# @param {string=""} [CMAKE_CXX_COMPILER] C++ compiler to use. cmake-build: label: "{{.TASK}}-{{.SOURCE_DIR}}-{{.BUILD_DIR}}" internal: true vars: CHECKSUM_FILE: >- {{default (printf "%s.md5" .BUILD_DIR) .CHECKSUM_FILE}} CMAKE_ARGS: >- {{default "" .CMAKE_ARGS}} + BUILD_TYPE: >- + {{default "Release" .BUILD_TYPE}} + CMAKE_CXX_COMPILER: >- + {{default "" .CMAKE_CXX_COMPILER}}
326-334: Add validation for tar archive integrityWhile the implementation is robust, it would be beneficial to validate the tar archive's integrity before extraction.
Consider adding a validation step:
mkdir -p "{{.OUTPUT_DIR}}" + # Validate tar archive integrity + if ! tar --test-label --file "{{.TAR_FILE}}"; then + echo "Invalid or corrupt tar archive: {{.TAR_FILE}}" + exit 1 + fi tar --extract
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
taskfiles/utils.yml(7 hunks)
🔇 Additional comments (1)
taskfiles/utils.yml (1)
Line range hint 8-66: LGTM! Well-structured changes to support multiple paths
The modifications to handle multiple paths are implemented correctly, maintaining backward compatibility while adding flexibility. The tar command structure properly handles the array of paths, and the validation logic is appropriately updated.
|
Review incoming. |
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
taskfiles/utils.yml (5)
10-11: Improve parameter documentation clarityThe description of
EXCLUDE_PATHScould be clearer about the wildcard pattern support.-# @param {string[]} [EXCLUDE_PATHS] List of paths, relative to any `DATA_PATHS`, to exclude from -# the checksum. +# @param {string[]} [EXCLUDE_PATHS] Wildcard patterns for paths, relative to any `DATA_PATHS`, to +# exclude from the checksum.
154-196: Consider renaming the task for clarityThe task name
cmake-buildcould be more descriptive since it handles both configuration and building.-cmake-build: +cmake-config-and-build:Additionally, consider adding error handling for the CMake commands to provide more descriptive error messages.
204-204: Remove redundant parameterThe
DATA_PATHSparameter defaults to[{{.INSTALL_PREFIX}}]which is redundant since that's typically the only path we need to checksum.Consider removing this parameter and always using
INSTALL_PREFIXfor consistency.
271-274: Improve retry attempt messagingThe current implementation will print the retry message multiple times unnecessarily.
Consider moving the success check to the end of the loop:
- echo "Attempt $attempt failed. Retrying..." - attempt=$((attempt + 1)) - sleep 5 + if [ $attempt -lt $max_attempts ]; then + echo "Attempt $attempt failed. Retrying..." + attempt=$((attempt + 1)) + sleep 5 + else + break + fi
284-284: Standardize parameter namingFor consistency with the
curltask, consider renamingURL_SHA256toFILE_SHA256.-# @param {string} URL_SHA256 Content hash to verify downloaded tar file against. +# @param {string} FILE_SHA256 Content hash to verify downloaded tar file against.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
taskfiles/utils.yml(7 hunks)
🔇 Additional comments (1)
taskfiles/utils.yml (1)
119-119: LGTM! Parameter type notation standardized
The change from []string to string[] improves consistency in parameter type documentation.
Also applies to: 136-136
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
taskfiles/utils.yml (2)
157-237: Consider enhancing SOURCE_DIR parameter handlingBased on the previous discussion, consider these improvements:
- Add an optional
CMAKELISTS_PATHparameter (defaulting toSOURCE_DIR)- Change
SOURCE_DIRtoSOURCE_PATHSto handle projects with multiple source directories- # @param {string} SOURCE_DIR Project source directory containing the CMakeLists.txt file. + # @param {string[]} SOURCE_PATHS Project source directories. + # @param {string={{index .SOURCE_PATHS 0}}} [CMAKELISTS_PATH] Directory containing CMakeLists.txt.
242-349: Enhance error messaging for better debuggingThe implementation is robust, but consider enhancing the error output to include the HTTP status code and response headers when a download fails.
if curl \ --fail \ --location \ --show-error \ + --write-out '\nHTTP Status: %{http_code}\nResponse headers:\n%{header_out}\n' \ --connect-timeout 10 \ --max-time 300 \ "{{.URL}}" \ --output "{{.OUTPUT_FILE}}";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
taskfiles/utils.yml(6 hunks)
🔇 Additional comments (3)
taskfiles/utils.yml (3)
Line range hint 8-69: LGTM! Well-structured changes to support multiple paths
The changes to support multiple paths and wildcard patterns are well-implemented. The addition of --no-anchored and --wildcards flags is appropriate for the new pattern-matching functionality.
86-86: LGTM! Appropriate use of sed -E flag
The addition of the -E flag for extended regular expressions is well-documented and maintains cross-platform compatibility.
122-122: Type annotation consistency improvement
Also applies to: 139-139
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
taskfiles/utils.yml (2)
26-33: Consider adding error handling for empty patterns.While the implementation supports multiple patterns effectively, it might be worth adding a check for empty patterns to provide better error messages.
{{- range .DATA_PATTERNS}} +"{{if . | empty}}{{fail \"DATA_PATTERNS cannot contain empty strings\"}}{{end}}" "{{.}}" {{- end}}
156-179: Consider adding build type support.The implementation is solid, but it might be helpful to add support for different build types (Debug, Release, etc.).
# @param {string} SOURCE_DIR Project source directory containing the CMakeLists.txt file. # @param {string=""} [CMAKE_ARGS] Any additional arguments to pass to CMake's configure step. +# @param {string="Release"} [BUILD_TYPE] CMake build type (Debug, Release, RelWithDebInfo, MinSizeRel). cmake-config-and-build: label: "{{.TASK}}-{{.SOURCE_DIR}}-{{.BUILD_DIR}}" internal: true vars: CMAKE_ARGS: >- {{default "" .CMAKE_ARGS}} + BUILD_TYPE: >- + {{default "Release" .BUILD_TYPE}} requires: vars: ["BUILD_DIR", "SOURCE_DIR"] cmds: - >- cmake -S "{{.SOURCE_DIR}}" -B "{{.BUILD_DIR}}" + -DCMAKE_BUILD_TYPE="{{.BUILD_TYPE}}" {{.CMAKE_ARGS}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
taskfiles/utils.yml(6 hunks)
🔇 Additional comments (4)
taskfiles/utils.yml (4)
8-11: LGTM: Parameter documentation accurately reflects the new array-based pattern matching.
The parameter documentation clearly describes the relationship between DATA_PATTERNS and EXCLUDE_PATTERNS, making it evident that exclusion patterns are relative to the data patterns.
85-85: LGTM: Platform-compatible sed usage.
The use of -E flag is well-documented and ensures compatibility across Linux and macOS.
121-121: LGTM: Consistent parameter type annotations.
The parameter type annotations for SRC_PATHS have been updated to consistently use string[] across all tasks.
Also applies to: 138-138
243-305: LGTM: Robust implementation of tar download and extraction.
The implementation includes:
- Proper file integrity verification
- Flexible pattern matching for inclusion/exclusion
- Appropriate cleanup and error handling
| if curl \ | ||
| --fail \ | ||
| --location \ | ||
| --show-error \ | ||
| --connect-timeout 10 \ | ||
| --max-time 300 \ | ||
| "{{.URL}}" \ | ||
| --output "{{.OUTPUT_FILE}}"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security headers to curl command.
Consider adding security headers to protect against potential MITM attacks.
if curl \
--fail \
--location \
--show-error \
--connect-timeout 10 \
--max-time 300 \
+ --proto '=https' \
+ --tlsv1.2 \
+ --ciphers HIGH \
"{{.URL}}" \
--output "{{.OUTPUT_FILE}}";📝 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.
| if curl \ | |
| --fail \ | |
| --location \ | |
| --show-error \ | |
| --connect-timeout 10 \ | |
| --max-time 300 \ | |
| "{{.URL}}" \ | |
| --output "{{.OUTPUT_FILE}}"; | |
| if curl \ | |
| --fail \ | |
| --location \ | |
| --show-error \ | |
| --connect-timeout 10 \ | |
| --max-time 300 \ | |
| --proto '=https' \ | |
| --tlsv1.2 \ | |
| --ciphers HIGH \ | |
| "{{.URL}}" \ | |
| --output "{{.OUTPUT_FILE}}"; |
taskfiles/utils.yml
Outdated
| && diff -q '{{.TMP_CHECKSUM_FILE}}' '{{.CHECKSUM_FILE}}' 2> /dev/null | ||
| ) || rm -f '{{.CHECKSUM_FILE}}' | ||
| {{- range .DATA_PATTERNS}} | ||
| test -e "{{.}}" && |
There was a problem hiding this comment.
(Ignoring the quotes) I probably should've anticipated this, but this won't work for patterns since if a wildcard path glob returns multiple files, test -e will fail because it only expects one argument.
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
taskfiles/utils.yml (1)
170-178: Improve error handling for cmake commandsWhile Task will fail if commands fail, capturing and displaying cmake errors more clearly would help with debugging. Consider redirecting stderr to a file and displaying it on failure.
- >- cmake -S "{{.SOURCE_DIR}}" -B "{{.BUILD_DIR}}" {{.CMAKE_ARGS}} + 2> cmake-config.err || { cat cmake-config.err; exit 1; } - >- cmake --build "{{.BUILD_DIR}}" --parallel + 2> cmake-build.err || { cat cmake-build.err; exit 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
taskfiles/utils.yml(6 hunks)
🔇 Additional comments (2)
taskfiles/utils.yml (2)
278-281: Verify wildcard pattern handling in validate-checksum task
This task uses validate-checksum which has an issue with wildcard pattern handling in its path existence check. This should be fixed after addressing the previous comment about test -e.
✅ Verification successful
Let me check the implementation of the validate-checksum task to understand how it handles patterns.
No wildcard pattern handling issues found in validate-checksum task
The validate-checksum task implementation shows that it correctly handles path existence checks using test -e for each pattern in DATA_PATTERNS. In this case, {{.OUTPUT_DIR}} is a single directory path without wildcards, so there are no pattern handling concerns. The task is being used appropriately in this context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if validate-checksum is used with wildcard patterns in other tasks
rg -A 5 'task: "validate-checksum"'
Length of output: 317
Script:
#!/bin/bash
# Find the validate-checksum task implementation
ast-grep --pattern 'task: "validate-checksum"' -A 20
Length of output: 3940
224-231: 🛠️ Refactor suggestion
Add security headers to curl command
Add security headers to protect against potential MITM attacks and ensure secure connections.
if curl \
--fail \
--location \
--show-error \
--connect-timeout 10 \
--max-time 300 \
+ --proto '=https' \
+ --tlsv1.2 \
+ --ciphers HIGH \
"{{.URL}}" \
--output "{{.OUTPUT_FILE}}";Likely invalid or redundant comment.
taskfiles/utils.yml
Outdated
| {{- range .DATA_PATTERNS}} | ||
| test -e "{{.}}" && | ||
| {{- end}} | ||
| diff -q "{{.TMP_CHECKSUM_FILE}}" "{{.CHECKSUM_FILE}}" 2> /dev/null |
There was a problem hiding this comment.
Fix path existence check for wildcard patterns
The test -e command doesn't support wildcard patterns and will fail if the pattern expands to multiple paths. Consider using compgen -G or find to check for matching paths.
- {{- range .DATA_PATTERNS}}
- test -e "{{.}}" &&
- {{- end}}
+ {{- range .DATA_PATTERNS}}
+ compgen -G "{{.}}" > /dev/null &&
+ {{- end}}📝 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.
| {{- range .DATA_PATTERNS}} | |
| test -e "{{.}}" && | |
| {{- end}} | |
| diff -q "{{.TMP_CHECKSUM_FILE}}" "{{.CHECKSUM_FILE}}" 2> /dev/null | |
| {{- range .DATA_PATTERNS}} | |
| compgen -G "{{.}}" > /dev/null && | |
| {{- end}} | |
| diff -q "{{.TMP_CHECKSUM_FILE}}" "{{.CHECKSUM_FILE}}" 2> /dev/null |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
taskfiles/utils.yml (2)
170-174: Ensure build directory exists before cmake configure.The cmake configure step assumes the build directory exists, but there's no explicit creation of it.
Consider adding a mkdir command before the cmake configure step:
+ - "mkdir -p {{.BUILD_DIR}}" - >- cmake -S "{{.SOURCE_DIR}}" -B "{{.BUILD_DIR}}" {{.CMAKE_ARGS}}
287-300: Add error handling for tar extraction.The tar extraction command suppresses stderr but doesn't check for errors.
Consider capturing and checking the exit status:
- tar - --extract - --strip-components="{{.NUM_COMPONENTS_TO_STRIP}}" - --directory "{{.OUTPUT_DIR}}" - --file "{{.TAR_FILE}}" - --no-anchored - --wildcards - {{- range .EXCLUDE_PATTERNS}} - --exclude="{{.}}" - {{- end}} - {{- range .DATA_PATTERNS}} - "{{.}}" - {{- end}} - 2> /dev/null + if ! tar \ + --extract \ + --strip-components="{{.NUM_COMPONENTS_TO_STRIP}}" \ + --directory "{{.OUTPUT_DIR}}" \ + --file "{{.TAR_FILE}}" \ + --no-anchored \ + --wildcards \ + {{- range .EXCLUDE_PATTERNS}} \ + --exclude="{{.}}" \ + {{- end}} \ + {{- range .DATA_PATTERNS}} \ + "{{.}}" \ + {{- end}}; then + echo "Failed to extract tar file" + exit 1 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
taskfiles/utils.yml(6 hunks)
🔇 Additional comments (2)
taskfiles/utils.yml (2)
64-67:
Fix path existence check for wildcard patterns.
The test -e command doesn't support wildcard patterns and will fail if the pattern expands to multiple paths.
Apply this diff to fix the issue:
- {{- range .DATA_PATTERNS}}
- test -e "{{.}}" &&
- {{- end}}
+ {{- range .DATA_PATTERNS}}
+ compgen -G "{{.}}" > /dev/null &&
+ {{- end}}Likely invalid or redundant comment.
224-231: 🛠️ Refactor suggestion
Add security headers to curl command.
Consider adding security headers to protect against potential MITM attacks.
Apply this diff to add security headers:
if curl \
--fail \
--location \
--show-error \
--connect-timeout 10 \
--max-time 300 \
+ --proto '=https' \
+ --tlsv1.2 \
+ --ciphers HIGH \
"{{.URL}}" \
--output "{{.OUTPUT_FILE}}";Likely invalid or redundant comment.
…rt wildcards; add set/shopt flags globally.
Description
This PR adds two new sets of utility tasks, while also updating the checksum util tasks.
cmake utils
This PR adds two new tasks related to building a cmake project. The first runs build and configure, while the second performs the installation. Separating these tasks allows up to build a project once, but install it multiple times to different locations.
remote utils
The main utility task is
fetch-srcwhich will download and extract a remote tar ball. Thecurltask is separated out to allow directly downloading any files.checksum utils
Previously the checksum utils only operated on a single data path, which is an issue for tasks that generate files across multiple directories. This PR just changes the arguments to be an array of paths rather than a single path.
Validation performed
Used in ffi-go irv2 beta branch.
Summary by CodeRabbit
cmake-config-and-build,cmake-install) and remote file handling (curl,download-and-extract-tar).