-
Notifications
You must be signed in to change notification settings - Fork 823
Introduce the dependency caching for Maven and Gradle #193
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
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 wrappersPlease 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 wrappersWe 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. |
|
@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 |
konradpabjan
left a comment
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! 🚀 |
jackromo888
left a comment
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.
* implement a core logic to cache dependnecies * integrate the cache logic to entry points * add a user doc about the dependency cache feature * reflect changes to the dist dir * add a prefix to the cache key https://github.com/actions/setup-java/pull/193/files#r669521434 * test: extract build.gradle to a file in __tests__ dir * run the restore e2e test on the specified OS * add an e2e test for maven * fix the dependency among workflows * stabilize the cache on the Windows in e2e test * add .gitignore files to __tests__/cache directories * try to run restore after the authentication * use the key in state to save caches in the post process * suggest users to run without daemon if fail to save Gradle cache on Windows * add missing description in the README.md * run clean-up tasks in serial * Add validation for post step (actions#3) * work on fixing cache post step * fix tests * Update src/cleanup-java.ts Co-authored-by: Konrad Pabjan <konradpabjan@github.com> * Update src/cache.ts Co-authored-by: Konrad Pabjan <konradpabjan@github.com> * style: put the name of input to the constants.ts * format: run `npm run build` to reflect changes to the dist dir * chore: update licensed files by `licensed cache` 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 * fix: rerun ncc on macOS with node v12 * build: follow the suggestion at PR page actions#193 (comment) * fix: throw error in case of no package manager file found Co-authored-by: Dmitry Shibanov <dmitry-shibanov@github.com> Co-authored-by: Konrad Pabjan <konradpabjan@github.com>


This PR suggests the dependency caching feature just like the dependency caching feature of
actions/setup-node.Benefits:
actions/cacheexplicitly 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-sizeconfiguration. I think it won't be problem for most users, andactions/setup-nodealso provides no config like this.actions/cache.Check list:
Thanks in advance for your review! 😃