-
Notifications
You must be signed in to change notification settings - Fork 1.6k
ci: Use ccache to cache build objects for speeding up building #6104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6104 +/- ##
=======================================
Coverage 79.1% 79.1%
=======================================
Files 836 836
Lines 71245 71245
Branches 8321 8318 -3
=======================================
+ Hits 56353 56354 +1
+ Misses 14892 14891 -1 🚀 New features to boost your workflow:
|
That's not true that ccache dir is outside of the working directory |
|
Also, please take a look at Clio's implementation; many of the features can be implemented here as well:
|
Logging into our macOS GitHub runner shows that the ccache dir is the user's |
I reluctantly moved the ccache CMake changes back into a separate file, simply for similarity with Clio. However, I don't see in what way this helps keep things more modular, since in my view it just increases the number of files in the directory (albeit by just 1) and it moves 5 lines of compiler-related statements into a different file than where all the compiler-related definitions are stored. I further looked at the ccache set up in Clio and noticed that upload+download are always enabled together, and generally not enabled for pushes into the develop and release branches or the nightly run; I simplified this by just only enabling ccache for PR commits. This also avoids me having to pass variables between multiple levels of actions. Note that I use the Also note that sanitizers are not yet enabled, but from the sanitizers PR I recall that they will also be part of the config name. However, @mathbunnyru, I noticed you completely disabled ccache for sanitizers - what's the motivation for doing so? Finally, what's the motivation for not using ccache for commits into the develop or release branches? It is just to be extra safe or is there an actual known risk with ccache not producing the correct build objects? |
Alright, I see now that the |
|
@mathbunnyru @vlntb This PR is now ready, and ccache now works on all platforms. |
| - name: Restore ccache | ||
| if: ${{ env.CCACHE_ENABLED == 'true' }} | ||
| uses: actions/cache/restore@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.3.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You always download cache, add files to it (while building new code), and then upload a bigger cache.
This will eventually grow the cache size, and make it useless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I had hoped it would be more like Bazel/BuildBuddy where the least recently used files would be dropped automatically once a certain time/size limit was reached, but here the caches are considered as a whole unit and dropped entirely.
My original idea was to use ccache for caching across PRs, so unchanged files wouldn't need to be built again. It seems like that is not possible with the current set up. Should a separate cache be used per PR?
Per actions/cache#1071, one comment states "This functioning is expected and cache is built on the principle that "each key uniquely identifies a cache". if a cache contents have changed and you need to update it, its key should also reflect that.". So would this mean that each commit that modifies some source files needs its own cache? That entirely defeats the purpose...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should a separate cache be used per PR?
I don't know if that would work well in rippled's case, because it depends on how active people are pushing their PRs.
I think if there are many active PRs and they override the same cache, it's rarely going to work.
Clio doesn't create caches on PRs, only on commits on develop.
This also solves the problem with fork/non-fork.
And we add a commit hash to the cache id.
| - name: Show ccache statistics before building | ||
| if: ${{ env.CCACHE_ENABLED == 'true' }} | ||
| run: ccache --show-stats ${{ runner.debug && '-vv' || '' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will all be zeroes and only show ccache size limit, so I don't think it's needed and gives useful information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will still show the size of the cache, even though the hits and misses are not shown (because they are zero), e.g.
Local storage:
Cache size (GB): 0.43 / 5.00 ( 8.64%)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This information is also shown after the build finishes (with new files included), and on previous upload (without them), that's why it's not that needed
| - name: Save ccache | ||
| if: ${{ env.CCACHE_ENABLED == 'true' }} | ||
| uses: actions/cache/save@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.3.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work when PR is created from fork?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes:
- Everyone has 10GB by default.
- Even if writing fails, the step succeeds. This is apparently a feature (although several developers have filed tickets to complain that they'd like the step to fail the pipeline if saving doesn't work).
Now, if you think it'd be better to not enable this by default in forks, I can make the change. I'm fine either way.
| uses: actions/cache/save@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.3.0 | ||
| with: | ||
| path: ${{ env.CCACHE_DIR }} | ||
| key: ${{ inputs.config_name }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will happen if 2 PRs run simultaneously and write to the same cache key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that the first one to attempt the write will succeed, while the second one will fail to write with the "Unable to reserve cache with key ..., another job may be creating this cache" message.
cmake/Ccache.cmake
Outdated
| find_program(CCACHE_PATH "ccache") | ||
| if (CCACHE_PATH) | ||
| if (MSVC) | ||
| # Chocolatey uses a shim executable that we cannot use directly, in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who installed ccache on Windows?
Is it part of deployment scripts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's part of the Terraform script that deploys the Windows runners, using chocolatey to install ccache. Unfortunately it cannot be installed via pip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we do it in prepare-runner?
The same way we use brew for Mac, we'll be using cholocatey for Windows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in XRPLF/actions#42.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to update the usage here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will in a follow-up PR.
I'm currently first considering taking advantage of ccache supporting Redis as a remote cache, so I can rip out the largely useless GitHub cache.
High Level Overview of Change
This change enables caching of build objects using
ccacheon Linux, macOS, and Windows.Context of Change
Right now, each pipeline invocation builds the source code from scratch. Although compiled Conan dependencies are cached in a remote server, the source build objects are not. We are able to further speed up our builds by leveraging
ccache.Type of Change
.gitignore, formatting, dropping support for older tooling)