Skip to content

Conversation

@UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Dec 12, 2025

Issue being fixed or feature implemented

  • CI in feature branches creates its own caches consuming GH limits which constantly forces develop caches to be removed making it unusable in forked repos (creating feature branches in a forked repo is a part of creating PRs in this repo)
  • We create a tiny empty ccache cache for linux64_fuzz
  • We cache the wrong folder 🤷‍♂️

What was done?

See individual commits

How Has This Been Tested?

Run CI for a feature branch in a forked repo with this branch merged into forked repo's develop. CI should use ccache caches created by develop, should not create its own ones anymore. CI for develop should not create tiny empty caches.

Breaking Changes

Not breaking but a tradeoff: no ccache cache if your feature branch has changes in 'contrib/containers/ci/ci.Dockerfile' or 'depends/packages/*'.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@UdjinM6 UdjinM6 added this to the 23.1 milestone Dec 12, 2025
@github-actions
Copy link

github-actions bot commented Dec 12, 2025

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link

coderabbitai bot commented Dec 12, 2025

Walkthrough

The workflow build-src.yml refactors ccache handling: an explicit "Manage ccache" step was replaced by an early actions/cache/restore@v4 step restoring /cache/ccache (key based on Dockerfile hash, depends/packages, build-target, commit SHA) and a conditional actions/cache/save@v4 step that saves /cache/ccache only on pushes to the default branch, with save skipped for linux64_fuzz. Additionally, ci/test/00_setup_env_native_fuzz.sh had --disable-ccache removed from BITCOIN_CONFIG so ccache is no longer explicitly disabled in that script.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Files to check closely:
    • .github/workflows/build-src.yml — verify cache restore/save keys, ordering, and conditional expressions.
    • ci/test/00_setup_env_native_fuzz.sh — ensure removing --disable-ccache doesn't affect CI reproducibility or sanitizer/fuzzer invocation.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'ci: improve ccache cache' directly aligns with the main changes: improved ccache caching strategy in CI workflows through conditional save logic and config adjustments.
Description check ✅ Passed The description is related to the changeset, explaining the motivation (CI caching issues in feature branches) and what was changed, though implementation details are deferred to individual commits.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e4a69ffc0f5f460d91b4b5042eeb14072bc49546 and 0f51d9c.

📒 Files selected for processing (2)
  • .github/workflows/build-src.yml (2 hunks)
  • ci/test/00_setup_env_native_fuzz.sh (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:

  • ci/test/00_setup_env_native_fuzz.sh
  • .github/workflows/build-src.yml
🧠 Learnings (1)
📓 Common learnings
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.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
⏰ 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: linux64_multiprocess-build / Build source
  • GitHub Check: linux64_fuzz-build / Build source
  • GitHub Check: linux64_ubsan-build / Build source
  • GitHub Check: linux64-build / Build source
  • GitHub Check: linux64_sqlite-build / Build source
  • GitHub Check: linux64_nowallet-build / Build source
  • GitHub Check: mac-build / Build source
  • GitHub Check: arm-linux-build / Build source
  • GitHub Check: win64-build / Build source
  • GitHub Check: linux64_tsan-build / Build source
🔇 Additional comments (3)
ci/test/00_setup_env_native_fuzz.sh (1)

18-18: LGTM! Enabling ccache for fuzz builds.

The removal of --disable-ccache aligns with the PR objective to fix the tiny empty ccache issue for linux64_fuzz. Based on past review comments, this change was tested and confirmed working, and is consistent with upstream Bitcoin's configuration.

.github/workflows/build-src.yml (2)

82-90: LGTM! Ccache restore step correctly placed.

The restore step executes before the build, ensuring ccache is available when needed. The key structure appropriately invalidates cache when dependencies or Dockerfile change.


104-113: Verify: Is the save skip for linux64_fuzz intentional or missing?

The conditional save to default branch only addresses the PR objective about feature branches not creating caches. However, the AI summary states "save skipped for linux64_fuzz," but no such condition exists in the code.

If the intent is to skip saving for linux64_fuzz (as mentioned in the PR description about stopping tiny empty cache creation), the condition should include:

if: |
  github.event_name == 'push' &&
  github.ref_name == github.event.repository.default_branch &&
  inputs.build-target != 'linux64_fuzz'

Please confirm whether skipping save for linux64_fuzz is intended.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@UdjinM6 UdjinM6 changed the title ci: improve cache ci: improve ccache cache Dec 12, 2025
@UdjinM6 UdjinM6 marked this pull request as draft December 12, 2025 15:28
@@ -80,6 +80,7 @@ jobs:
shell: bash

- name: Restore ccache cache
if: inputs.build-target != 'linux64_fuzz'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does the fuzz build disable ccache?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I have no idea tbh, I thought it was maybe conflicting somehow or wasn't useful. It was like that since it was originally introduced in #4732. @kwvg any hints?

Copy link
Author

@UdjinM6 UdjinM6 Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping --disable-clang and running it twice - 100% hit rate, don't see any issues (though we don't really test CI fuzz builds in any way).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@UdjinM6 no objections to enabling ccache, it is enabled upstream (source).

it might have been due to local issues (i remember ccache being a bit finicky back when i used it so it's likely i had disabled it when working on the PR and then kept it in the final iteration as it was known-good) but since CI is happy, no reason not to use it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped b8c95ee00ad10b40787bfab7cf02bbab6535db47 and added 0f51d9c

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally concept ACK

@UdjinM6 UdjinM6 marked this pull request as ready for review December 12, 2025 16:52
@UdjinM6 UdjinM6 requested review from knst and kwvg December 15, 2025 15:40
Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 0f51d9c

@PastaPastaPasta PastaPastaPasta merged commit 4ab650b into dashpay:develop Dec 16, 2025
41 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants