actions-rust-lang / setup-rust-toolchain

Setup a specific Rust toolchain with extra features like problem matchers
MIT License
180 stars 32 forks source link

Add support for adding to cache key #41

Closed iainlane closed 3 weeks ago

iainlane commented 3 months ago

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.

jonasbb commented 3 months ago

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 Swatinem/rust-cache directly when they need all the bells and whistles. Messing with the cache key to me already goes into the more advanced territory. For this change, I would like to check the reasoning. This is the description of the key argument of the rust-cache action:

    # 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 GITHUB_JOB environment variable. So no effort should be necessary "to generate a unique cache key for each job."

iainlane commented 3 months ago

I would like to check the reasoning. This is the description of the key argument of the rust-cache action:

    # 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 GITHUB_JOB environment variable. So no effort should be necessary "to generate a unique cache key for each job."

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 job_id is the same. I've constructed an example in which one matrix job saves a cache which is then restored by the other matrix job.

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.

jonasbb commented 3 weeks ago

Hi, I think I have come around to exposing more of Swatinem/rust-cache, since there seems to be demand for it. I try to release this during the weekend.