search

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

feat: adds "cache-on-failure" propagation to Swatinem/rust-cache #39

Closed samuelhnrq closed 4 months ago

samuelhnrq commented 4 months ago

Pretty self explanatory, adds the propagation of cache-on-failure alongside cache-workspaces.

This action works really nicely and I didn't want to manually setup cache just because of this, neither did I want to recompile axum every-time my unit tests failed 😆

Backwards compatible, false already is the default value.

jonasbb commented 4 months ago

The change looks fine and the setting important enough to change. Is there a downside to enabling cache-on-failure by default for this action? It feels like the more useful default to have as it could mean a higher cache hit rate. Unless storing after a failure could ruin/overwrite a good cache state. And if it is the default do we need to expose that as a toggle?

samuelhnrq commented 4 months ago

I do think its a saner default and but I also like the toggle, to avoid people having the same dilemma as me: the action works perfectly fine but now I have to drop it and internalise a lot of functionality myself for a silly config value without a toggle.

I have no idea on the details implementation of the actual caching of but I would expect it to keep the cache 1:1 even if unread/unwritten. I guess the point of turning this off is not to download a Gig of cache to fail on something unrelated to cargo and upload the gig back, unchanged, because I really dont expect it to be a diffing on the cache (pretty sure it's GIGO)

I'm pretty confident that cargo is smart enough to not ruin its own cache on failed runs (except on absurdities like getting a SIGKILL midway)