hendrikmuhs / ccache-action

github action to speedup building using ccache
MIT License
122 stars 54 forks source link

Date/time appended to cache key #117

Closed lballabio closed 2 years ago

lballabio commented 2 years ago

Currently, the action appends the current date and time to the key when saving. This causes it to save a new cache, instead of overwriting the existing one. In turn, this leads to using more space and possibly makes it more likely for caches to be evicted.

What was the reason for this? Would it be possible to add an option so that it's possible to use the given key as-is and thus overwrite existing caches? Thanks a lot!

ozgurakgun commented 2 years ago

I came here wondering this and found this issue opened 11 hours ago! Anyway: I am interested in the answer too, please.

hendrikmuhs commented 2 years ago

As stated in the Readme, the action is based on a blog post: https://cristianadam.eu/20200113/speeding-up-c-plus-plus-github-actions-using-ccache/

In turn, this leads to using more space and possibly makes it more likely for caches to be evicted.

I am certain the cache evicts unused/less used items first. I therefore don't see a problem. Space as far as I know is fixed by project, in case it runs full, items get evicted. I don't see a problem with this.

What was the reason for this?

As said I mainly followed the blog post. I assumed good intent and a reason for this, unfortunately the blog doesn't talk about this detail. I assume it either has something to do with concurrent access or cache eviction.

Would it be possible to add an option so that it's possible to use the given key as-is and thus overwrite existing caches?

I happily merge a PR that implements more cache key flexibility. I wouldn't use a boolean flag however, because it limits flexibility to true/false. I would choose an enum with different strategies or supplying the formatting string. The existing default should stay as I don't want to risk breaking many projects that work fine with the current action.

lballabio commented 2 years ago

You're right, removing the timestamp might cause problems with concurrent access. Thanks!