-
Notifications
You must be signed in to change notification settings - Fork 952
build: always cache LLVM source/build even if the tests fail to avoid extra rebuilds #3453
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
Conversation
3f5d9df
to
ab99595
Compare
…to avoid extra rebuilds Signed-off-by: deadprogram <[email protected]>
ab99595
to
98f9e3b
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.
Yes yes yes! We had this in CircleCI but at the time it wasn't possible in GitHub Actions.
I don't think the path parameter is necessary for restore. Otherwise it looks good to me.
The example I looked at in the docs here did that with the path, so I copied it: https://github.com/actions/cache#using-a-combination-of-restore-and-save-actions |
Once I have this working correctly for Windows, I will do the same thing for the other targets in a subsequent PR, just so we can get the Windows builds unstuck. |
It doesn't include the cache here: https://github.com/actions/cache/tree/main/save#re-evaluate-cache-key-while-saving |
OK I will remove that in the later PR just to let the current build try to finish. |
ccd5d56
to
23d9983
Compare
@aykevl turns out you do need that:
|
23d9983
to
bcdcdf2
Compare
Signed-off-by: deadprogram <[email protected]>
bcdcdf2
to
40cfb86
Compare
Signed-off-by: deadprogram <[email protected]>
f1d1c2c
to
ba4553f
Compare
Yeah you are correct, I later found out the path is required. Which doesn't make a lot of sense to me to be honest (it wasn't required in CircleCI). |
@aykevl just wonder if we should either remove the LLVM source cache tasks, or remove this? tinygo/.github/workflows/windows.yml Lines 75 to 77 in ba4553f
Seems like something not quite correct here. |
I believe this is working as intended. The way it works now is as follows:
This provides quick cache restores in the common case when LLVM isn't built (the llvm-project directory is large so caching only a subset provides a significant speedup. But when LLVM needs to be rebuilt, this cached version isn't good enough. It's possible to also cache the full LLVM source tree but as it's only needed for LLVM builds it won't help CI times much. |
Signed-off-by: deadprogram <[email protected]>
ba4553f
to
e1b00c5
Compare
…M source and LLVM builds Signed-off-by: deadprogram <[email protected]>
09a0ee2
to
6f53911
Compare
Some notes:
|
6f53911
to
b0776b4
Compare
Finally passed, but seems like it fails more often then passes. |
- name: Save LLVM source cache | ||
uses: actions/cache/save@v3 | ||
if: steps.cache-llvm-source.outputs.cache-hit != 'true' | ||
with: | ||
key: ${{ steps.cache-llvm-source.outputs.cache-primary-key }} | ||
path: | | ||
llvm-project/clang/lib/Headers | ||
llvm-project/clang/include | ||
llvm-project/compiler-rt | ||
llvm-project/lld/include | ||
llvm-project/llvm/include |
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.
Why did you add this step?
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.
All of the LLVM source is cached and also all of the LLVM builds are cached for each of the build platforms which need it.
- name: Configure pagefile | ||
uses: al-cheb/[email protected] | ||
with: | ||
minimum-size: 8GB | ||
maximum-size: 24GB | ||
disk-root: "C:" |
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.
Was the page file too small? Or why did you add this?
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.
The idea is to give more virtual memory via pagefile to the Windows build instances, since they have unused space on C: drive. See https://github.com/easimon/maximize-build-space#how-it-works for the inspiration.
Did it actually help? I cannot say really.
… extra rebuilds (tinygo-org#3453) builds/macos, linux, windows: update to explicit restore/save for LLVM source and LLVM builds
… extra rebuilds (tinygo-org#3453) builds/macos, linux, windows: update to explicit restore/save for LLVM source and LLVM builds
… extra rebuilds (#3453) builds/macos, linux, windows: update to explicit restore/save for LLVM source and LLVM builds
… extra rebuilds (tinygo-org#3453) builds/macos, linux, windows: update to explicit restore/save for LLVM source and LLVM builds
This PR modifies the GH actions builds to always cache LLVM source/build even if the tests fail to avoid extra rebuilds.
See https://github.com/actions/cache/tree/main/save#always-save-cache for details