-
Notifications
You must be signed in to change notification settings - Fork 751
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
actions/setup-java@v2 - Support different distributions #132
Conversation
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.
Looking great! Did an initial sweep, just a few things
FYI, the Disco API (https://github.com/foojay2020/discoapi) is coming along nicely, with multiple tools starting to use it. It provides a sweeping solution for the "find a JDK" task across multiple distros, and an actions/setup-java PR is coming soon for using it directly, replacing the current distro-specific code, but keeping all existing functionality. This will likely overlap with this PR. See some FOSDEM discussion from last month: https://fosdem.org/2021/schedule/event/discoapi/ |
Co-authored-by: George Adams <[email protected]>
Where might I have seen that before? |
Co-authored-by: Konrad Pabjan <[email protected]>
Down the road we can consider utilizing the Disco API or any other tools to expand support for more distributions. With the initial v2 preview and release we're primarily focusing on just expanding support for Adoptium and ironing out any bugs that might come up since that was the biggest ASK from users. |
@giltene , caching logic will work the same way like it works for other tools (go, node, python, haskell) - compare version only, not checksum. |
@sormuras , we have validated |
@maxim-lobanov I'm a somewhat heavy user of setup-java and cannot think of a single reason not to specify |
Thank you for providing your input, I have pushed PR update to mark |
actions/setup-java@v2 - Test coverage
actions/setup-java@v2 - Documentation
@maxim-lobanov I think this will cause a lot of issues. Bringing in a later-than-cached-version only if it is explicitly specified is both counter-intuitive and will likely break many of the commonly relied on behaviors of setup-java. For example, a common [best] practice is to run tests both against a statically and explicitly selected update version (e.g. "11.0.7") and the latest update version (e.g. "11") of the same java versions, for any version that is currently maintained and has new updates posted multiple times per year. The simple reason for this is to help triage and identify whether tests break because of a change in the jdk or a change in the repo. It is [unfortunately] common for new updates to break things. E.g. just in the recent few months, OpenJDK 8u262, 8u272, and 11.0.9 all had significant real-world breakages found post release, leading to the release of the out of cycle 8u265, 8u275, and 11.0.9.1 within weeks. Things that had such breakages revealed in post-release tests included e.g. hadoop and solr. A one week delay in testing new updates due to caching of e.g. LTS updates would create wide ranging negative impacts, both for the tested repos (which will find out a week late that stuff is newly broken that they may need to be aware of work around, or that new out of cycle stuff has a fix that needs to be urgently used), and for OpenJDK in general: the wide ranging testing of new updates in the hours and days immediately after release can help drive the feedback that identify these things. With 60K+ Github workflows currently testing openjdk updates as they appear, this quick post-release exposure is very helpful, and delaying it by a week would be sad. I'd like to suggest that, at the very least, the lookup and resolution of the semver specified versions to URIs be done on each run with no attempt to cache that logic, but that the download URI itself be used as a key for caching. This way updates will show up immediately in tests that run after they appear. A preferred solution would cache the hash or checksum to make sure that even replaced binaries at the same URI are treated properly, but URI based caching is probably the next best thing. |
By default, action should always check cache firstly. The most of users prefer to use cached version (speed up their builds by 10-30 seconds and more reliable since there is no any change to fail randomly via connection issue) but get up-to-date version with a few days delay. The same way like actions/setup-go, actions/setup-python, actions/setup-node work. The use-case that you have described makes sense but it shouldn't be a default behavior. For Node.JS, it is achieved via separate flag check-latest so probably we should consider adding something similar to cover this use-case. |
Merged these changes to
|
I have to admit, I don't know where the cache duration is handled. But I have to concur with @giltene's reasoning.
Caching, yes, please. But definitely not for a week! If I specify Also, you have discarded my comment about downloading the entire list of available JDKs and the possible connection failures. Now you're using the exact same argument for an insanely long cache timeout.
A flag/timeout option is fine with me, but it should default to getting new releases fast. |
Caching the contents of a chosen JDK download makes perfect sense, and can save 20-30 seconds and a bunch of bandwidth. Caching the decision of which JDK to choose to download does not. It would only save a small fraction of a second and practically no bandwidth, and lead to operational changes compared to the current behavior seen by the many users of 1.x. In addition, inconsistent functional behavior that depends on under-the-hood choice to selectively cache JDKs (only LTS? Only some distros?) in ways that cause widely varying behavior in terms of how up to date the JDK is (compared to what is currently available for that version spec and distro) will add “surprising” outcomes. Java has an industry-aligned cadence for quarterly updates (across all coordinating distros), so varying this behavior by version spec and distro will lead to some weird stuff. is there any reason not to just reactively cache all resolved JDK URIs for some reasonable amount of time? Or maybe cache the few hundred recent or hotly used ones if the number or space is a concern? |
@maxim-lobanov would you please get back to the caching issue? Just not answerig anymore isn't a solution. Or did I miss a separate thread? |
As for the caching timeline - Hosted images are updated on weekly basis so the image cache can't be updated more often.
We have two different behavior options:
@konradpabjan , The first option will satisfy users who more interested in getting fast and lower-cost builds and don't care much about 1-week delay with updates. We use this approach with other tools and it works perfectly. In |
As @giltene already mentioned, the delay of case 2) is only there if there is actually a newer version. In the majority of builds, there is no newer version and the only delay is the API call, not the actual JRE/JDK download. |
I really like the approach that @maxim-lobanov let's try to squeeze this into Thanks everyone for the feedback and discussion! |
Hello everyone. We prepared pull request with adding check-latest flag. At the moment the tests are failing but it will be fixed by this pull request. |
* actions/setup-java@v2 - Support different distributions (#132) * Implement support for custom vendors in setup-java * minor improvements * minor refactoring * Add unit tests and e2e tests * Update documentation for setup-java@v2 release * minor improvements * regenerate dist * fix comments * resolve comments * resolve comments * fix tests * Update README.md Co-authored-by: George Adams <[email protected]> * Apply suggestions from code review Co-authored-by: Konrad Pabjan <[email protected]> * fix minor nitpicks * handle 4th digit * pull latest main * Update README.md * rename adoptium to adopt * rename adoptium to adopt * rename adoptium to adopt * Update README.md * make java-version and distribution required for action * update readme * fix tests * fix e2e tests Co-authored-by: George Adams <[email protected]> Co-authored-by: Konrad Pabjan <[email protected]> * Add "overwrite-settings" input parameter (#136) * add overwrite-settings parameter * fix e2e tests * print debug * fix e2e tests * add comment * remove comment * Add "Contents/Home" postfix on macOS if provider creates it (#139) * Update e2e-versions.yml * Update e2e-versions.yml * implement fix * Update e2e-versions.yml * Update installer.ts * fix filter logic * Update e2e-versions.yml * remove extra logic * Update e2e-versions.yml * Add check-latest flag (#141) * add changes for check-latest * run prerelease script * resolving comments * fixing tests * fix spelling * improve core.info messages * run format * run prerelease * change version to fix test * resolve comment for check-latest * Update README.md * added hosted tool cache section * Apply suggestions from code review Co-authored-by: Maxim Lobanov <[email protected]> Co-authored-by: Konrad Pabjan <[email protected]> * Avoid "+" sign in Java path in v2-preview (#145) * try to handle _ versions * more logs * more debug * test 1 * more fixes * fix typo * Update e2e-versions.yml * add unit-tests * remove debug info from tests * debug pre-cached versions * change e2e tests to ubuntu-latest * update npm licenses Co-authored-by: George Adams <[email protected]> Co-authored-by: Konrad Pabjan <[email protected]> Co-authored-by: Dmitry Shibanov <[email protected]>
Description:
This pull-request reworks installer approach and create abstract base that can be used for adding new Java distributions. Also this pull-request adds support for Azul and Adoptium (AdoptOpenJDK) distributions.
These changes are going to be released as
actions/setup-java@v2
so breaking changes are expected.Note: we have decided to split changes to a few pull-requests to simplify review process:
A few technical notes:
distribution
input without default value.zulu
(Azul OpenJDK),adoptium
(Adopt OpenJDK)https://static.azul.com/zulu/bin/
and use regex to find Java versions.We have reworked it to use Azul API: https://app.swaggerhub.com/apis-docs/azul/zulu-download-community/1.0. Potentially, it could cause some differences in version resolving but we have tested all major use-cases and didn't find any issues.
Java_${distribution}_${packageType}
to make sure that we can cache different vendors side by side.Also, adds
-ea
postfix for version in toolcache to distinguish stable and unstable versions (in V1 - it could cause issues)distribution
andjava-package
properties optional to simplify use-cases when task is used for setting up Maven publishing (settings.xml), customers don't really need to spend time on Java setup. So ifdistribution
andjava-version
properties are specified - Java will be installed. Otherwise - only settings.xml will be created. Also bothdistribution
andjava-package
should be specified together (if only one is specified - error will be raised)build
). The same behavior was in V1. It could cause some confusing but neither V1 nor V2 support specifying versions with 4 digits in input - so it shouldn't be an issue. Also, looks liketoolkit/toolcache
doesn't support 4 digits versions because ofsemver.clean
invocation.console.log
withcore.info
, apply styling. No major changes there.version
input in favor ofjava-version
(V1 supported both for backward compatibility)