Add support for Eclipse Temurin #201
Conversation
|
The proper name for this OpenJDK distribution is Eclipse Temurin, and “temurin” is likely the best way to spell it in the distribution field. In addition, there is no need for the “-hotspot” variant in the name, as Eclipse Temurin will only be building hotspot (explicitly no plans currently for a -openj9 variant). For context: There is no such thing as a (single) Adoptium OpenJDK distribution. Eclipse Adoptium is a multi-vendor project that expects to have a marketplace of (multiple) quality OpenJDK distributions, of which Eclipse Temurin (built in a sub project of Ecplise Adoptium) is one. There will likely be multiple others. E.g. Zulu is also expected to be on the Adoptium Marketplace, side by side with Temurin, and so are others. |
README.md
Outdated
| @@ -13,7 +13,7 @@ This action provides the following functionality for GitHub Actions runners: | |||
| - Registering problem matchers for error output | |||
|
|
|||
| ## V2 vs V1 | |||
| - V2 supports custom distributions and provides support for Zulu OpenJDK and Adopt OpenJDK out of the box. V1 supports only Zulu OpenJDK | |||
| - V2 supports custom distributions and provides support for Zulu OpenJDK, Adopt OpenJDK and Adoptium OpenJDK out of the box. V1 supports only Zulu OpenJDK | |||
There was a problem hiding this comment.
"Adoptium OpenJDK" should be changed to "Eclipse Temurin"
There was a problem hiding this comment.
"Adopt OpenJDK" should also change to "AdoptOpenJDK"
There was a problem hiding this comment.
@brunoborges In fact it's the "AdoptOpenJDK OpenJDK", but I'd like to concentrate on Adoptium/Eclipse Temurin changes in this pull request. 😅
README.md
Outdated
| - uses: actions/checkout@v2 | ||
| - uses: actions/setup-java@v2 | ||
| with: | ||
| distribution: 'adoptium' # See 'Supported distributions' for available options |
There was a problem hiding this comment.
"adoptium" should be changed to "temurin"
README.md
Outdated
| @@ -55,6 +66,7 @@ Currently, the following distributions are supported: | |||
| | `zulu` | Zulu OpenJDK | [Link](https://www.azul.com/downloads/zulu-community/?package=jdk) | [Link](https://www.azul.com/products/zulu-and-zulu-enterprise/zulu-terms-of-use/) | | |||
| | `adopt` or `adopt-hotspot` | Adopt OpenJDK Hotspot | [Link](https://adoptopenjdk.net/) | [Link](https://adoptopenjdk.net/about.html) | |||
| | `adopt-openj9` | Adopt OpenJDK OpenJ9 | [Link](https://adoptopenjdk.net/) | [Link](https://adoptopenjdk.net/about.html) | |||
| | `adoptium` or `adoptium-hotspot` | Adoptium OpenJDK Hotspot | [Link](https://adoptium.net/) | [Link](https://adoptium.net/about.html) | |||
There was a problem hiding this comment.
"adoptium or adoptium-hotspot" should be changed to "temurin"
"Adoptium OpenJDK HotSpot" should be changed to "Eclipse Temurin"
| import { HttpClient } from '@actions/http-client'; | ||
|
|
||
| import { | ||
| AdoptiumDistribution, |
There was a problem hiding this comment.
"AdoptiumDistribution" should change to "TemurinDistribution"
|
|
||
| import { | ||
| AdoptiumDistribution, | ||
| AdoptiumImplementation |
There was a problem hiding this comment.
"AdoptiumImplementation" should change to "TemurinImplementation"
| it.each([ | ||
| [ | ||
| { version: '16', architecture: 'x64', packageType: 'jdk', checkLatest: false }, | ||
| AdoptiumImplementation.Hotspot, |
There was a problem hiding this comment.
s/Adoptium/Temurin/g in this file. Also, consider dropping the .Hotspot part as it is not needed for Temurin.
| import { JavaDownloadRelease, JavaInstallerOptions, JavaInstallerResults } from '../base-models'; | ||
| import { extractJdkFile, getDownloadArchiveExtension, isVersionSatisfies } from '../../util'; | ||
|
|
||
| export enum AdoptiumImplementation { |
There was a problem hiding this comment.
AdoptiumImplementation should change to TemurinImplementation
| Hotspot = 'Hotspot' | ||
| } | ||
|
|
||
| export class AdoptiumDistribution extends JavaBase { |
There was a problem hiding this comment.
AdoptiumDistribution should change to TemurinDistribution
| installerOptions: JavaInstallerOptions, | ||
| private readonly jvmImpl: AdoptiumImplementation | ||
| ) { | ||
| super(`Adoptium-${jvmImpl}`, installerOptions); |
There was a problem hiding this comment.
Adoptium-${jvmImpl} should change to Temurin
| export class AdoptiumDistribution extends JavaBase { | ||
| constructor( | ||
| installerOptions: JavaInstallerOptions, | ||
| private readonly jvmImpl: AdoptiumImplementation |
There was a problem hiding this comment.
AdoptiumImplementation should change to TemurinImplementation
I chose Happy to rename the identifier if that's the consensus. |
|
@jochi yeah, the name space and Eclipse trademark meanings seem to have had some flux in general understanding, but I think they are now quite clear and settled. We can probably get others from the Adoptium Working Group to chime in as well, and should certainly gain a sense of consensus before you merge this in and solidify the spellings. In the mean time, to highlight that the likely consensus will be "temurin" and "Temurin" for distribution spelling and names, here is some supporting evidence: Here the temurin distribution binary identifies itself: % jdk-11.0.12+7/Contents/Home/bin/java -version And here it is with more details about e.g. the "vendor" and "vendor version". Note that the word "adoptium" only shows up in the url (as Eclipse Temruin is a subproject of Eclipse Adoptium, it is natural for the Temurin distribution to be hosted at adoptium.net, but that's not the only thing that will be there. The Adoptium project expects to have a marketplace of multiple quality Java distributions at that url) % jdk-11.0.12+7/Contents/Home/bin/java -XshowSettings:properties -version openjdk version "11.0.12" 2021-07-20 |
|
@joschi thank you for contribution 🚀
Makes sense to me +1 on removing +@gdams for any suggestions about naming |
joschi
changed the title
|
@maxim-lobanov completely agree with the comments from @giltene. |
There was a problem hiding this comment.
Eclipse Temurin have not yet released binaries for all platforms yet (only Linux/x64, Windows/x64, MacOS/x64) - if this is used for other platforms it may cause a break (note that the existing AdoptOpenJDK API has a redirect in place now, so if there's a potential problem we could hold off on this PR
These are exactly the platforms supported by GitHub Actions hosted runners: |
|
I've update the end-to-end tests in the GitHub workflows, too. |
|
@joschi thank you for updating. Some e2e tests failed. Looks like |
|
@joschi , I think |
We provide nightly builds of JDK17 (as does AdoptOpenJDK). There must be some logic that needs fixing here |
|
@gdams @maxim-lobanov I think we have to relax some version check here. With the latest commit (using
https://github.com/actions/setup-java/pull/201/checks?check_run_id=3252530885 Before they were failing because of this error:
https://github.com/actions/setup-java/runs/3250670822?check_suite_focus=true I'll add handling for the |
|
@joschi, looks like it failed again. I think we should rather normalize versions that come from API than customers input. I think that with your latest changes, action treat input I have a quick fix for this issue. Let me contribute to your PR. |
|
@joschi, pushed changes. Let's see how it goes. |
|
@joschi, Okay, looks like all tests are green and pass successfully
This way we keep Any concerns about my latest commit and final view of this pull-request? P.S. Sorry, initially I wanted to create PR with my fix to your branch but accidentally pushed directly to your branch. |
|
@maxim-lobanov Thanks for your help! Let's get this merged. 😀 |
|
@joschi , thank you for contribution! Merged PR. We will release new task version on Monday because we are trying to avoid releases on Friday! |
Description:
Add support for Adoptium, the successor of the AdoptOpenJDK project.
They just published their first release of Eclipse Temurin 8u302, 11.0.12, and 16.0.2.
I decided against refactoring the AdoptOpenJDK integration to share more code with the Adoptium implementation because while the AdoptOpenJDK API v3 and the Adoptium API are currently identical, they might diverge in the future and I think having some duplicated code for two different implementation now trumps having to refactor things again in the mid-term.
Related issue:
#191
Check list: