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

IBM Semeru (OpenJ9) Support #289

Merged
merged 15 commits into from Mar 21, 2023
Merged

Conversation

jord1e
Copy link
Contributor

@jord1e jord1e commented Feb 3, 2022

Description:
This adds the semeru distribution for the IBM Semeru Runtime shipping with the Eclipse OpenJ9 VM.

As can be read in the Semeru Runtimes getting started guide, IBM does not yet have a public API:

IBM is working to host a RESTful API server that uses the new Eclipse Adoptium™ OpenAPI definition. Until that is ready we offer the IBM Semeru Runtime Open Edition from the RESTful API provided by the AdoptOpenJDK community, and follow the AdoptOpenJDK OpenAPI definition.

When using the AdoptOpenJDK API service to retrieve IBM Semeru Runtimes use "ibm" for the vendor field (vendor) and "openj9" for the JVM implementation field (jvm_impl).

This PR adds the semeru distribution and does exactly as stated above. It gives action/library consumers an option to future-proof their applications (by replacing adopt-openj9). When IBM releases their official API we can use that. As stated in the getting started guide, they will be using the Eclipse Adoptium OpenAPI definition, which is compatible with current changes. Thus we only need to change the URL in the future.

Implementation snoops code from the Adoptium distribution implementation.

Cc-ing/Tagging @mstoodle as you are the lead for Eclipse OpenJ9, and can maybe give a timeline on the Semeru API. Sorry if you are not the correct person.

Related issue:
Fixes #279 #239 (already closed)

Check list:

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

Copy link

@mstoodle mstoodle left a comment

Choose a reason for hiding this comment

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

Since Semeru Runtimes are produced by IBM, I'll comment as the chief architect (not as OpenJ9 project lead) :) . First of all, thanks for putting this all together!

The API for Semeru Runtimes is actually up we just don't have the documentation up yet (hopefully coming soon). It leverages the Adoptium v3 API, of course, so it should be familiar...

Some specific examples:
ibm-semeru-open-jdk_x64_linux_11.0.13_8_openj9-0.29.0.tar.gz:
. curl -OLJk https://ibm.com/semeru-runtimes/api/v3/binary/version/jdk-11.0.13+8_openj9-0.29.0/linux/x64/jdk/openj9/normal/ibm
latest:
. curl -OLJk https://ibm.com/semeru-runtimes/api/v3/binary/latest/11/ga/linux/x64/jdk/openj9/normal/ibm

Schema should be familiar:
. latest: https://ibm.com/semeru-runtimes/api/v3/binary/latest/{feature_version}/{release_type}/{os}/{arch}/{image_type}/{jvm_impl}/{heap_size}/{vendor}
. specific release: https://ibm.com/semeru-runtimes/api/v3/binary/version/{release_name}/{os}/{arch}/{image_type}/{jvm_impl}/{heap_size}/{vendor}

The Certified Edition binaries are also available but aren't available on all platforms so you'll probably want to stick with the Open Edition binaries.

README.md Outdated Show resolved Hide resolved
@jord1e
Copy link
Contributor Author

jord1e commented Feb 4, 2022

Thanks for answering @mstoodle

Since Semeru Runtimes are produced by IBM, I'll comment as the chief architect (not as OpenJ9 project lead) :)

I summoned the big boss himself ;)

It leverages the Adoptium v3 API

Does it support the /assets/version/{version} endpoint?
Using GET https://ibm.com/semeru-runtimes/api/v3/assets/version/%5B1.0,100.0%5D?project=jdk&vendor=ibm&heap_size=normal&sort_method=DEFAULT&sort_order=DESC&os=windows&architecture=x64&image_type=jdk&release_type=ga&jvm_impl=openj9&page_size=20&page=0 I am getting a 400 Bad Request response

GET https://api.adoptopenjdk.net/v3/assets/version/%5B1.0,100.0%5D?project=jdk&vendor=ibm&heap_size=normal&sort_method=DEFAULT&sort_order=DESC&os=windows&architecture=x64&image_type=jdk&release_type=ga&jvm_impl=openj9&page_size=20&page=0 does return correct results

In the current action code we cannot directly use the jdk-11.0.13+8_openj9-0.29.0 version form. We need to use 11.0.13+8.
We also need the /assets/version/{version} to check if versions are available, and to resolve the latest version.

If you do not want to implement the maven version range string ({version}) it is also OK to just create an endpoint with a list of all the available versions and their links et cetra. This will deviate of the AdoptOpenJDK and Adoptium APIs though

