ci: better depends caching #7029
Conversation
|
WalkthroughThe PR restructures CI workflows: Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 0
🧹 Nitpick comments (1)
.github/workflows/build-depends.yml (1)
46-59: Ensure cache key computation is deterministic and complete.The cache key combines
BUILD_TARGET, computedDEP_HASH(fromBUILD_TARGET,DEP_OPTS,HOST),DOCKERFILE_HASH, andPACKAGES_HASH. However, theDEP_HASHappears to redundantly includeBUILD_TARGETwhen the key already includes it. Additionally, verify that all environment-specific variables that affect the build are captured.Consider simplifying line 53 to avoid redundancy:
- DEP_HASH="$(echo -n "${BUILD_TARGET}" "${DEP_OPTS}" "${HOST}" | sha256sum | head -c 64)" + DEP_HASH="$(echo -n "${DEP_OPTS}" "${HOST}" | sha256sum | head -c 64)"However, this is only if
BUILD_TARGETdoes not influenceDEP_OPTSorHOSTin ways not already captured by those values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build-depends.yml(1 hunks).github/workflows/build-src.yml(2 hunks).github/workflows/build.yml(17 hunks).github/workflows/cache-depends-sources.yml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{guix-build*,releases,**/guix-build*,releases/**,.github/**,depends/**,ci/**,contrib/**,doc/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not make changes to build system files (guix-build*), release artifacts, or avoid changes to .github, depends, ci, contrib, and doc directories unless specifically prompted
Files:
.github/workflows/cache-depends-sources.yml.github/workflows/build.yml.github/workflows/build-depends.yml.github/workflows/build-src.yml
🔇 Additional comments (15)
.github/workflows/build-depends.yml (4)
62-68: Verify cache lookup-only step properly exposes cache-hit status.The cache-check step uses
lookup-only: true, which should prevent unnecessary cache saves. Ensure that thecache-hitoutput is correctly referenced in the build job's conditional on line 73.The approach is sound: check-cache performs a lookup-only cache check and exposes
cache-hitstatus, allowing the build job to skip when the cache exists.
70-74: Verify conditional logic prevents cache misses from causing workflow failure.The build job only runs when
needs.check-cache.outputs.cache-hit != 'true'. If the cache check job fails (not just a cache miss), the build job will not run. Verify that a failed cache-check step does not silently skip the build.Ensure that if
check-cachejob fails (e.g., step execution error), the workflow does not proceed with an implicitcache-hit=false. GitHub Actions should fail the build job's condition, but this should be explicitly verified in test runs.
99-106: Verify restore-keys fallback is sufficient for partial cache matches.The Restore cached depends step includes a fallback
restore-keysthat matches based on Dockerfile and build-target. However, ifDEP_OPTSchanges, the fallback will restore a potentially incompatible build. Consider whether this partial restore is acceptable or if the build should fail-fast on cache miss.Confirm that restoring a cache with mismatched
DEP_OPTSdoes not cause build failures downstream inbuild-src.yml.
39-44: Verify sparse-checkout includes all matrix.sh dependencies.The sparse-checkout only retrieves
ci/dash,ci/test,depends/packages,depends/hosts, andcontrib/containers/ci/ci.Dockerfile. Ifmatrix.shsources or includes files outside these paths (e.g., additional scripts inci/or other environment-specific configs), the cache key computation could produce incomplete hashes and lead to incorrect cache hits..github/workflows/cache-depends-sources.yml (4)
5-7: Verify cron syntax and timezone interpretation.The cron schedule
'0 6 * * *'runs daily at 06:00 UTC. GitHub Actions interprets cron in UTC by default, so this should be correct. However, document this clearly if it's intentional, as users may misunderstand the schedule.The cron syntax is valid and will execute daily at 06:00 UTC as intended.
12-12: Clarify rationale for ARM-based runner (ubuntu-24.04-arm).The workflow uses an ARM-based Ubuntu runner. If the depends sources are architecture-independent (which is typical for source archives), using a standard x86 runner would be more efficient and likely faster. If ARM is intentional (e.g., to pre-build architecture-specific sources), this decision should be documented.
Confirm whether
make -C depends downloadproduces architecture-specific outputs that benefit from running on ARM, or if a standard runner would be more cost-effective.
14-17: Verify checkout ref fallback behavior in scheduled runs.Line 17 uses
${{ github.event.pull_request.head.sha || github.sha }}. In scheduled runs (not PRs),github.event.pull_request.head.shawill be empty, so it falls back togithub.sha(the default branch tip). This is correct behavior, but verify that scheduled runs are intended to cache sources for the default branch.The ref fallback is correct: scheduled runs will use the default branch tip, and manual calls via workflow_call can override if needed.
28-37: Ensure download step failures are propagated.The Download sources step (line 30) runs only if cache is missed but has no explicit error handling. If
make -C depends downloadfails, the workflow will fail, which is correct. However, verify that partial downloads are cleaned up to avoid poisoning the cache.Confirm that
make -C depends downloadeither succeeds completely or leaves depends/sources in a clean state. If partial downloads are possible, add a cleanup step on failure..github/workflows/build-src.yml (3)
71-71: Verify depends-host path matches the cache key path from build-depends.yml.Line 71 restores from
depends/built/${{ inputs.depends-host }}. Ensure this exactly matches the save path inbuild-depends.yml(line 118:depends/built/${{ needs.check-cache.outputs.host }}).Path alignment looks correct: both use the host triplet to construct the depends/built path.
75-80: Carefully handle depends-dep-opts expansion in make command.Line 79 uses
env ${{ inputs.depends-dep-opts }} make .... Ifdepends-dep-optsis empty (the default), this expands toenv make ...(with extra space), which is harmless. However, ifdepends-dep-optscontains environment variable assignments (e.g.,FOO=bar BAR=baz), theenvcommand will interpret them correctly. Verify that this is the intended behavior and that depends-dep-opts is always a valid environment variable assignment string or empty.Confirm the format and expected content of
depends-dep-opts. If it can contain special characters or spaces, consider adding quoting or validation to prevent command injection.
18-26: Verify depends inputs are correctly validated and propagated across workflow chain.Two new required/optional inputs (
depends-hostanddepends-dep-opts) are added to control the build prefix alignment. Ensure that:
- All callers of this workflow in
build.ymlprovide these inputs correctly- Values match those used in
build-depends.yml- The
env ${{ inputs.depends-dep-opts }} makesyntax on line 79 correctly handles empty or special characters independs-dep-opts.github/workflows/build.yml (4)
40-44: Verify cache-sources job is critical path dependency.The new
cache-sourcesjob runs daily to warm the depends/sources cache. Alldepends-*jobs now depend on it (lines 69, 78, etc.). This adds a sequential dependency that delays all depends builds until cache-sources completes. Verify that this delay is acceptable and that cache-sources job failures should block the entire CI pipeline.Confirm whether cache-sources should be a hard dependency (blocking if it fails) or if its failure should be non-blocking (best-effort). If it's best-effort, consider using a separate workflow trigger or allowing soft failures.
69-69: Verify that cache-sources job outputs are actually consumed by depends- jobs.*Lines 69, 78, 91, 102, 111, 120 all add
cache-sourcesto theneedslist for platform-specific depends builds. Confirm whether the depends-* jobs referencecache-sourcesoutputs (e.g., vianeeds.cache-sources.outputs.*) or if this dependency is only for job sequencing. If outputs are not consumed, the dependency should be removed to avoid unnecessary job ordering constraints.
141-142: Verify all src- jobs receive depends-host and depends-dep-opts from their corresponding depends- jobs.**Ensure that each
src-*job references the correctdepends-*job in itsneedsfield, and that the output variable names (depends-hostanddepends-dep-opts) match exactly across all instances at lines 141-142, 153-154, 165-166, 177-178, 188-189, 200-201, 212-213, 224-225, 235-236, and 246-247.
1-50: DAG verified as acyclic and correct.The workflow dependency graph is properly structured with no circular dependencies. The cache-sources → depends-* → build-src-* → test-* → summary → build-docker chain follows correct data flow, with independent parallel builds for each architecture (amd64, arm64, i686, x86_64) that converge at the summary job.
|
This pull request has conflicts, please rebase. |
… we have a cache hit
PastaPastaPasta
force-pushed
the
ci/better-depends-caching
branch
from
7444565 to
5c10d85
Compare
.github/workflows/build-depends.yml
Outdated
| echo "DEP_HASH=${DEP_HASH}" >> "${GITHUB_OUTPUT}" | ||
| DOCKERFILE_HASH="${{ hashFiles('contrib/containers/ci/ci.Dockerfile', 'contrib/containers/ci/ci-slim.Dockerfile') }}" | ||
| PACKAGES_HASH="${{ hashFiles('depends/packages/*') }}" |
There was a problem hiding this comment.
should add depends/Makefile too. It is changed rarely, but it happens couple times each year
There was a problem hiding this comment.
should be this file .github/workflows/build-depends.yml also be part of hash?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/build-depends.yml (1)
39-46: Remove unusedci/testfrom sparse-checkout.The sparse-checkout list at line 41 includes
ci/test, but this directory is not referenced or sourced anywhere in the workflow. Onlyci/dash/matrix.shis used (line 52).Remove line 41 to reduce checkout size:
sparse-checkout: | ci/dash - ci/test depends/Makefile
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-depends.yml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{guix-build*,releases,**/guix-build*,releases/**,.github/**,depends/**,ci/**,contrib/**,doc/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not make changes to build system files (guix-build*), release artifacts, or avoid changes to .github, depends, ci, contrib, and doc directories unless specifically prompted
Files:
.github/workflows/build-depends.yml
🧠 Learnings (7)
📚 Learning: 2025-11-20T20:03:59.701Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6987
File: contrib/containers/ci/ci-slim.Dockerfile:126-126
Timestamp: 2025-11-20T20:03:59.701Z
Learning: In contrib/containers/ci/ci-slim.Dockerfile, the group handling logic should follow this pattern: if the target GROUP_ID exists, change the ubuntu user's primary group to that GID using `usermod -g ${GROUP_ID} ubuntu`; if the GID doesn't exist, change the existing ubuntu group's GID to the target and rename it to "dash" using `groupmod -g ${GROUP_ID} -n dash ubuntu`. The complete command is: `(getent group ${GROUP_ID} && usermod -g ${GROUP_ID} ubuntu) || groupmod -g ${GROUP_ID} -n dash ubuntu`.
Applied to files:
.github/workflows/build-depends.yml
📚 Learning: 2025-02-06T14:34:30.466Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Applied to files:
.github/workflows/build-depends.yml
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
.github/workflows/build-depends.yml
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
.github/workflows/build-depends.yml
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Dash extends Bitcoin Core through composition with minimal changes to the Bitcoin Core foundation
Applied to files:
.github/workflows/build-depends.yml
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
.github/workflows/build-depends.yml
📚 Learning: 2025-07-23T09:30:34.631Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Applied to files:
.github/workflows/build-depends.yml
⏰ 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). (10)
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: Lint / Run linters
| source ./ci/dash/matrix.sh | ||
| echo "DEP_OPTS=${DEP_OPTS}" >> "${GITHUB_OUTPUT}" | ||
| echo "HOST=${HOST}" >> "${GITHUB_OUTPUT}" | ||
| DEP_HASH="$(echo -n "${BUILD_TARGET}" "${DEP_OPTS}" "${HOST}" | sha256sum | head -c 64)" |
There was a problem hiding this comment.
Potential hash collision risk from space-separated concatenation.
The echo -n command at line 55 concatenates BUILD_TARGET, DEP_OPTS, and HOST with spaces between them, which could produce identical strings from different inputs. For example, if BUILD_TARGET contains a space, the boundary between variables becomes ambiguous:
BUILD_TARGET="a b" DEP_OPTS="c"→"a b c"BUILD_TARGET="a" DEP_OPTS="b c"→"a b c"(collision)
To ensure deterministic and collision-free hashing, use explicit separators between variables.
Consider using explicit delimiters:
- DEP_HASH="$(echo -n "${BUILD_TARGET}" "${DEP_OPTS}" "${HOST}" | sha256sum | head -c 64)"
+ DEP_HASH="$(echo -n "${BUILD_TARGET}:${DEP_OPTS}:${HOST}" | sha256sum | head -c 64)"📝 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.
| DEP_HASH="$(echo -n "${BUILD_TARGET}" "${DEP_OPTS}" "${HOST}" | sha256sum | head -c 64)" | |
| DEP_HASH="$(echo -n "${BUILD_TARGET}:${DEP_OPTS}:${HOST}" | sha256sum | head -c 64)" |
🤖 Prompt for AI Agents
.github/workflows/build-depends.yml around line 55: the current echo -n
concatenation with spaces can produce ambiguous inputs and hash collisions when
variables contain spaces; change the concatenation to use an unambiguous
separator or fixed-length encoding (for example, use printf to output each
variable with a NUL or other explicit delimiter between them, or prefix each
value with its length) before piping to sha256sum so the resulting hash is
deterministic and collision-free.
There was a problem hiding this comment.
This shouldn't happen because BUILD_TARGET, DEP_OPTS and HOST (in that specific order) can have no common parts I think.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Examine the ci/dash/matrix.sh file to understand possible values for BUILD_TARGET, DEP_OPTS, and HOST
cat ci/dash/matrix.sh
Length of output: 2040
🏁 Script executed:
#!/bin/bash
# Check a few setup files to see what HOST and DEP_OPTS values look like
echo "=== arm-linux setup ==="
grep -E '(HOST=|DEP_OPTS=)' ci/test/00_setup_env_arm.sh | head -5
echo -e "\n=== linux64 setup ==="
grep -E '(HOST=|DEP_OPTS=)' ci/test/00_setup_env_native_qt5.sh | head -5
echo -e "\n=== mac setup ==="
grep -E '(HOST=|DEP_OPTS=)' ci/test/00_setup_env_mac.sh | head -5
echo -e "\n=== win64 setup ==="
grep -E '(HOST=|DEP_OPTS=)' ci/test/00_setup_env_win64.sh | head -5
echo -e "\n=== Check if DEP_OPTS is ever set with spaces in values ==="
rg 'DEP_OPTS=' ci/test/ -A 1
Length of output: 1657
@UdjinM6, you're absolutely right. After examining the actual values from ci/dash/matrix.sh and the setup files:
- BUILD_TARGET: Fixed set of identifiers like
linux64,mac,win64,arm-linux(no spaces) - HOST: Standard triplets like
x86_64-pc-linux-gnu,x86_64-apple-darwin - DEP_OPTS: Build options like
NO_UPNP=1 DEBUG=1orCC=clang-19 CXX='clang++-19 -stdlib=libc++'
These three variables have completely distinct formats that don't overlap. A BUILD_TARGET will never contain values that look like DEP_OPTS, and DEP_OPTS won't look like a HOST triplet. Given these domain constraints, the collision scenario I described isn't practically possible here.
My concern was theoretically valid for arbitrary strings, but your understanding of the actual values in this codebase is correct—no collision can occur. Thanks for the clarification!
🧠 Learnings used
Learnt from: kwvg
Repo: dashpay/dash PR: 6421
File: .github/workflows/build-depends.yml:80-85
Timestamp: 2025-02-17T10:21:55.551Z
Learning: In GitHub Actions workflow files, when using the `env` command with `DEP_OPTS` or similar variables containing space-delimited key-value pairs for environment variables, word splitting is desired behavior and the variables should not be quoted.
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
utACK 6f7f799
Could apply #7029 (comment) to be on the safe side but the issue is theoretical so up to you :)
3 participants
Issue being fixed or feature implemented
depends on another
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.