-
Notifications
You must be signed in to change notification settings - Fork 765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce the dependency caching for Maven and Gradle #193
Conversation
Hello @KengoTODA , thank you for contribution! |
@dmitry-shibanov I've updated the code, please check. I also found that the Gradle cache on Windows isn't be saved as expected, see this build result for detail. I will investigate. |
Hello @KengoTODA. Thank you for your help. It looks like a known issue with gradle on windows-latest. Possibly it will help: actions/cache#454 |
Hi, I see the usage in some test like this: - uses: actions/setup-java@v2
with:
distribution: 'adopt'
java-version: '11'
cache: 'gradle' # will restore cache of dependencies and wrappers Please will it be possible to use - uses: actions/setup-java@v2
working-directory: server
with:
distribution: 'adopt'
java-version: '11'
cache: 'gradle' # will restore cache of dependencies and wrappers We use monorepo structure, where Thank you very much for such a great feature! |
Thanks all, I've finished the necessary improvements, please check updates here. |
To make the permission trouble on Windows more intuitive, added a warning like below: Refer to the demo in my repo if necessary. |
src/cache.ts
Outdated
core.debug(`primary key is ${primaryKey}`); | ||
core.saveState(STATE_CACHE_PRIMARY_KEY, primaryKey); | ||
if (primaryKey.endsWith('-')) { | ||
core.warning( |
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 wonder if we would like to throw new Error()
if no files are found matched patterns like we have done in setup-node.
@AlenaSviridenko @MaksimZhukov what do you think about it?
If we would like to keep it as a warning, I think we will need one more condition in save
function because current implementation will try to save cache even in case when no files are matched patterns. I guess we don't want it.
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.
Thanks for your review! I just kept the existing behaviour (@actions/cache
with hashFiles(...)
), but it would be a great improvement making cache more intuitive. 👍
I'll wait for the reply from other experts, and change the code once we made a consensus.
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.
- @konradpabjan for any thoughts
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'm leaning towards this throwing an error if no files are found.
Given that the new cache
input is optional, it's reasonable to expect that someone who specifies gradle
or maven
has the appropriate files in their repository.
A lot of users today also incorrectly use actions/cache
for a variety of reasons an an error would be pretty explicit in telling users to fix their code or not to use the cache feature.
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 made a proposal in my repo: https://github.com/KengoTODA/setup-java/pull/4/files
If it looks good to you, I'll merge this commit to this PR. :)
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.
@KengoTODA , Proposal looks good to me. Please merge PR.
Also, I see that two checks fail right now:
- license - please ignore this check for now. I will fix it in the separate PR.
- build fails on verify_no_unstaged_changes - may be you haven't rebuild it last time? Let's try to rebuild one more time after merging your proposal via
npm run build && npm run release
@KengoTODA, thank you for your contribution. I have left a few minor comments. Please let me know if you have any suggestions / concerns here. |
Hello @KengoTODA. Sorry for the late reply. Could you please take a look on pull request we've created in your repository ? We've added logic to save cache only for successful builds. |
@dmitry-shibanov Thanks for your PR! I've merged it and resolved a conflict between the upstream repo. Please check :) |
@KengoTODA, sorry for delay and thank you for your patience. Issue with post-job required to apply "hacks" :) |
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.
Awesome PR and thank you for the contribution! 🎉
Left a few minor comments.
Edit:
The changes to the index.js
files under /dist
are also huge (pretty much impossible to manually verify). I've created a separate PR to add a automated check to make it easier to verify the files are correct. Want to do a bit more verification once #206 goes in.
src/cache.ts
Outdated
core.debug(`primary key is ${primaryKey}`); | ||
core.saveState(STATE_CACHE_PRIMARY_KEY, primaryKey); | ||
if (primaryKey.endsWith('-')) { | ||
core.warning( |
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'm leaning towards this throwing an error if no files are found.
Given that the new cache
input is optional, it's reasonable to expect that someone who specifies gradle
or maven
has the appropriate files in their repository.
A lot of users today also incorrectly use actions/cache
for a variety of reasons an an error would be pretty explicit in telling users to fix their code or not to use the cache feature.
Co-authored-by: Konrad Pabjan <konradpabjan@github.com>
Co-authored-by: Konrad Pabjan <konradpabjan@github.com>
it still has three errors as follows: >* setup-java.npm.sax > filename: /Users/kengo/GitHub/setup-java/.licenses/npm/sax.dep.yml > - license needs review: other > >* setup-java.npm.tslib-1.14.1 > filename: /Users/kengo/GitHub/setup-java/.licenses/npm/tslib-1.14.1.dep.yml > - license needs review: 0bsd > >* setup-java.npm.tslib-2.3.0 > filename: /Users/kengo/GitHub/setup-java/.licenses/npm/tslib-2.3.0.dep.yml > - license needs review: 0bsd
I've updated licensed files with licensed v3.1.0. It still has three status check failures:
The license of sax v1.2.4 is ISC. And all of them are already used in the |
@KengoTODA ,
Could you please also merge your proposal KengoTODA#4 and |
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.
🚀 🥳 LGTM!
@KengoTODA thank you one more time for great contribution, very appreciate! |
I'm very glad to see a changelog entry announcing this change. Thanks again everyone! 🚀 |
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.
This PR suggests the dependency caching feature just like the dependency caching feature of
actions/setup-node
.Benefits:
actions/cache
explicitly any more.About the performance, we can expect that it'll shorten about 28% of the time to build, even when we have no dependency. You can find the code to run the test in my test project, or see setup-java-cache.csv or Google Docs to grab detailed data.
Note that in this graph I run 10 tests and use 9 of them (treated 1 of them as error) for each dependency size.
Considerations:
upload-chunk-size
configuration. I think it won't be problem for most users, andactions/setup-node
also provides no config like this.actions/cache
.Check list:
Thanks in advance for your review! 😃