we just don't have the documentation up yet

Regarding the above, do you have Swagger docs?

The Certified Edition binaries are also available but aren't available on all platforms so you'll probably want to stick with the Open Edition binaries.

I agree, z/OS is also explicitly disabled in the tests and licensing would probably complicate things

@jord1e jord1e marked this pull request as draft February 4, 2022 12:56
@mstoodle
Copy link

mstoodle commented Feb 4, 2022

I'll defer to @AdamBrousseau to comment on the endpoint you're asking about. On the Swagger docs: it's in the works, but I jumped the gun a bit telling you about the API :) .

src/distributions/semeru/installer.ts Outdated Show resolved Hide resolved
src/distributions/semeru/installer.ts Outdated Show resolved Hide resolved
@AdamBrousseau
Copy link

Does it support the /assets/version/{version} endpoint?

We have forked Adopt's api so it should work the same. That being said our fork is a bit out of date as I had disabled our auto-mirror while we get it up and running. I started to look through the diff to see if there is something we are missing that would explain the 400. Haven't found anything obvious yet. I will follow up and report back.

@jord1e
Copy link
Contributor Author

jord1e commented Mar 3, 2022

Haven't found anything obvious yet. I will follow up and report back.

Do you have any updates for us @AdamBrousseau 🙂?

@guizmaii
Copy link

Any news? I'm eagerly waiting for this! 🙂

@AdamBrousseau
Copy link

Hey @jord1e @guizmaii ,
My apologies, I was not ignoring this. Unfortunately I had not gotten a chance to dig into it further. I spent a bit more time tonight but I have not been able to see why it is failing. I have reached out to the team that hosts the API to see if they can help diagnose from their end.

@jord1e
Copy link
Contributor Author

jord1e commented May 10, 2022

@AdamBrousseau any updates :)?

@bmarwell
Copy link

bmarwell commented Jun 2, 2022

@mstoodle @AdamBrousseau @jord1e @dmitry-shibanov
I think since the market place is now up, we could go forward here?

@jord1e
Copy link
Contributor Author

jord1e commented Jun 3, 2022

I think since the market place is now up, we could go forward here?

https://www.ibm.com/support/pages/semeru-runtimes-getting-started/

Under "Downloading packages" it still lists the AdoptOpenJDK API. Could you send the link to the "market place", I will update the PR on Sunday if I have it before then :).

@AdamBrousseau
Copy link

Now that the Marketplace is up, I'm hoping i can spend some cycles on the Semeru API so we can get it "published" and get this resolved. In the meantime, is there anything preventing you from using the existing AdiotOpenJDK API? It still supports Semeru Open Edition. Does it give the same 400 error as the Semeru API? Also keep in mind the Adoptium Marketplace only promotes Semeru Certified Edition.

@tbradellis
Copy link

We'd really like to see this go in as well at New Relic. We just recently did some work to support these runtimes with our Java Agent and need Semeru available for our test suites.

@jord1e
Copy link
Contributor Author

jord1e commented Sep 16, 2022

Ill try to do it today (have been saying this for a long time, will make a calendar entry).

# Conflicts:
#	.github/workflows/e2e-versions.yml
#	README.md
#	dist/cleanup/index.js
#	dist/setup/index.js
#	src/distributions/distribution-factory.ts
@jord1e jord1e marked this pull request as ready for review September 16, 2022 16:43
@jord1e jord1e requested a review from a team September 16, 2022 16:43
@jord1e
Copy link
Contributor Author

jord1e commented Sep 16, 2022

@dmitry-shibanov cant get the dist to work but all checks are passing on jord1e-forks#1

Can we get one more review (or merge) :)?

@mstoodle
Copy link

mstoodle commented Mar 7, 2023

As an Eclipse OpenJ9 project lead and the chief architect for IBM Semeru Runtimes, I really do appreciate all the effort and persistence that the contributor and supporters have volunteered into this pull request and feel very badly that this effort hasn't yet been rewarded (now at 13 months after the PR was first opened despite all the support and upvoting from others).

This may be a complete red herring (and apologies if it is), but roughly comparing the corresponding Alibaba Dragonwell PR (ominously numbered #444) with this one, I noticed that this PR changes dist/cleanup/index.js and dist/setup/index.js whereas the Alibaba one only changes dist/setup/index.js . That observation leads to a Totally Speculative Question (mostly because 1) the log for the check dist job has expired now so I can't see what was reported to go wrong and 2) the change to dist/cleanup/index.js is huge and confusingly shows changes where things don't appear to have actually changed): is it possible that including these dist/cleanup/index.js changes aren't needed in this PR and are responsible somehow for the problematic "Check dist" test?

@mstoodle
Copy link

mstoodle commented Mar 7, 2023

@brcrista is there still a license concern that needs to be addressed here?

@Accelerator1996
Copy link
Contributor

As an Eclipse OpenJ9 project lead and the chief architect for IBM Semeru Runtimes, I really do appreciate all the effort and persistence that the contributor and supporters have volunteered into this pull request and feel very badly that this effort hasn't yet been rewarded (now at 13 months after the PR was first opened despite all the support and upvoting from others).

This may be a complete red herring (and apologies if it is), but roughly comparing the corresponding Alibaba Dragonwell PR (ominously numbered #444) with this one, I noticed that this PR changes dist/cleanup/index.js and dist/setup/index.js whereas the Alibaba one only changes dist/setup/index.js . That observation leads to a Totally Speculative Question (mostly because 1) the log for the check dist job has expired now so I can't see what was reported to go wrong and 2) the change to dist/cleanup/index.js is huge and confusingly shows changes where things don't appear to have actually changed): is it possible that including these dist/cleanup/index.js changes aren't needed in this PR and are responsible somehow for the problematic "Check dist" test?

Hi, @mstoodle. The problem you mentioned about the lack of dist/cleanup/index.js. I executed the 'ncc build', but my PR will not modify the dist/cleanup/index.js.

@mstoodle
Copy link

mstoodle commented Mar 10, 2023

Hi @Accelerator1996 thanks for commenting. With my earlier comment here, I was just trying to see if we could learn from your PR to help resolve the failing "check dist" test in this PR. Everything I said was pure guesswork.

I wasn't suggesting your PR had any problem, so I'm sorry if I gave you that impression!

@Accelerator1996
Copy link
Contributor

Hi @Accelerator1996 thanks for commenting. With my earlier comment here, I was just trying to see if we could learn from your PR to help resolve the failing "check dist" test in this PR. Everything I said was pure guesswork.

I wasn't suggesting your PR had any problem, so I'm sorry if I gave you that impression!

I have actually been repeatedly comparing the difference between me and other PRs, and learn from them. Thank you very much for your reminder.

# Conflicts:
#	.github/workflows/e2e-versions.yml
#	README.md
#	dist/setup/index.js
#	src/distributions/distribution-factory.ts
@jord1e jord1e requested a review from a team as a code owner March 14, 2023 09:58
@jord1e
Copy link
Contributor Author

jord1e commented Mar 14, 2023

@dmitry-shibanov @brcrista checks are passing on my branch thanks to @thc202 suggesting of using the Actions artefacts.

jord1e-forks#1

Could you please activate the workflow and merge this?

@IvanZosimov
Copy link
Contributor

Hi, @jord1e 👋 Thank for the PR update! Please take a look at these changes which we suggest to add to this PR.

@jord1e
Copy link
Contributor Author

jord1e commented Mar 14, 2023

Hi @IvanZosimov 🙂
I merged the changes, LookedGTM - thank you.

CC @mstoodle

Copy link
Contributor

@marko-zivic-93 marko-zivic-93 left a comment

Choose a reason for hiding this comment

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

LGTM!

@samvantran
Copy link

Hi @mstoodle and @AdamBrousseau, looks like this PR is good to merge and license-wise is A-okay. Just want to get a confirmation from either of you as IBM employees (or anyone else you think more appropriate) that you support merging in this PR and Github including the Semeru distribution in setup-java. Please add a comment if you do.

@mstoodle
Copy link

@samvantran I support merging this PR absolutely, 100%, with no reservations.

@jord1e
Copy link
Contributor Author

jord1e commented Mar 16, 2023

🚀 13 months, ready for takeoff!

@sideeffffect
Copy link

The installers are still downloaded from the deprecated AdoptOpenJDK. It would be good to switch to the official source.

https://github.com/actions/setup-java/pull/289/files#r979462471

@bmarwell
Copy link

The installers are still downloaded from the deprecated AdoptOpenJDK. It would be good to switch to the official source.

https://github.com/actions/setup-java/pull/289/files#r979462471

Yes, but can we PLEASE just merge this and fix this issue in a new PR???

@dmitry-shibanov dmitry-shibanov merged commit 5ffc13f into actions:main Mar 21, 2023
@mstoodle
Copy link

Thanks everyone for making this happen!! Woohoo!!!

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.

Support Semeru distribution