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

Add support for Eclipse Temurin #201

Merged
merged 11 commits into from
Aug 6, 2021
Merged

Conversation

joschi
Copy link
Contributor

@joschi joschi commented Aug 1, 2021

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:

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

@giltene
Copy link
Contributor

giltene commented Aug 1, 2021

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.

Copy link
Contributor

@giltene giltene left a comment

Choose a reason for hiding this comment

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

Systemic comments about name changes needed form Adoptium/adoptium to Temurin/temurin.

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

Choose a reason for hiding this comment

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

"Adoptium OpenJDK" should be changed to "Eclipse Temurin"

Choose a reason for hiding this comment

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

"Adopt OpenJDK" should also change to "AdoptOpenJDK"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

"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)
Copy link
Contributor

Choose a reason for hiding this comment

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

"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,
Copy link
Contributor

Choose a reason for hiding this comment

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

"AdoptiumDistribution" should change to "TemurinDistribution"


import {
AdoptiumDistribution,
AdoptiumImplementation
Copy link
Contributor

Choose a reason for hiding this comment

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

"AdoptiumImplementation" should change to "TemurinImplementation"

it.each([
[
{ version: '16', architecture: 'x64', packageType: 'jdk', checkLatest: false },
AdoptiumImplementation.Hotspot,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

AdoptiumImplementation should change to TemurinImplementation

Hotspot = 'Hotspot'
}

export class AdoptiumDistribution extends JavaBase {
Copy link
Contributor

Choose a reason for hiding this comment

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

AdoptiumDistribution should change to TemurinDistribution

installerOptions: JavaInstallerOptions,
private readonly jvmImpl: AdoptiumImplementation
) {
super(`Adoptium-${jvmImpl}`, installerOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Adoptium-${jvmImpl} should change to Temurin

export class AdoptiumDistribution extends JavaBase {
constructor(
installerOptions: JavaInstallerOptions,
private readonly jvmImpl: AdoptiumImplementation
Copy link
Contributor

Choose a reason for hiding this comment

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

AdoptiumImplementation should change to TemurinImplementation

@joschi
Copy link
Contributor Author

joschi commented Aug 1, 2021

The proper name for this OpenJDK distribution is Eclipse Temurin, and “temurin” is likely the best way to spell it in the distribution field.

I chose adoptium based on the name used as an example in ADR 0: V2 setup-java.

Happy to rename the identifier if that's the consensus.

@giltene
Copy link
Contributor

giltene commented Aug 1, 2021

@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
openjdk version "11.0.12" 2021-07-20
OpenJDK Runtime Environment Temurin-11.0.12+7 (build 11.0.12+7)
OpenJDK 64-Bit Server VM Temurin-11.0.12+7 (build 11.0.12+7, mixed mode)

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
Property settings:
awt.toolkit = sun.lwawt.macosx.LWCToolkit
file.encoding = UTF-8
file.separator = /
ftp.nonProxyHosts = local|.local|169.254/16|.169.254/16
gopherProxySet = false
http.nonProxyHosts = local|.local|169.254/16|.169.254/16
java.awt.graphicsenv = sun.awt.CGraphicsEnvironment
java.awt.printerjob = sun.lwawt.macosx.CPrinterJob
java.class.path =
java.class.version = 55.0
java.home = /Users/gil/Downloads/termurin/jdk-11.0.12+7/Contents/Home
java.io.tmpdir = /var/folders/m7/4c3y_mvd5710_8tmbcjd2l1r0000gn/T/
java.library.path = /Users/gil/Library/Java/Extensions
/Library/Java/Extensions
/Network/Library/Java/Extensions
/System/Library/Java/Extensions
/usr/lib/java
.
java.runtime.name = OpenJDK Runtime Environment
java.runtime.version = 11.0.12+7
java.specification.name = Java Platform API Specification
java.specification.vendor = Oracle Corporation
java.specification.version = 11
java.vendor = Eclipse Foundation
java.vendor.url = https://adoptium.net/
java.vendor.url.bug = https://github.com/adoptium/adoptium-support/issues
java.vendor.version = Temurin-11.0.12+7
java.version = 11.0.12
java.version.date = 2021-07-20
java.vm.compressedOopsMode = Zero based
java.vm.info = mixed mode
java.vm.name = OpenJDK 64-Bit Server VM
java.vm.specification.name = Java Virtual Machine Specification
java.vm.specification.vendor = Oracle Corporation
java.vm.specification.version = 11
java.vm.vendor = Eclipse Foundation
java.vm.version = 11.0.12+7
jdk.debug = release
line.separator = \n
os.arch = x86_64
os.name = Mac OS X
os.version = 11.5
path.separator = :
socksNonProxyHosts = local|.local|169.254/16|.169.254/16
sun.arch.data.model = 64
sun.boot.library.path = /Users/gil/Downloads/termurin/jdk-11.0.12+7/Contents/Home/lib
sun.cpu.endian = little
sun.cpu.isalist =
sun.io.unicode.encoding = UnicodeBig
sun.java.launcher = SUN_STANDARD
sun.jnu.encoding = UTF-8
sun.management.compiler = HotSpot 64-Bit Tiered Compilers
sun.os.patch.level = unknown
user.country = US
user.dir = /Users/gil/Downloads/termurin
user.home = /Users/gil
user.language = en
user.name = gil
user.timezone =

openjdk version "11.0.12" 2021-07-20
OpenJDK Runtime Environment Temurin-11.0.12+7 (build 11.0.12+7)
OpenJDK 64-Bit Server VM Temurin-11.0.12+7 (build 11.0.12+7, mixed mode)

@maxim-lobanov
Copy link
Contributor

@joschi thank you for contribution 🚀

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, the 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.

Makes sense to me

+1 on removing Hotspot part as giltene suggested above.

+@gdams for any suggestions about naming

@joschi joschi changed the title Add support for Adoptium OpenJDK Add support for Eclipse Temurin Aug 2, 2021
@gdams
Copy link
Contributor

gdams commented Aug 3, 2021

@maxim-lobanov completely agree with the comments from @giltene.

@maxim-lobanov
Copy link
Contributor

@joschi thank you for contribution. I think changes are ready for merge.
If @gdams @giltene don't have other comments, I think we will be able to merge it and release new version tomorrow

Copy link

@sxa sxa left a comment

Choose a reason for hiding this comment

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

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

@joschi
Copy link
Contributor Author

joschi commented Aug 3, 2021

Eclipse Temurin have not yet released binaries for all platforms yet (only Linux/x64, Windows/x64, MacOS/x64)

These are exactly the platforms supported by GitHub Actions hosted runners:
https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources

@joschi
Copy link
Contributor Author

joschi commented Aug 3, 2021

I've update the end-to-end tests in the GitHub workflows, too.

@maxim-lobanov
Copy link
Contributor

@joschi thank you for updating. Some e2e tests failed. Looks like temurin doesn't provide 17-ea and 16 jre yet and these cases should be excluded.

@maxim-lobanov
Copy link
Contributor

@joschi , I think 17.* should be excluded fully (remove job) because temurin doesn't provide it yet: https://adoptium.net/index.html?variant=openjdk16&jvmVariant=hotspot

@gdams
Copy link
Contributor

gdams commented Aug 5, 2021

@joschi , I think 17.* should be excluded fully (remove job) because temurin doesn't provide it yet: https://adoptium.net/index.html?variant=openjdk16&jvmVariant=hotspot

We provide nightly builds of JDK17 (as does AdoptOpenJDK). There must be some logic that needs fixing here

@joschi
Copy link
Contributor Author

joschi commented Aug 5, 2021

@gdams @maxim-lobanov I think we have to relax some version check here.

With the latest commit (using java-version: 17-beta), the e2e tests for Early Access releases is failing because of this error message:

Error: The string '17-beta' is not valid SemVer notation for a Java version. Please check README file for code snippets and more detailed information

https://github.com/actions/setup-java/pull/201/checks?check_run_id=3252530885

Before they were failing because of this error:

Error: Could not find satisfied version for SemVer '17'. 
Available versions: 17.0.0-beta+33.0.202107301459, 17.0.0-beta+32.0.202107222345, 17.0.0-beta+31.0.202107202346, 17.0.0-beta+31.0.202107152349, 17.0.0-beta+30.0.202107132351, 17.0.0-beta+30.0.202107082345, 17.0.0-beta+29.0.202107062349, 17.0.0-beta+29.0.202107012342, 17.0.0-beta+28.0.202106300010, 17.0.0-beta+28.0.202106242342, 17.0.0-beta+27.0.202106230001, 17.0.0-beta+27.0.202106180017, 17.0.0-beta+26.0.202106152346, 17.0.0-beta+26.0.202106111624, 16.0.2-beta+7.0.202108042340, 16.0.2-beta+7.0.202108022341, 16.0.2-beta+7.0.202107302341, 16.0.2-beta+7.0.202107301342, 16.0.2-beta+7.0.202107262340, 16.0.2-beta+7.0.202107232341, 16.0.1-beta+9.0.202107192342, 16.0.1-beta+9.0.202107162342, 16.0.1-beta+9.0.202107122342, 16.0.1-beta+9.0.202107092340, 16.0.1-beta+9.0.202107080019, 16.0.1-beta+9.0.202107052341, 16.0.1-beta+9.0.202107022341, 16.0.1-beta+9.0.202106302342, 16.0.1-beta+9.0.202106282345, 16.0.1-beta+9.0.202106252343, 16.0.1-beta+9.0.202106232342, 16.0.1-beta+9.0.202106212342, 16.0.1-beta+9.0.202106182343, 16.0.1-beta+9.0.202106162345, 16.0.1-beta+9.0.202106142345, 16.0.1-beta+9.0.202106112340, 11.0.12-beta+7.0.202107301536, 11.0.12-beta+7.0.202107301536, 11.0.12-beta+7.0.202107221816, 11.0.12-beta+6.0.202107201816, 11.0.12-beta+6.0.202107151815, 11.0.12-beta+6.0.202107131816, 11.0.12-beta+6.0.202107081816, 11.0.12-beta+6.0.202107061816, 11.0.12-beta+6.0.202107011816, 11.0.12-beta+5.0.202106291818, 11.0.12-beta+5.0.202106241920, 11.0.12-beta+5.0.202106221816, 11.0.12-beta+5.0.202106210907, 11.0.12-beta+5.0.202106180009, 11.0.12-beta+5.0.202106151820, 11.0.12-beta+5.0.202106101820, 8.0.302-beta+8.0.202108041822, 8.0.302-beta+8.0.202108021822, 8.0.302-beta+8.0.202107301822, 8.0.302-beta+8.0.202107301121, 8.0.302-beta+8.0.202107261822, 8.0.302-beta+8.0.202107231822, 8.0.302-beta+7.0.202107191822, 8.0.302-beta+7.0.202107161825, 8.0.302-beta+7.0.202107141618, 8.0.302-beta+7.0.202107091822, 8.0.302-beta+7.0.202107071942, 8.0.302-beta+7.0.202107051824, 8.0.302-beta+7.0.202107021823, 8.0.302-beta+7.0.202106301826, 8.0.302-beta+6.0.202106281824, 8.0.302-beta+6.0.202106251823, 8.0.302-beta+6.0.202106231832, 8.0.302-beta+5.0.202106211822, 8.0.302-beta+5.0.202106181822, 8.0.302-beta+5.0.202106161823, 8.0.302-beta+5.0.202106150453, 8.0.302-beta+5.0.202106111823

https://github.com/actions/setup-java/runs/3250670822?check_suite_focus=true

I'll add handling for the beta suffix. 🙃

@maxim-lobanov
Copy link
Contributor

@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 17-beta as stable version and fail to find stable 17 version.

I have a quick fix for this issue. Let me contribute to your PR.

@maxim-lobanov
Copy link
Contributor

@joschi, pushed changes.
See these 2 commits:

Let's see how it goes.

@maxim-lobanov
Copy link
Contributor

maxim-lobanov commented Aug 5, 2021

@joschi, Okay, looks like all tests are green and pass successfully
I also validate it from my side with running setup-java twice per job to see that java is cached properly in toolcache:

This way we keep -ea input format for beta versions to be consistent with adopt / zulu and readme in this repo.

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.

@joschi
Copy link
Contributor Author

joschi commented Aug 5, 2021

@maxim-lobanov Thanks for your help!

Let's get this merged. 😀

@maxim-lobanov maxim-lobanov merged commit 4b1b3d4 into actions:main Aug 6, 2021
@maxim-lobanov
Copy link
Contributor

@joschi , thank you for contribution! Merged PR.

We will release new task version on Monday because we are trying to avoid releases on Friday!

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.

9 participants