Externalise cache keys to prevent unneeded invalidation - #3922
Conversation
fdae3db to
4b51804
Compare
softins
left a comment
There was a problem hiding this comment.
Looks ok in general, just a couple of comments.
73de281 to
3f7be01
Compare
|
Well, it's not quite there, is it.... |
|
🤖 AI: The four former |
48e4892 to
081b45d
Compare
54278a8 to
d364a54
Compare
|
Getting there -- the caches are being made and restored. Windows and macOS don't realise this - they go off and install again... iOS gets it right. Bump dependencies is working, too. |
d7b1d7b to
c411a51
Compare
|
Right - all tests now passing:
|
| @@ -0,0 +1,6 @@ | |||
| #!/bin/bash | |||
|
|
|||
There was a problem hiding this comment.
This then dropped the comment about pinning?
| PROJECT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" | ||
| readonly PROJECT_DIR | ||
| # shellcheck disable=SC1090 | ||
| source "${PROJECT_DIR}/.github/autobuild/mac-dependencies_qt${QT}.sh" |
There was a problem hiding this comment.
Do we have a guarantee that ${QT} is safe and correct? I'd add an error check if the file exists before.
There was a problem hiding this comment.
It's set from the autobuild workflow file and, if not, defaults to 6 at the top of the file. If someone sets the wrong value, this command will safely crash the workflow.
There was a problem hiding this comment.
I'd still like a guard to reject invalid values.
| # Important: | ||
| # - Do not update ASIO SDK without checking for license-related changes. | ||
| # - Do not copy (parts of) the ASIO SDK into the Jamulus source tree without | ||
| # further consideration as it would make the license situation more complicated. |
There was a problem hiding this comment.
Probably outdated as they moved to the GPL.
There was a problem hiding this comment.
As it's version dependent (if the old version can still be found), I suppose it makes sense to keep it. If and when the old version vanishes, this can go, too.
There was a problem hiding this comment.
The old version will probably not vanish. But we can of course keep this.
|
I think we might have an issue about some OS not being cached. Worth checking the issues for that and linking it here. If the issue is iOS related, we can close it. |
|
The iOS caching issue was already documented. Thus: Fixes: #2462 |
| # The following version pinnings are semi-automatically checked for | ||
| # updates. Verify .github/workflows/bump-dependencies.yaml when changing those manually: |
There was a problem hiding this comment.
@pljones Maybe I am blind, but I think this comment got dropped?
There was a problem hiding this comment.
Oh, right. I should have put that on all the *-dependencies*.
Short description of changes
I noticed whilst I've been working on
autobuild.shthat every change invalidates all the caches. Becauseautobuild.shitself is included in the cache key calculation. This change replaces that mechanism on macOS and Windows with am externalised list of dependencies. It adds the same mechanism to iOS, which lacked caching.Github cache clean up will remove unused cache entries with no need to have the cache consumer included in the cache key.
(My work in progress on the Android build uses the same approach, which is where this comes from.)
Scope, guidance, testing and review by myself, coding my VSCode Github CoPilot.
CHANGELOG: Build: improve cache persistence
Context: Fixes an issue?
Caching should depend on what is being cached, rather than the cache consumer.
Does this change need documentation? What needs to be documented and how?
No.
Status of this Pull Request
Tested against the current branch in my own repository before raising the PR here.
What is missing until this pull request can be merged?
Need to do some more testing.
Checklist
AUTOBUILD: Please build all targets