feat(taskfiles)!: Update and refactor Boost tasks: #80
Conversation
…ed file in `remote:curl`.
… of Boost tasks; Add checksum unit tests.
WalkthroughRefactored Boost task signatures from GENERATE_DIR-based flow to unified BUILD_DIR/SOURCE_DIR/INSTALL_PREFIX model with per-stage checksums. Removed Boost.System component dependency, updated Boost version from 1.86.0 to 1.89.0, and adjusted Process header inclusion path. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
exports/taskfiles/utils/checksum.yaml (1)
89-95: Missing-path check can pass incorrectly; make each test -e failure fatal within the subshellInside the validation subshell you loop over all INCLUDE_PATTERNS and run test -e "$path" for each. The subshell’s exit status becomes the status of the last test, so a missing earlier path can be masked by a later successful test. This makes checksum validation and the subsequent FAIL/cleanup logic unreliable.
Apply this diff to fail fast on any missing path:
- for path in {{.}}; do - test -e "$path" - done + for path in {{.}}; do + test -e "$path" || exit 1 + doneexports/taskfiles/utils/boost.yaml (3)
7-8: Enable fail-fast: add -e to set flags.Currently only
-uand-o pipefailare enabled. Without-e, failed commands in a multi-linecmdsblock may be ignored, causing subtle corruption of checksums or partial installs.Apply:
-set: ["u", "pipefail"] +set: ["e", "u", "pipefail"]
42-53: Replace pushd/popd with POSIX-safe subshell to avoid Bash dependency.
pushd/popdandshoptare Bash-specific. If the global shell isn’t guaranteed to be Bash, the step may fail. A subshell withcdis portable and avoids side effects.- - >- - pushd "{{.SOURCE_DIR}}"; - ./bootstrap.sh - --prefix="{{.INSTALL_PREFIX}}" - --exec-prefix="{{.INSTALL_PREFIX}}" - --with-libraries={{(join "," .TARGETS)}} - {{- range .EXTRA_ARGS}} - "{{.}}" - {{- end}}; - popd + - > + ( + cd "{{.SOURCE_DIR}}" && + ./bootstrap.sh + --prefix="{{.INSTALL_PREFIX}}" + --exec-prefix="{{.INSTALL_PREFIX}}" + --with-libraries={{(join "," .TARGETS)}} + {{- range .EXTRA_ARGS}} + "{{.}}" + {{- end}} + )
119-131: Same portability concern: replace pushd/popd with a subshell.- - >- - pushd "{{.SOURCE_DIR}}"; - ./b2 - install - --build-dir="{{.BUILD_DIR}}" - {{- range .EXTRA_ARGS}} - "{{.}}" - {{- end}} - {{- if .JOBS}} - "-j{{.JOBS}}" - {{- end}}; - popd; + - > + ( + cd "{{.SOURCE_DIR}}" && + ./b2 install + --build-dir="{{.BUILD_DIR}}" + {{- range .EXTRA_ARGS}} + "{{.}}" + {{- end}} + {{- if .JOBS}} + "-j{{.JOBS}}" + {{- end}} + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
exports/taskfiles/utils/boost.yaml(3 hunks)exports/taskfiles/utils/checksum.yaml(2 hunks)taskfiles/boost/CMakeLists.txt(0 hunks)taskfiles/boost/test_boost.cpp(1 hunks)taskfiles/boost/tests.yaml(1 hunks)taskfiles/checksum/tests.yaml(1 hunks)taskfiles/tests.yaml(2 hunks)
💤 Files with no reviewable changes (1)
- taskfiles/boost/CMakeLists.txt
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
taskfiles/boost/test_boost.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests (ubuntu-22.04)
- GitHub Check: unit-tests (ubuntu-24.04)
- GitHub Check: unit-tests (macos-15)
🔇 Additional comments (8)
taskfiles/boost/test_boost.cpp (1)
14-14: Header change to v2/process.hpp looks goodSwitching from the umbrella v2 header to the specific v2/process.hpp is appropriate and keeps compile surfaces tighter. No behaviour change implied.
exports/taskfiles/utils/checksum.yaml (1)
49-51: Good addition: configurable failure vs. auto-clean on mismatchThe new behaviour—failing when FAIL="true" and otherwise removing the stale checksum file—matches the PR goals (skip when possible, rerun when necessary) and keeps downstream tasks simple.
taskfiles/boost/tests.yaml (2)
55-63: Removing the 'system' target is consistent; confirm linking remains header-only across platformsBoost.System has been effectively header-only for years, but some environments can still link a libboost_system artefact depending on build flags. Since test_boost.cpp includes <boost/system.hpp> and uses boost::system::result, please confirm no link errors arise on all supported platforms/compilers.
51-53: Verify the pinned Boost tarball SHA256The sandbox environment doesn’t include
sha256sum, so the previous script couldn’t compute the checksum. Please re-verify the hash to avoid CI surprises. For example, from a clean directory:#!/usr/bin/env bash set -euo pipefail url="https://github.com/boostorg/boost/releases/download/boost-1.89.0/boost-1.89.0-b2-nodocs.tar.gz" expected="aa25e7b9c227c21abb8a681efd4fe6e54823815ffc12394c9339de998eb503fb" curl -Ls "$url" -o boost.tar.gz # Option 1: using openssl (most CI images include this) actual=$(openssl dgst -sha256 boost.tar.gz | awk '{print $2}') # Option 2: using Python 3 if openssl isn’t available # actual=$(python3 -c "import hashlib; print(hashlib.sha256(open('boost.tar.gz','rb').read()).hexdigest())") if [ "$actual" != "$expected" ]; then echo "SHA mismatch: $actual != $expected" exit 1 fi echo "OK: $actual"taskfiles/tests.yaml (2)
5-5: Nice: checksum test suite exposed publiclyAdding the checksum include keeps the test surface coherent with the new checksum tasks.
14-14: Good ordering: run checksum tests after BoostRunning checksum after Boost mirrors real-world usage where checksum gating influences subsequent build steps.
taskfiles/checksum/tests.yaml (1)
135-139: Dependency pattern is sound; confirm Bash availability due to process substitution elsewherecreate-dir-with-checksum correctly uses checksum:validate as a dep plus sources/generates to enable skipping. Given other commands in this file previously used process substitution, please ensure CI sets the task shell to Bash (or address the portability issues noted above).
exports/taskfiles/utils/boost.yaml (1)
32-41: The scripts above will:
- Print the first 80 lines of the
boost.yamlyou’re modifying, so we can see its full context.- Check whether
method:is explicitly set inboost.yamlor in any root-level Taskfile (to determine if you’re globally using thetimestampmethod instead of the defaultchecksum).- Search under
exports/for any occurrences ofmethod: timestamp.Once we have that output, we can confirm if your up-to-date logic indeed depends on mtime heuristics (and thus risks false skips) or if you’re already using the checksum method (where “generates” doesn’t factor into up-to-date checks).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
taskfiles/checksum/tests.yaml (2)
64-65: Replace Bash-only process substitution and non-portable date -r with cross-platform statProcess substitution breaks under /bin/sh. Also, date -r FILE is GNU-specific (macOS/BSD date uses -r for epoch input). Use stat with a GNU/BSD fallback to compare mtimes portably.
- - "date -r '{{.CHECKSUM_FILE}}' > '{{.CHECKSUM_MOD_TS}}'" + - |- + # Write mtime seconds; works on BSD/macOS (stat -f) and GNU (stat -c). + mtime="$( { stat -f '%m' '{{.CHECKSUM_FILE}}' 2>/dev/null || stat -c '%Y' '{{.CHECKSUM_FILE}}'; } )" + printf '%s\n' "$mtime" > '{{.CHECKSUM_MOD_TS}}' - - "diff '{{.CHECKSUM_MOD_TS}}' <(date -r '{{.CHECKSUM_FILE}}')" + - |- + # Compare stored mtime with current mtime without Bash-only features. + current_mtime="$( { stat -f '%m' '{{.CHECKSUM_FILE}}' 2>/dev/null || stat -c '%Y' '{{.CHECKSUM_FILE}}'; } )" + test "$(cat '{{.CHECKSUM_MOD_TS}}')" = "$current_mtime"Also applies to: 73-73
123-124: Dangerous glob: rm -rf {{.OUTPUT_DIR}} can remove unintended siblings*Trailing globs can nuke paths sharing the prefix (e.g., /tmp/out-old). Delete the exact directory.
- - "rm -rf {{.OUTPUT_DIR}}*" - - "mkdir -p '{{.OUTPUT_DIR}}'" + - "rm -rf '{{.OUTPUT_DIR}}'" + - "mkdir -p '{{.OUTPUT_DIR}}'"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
taskfiles/checksum/tests.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sitaowang1998
PR: y-scope/yscope-dev-utils#64
File: exports/taskfiles/utils/boost.yaml:31-37
Timestamp: 2025-06-08T17:55:11.362Z
Learning: For Boost builds in yscope-dev-utils, symlinking only the top-level directories from SOURCE_DIR to BUILD_DIR is sufficient for the build to succeed. Recursive symlinking of all nested files and directories is not necessary.
📚 Learning: 2024-10-28T19:02:02.131Z
Learnt from: AVMatthews
PR: y-scope/yscope-dev-utils#13
File: taskfiles/utils.yml:149-149
Timestamp: 2024-10-28T19:02:02.131Z
Learning: In `taskfiles/utils.yml`, avoid using GNU-specific options like `-r` and `--no-run-if-empty` with `xargs` for better portability; instead, use `find`'s `-exec` option.
Applied to files:
taskfiles/checksum/tests.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests (ubuntu-24.04)
- GitHub Check: unit-tests (macos-15)
- GitHub Check: unit-tests (ubuntu-22.04)
🔇 Additional comments (2)
taskfiles/checksum/tests.yaml (2)
1-14: Solid test harness structure and focusGood job composing a self-contained suite with clear orchestration (default → rerun/skip/update) and isolating temp artefacts under G_OUTPUT_DIR. Using generates with the validate dep to exercise the skip path is clean.
20-21: No action required:.md5extension is correctThe checksum task in
exports/taskfiles/utils/checksum.yamlinvokesmd5sumto generate the checksum (see line 45), so the.md5filename extension accurately reflects the algorithm in use. You can safely ignore the original suggestion to change the extension.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
taskfiles/checksum/tests.yaml (4)
44-47: Portability: use cmp -s instead of GNU-only --silentcmp --silent is not available on macOS/BSD. Use the portable -s flag.
Apply this diff:
- if cmp --silent "{{.CHECKSUM_FILE}}" "{{.CHECKSUM_FILE_REF}}"; then + if cmp -s "{{.CHECKSUM_FILE}}" "{{.CHECKSUM_FILE_REF}}"; then echo "'{{.CHECKSUM_FILE}}' and '{{.CHECKSUM_FILE_REF}}' should differ." exit 1 fi
112-116: Portability: replace cmp --silent with cmp -sSame portability issue; also standardize all equality checks on cmp -s.
Apply this diff:
- - "cmp --silent '{{.FILE_0}}' '{{.CHECKSUM_FILE_REF0}}'" - - "cmp --silent '{{.CHECKSUM_FILE}}' '{{.CHECKSUM_FILE_REF1}}'" + - "cmp -s '{{.FILE_0}}' '{{.CHECKSUM_FILE_REF0}}'" + - "cmp -s '{{.CHECKSUM_FILE}}' '{{.CHECKSUM_FILE_REF1}}'" @@ - if cmp --silent "{{.CHECKSUM_FILE}}" "{{.CHECKSUM_FILE_REF0}}"; then + if cmp -s "{{.CHECKSUM_FILE}}" "{{.CHECKSUM_FILE_REF0}}"; then echo "'{{.CHECKSUM_FILE}}' and '{{.CHECKSUM_FILE_REF0}}' should differ." exit 1 fi
125-125: Risky cleanup: avoid globbing with rm -rf {{.OUTPUT_DIR}}*Using a trailing * can delete unintended siblings (e.g., /tmp/foo-old). Remove the exact target directory instead and always quote variables.
Apply this diff:
- - "rm -rf {{.OUTPUT_DIR}}*" + - "rm -rf '{{.OUTPUT_DIR}}'"
137-141: Tighten validation: enforce failure on checksum mismatchIf checksum:validate supports a FAIL flag (per PR description), pass it so mismatches hard-fail and guard against silent drifts in dependencies.
Apply this diff:
- task: "checksum:validate" vars: CHECKSUM_FILE: "{{.CHECKSUM_FILE}}" INCLUDE_PATTERNS: ["{{.DIR}}"] + FAIL: "true"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/unit-tests.yaml(1 hunks)taskfiles/checksum/tests.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: davidlion
PR: y-scope/yscope-dev-utils#80
File: exports/taskfiles/utils/boost.yaml:20-27
Timestamp: 2025-08-24T14:25:42.393Z
Learning: In the Boost taskfiles (exports/taskfiles/utils/boost.yaml), sentinel files are intentionally placed inside directories that the task needs to modify (e.g., "{{.SOURCE_DIR}}/{{.TASK}}.sentinel"). This is by design - the sentinel indicates that the task has altered the directory contents. It would be a logical bug for upstream workflows to call these tasks while expecting the directories to remain unchanged.
Learnt from: sitaowang1998
PR: y-scope/yscope-dev-utils#64
File: exports/taskfiles/utils/boost.yaml:31-37
Timestamp: 2025-06-08T17:55:11.362Z
Learning: For Boost builds in yscope-dev-utils, symlinking only the top-level directories from SOURCE_DIR to BUILD_DIR is sufficient for the build to succeed. Recursive symlinking of all nested files and directories is not necessary.
📚 Learning: 2024-10-28T19:02:02.131Z
Learnt from: AVMatthews
PR: y-scope/yscope-dev-utils#13
File: taskfiles/utils.yml:149-149
Timestamp: 2024-10-28T19:02:02.131Z
Learning: In `taskfiles/utils.yml`, avoid using GNU-specific options like `-r` and `--no-run-if-empty` with `xargs` for better portability; instead, use `find`'s `-exec` option.
Applied to files:
taskfiles/checksum/tests.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests (ubuntu-22.04)
- GitHub Check: unit-tests (ubuntu-24.04)
- GitHub Check: unit-tests (macos-15)
🔇 Additional comments (2)
.github/workflows/unit-tests.yaml (1)
41-42: Switching to verbose test output is fineUsing task test -v improves CI diagnosability when tests fail. No issues spotted.
taskfiles/checksum/tests.yaml (1)
66-66: Fix non-POSIX timestamp checks; avoid Bash-only process substitution and GNU date -rdate -r file is GNU-only and fails on macOS; process substitution <(...) requires Bash. Use a stat-based fallback that works on both GNU/Linux and BSD/macOS.
Apply this diff:
- - "date -r '{{.CHECKSUM_FILE}}' > '{{.CHECKSUM_MOD_TS}}'" + - |- + # Write mtime (epoch seconds) in a GNU/BSD portable way + if stat -c %Y '{{.CHECKSUM_FILE}}' >/dev/null 2>&1; then + stat -c %Y '{{.CHECKSUM_FILE}}' > '{{.CHECKSUM_MOD_TS}}' + else + stat -f %m '{{.CHECKSUM_FILE}}' > '{{.CHECKSUM_MOD_TS}}' + fi @@ - - "cmp --silent '{{.CHECKSUM_MOD_TS}}' <(date -r '{{.CHECKSUM_FILE}}')" + - |- + # Compare saved mtime to current mtime (portable) + if stat -c %Y '{{.CHECKSUM_FILE}}' >/dev/null 2>&1; then + mtime="$(stat -c %Y '{{.CHECKSUM_FILE}}')" + else + mtime="$(stat -f %m '{{.CHECKSUM_FILE}}')" + fi + test "$(cat '{{.CHECKSUM_MOD_TS}}')" = "$mtime"Also applies to: 75-75
⛔ Skipped due to learnings
Learnt from: davidlion PR: y-scope/yscope-dev-utils#70 File: taskfiles/remote/tests.yaml:62-72 Timestamp: 2025-07-12T05:52:11.439Z Learning: The `date -r <file>` command is supported on both Linux and macOS to get file modification times, making it portable across these platforms.Learnt from: AVMatthews PR: y-scope/yscope-dev-utils#13 File: taskfiles/utils.yml:149-149 Timestamp: 2024-10-28T19:02:02.131Z Learning: In `taskfiles/utils.yml`, avoid using GNU-specific options like `-r` and `--no-run-if-empty` with `xargs` for better portability; instead, use `find`'s `-exec` option.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
taskfiles/boost/test_boost.cpp (1)
106-106: v2::process_stdio fix resolves the namespace mismatch; confirm C++20 designated-initializer supportAligning the stdio type with the v2 namespace addresses the earlier compilation hazard. One follow-up: the designated-initializer syntax requires C++20—please verify the project enforces C++20 (or newer) so this remains portable across compilers/CI.
Run this script from repo root to confirm the C++ standard settings used by CMake/tooling:
#!/bin/bash set -euo pipefail echo "Searching CMake files for explicit standard settings..." fd -t f CMakeLists.txt -0 2>/dev/null | xargs -0 rg -nP 'CMAKE_CXX_STANDARD\s*[:=]\s*(\d+)|target_compile_features\([^)]*\bcxx_std_(\d+)\b' -C2 || true fd -t f -e cmake -0 2>/dev/null | xargs -0 rg -nP 'CMAKE_CXX_STANDARD\s*[:=]\s*(\d+)|target_compile_features\([^)]*\bcxx_std_(\d+)\b' -C2 || true echo echo "Scanning for explicit compiler flags selecting C++20/23..." rg -nP --glob '!**/build/**' '-std=c\+\+2(0|3|a|b)' -S || true echo echo "If no occurrences show C++20 (20/23), consider setting:" echo " set(CMAKE_CXX_STANDARD 20)" echo "or:" echo " target_compile_features(<target> PRIVATE cxx_std_20)"exports/taskfiles/utils/boost.yaml (4)
58-71: Docs: clarify checksum failure semantics (SOURCE = strict, BUILD/INSTALL = advisory).State explicitly that SOURCE_CHECKSUM_FILE is validated with FAIL=true (hard guard for “generate was run”), while BUILD/INSTALL validations are advisory by design and do not fail the task unless the caller opts in.
Suggested addition to the docblock:
@@ -# Fails if the caller has not successfully called `generate` on `SOURCE_DIR`. +# Fails if the caller has not successfully called `generate` on `SOURCE_DIR`. +# Checksum semantics: +# - SOURCE_CHECKSUM_FILE: validated with FAIL=true to ensure `generate` ran and the source tree +# matches the recorded state. +# - BUILD_CHECKSUM_FILE and INSTALL_CHECKSUM_FILE: validated without FAIL by default (advisory); +# mismatches trigger recomputation at the end but do not stop the task unless explicitly set.
98-103: Do not list checksum files under generates; keep only the sentinel.Including the checksum files as generates can cause the task to be skipped before deps/validate run, depending on mtimes and runner version. Use the sentinel as the single generate.
Apply:
generates: - - "{{.BUILD_CHECKSUM_FILE}}" - - "{{.INSTALL_CHECKSUM_FILE}}" - - "{{.SOURCE_CHECKSUM_FILE}}" - "{{.SOURCE_SENTINEL_FILE}}"
200-206: Create CMAKE_SETTINGS_DIR before writing Boost.cmake.Without ensuring the directory exists, the echo redirection will fail when the directory is absent.
Apply:
- {{- if .CMAKE_SETTINGS_DIR}} - echo "set(Boost_ROOT + {{- if .CMAKE_SETTINGS_DIR}} + mkdir -p "{{.CMAKE_SETTINGS_DIR}}" && + echo "set(Boost_ROOT \"{{.INSTALL_PREFIX}}\" CACHE PATH \"Package root for Boost.\" )" > "{{.CMAKE_SETTINGS_DIR}}/Boost.cmake" {{- end}}
33-35: Do not list CHECKSUM_FILE under generates; it can short‑circuit validation.When CHECKSUM_FILE is a generated artefact, Task may mark the task up‑to‑date purely by mtimes and skip running deps/validate even if the directory contents changed. Rely on the sentinel as the sole generate instead.
Apply:
generates: - - "{{.CHECKSUM_FILE}}" - "{{.SOURCE_SENTINEL_FILE}}"taskfiles/checksum/tests.yaml (5)
25-27: Remove no-op defer that references an undefined variable
.EXIT_CODEis not defined here; this prints nothing and adds noise. Drop the block.- - defer: |- - echo "[debug] rerun {{.EXIT_CODE}}"
44-48: Make the test actually fail on unexpected equality and use portable cmp -sTwo issues:
- The test only echoes when checksums unexpectedly match; it does not fail.
cmp --silentis GNU-specific; use portablecmp -s.- - |- - if cmp --silent "{{.CHECKSUM_FILE}}" "{{.CHECKSUM_FILE_REF}}"; then - echo "'{{.CHECKSUM_FILE}}' and '{{.CHECKSUM_FILE_REF}}' should differ." - else - echo "[debug] cmp failed successfully." - fi + - |- + if cmp -s "{{.CHECKSUM_FILE}}" "{{.CHECKSUM_FILE_REF}}"; then + echo "'{{.CHECKSUM_FILE}}' and '{{.CHECKSUM_FILE_REF}}' should differ." + exit 1 + else + echo "[debug] cmp failed successfully." + fi
77-77: Avoid Bash-only process substitution; compare mtimes with POSIX command substitution
<(...)requires Bash and breaks under plain /bin/sh. Also switch tocmp -s/testsemantics for portability.- - "cmp --silent '{{.CHECKSUM_MOD_TS}}' <(date -r '{{.CHECKSUM_FILE}}')" + - "test \"$(cat '{{.CHECKSUM_MOD_TS}}')\" = \"$(date -r '{{.CHECKSUM_FILE}}')\""
114-120: Portability: prefer cmp -s over GNU-only --silentReplace all
cmp --silentwithcmp -s. Logic unchanged; exit(1) on unexpected equality is already present.- - "cmp --silent '{{.FILE_0}}' '{{.CHECKSUM_FILE_REF0}}'" - - "cmp --silent '{{.CHECKSUM_FILE}}' '{{.CHECKSUM_FILE_REF1}}'" + - "cmp -s '{{.FILE_0}}' '{{.CHECKSUM_FILE_REF0}}'" + - "cmp -s '{{.CHECKSUM_FILE}}' '{{.CHECKSUM_FILE_REF1}}'" - if cmp --silent "{{.CHECKSUM_FILE}}" "{{.CHECKSUM_FILE_REF0}}"; then + if cmp -s "{{.CHECKSUM_FILE}}" "{{.CHECKSUM_FILE_REF0}}"; then echo "'{{.CHECKSUM_FILE}}' and '{{.CHECKSUM_FILE_REF0}}' should differ." exit 1 fi
139-143: Enforce validation failures to fail loudlyIf
checksum:validatesupports a strict mode (as per PR), pass it so stale checksums cause a hard failure, tightening the test oracle.- task: "checksum:validate" vars: CHECKSUM_FILE: "{{.CHECKSUM_FILE}}" INCLUDE_PATTERNS: ["{{.DIR}}"] + FAIL: "true"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
exports/taskfiles/utils/boost.yaml(3 hunks)taskfiles/boost/test_boost.cpp(2 hunks)taskfiles/checksum/tests.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
taskfiles/boost/test_boost.cpp
🧠 Learnings (2)
📚 Learning: 2025-08-24T14:25:42.393Z
Learnt from: davidlion
PR: y-scope/yscope-dev-utils#80
File: exports/taskfiles/utils/boost.yaml:20-27
Timestamp: 2025-08-24T14:25:42.393Z
Learning: In the Boost taskfiles (exports/taskfiles/utils/boost.yaml), sentinel files are intentionally placed inside directories that the task needs to modify (e.g., "{{.SOURCE_DIR}}/{{.TASK}}.sentinel"). This is by design - the sentinel indicates that the task has altered the directory contents. It would be a logical bug for upstream workflows to call these tasks while expecting the directories to remain unchanged.
Applied to files:
exports/taskfiles/utils/boost.yaml
📚 Learning: 2024-10-28T19:02:02.131Z
Learnt from: AVMatthews
PR: y-scope/yscope-dev-utils#13
File: taskfiles/utils.yml:149-149
Timestamp: 2024-10-28T19:02:02.131Z
Learning: In `taskfiles/utils.yml`, avoid using GNU-specific options like `-r` and `--no-run-if-empty` with `xargs` for better portability; instead, use `find`'s `-exec` option.
Applied to files:
taskfiles/checksum/tests.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests (ubuntu-22.04)
- GitHub Check: unit-tests (ubuntu-24.04)
- GitHub Check: unit-tests (macos-15)
🔇 Additional comments (7)
taskfiles/boost/test_boost.cpp (1)
14-16: Use of fine-grained v2 headers — LGTMSwitching from the umbrella header to <boost/process/v2/process.hpp> (with stdio in a separate header) is correct for this translation unit and can reduce compile time. No further action needed.
exports/taskfiles/utils/boost.yaml (3)
26-27: Sentinel in SOURCE_DIR is acceptable (intent confirmed).Acknowledged the intentional placement of the sentinel under SOURCE_DIR for tasks that mutate that tree, per maintainer clarification and stored learnings. No change requested.
104-117: Decide whether stale BUILD/INSTALL checksums should fail the task.If a drift in BUILD_DIR or INSTALL_PREFIX must force remediation instead of proceeding, opt into FAIL=true for those validations. Otherwise, keep current advisory behaviour, but ensure the docblock calls it out (see earlier comment).
Apply if strict failure is desired:
- task: "checksum:validate" vars: CHECKSUM_FILE: "{{.BUILD_CHECKSUM_FILE}}" INCLUDE_PATTERNS: ["{{.BUILD_DIR}}"] + FAIL: "true" - task: "checksum:validate" vars: CHECKSUM_FILE: "{{.INSTALL_CHECKSUM_FILE}}" INCLUDE_PATTERNS: ["{{.INSTALL_PREFIX}}"] + FAIL: "true"
165-165: Label fix looks good.label now references .TAR_URL (not .URL). Thanks for correcting the undefined variable.
taskfiles/checksum/tests.yaml (3)
127-129: Good: safe cleanup without risky globsUsing
rm -rf '{{.OUTPUT_DIR}}'avoids the dangerous*suffix that can nuke sibling paths with the same prefix.
1-14: Overall: solid, self-contained test flowsThe three scenarios (rerun, skip, update) clearly exercise checksum semantics using Task’s deps/sources/generates. Variable quoting and path handling look correct.
15-24: Add arequiresguard forG_OUTPUT_DIR, or confirm it’s always provided by the test harnessAll three checksum‐test tasks in
taskfiles/checksum/tests.yamlcomputeOUTPUT_DIRfrom.G_OUTPUT_DIR. If that variable were ever unset or empty, future cleanup steps could operate on the wrong directory. To prevent this:• In each of the following tasks, immediately after the
vars:block, add a:requires: vars: ["G_OUTPUT_DIR"]– checksum-test-rerun (lines 15–24)
– checksum-test-skip (lines 51–60)
– checksum-test-update (lines 79–89)• If the test harness already guarantees that
.G_OUTPUT_DIRis always set, you may safely omit these guards. Otherwise, including them is a cheap, low‐risk safety net.
There was a problem hiding this comment.
Actionable comments posted: 2
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/boost.yaml (2)
42-52: Prefer Task’s dir: over pushd/popd; safer shells, cleaner logsUse dir: to run bootstrap in SOURCE_DIR and avoid relying on bash-only builtins. This also improves portability across shells.
- - >- - pushd "{{.SOURCE_DIR}}"; - ./bootstrap.sh + - dir: "{{.SOURCE_DIR}}" + cmd: >- + ./bootstrap.sh --prefix="{{.INSTALL_PREFIX}}" --exec-prefix="{{.INSTALL_PREFIX}}" --with-libraries={{(join "," .TARGETS)}} {{- range .EXTRA_ARGS}} "{{.}}" {{- end}}; - popd
118-129: Use dir: instead of pushd/popd for b2 invocationSame rationale as generate; this removes bash-only builtins and makes the step more robust.
- - >- - pushd "{{.SOURCE_DIR}}"; - ./b2 + - dir: "{{.SOURCE_DIR}}" + cmd: >- + ./b2 install --build-dir="{{.BUILD_DIR}}" {{- range .EXTRA_ARGS}} "{{.}}" {{- end}} {{- if .JOBS}} "-j{{.JOBS}}" - {{- end}}; - popd; + {{- end}};
♻️ Duplicate comments (10)
.github/workflows/unit-tests.yaml (1)
41-42: Gate verbosity by event to keep scheduled/push logs leanSplit the test step into two mutually exclusive steps so only PRs run verbose output; keep scheduled/push runs quiet.
- - name: "Run unit tests" - run: "task test --concurrency 1 --verbose" + - name: "Run unit tests (PR, verbose)" + if: ${{ github.event_name == 'pull_request' }} + run: task test --concurrency 1 --verbose + + - name: "Run unit tests (non-PR)" + if: ${{ github.event_name != 'pull_request' }} + run: task test --concurrency 1exports/taskfiles/utils/boost.yaml (9)
58-61: Docs: call out strict failure only for source checksum; others are advisoryExplicitly state here that only SOURCE_CHECKSUM_FILE validation uses FAIL: "true"; BUILD/INSTALL checksum mismatches are advisory and the build proceeds. This avoids ambiguity for future maintainers.
67-71: Docs: same clarification for checksum parametersMirror the clarification that SOURCE checksum is strict; BUILD/INSTALL are advisory unless explicitly opted into FAIL.
98-103: “generates” with checksum files can skip validate; prefer a single sentinel or statusAs with generate, declaring the checksum files as generates can prevent checksum:validate from running due to mtimes. Either:
- keep only a single sentinel under generates and add a status block invoking checksum:validate for BUILD/INSTALL/SOURCE, or
- remove generates entirely and rely on status for up‑to‑date detection.
This avoids stale runs slipping through.
Example:
- generates: - - "{{.BUILD_CHECKSUM_FILE}}" - - "{{.INSTALL_CHECKSUM_FILE}}" - - "{{.SOURCE_CHECKSUM_FILE}}" - - "{{.SOURCE_SENTINEL_FILE}}" + generates: + - "{{.SOURCE_SENTINEL_FILE}}" + status: + - task: checksum:validate + vars: { CHECKSUM_FILE: "{{.BUILD_CHECKSUM_FILE}}", INCLUDE_PATTERNS: ["{{.BUILD_DIR}}"] } + - task: checksum:validate + vars: { CHECKSUM_FILE: "{{.INSTALL_CHECKSUM_FILE}}", INCLUDE_PATTERNS: ["{{.INSTALL_PREFIX}}"] } + - task: checksum:validate + vars: { CHECKSUM_FILE: "{{.SOURCE_CHECKSUM_FILE}}", INCLUDE_PATTERNS: ["{{.SOURCE_DIR}}"], FAIL: "true" }
104-117: Decide on strictness for BUILD/INSTALL driftIf manual edits or unexpected changes in BUILD_DIR/INSTALL_PREFIX should fail the job, add FAIL: "true" to those validations. Otherwise, leave relaxed as a conscious choice.
- task: "checksum:validate" vars: CHECKSUM_FILE: "{{.BUILD_CHECKSUM_FILE}}" INCLUDE_PATTERNS: ["{{.BUILD_DIR}}"] + FAIL: "true" - task: "checksum:validate" vars: CHECKSUM_FILE: "{{.INSTALL_CHECKSUM_FILE}}" INCLUDE_PATTERNS: ["{{.INSTALL_PREFIX}}"] + FAIL: "true"
156-159: TARGETS optionality: align docs, requires, and call‑sitesIf empty TARGETS should be allowed (common when building a minimal set), make it optional end‑to‑end and guard the command flag accordingly; else update docs to say “Required”.
Command guard (if made optional):
- --with-libraries={{(join "," .TARGETS)}} + {{- if gt (len .TARGETS) 0 -}} + --with-libraries={{(join "," .TARGETS)}} + {{- end -}}
172-177: Drop hard require on TARGETS if optional in docsIf you keep TARGETS optional, remove it from requires.vars and provide a default empty list to avoid call‑site failures.
requires: vars: - "TAR_SHA256" - "TAR_URL" - - "TARGETS" - "WORK_DIR"
200-207: Ensure CMAKE_SETTINGS_DIR exists before writing Boost.cmakeCreate the directory to avoid failures when the path is absent.
- - >- - {{- if .CMAKE_SETTINGS_DIR}} - echo "set(Boost_ROOT + - >- + {{- if .CMAKE_SETTINGS_DIR}} + mkdir -p "{{.CMAKE_SETTINGS_DIR}}" && + echo "set(Boost_ROOT \"{{.INSTALL_PREFIX}}\" CACHE PATH \"Package root for Boost.\" )" > "{{.CMAKE_SETTINGS_DIR}}/Boost.cmake" {{- end}}
81-88: Expose combined checksum override (optional)If you intended a single file to represent both BUILD and INSTALL states, surface an explicit mapping to reduce API confusion; otherwise, ignore.
BUILD_CHECKSUM_FILE: >- {{default (printf "%s.md5" .BUILD_DIR) .BUILD_CHECKSUM_FILE}} INSTALL_CHECKSUM_FILE: >- {{default (printf "%s.md5" .INSTALL_PREFIX) .INSTALL_CHECKSUM_FILE}} + # Optional: allow a single combined checksum file to be used for both + {{- if .BUILD_AND_INSTALL_CHECKSUM_FILE }} + BUILD_CHECKSUM_FILE: "{{.BUILD_AND_INSTALL_CHECKSUM_FILE}}" + INSTALL_CHECKSUM_FILE: "{{.BUILD_AND_INSTALL_CHECKSUM_FILE}}" + {{- end }} SOURCE_CHECKSUM_FILE: >- {{default (printf "%s.md5" .SOURCE_DIR) .SOURCE_CHECKSUM_FILE}}
33-41: Avoid short‑circuiting generate via “generates”; ensure validate always runsListing the checksum file under generates can cause the task (and its deps) to be considered up‑to‑date based solely on mtimes of TASKFILE vs. CHECKSUM_FILE. To guarantee the checksum validation runs when SOURCE_DIR changes, prefer a status check that delegates to checksum:validate (or remove generates for the checksum and keep a single sentinel as the generate output).
If you want to keep a single generated artifact while ensuring validation runs, consider:
sources: ["{{.TASKFILE}}"] - generates: - - "{{.CHECKSUM_FILE}}" - - "{{.SOURCE_SENTINEL_FILE}}" + generates: + - "{{.SOURCE_SENTINEL_FILE}}" + status: + - task: checksum:validate + vars: + CHECKSUM_FILE: "{{.CHECKSUM_FILE}}" + INCLUDE_PATTERNS: ["{{.SOURCE_DIR}}"]This way, generate re-executes when the checksum is stale, and only the sentinel governs up‑to‑date detection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/unit-tests.yaml(1 hunks)exports/taskfiles/utils/boost.yaml(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-24T14:25:42.393Z
Learnt from: davidlion
PR: y-scope/yscope-dev-utils#80
File: exports/taskfiles/utils/boost.yaml:20-27
Timestamp: 2025-08-24T14:25:42.393Z
Learning: In the Boost taskfiles (exports/taskfiles/utils/boost.yaml), sentinel files are intentionally placed inside directories that the task needs to modify (e.g., "{{.SOURCE_DIR}}/{{.TASK}}.sentinel"). This is by design - the sentinel indicates that the task has altered the directory contents. It would be a logical bug for upstream workflows to call these tasks while expecting the directories to remain unchanged.
Applied to files:
exports/taskfiles/utils/boost.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests (ubuntu-22.04)
- GitHub Check: unit-tests (macos-15)
- GitHub Check: unit-tests (ubuntu-24.04)
🔇 Additional comments (2)
exports/taskfiles/utils/boost.yaml (2)
26-27: Sentinel in SOURCE_DIR acknowledged as intentionalSOURCE_SENTINEL_FILE under SOURCE_DIR is consistent with the documented design intent (signals the task mutated the directory). No change requested.
165-165: Label fix looks goodSwitched to .TAR_URL; avoids undefined .URL. No action needed.
|
@coderabbitai pause |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
taskfiles/checksum/tests.yaml (2)
38-41: Use portable cmp -s instead of GNU-only --silentcmp --silent is GNU-only; cmp -s is portable across GNU/BSD/macOS and expresses the same intent.
- - "! cmp --silent '{{.CHECKSUM_FILE}}' '{{.CHECKSUM_FILE_REF}}'" + - "! cmp -s '{{.CHECKSUM_FILE}}' '{{.CHECKSUM_FILE_REF}}'"
66-70: Bashism and GNUism: process substitution and cmp --silent break on /bin/sh and BSD
- <(…) requires Bash; /bin/sh on many systems (e.g., Debian’s dash) will error.
- cmp --silent is GNU-only. Use cmp -s and POSIX command substitution.
- - "cmp --silent '{{.CHECKSUM_MOD_TS}}' <(date -r '{{.CHECKSUM_FILE}}')" + - 'test "$(cat "{{.CHECKSUM_MOD_TS}}")" = "$(python3 -c '\''import os,sys; print(int(os.path.getmtime(sys.argv[1])))'\'' "{{.CHECKSUM_FILE}}")"'If you prefer avoiding Python, pair this with the stat-based line suggested for Line 60 and mirror it here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
taskfiles/checksum/tests.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-28T19:02:02.131Z
Learnt from: AVMatthews
PR: y-scope/yscope-dev-utils#13
File: taskfiles/utils.yml:149-149
Timestamp: 2024-10-28T19:02:02.131Z
Learning: In `taskfiles/utils.yml`, avoid using GNU-specific options like `-r` and `--no-run-if-empty` with `xargs` for better portability; instead, use `find`'s `-exec` option.
Applied to files:
taskfiles/checksum/tests.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests (macos-15)
- GitHub Check: unit-tests (ubuntu-24.04)
- GitHub Check: unit-tests (ubuntu-22.04)
🔇 Additional comments (6)
taskfiles/checksum/tests.yaml (6)
8-14: Well-structured default target orchestrating the three scenariosClear sequencing of rerun/skip/update keeps the suite readable and maintainable. Nice separation of concerns.
15-37: Test flow and variable naming are clearGood use of per-test OUTPUT_DIR and explicit FILE_0/FILE_1 variables. The “rename checksum to reference, then recompute” pattern convincingly asserts rerun behaviour.
82-101: Solid update test; good use of intermediate referencesThe “cat checksum into FILE_0 + recompute” trick is clever to force a content-based delta without introducing timing flakiness.
111-118: Safe cleanup (no trailing globs) — goodExplicitly removing OUTPUT_DIR without a trailing wildcard avoids accidental sibling deletions.
133-140: Helper is clear and idempotent; pairs well with checksum tasksThe “wipe dir, create file, then compute checksum” flow is easy to reason about and makes the tests deterministic.
60-60: Non-portable: date -r FILE is GNU-only; use a portable mtime probeOn macOS/BSD, date -r expects an epoch, not a filename. This will fail there. Prefer a cross-platform stat or a tiny Python one-liner to read mtime.
Option A (portable via Python, typically available in CI/macOS/Linux):
- - "date -r '{{.CHECKSUM_FILE}}' > '{{.CHECKSUM_MOD_TS}}'" + - "python3 -c 'import os,sys; print(int(os.path.getmtime(sys.argv[1])))' '{{.CHECKSUM_FILE}}' > '{{.CHECKSUM_MOD_TS}}'"Option B (pure shell with stat on both GNU/BSD):
- - "date -r '{{.CHECKSUM_FILE}}' > '{{.CHECKSUM_MOD_TS}}'" + - >- + sh -c 'stat -c %Y "$1" 2>/dev/null || stat -f %m "$1"' -- '{{.CHECKSUM_FILE}}' > '{{.CHECKSUM_MOD_TS}}'⛔ Skipped due to learnings
Learnt from: davidlion PR: y-scope/yscope-dev-utils#70 File: taskfiles/remote/tests.yaml:62-72 Timestamp: 2025-07-12T05:52:11.439Z Learning: The `date -r <file>` command is supported on both Linux and macOS to get file modification times, making it portable across these platforms.
✅ Actions performedReviews paused. |
Co-authored-by: Bingran Hu <[email protected]>
exports/taskfiles/utils/boost.yaml
Outdated
| # Directory parameters | ||
| BUILD_DIR: "{{.WORK_DIR}}/boost-build" | ||
| INSTALL_PREFIX: "{{.WORK_DIR}}/boost-install" | ||
| SOURCE_DIR: "{{.WORK_DIR}}/boost-src" |
There was a problem hiding this comment.
| SOURCE_DIR: "{{.WORK_DIR}}/boost-src" | |
| EXTRACTION_DIR: "{{.WORK_DIR}}/boost-extracted" |
if we want to keep it consistent among libs
Co-authored-by: Bingran Hu <[email protected]> Co-authored-by: Junhao Liao <[email protected]>
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
davidlion
changed the title
2 participants
Description
(Depends on #81.)
Previously, we avoided using taskfile
sourcesandgenerateswith build systems (CMake and Boost's b2) and deferred to them to decide what work to repeat. This has been somewhat acceptable for CMake projects, but causes long build times for projects using Boost.This PR modifies the Boost tasks to use the existing checksum tasks to mark successful states of the build process, allowing future builds to skip running unnecessary tasks.
In addition it makes a few small changes:
Checklist
breaking change.
Validation performed
Tested locally using the Boost unit tests and ystdlib-cpp by deleting certain fails and observing what tasks ran.
Added some unit tests for the checksum tasks to verify behaviour.
Summary by CodeRabbit