-
-
Notifications
You must be signed in to change notification settings - Fork 12
Use GitHub Actions cache for installed binaries #5
base: master
Are you sure you want to change the base?
Conversation
This was moved there, because it was also required for caching installed binaries
Do i understand it correctly that this would work with any crate installed through actions-rs/install and not just specific precompiled binaries like it is currently? |
@Blisto91 exactly. It will cache every binary in a cache named You can still fall back to
|
@flip1995 Sounds awesome! Hope it gets merged soon :). |
@flip1995 I published new |
@svartalf did something during publishing go wrong?
all of this should be in the new release 🤔 |
@flip1995 ah yes, sorry, that was an incorrect release indeed. Just published |
deff6bb
to
9f5d8b9
Compare
@svartalf CI looks good 👍 |
src/input.ts
Outdated
} | ||
|
||
export function get(): Input { | ||
const crate = input.getInput("crate", { required: true }); | ||
const version = input.getInput("version", { required: true }); | ||
const useToolCache = input.getInputBool("use-tool-cache") || false; | ||
const useCache = input.getInputBool("use-cache") || true; | ||
const primaryKey = input.getInput("key") || undefined; | ||
const restoreKeys = input.getInputAsArray("restore-keys") || undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is a good idea to force users to provide all necessary cache keys, as Action can do that by itself; grab the crate name, version and current runner name (ex. ubuntu-18.04
), join them together - we got a cache key.
That also mean that we will need to use resolveVersion
if there is no version specifically set in the Action arguments or set to "latest"
, for example.
Ideally, from the user side I would expect to write
- uses: actions-rs/install@v0.2
with:
crate: cargo-make
and it will handle all the stuff by itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought behind adding those keys was that there is currently no way to force-update a cache. If a new cache should be created, the cache has to get a new name. With actions/cache
in rust projects, this is for example done by hashing the Cargo.lock
files.
But when caching binaries I guess only naming the hash with the version number should do the trick. There should be no need to add a hash to the cache name.
Should I remove the keys completely or should I just make them optional?
The part with resolving the version is already implemented. But I still have to add the runner name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@svartalf Do you have a suggestion how I might get the name of the runner? I looked at the @actions/github
and @actions/core
package, but couldn't find any functions with which I'm able to access the runner name.
If we're able to get the runner name $somehow, we can get rid of the key
/restoreKeys
arguments of the action and go back to useCache
. This change has to be implemented in actions-rs/core
though.
This depends on a change in @actions-rs/core
@svartalf I removed the |
@flip1995 I don't think there is any reliable and already existing way to do that, our best shot would be to use combination of |
@svartalf In actions-rs/core#125 I use a combination of |
@flip1995 yeah, |
@svartalf Can you take another look at actions-rs/core#125 in combination with 57e52b9 then, please? |
Has there been further development on this? With the old tool-cache repo not updating with new binaries it's very painful. |
This is still waiting on review of actions-rs/core#125 which then will require an update of 57e52b9 |
So am I getting this right? Even though core supports cached installs for over 2 years, it has not made it into the install action, because we want core also to run |
To do this cleanly without backwards breaking changes, yes a the actions-rs/core#125 PR has to be merged. The alternative would be to merge this with a breaking change, just to revert the breaking change when doing this cleanly, which again would be a breaking change. |
I just came across https://github.com/ryankurte/cargo-binstall which might be useful for other people interested in this feature. |
Fixes #4
This uses the changes from actions-rs/core#92 to install
For easier review, I'll comment some of the code changes.
This is the first time I'm writing TS code ever, so I'm not confident that this is the right way to do it. I think, after merging actions-rs/core#92, I have to rebuild and commit
dist/index.js
?This change has to be merged together with/after actions-rs/core#92
Drawbacks
This removes the
use-cache
key, since with this implementation it is useless. I wouldn't consider this breaking change a problem though, since this is an experimental action and this key was unused anyway.