-
Notifications
You must be signed in to change notification settings - Fork 128
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
Use ccache in github actions builds #3705
Use ccache in github actions builds #3705
Conversation
e5222fb
to
606e59e
Compare
@scottwittenburg let me know when you need review, I was asked to review automatically since I was listed in the codeowner. |
I don't see it halting the feature. We can disable it on windows for now, and as you suggest, come back to it later. Also, as an alternate option, I think we could leave it enabled, even though there's no benefit currently, since the cost is low (based on the times listed in the action log, only a few seconds to install Do you have a preference? I can see pros/cons both ways, and am interested in your thoughts. |
Disable it in Windows, using ccache we always incur in a overhead, cache miss + build TU is worse than build TU. |
139b86e
to
78dd8a7
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.
Great that this is working! Thanks a lot! lets try to get rid of that gh action since we could replace it by a few env var exports.
@vicentebolea Please take a look when you get a chance. If you don't have any more suggestions, I will just rewrite the history into two commits, and then we can merge. |
296fe28
to
8562885
Compare
@scottwittenburg I just rebased your branch so that it contains the latest commit that "fixes" the tests failures. |
8562885
to
65e7807
Compare
It seems intel recently came out with the 2023.2.0 versions of these packages, and those were picked with the most recent build. When the newest versions were chosen, we could no longer load the "icc" or "compiler" modules, so pin to the previous working version.
65e7807
to
af83421
Compare
Cache was not saved on any of the runs on In my PR testing, I exercised half of this conditional before adding the additional criteria So I'm not sure yet why it didn't save cache. Here it says that the |
That is odd, I guess that we need to merge it to check it |
Add an action available on the marketplace to GithubActions builds, which simplifies using
ccache
to speed up compilation. The table below shows the speed up for some of the affected builds (note: times are build times only, and do not include any of the other steps). On the first run, the cache was empty, so there was no benefit at all. On the second run, the cache hit rates were all around 100%, since I didn't change any source files in between.I pre-installed
ccache
in builds where we control the image being used, butccache-action
installs it if it doesn't exist.As a follow-on, or even in this PR, we could consider using dependabot to keep this action (and any others we use, eg
checkout
,upload-artifact
, etc) up to date. See here.