Skip to content
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

Merged
merged 26 commits into from Aug 19, 2021

Conversation

KengoTODA
Copy link
Contributor

This PR suggests the dependency caching feature just like the dependency caching feature of actions/setup-node.

Benefits:

  • It makes the workflow YAML file shorter, no need to run actions/cache explicitly any more.
  • It can run Java installation and cache restoring process in parallel, so elapsed time for initialization will be shorter.

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.

Perf  comparison for Java cache

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:

  • This feature does not provide the upload-chunk-size configuration. I think it won't be problem for most users, and actions/setup-node also provides no config like this.
  • We may support other package managers like ivy and grape, but first let's focus on package managers described in the doc of actions/cache.

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

Thanks in advance for your review! 😃

@maxim-lobanov
Copy link
Contributor

Hello @KengoTODA , thank you for contribution!
Your changes look really awesome. Please give us some time to review it properly and make sure that everything is okay and we will be happy to accept it.

src/setup-java.ts Outdated Show resolved Hide resolved
src/cache.ts Outdated Show resolved Hide resolved
.github/workflows/e2e-cache.yml Show resolved Hide resolved
.github/workflows/e2e-cache.yml Outdated Show resolved Hide resolved
.github/workflows/e2e-cache.yml Outdated Show resolved Hide resolved
@KengoTODA
Copy link
Contributor Author

@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.

@dmitry-shibanov
Copy link
Contributor

@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

@morki
Copy link

morki commented Jul 14, 2021

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 working-directory to make it use other Gradle directory than root like this?

- 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 server directory is for Java application and client for Node one.
Maybe some other solution can be used to accomplish this.

Thank you very much for such a great feature!

@KengoTODA
Copy link
Contributor Author

Thanks all,

I've finished the necessary improvements, please check updates here.
Now e2e tests on Windows are stable, we also have tests for Maven, and the restore process runs after the auth part.

src/cache.ts Outdated Show resolved Hide resolved
@KengoTODA
Copy link
Contributor Author

KengoTODA commented Jul 20, 2021

To make the permission trouble on Windows more intuitive, added a warning like below:

warning-for-gradle-on-windows

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(
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

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.

Copy link
Contributor Author

@KengoTODA KengoTODA Aug 19, 2021

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. :)

Copy link
Contributor

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

src/cleanup-java.ts Outdated Show resolved Hide resolved
@maxim-lobanov
Copy link
Contributor

@KengoTODA, thank you for your contribution. I have left a few minor comments.
There is one more issue that we need to resolve before this pull-request can be merged:
Post-job step does two action: clean up auth and save cache.
Post-job condition is always() currently. It means that post-job step will be run even job is failed. It is necessary because auth data should be clean up always.
The problem is that we don't want to save cache if job fails because obviously, it will cause invalid caching and save and etc (for example, when gradle install fails).
I didn't find the way to determine if the whole job is successful or not in post job step. Will try to ask our engineers if there is a way to overcome it.

Please let me know if you have any suggestions / concerns here.

@dmitry-shibanov
Copy link
Contributor

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.
Besides, could you please pull latest main to resolve the conflict ?

@KengoTODA
Copy link
Contributor Author

@dmitry-shibanov Thanks for your PR! I've merged it and resolved a conflict between the upstream repo. Please check :)

@maxim-lobanov
Copy link
Contributor

@KengoTODA, sorry for delay and thank you for your patience. Issue with post-job required to apply "hacks" :)
I think we are on the final line with this PR and it will be merged soon. We will double validate from our side and engage a few more experts to review.

Copy link
Collaborator

@konradpabjan konradpabjan left a 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(
Copy link
Collaborator

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.

src/cleanup-java.ts Outdated Show resolved Hide resolved
src/cache.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
KengoTODA and others added 4 commits August 19, 2021 10:28
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
@KengoTODA
Copy link
Contributor Author

I've updated licensed files with licensed v3.1.0. It still has three status check failures:

  • 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

The license of sax v1.2.4 is ISC. And all of them are already used in the setup-node action, so I hope there is no problem using them in this project:

@maxim-lobanov
Copy link
Contributor

@KengoTODA ,
As for the licenses:

Could you please also merge your proposal KengoTODA#4 and npm run build && npm run release after that?

@KengoTODA
Copy link
Contributor Author

Here is (probably) the final performance comparison chart. As expected, the performance is better than existing way:

Perf  comparison for Java cache (67b74a15) (1)

Source (workflow runs): large, medium, and small

Copy link
Collaborator

@konradpabjan konradpabjan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 🥳 LGTM!

@maxim-lobanov maxim-lobanov merged commit 08e4e81 into actions:main Aug 19, 2021
@maxim-lobanov
Copy link
Contributor

@KengoTODA thank you one more time for great contribution, very appreciate!
I have merged PR and changes are already in main branch.
I will cut the new action release on Monday (we are trying to avoid releases on Friday)

@KengoTODA
Copy link
Contributor Author

I'm very glad to see a changelog entry announcing this change. Thanks again everyone! 🚀

Copy link

@jackromo888 jackromo888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants