-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add support for adding to cache key #41
Add support for adding to cache key #41
Conversation
9ed8cf5
to
d4e5302
Compare
We run the build for amd64 and arm64, and we use `setup-rust-toolchain` to install and cache the Rust toolchain and project dependencies. The caching part of that action doesn't currently work if it's run in multiple jobs from the same workflow run. The cache key doesn't contain anything that would disambiguate and there is no input to `setup-rust-toolchain` to allow it to be customised. To fix that, I've just [submitted a PR][PR] to the action to allow the cache key to be customised. This will allow us to set a key that includes the OS and architecture of the build, so that the caches are kept separate. We're making use of that here. [PR]: actions-rust-lang/setup-rust-toolchain#41
We run the build for amd64 and arm64, and we use `setup-rust-toolchain` to install and cache the Rust toolchain and project dependencies. The caching part of that action doesn't currently work if it's run in multiple jobs from the same workflow run. The cache key doesn't contain anything that would disambiguate and there is no input to `setup-rust-toolchain` to allow it to be customised. To fix that, I've just [submitted a PR][PR] to the action to allow the cache key to be customised. This will allow us to set a key that includes the OS and architecture of the build, so that the caches are kept separate. We're making use of that here. [PR]: actions-rust-lang/setup-rust-toolchain#41
I am really wary of just adding too many cache-specific arguments to this action. I rather have the cache part simple and have people use # An additional cache key that is added alongside the automatic `job`-based
# cache key and can be used to further differentiate jobs.
# default: empty
key: "" According to this, each job already uses a separate cache key. From the code, it looks like they include the |
action.yml
Outdated
@@ -32,6 +32,9 @@ inputs: | |||
description: "Also cache on workflow failures" | |||
default: "true" | |||
required: false | |||
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.
This should be cache-key
instead of key
to make it clear that this is a cache related setting.
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.
Cheers, fixed.
Thank you for the feedback. It's made me understand that I could be more explicit in the description/commit message. I've edited those now. Let me know if you still consider this too advanced and we can close the pull request. The problem occurs when using matrices. In that case, the In real usage, it's likely you'd be performing a build after calling this action, so the race would not be that matrix jobs can restore the wrong cache in the same run, but that they both try to save the cache at the end and one of them fails. |
When using this action in multiple matrix jobs in the same workflow, the generated cache key is the same for all of them, because they all get the same job ID. This means that all apart from the first job are unable to save the cache, and subsequent runs might restore the wrong cache. The `Swatinem/rust-cache` action which we use for caching has a `key` input which it puts in its cache key. (It doesn't override the key, just adds to it.) Providing this as an input here will allow us to generate a unique cache key for each job in the matrix.
d4e5302
to
b01657d
Compare
Hi, I think I have come around to exposing more of |
When using this action in multiple matrix jobs in the same workflow, the generated cache key is the same for all of them, because they all get the same job ID. This means that all apart from the first job are unable to save the cache, and subsequent runs might restore the wrong cache.
The
Swatinem/rust-cache
action which we use for caching has akey
input which it puts in its cache key. (It doesn't override the key, just adds to it.) Providing this as an input here will allow us to generate a unique cache key for each job in the matrix.