obi1kenobi / cargo-semver-checks-action

A GitHub Action for running cargo-semver-checks
MIT License
60 stars 15 forks source link

Include target name in the cache key #55

Open obi1kenobi opened 1 year ago

obi1kenobi commented 1 year ago

Right now, our cache key includes rustc --version but does not include the target name. This can be a caching problem if someone wants to test multiple targets on the same commit and with the same Rust version, which is entirely reasonable (e.g. #54), if a bit rare.

Sh0g0-1758 commented 1 year ago

Hi, @obi1kenobi. Can you please point out exactly where in the codebase cache key is located?

obi1kenobi commented 1 year ago

This is it: https://github.com/obi1kenobi/cargo-semver-checks-action/blob/e275dda72e250d4df5b564e969e1348d67fefa52/src/rustdoc-cache.ts#L30

It's constructed from a few different components, but all the building blocks are in the same file so it shouldn't be too hard to see what the key structure currently looks like.

Sh0g0-1758 commented 1 year ago

@obi1kenobi Can you please take a look at my PR and suggest changes in it, if required?

obi1kenobi commented 3 months ago

@memark if you might have the time and interest, this would be a good issue to tackle to make the target selection a bit more robust.

Otherwise, there's some risk that runs using different targets might end up inappropriately sharing a cache, which could lead to erroneous results.

memark commented 3 months ago

Sure, I'd be happy too.

In the declined PR you mentioned that a lot of Rust coding needed to be done in addition to the TypeScript changes. Does that still hold true, and if so what changes would that be?

obi1kenobi commented 3 months ago

The concern I have (which is possibly unfounded, since I don't know the details of the --target flag) is in a scenario like the following:

These two runs should not share baseline rustdoc JSON files via the cache. The equivalent scenario using the GitHub Action shouldn't result in cache reuse either. But I'm not convinced that this is correctly handled at the moment — I think cache reuse would happen in both cases.

If so, I think we'll have to: