-
Notifications
You must be signed in to change notification settings - Fork 24
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
ROX-26272 Enable ccache for faster builds #1838
base: master
Are you sure you want to change the base?
Conversation
bdb810b
to
2231b2f
Compare
6e4c460
to
17ba285
Compare
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.
Just some minor comments, but overall looking great!
# create a wrapper utility for clang-17 used by modern bpf builds in falco | ||
printf '#!/bin/sh\nexec ccache /usr/bin/clang-17 "$@"\n' > /usr/local/bin/ccache-clang | ||
chmod +x /usr/local/bin/ccache-clang | ||
echo /usr/local/bin/ccache-clang |
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.
Side note, after we pull this change we shouldn't need this wrapper anymore: falcosecurity/libs#2032
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.
Nice! I'll add a note to remind us to remove
git clone https://github.com/ccache/ccache.git third_party/ccache | ||
|
||
cd third_party/ccache | ||
git fetch --tags | ||
git checkout "${CCACHE_VERSION}" |
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.
We could use a submodule checked out to CCACHE_VERSION
, but since it won't be used for konflux or downstream, this is fine as well.
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.
Yeah I made the same calculation. I don't feel that strongly about it.
- name: Set up builder ccache in docker cache | ||
if: env.USE_CCACHE && matrix.arch != 's390x' | ||
uses: reproducible-containers/[email protected] | ||
with: | ||
cache-map: | | ||
{ | ||
"${{ github.workspace }}/builder/.ccache": "/root/.ccache" | ||
} |
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.
Since you had to implement the logic that this action provides for s390x, I wonder if it makes sense to just handle all archs the same way in our ansible playbooks and drop this step, mostly for consistency, this approach is fine.
@@ -17,6 +17,7 @@ env: | |||
COLLECTOR_TAG: ${{ inputs.collector-tag }} | |||
DEFAULT_BUILDER_TAG: master | |||
ANSIBLE_CONFIG: ${{ github.workspace }}/ansible/ansible.cfg | |||
USE_CCACHE: ${{ !contains(github.event.pull_request.labels.*.name, 'no-ccache') }} |
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.
Do we want to run with ccache on the master branch and nightlies? That way PRs could just pull the cache from there and see a speed up from the very first commit that gets pushed, otherwise the first build will be as slow as the ones we have now. Maybe we can check if the branch being used is master
somewhere in our build and clean the cache before building? That way the master branch would always be built from a clean slate, but the cache would still be provided for PRs.
USE_CCACHE: ${{ !contains(github.event.pull_request.labels.*.name, 'no-ccache') }} | |
USE_CCACHE: ${{ github.event_name != 'pull_request' || !contains(github.event.pull_request.labels.*.name, 'no-ccache') }} |
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.
Ah good idea. We could skip cache restore on master but always save the cache.
0116bf7 ccache 7fe4a87 typo 4ba08ca ansible format 31fb575 add .ccache to .gitignore f5e5fd3 ccache on builder image 4c75f3a debug logs for builder 3412c94 create logs 29778a0 typo b22b325 print 3ba1265 key a414bb1 remove debug d12f47b enable s390x caching 5578333 typo 10b4a40 s390x 9c4ffa1 skip docker inject action on s390x 66d4e7f fix path 4a58a89 builtin f16c110 fix deda8ba format 23d5ed4 var ef8e17a order 65b1816 fmt 2378492 simplify 89c69c6 scripts 4170881 version 1b191f8 path be6e04b task time b8f61ba quote 30382e5 cache file for remote vm b8c982b fmt 15c70af var a2f148b update a927675 check dir a77bd93 enable builder, s390x collector 0ba7d0a typo 664e31a dbg 28ace91 broken cache f822219 ls 9d5ccdf rm 8ce9e57 slash e80f8f5 slash 1482ea1 permissions and fix builder ccache 3fb4a1a s390x 4ee116e s390x 0887d1d root 6bb2700 dbg b9e8ed1 Revert "dbg" ab60dff rm opts c85e21e collector bdb810b no ccache avail
optimize git on s390x
5e622ca
to
4f69acf
Compare
Description
no-ccache
label on PRs.Total speed ~3x comparing recent runs for
Main Collector CI
(~3-4hrs to ~1.5hr). The bulk coming from the very slow buildx builds. The running times below are from representative builds (not avg) and show the time for the the entire job, including set up and teardown.Prior art from @Molter73
Checklist
Automated testing
Testing Performed
Inspected CI.