fix(build): Use checksum to skip mariadb-connector-cpp rebuild.
#1415
Conversation
WalkthroughAdds an internal task to download, extract and checksum-validate the MariaDB Connector/C++ tarball. Updates Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Runner as Task Runner
participant D as mariadb-connector-cpp-download-and-validate-checksum
participant M as mariadb-connector-cpp
participant FS as Filesystem
participant C as CMake/Build
participant CS as Checksum Utility
Runner->>D: invoke download & extract
D->>FS: download tarball
D->>FS: extract tarball
D->>CS: validate checksum
CS-->>D: OK / Fail
alt checksum OK
Runner->>M: run build (depends on D)
M->>FS: copy extracted sources -> SOURCE_DIR
M->>C: configure with SOURCE_DIR
C->>C: build & install
M->>CS: compute CHECKSUM_FILE (include-patterns)
CS-->>M: write CHECKSUM_FILE
M-->>Runner: expose CHECKSUM_FILE as source/generate
else checksum fail
D-->>Runner: abort with error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (5 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
taskfiles/deps/main.yaml (1)
414-462: Align CMake args and validate up-to-date semantics
- Add standard flags used elsewhere for consistency and quieter logs.
- Optional: Set CMP0135 NEW to avoid timestamp-based rebuild surprises.
- task: "yscope-dev-utils:cmake:generate" vars: BUILD_DIR: "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-build" EXTRA_ARGS: - "-DUSE_SYSTEM_INSTALLED_LIB=ON" - "-DINSTALL_LAYOUT=DEB" + - "-DCMAKE_BUILD_TYPE=Release" + - "-DCMAKE_INSTALL_MESSAGE=LAZY" + - "-DCMAKE_POLICY_DEFAULT_CMP0135=NEW" SOURCE_DIR: "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-src" ... - - task: "yscope-dev-utils:checksum:compute" + # This command must be last + - task: "yscope-dev-utils:checksum:compute" vars: CHECKSUM_FILE: "{{.CHECKSUM_FILE}}" INCLUDE_PATTERNS: *mariadb-cmake-include-patternsAlso, you declared the checksum file as both a source and a generate (Lines 428-431). This is clever for leveraging Task’s built-in up-to-date checks, but can create edge cases if the compute step updates the file’s mtime without content changes. Please confirm locally that:
- Re-running the parent flow after a no-op change truly skips this task, and
- The checksum compute does not cause a flip-flop where the file’s mtime triggers a rebuild on subsequent runs.
If any flakiness is observed, consider adding an early yscope-dev-utils:checksum:validate in this task too (before the copy/generate/build) to short-circuit commands deterministically, mirroring patterns used by antlr-runtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
taskfiles/deps/main.yaml(2 hunks)
⏰ 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). (12)
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: rust-checks (ubuntu-24.04)
| rm -rf "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-src" | ||
| cp -r "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-extracted" \ | ||
| "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-src" | ||
| - task: "yscope-dev-utils:cmake:generate" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Copy step is fine; consider rsync for robustness (optional)
Current rm/cp works. If large trees or hidden files become an issue, rsync reduces risk and handles deletions:
-rm -rf "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-src"
-cp -r "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-extracted" \
-"{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-src"
+rsync -a --delete "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-extracted/" \
+ "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-src/"🤖 Prompt for AI Agents
In taskfiles/deps/main.yaml around lines 438-441, the current remove-and-cp
sequence is fragile for large trees and hidden files; replace it with an
rsync-based copy so deletions and hidden files are handled robustly. Use rsync
in archive mode with delete semantics (preserve permissions, links, times, and
handle hidden files) from the extracted directory to the src directory, ensure
correct trailing-slash semantics on the source to copy contents not the parent
dir, and remove the preceding rm -rf so rsync alone manages updates and
deletions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
taskfiles/deps/main.yaml (1)
439-445: Use rsync instead of rm -rf + cp for robustness and correctness on updatesrsync handles deletions, permissions, and hidden files and avoids a full delete each run.
- - |- - rm -rf "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-src" - cp -r "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-extracted" \ - "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-src" + - |- + rsync -a --delete "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-extracted/" \ + "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-src/"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
taskfiles/deps/main.yaml(2 hunks)
⏰ 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). (13)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: package-image
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (2)
taskfiles/deps/main.yaml (2)
426-433: Resolved: CHECKSUM_FILE is now defined and wired; incremental config looks saneDefining CHECKSUM_FILE here and passing it to the subtask fixes the earlier undefined-var issue. sources/generates referencing the same file is acceptable for go-task’s up-to-date checks.
447-466: LGTM: CMake generate/build/install, and checksum compute referencing shared patternsBuild dirs, install prefix, and reuse of the include patterns via YAML anchor are consistent. No blocking issues.
There was a problem hiding this comment.
the logic looks reasonable to me though the changes look a bit verbose. I vaguely remember discussing a similar approach with @davidlion though i don't know if there's any convention we should follow / any example we can refer to.
@davidlion / @Bill-hbrhbr would you mind guiding / completing this review as I observe?
|
I think the current checksum system is flawed and most libraries rebuild themselves even when nothing has changed. The proper solution is to fix the checksum system used in There are also smaller problems such as I don't really get how the combined checksum file in |
|
@junhaoliao Just to make sure I understand the problem root cause: In summary, this PR seems to be too much effort to fix a single library while there's a more general and methodical solution. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
taskfiles/deps/main.yaml (3)
416-466: Order task attributes per contribution guide (consistency)Reorder attributes to match the documented convention (name, description, summary, dir, platforms, env, vars, requires, preconditions, sources, generates, run, deps, cmds, status).
398-404: Set TAR_FILE (+ pass CHECKSUM_FILE) so checksum tracks the tarball and skips download/extractWithout TAR_FILE, the tar path in INCLUDE_PATTERNS is never produced; passing CHECKSUM_FILE here aligns the helper’s caching.
- task: "yscope-dev-utils:remote:download-and-extract-tar" vars: FILE_SHA256: "0e3dfe9f2bc3f7bb6f7c159009556290064a7c23402ea08019fa8aebfc3ff2c9" + TAR_FILE: "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-extracted.tar.gz" + CHECKSUM_FILE: "{{.CHECKSUM_FILE}}" OUTPUT_DIR: "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-extracted" URL: "https://github.com/mariadb-corporation/mariadb-connector-cpp/archive/refs/tags/\ 1.1.5.tar.gz"
442-444: Prefer rsync over rm/cp for robustness (handles deletions, hidden files)Use rsync with trailing slash on source and --delete; drop the preceding rm.
- rm -rf "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-src" - cp -r "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-extracted" \ - "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-src" + rsync -a --delete "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-extracted/" \ + "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-src/"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
taskfiles/deps/main.yaml(2 hunks)
⏰ 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). (15)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: lint-check (macos-15)
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: rust-checks (macos-15)
|
@Bill-hbrhbr makes some good points. I now switch to only checksum the |
Fwiw y-scope/yscope-dev-utils#81 and y-scope/yscope-dev-utils#80 are directly what you're talking about, but have been needing reviewers for a while. Bill recently did a pass so hopefully we can land them soon. Since this is an optimization, I think it makes sense to have those 2 PRs land and do this properly. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
taskfiles/deps/main.yaml (1)
435-441: Use rsync instead of rm/cp for robustness (optional)Rsync handles deletions, permissions, and hidden files without a pre-emptive rm -rf.
- - |- - rm -rf "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-src" - cp -r "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-extracted" \ - "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-src" + - |- + rsync -a --delete "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-extracted/" \ + "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-src/"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
taskfiles/deps/main.yaml(1 hunks)
⏰ 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). (14)
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build (macos-15)
- GitHub Check: lint-check (macos-15)
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: rust-checks (macos-15)
🔇 Additional comments (1)
taskfiles/deps/main.yaml (1)
458-461: Align compute with validate (looks good; keep it this way)Using the same INCLUDE_PATTERNS anchor (install-only) for compute avoids false rebuilds. No change needed.
| mariadb-connector-cpp-download-and-validate-checksum: | ||
| internal: true | ||
| requires: | ||
| vars: ["CHECKSUM_FILE"] | ||
| cmds: | ||
| - task: "yscope-dev-utils:remote:download-and-extract-tar" | ||
| vars: | ||
| FILE_SHA256: "0e3dfe9f2bc3f7bb6f7c159009556290064a7c23402ea08019fa8aebfc3ff2c9" | ||
| OUTPUT_DIR: "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-extracted" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Validate before downloading; add run: once and stable TAR_FILE
Run checksum:validate first to bail out early and skip the whole download/extract when install is unchanged. Also mark the helper run: "once" and set a stable TAR_FILE (and forward CHECKSUM_FILE) to maximise caching.
mariadb-connector-cpp-download-and-validate-checksum:
internal: true
+ run: "once"
requires:
vars: ["CHECKSUM_FILE"]
cmds:
- - task: "yscope-dev-utils:remote:download-and-extract-tar"
- vars:
- FILE_SHA256: "0e3dfe9f2bc3f7bb6f7c159009556290064a7c23402ea08019fa8aebfc3ff2c9"
- OUTPUT_DIR: "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-extracted"
- URL: "https://github.com/mariadb-corporation/mariadb-connector-cpp/archive/refs/tags/\
- 1.1.5.tar.gz"
- # Uses checksum to skip mariadb-connector-cpp installation because the build updates the
- # source directory and will always rebuild.
- task: "yscope-dev-utils:checksum:validate"
vars:
CHECKSUM_FILE: "{{.CHECKSUM_FILE}}"
INCLUDE_PATTERNS: &mariadb-cmake-include-patterns
- "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-install"
+ - task: "yscope-dev-utils:remote:download-and-extract-tar"
+ vars:
+ CHECKSUM_FILE: "{{.CHECKSUM_FILE}}"
+ FILE_SHA256: "0e3dfe9f2bc3f7bb6f7c159009556290064a7c23402ea08019fa8aebfc3ff2c9"
+ OUTPUT_DIR: "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-extracted"
+ TAR_FILE: "{{.G_DEPS_CPP_DIR}}/mariadb-connector-cpp-extracted.tar.gz"
+ URL: "https://github.com/mariadb-corporation/mariadb-connector-cpp/archive/refs/tags/\
+ 1.1.5.tar.gz"Also applies to: 406-411
🤖 Prompt for AI Agents
In taskfiles/deps/main.yaml around lines 393 to 401 (and similarly lines 406 to
411), the download-and-extract helper is invoked unconditionally without running
checksum:validate first, without run: once, and without a stable TAR_FILE or
forwarding CHECKSUM_FILE; update the task entries to first run checksum:validate
to short-circuit when unchanged, add run: "once" to the helper invocation to
enable caching, set a stable TAR_FILE var pointing to the exact tarball name (so
cache keys remain stable), and forward the CHECKSUM_FILE variable into the
helper so it can validate against the provided checksum before
downloading/extracting.
4 participants
Description
Root cause
As discussed in #1409 and later tracked in #1412,
mariadb-connector-cpprebuilds every time because the build modifies the source tree, so in every run we re-extract the directory and the files are updated so the build reruns.Solution
This PR fixes #1412 by
yscope-dev-utilsto avoid re-executemariadb-connector-cpp.Checklist
breaking change.
Validation performed
task deps:spiderdoes not rebuiltmariadb-connector-cppandspider.Summary by CodeRabbit
Chores
Refactor