Include target name in the cache key

[obi1kenobi]

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]

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

[obi1kenobi]

This is it:

cargo-semver-checks-action/src/rustdoc-cache.ts

Line 30 in e275dda

const cacheKeyWithLocalHash = `${await this.cacheKey()}-${await this.getLocalCacheHash()}`;

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]

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

[obi1kenobi]

@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]

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]

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:

• Say a user is on Linux x86, and runs cargo-semver-checks with default settings.
• Then say they run cargo-semver-checks on an alternative target, on the same system and after the previous step.

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:

• reproduce both bugs with tests, so we know our fixes did indeed fix the problem -- some platform-based cfg!() item inclusions or exclusions should be sufficient;
• include the rustc target as part of the cache key in cargo-semver-checks itself here
• include the configured rustc target (if any) to the GitHub Action's own cache key here