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

Added 'adopt-hotspot' and 'adopt-openj9' #155

Merged
merged 2 commits into from May 17, 2021
Merged

Added 'adopt-hotspot' and 'adopt-openj9' #155

merged 2 commits into from May 17, 2021

Conversation

MarcelCoding
Copy link
Contributor

@MarcelCoding MarcelCoding commented Apr 5, 2021

Description:
Added the ability to select the implementation of the adoptopenjdk distributions.

Related issue:
#13 (comment)

Check list:

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

Copy link
Contributor

@maxim-lobanov maxim-lobanov left a comment

Choose a reason for hiding this comment

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

@MarcelCoding Thank you for submitting this great PR!
I have left a few comments. Please take a look.

.github/workflows/e2e-versions.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/distributions/adopt/installer.ts Outdated Show resolved Hide resolved
@MarcelCoding
Copy link
Contributor Author

@maxim-lobanov thanks for your feedback. I hope I changed it as you expected.

@maxim-lobanov
Copy link
Contributor

@MarcelCoding , Yes! It is what I expected. I have left minor nitpick with variable naming.
In general, looks good to me.

@MarcelCoding
Copy link
Contributor Author

I did it in camel case.

@maxim-lobanov
Copy link
Contributor

maxim-lobanov commented Apr 8, 2021

I did it in camel case.

Camel case is definitely more preferable :)
Looks like you didn't run npm run release with latest commit so 2 checks failed

@konradpabjan could you please review this PR too?

@MarcelCoding
Copy link
Contributor Author

Sorry, I did it now.

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.

Looks great! Thanks for the contribution ❤️

As pointed out earlier in some comments by @maxim-lobanov, the test matrix is getting a little bit large but that's up to us to sort out. If we add more distributions then we could easily have 150+ or 200 builds per PR and that's getting into overkill territory that can start to cause delays for other PRs. We need some way better way to control which tests kick off per type of PR

@giltene
Copy link
Contributor

giltene commented Apr 8, 2021

Before adding additional distro names like this incrementally and/or choosing their names on an ad-hoc basis, it would be useful to examine the full set of names expected for the distribution property, thinking a few months and steps ahead. The names chosen will potentially end up being “set in stone”, so thinking of what happens to them e.g. 1 or 2 years down the road is important.

Other (currently available) distributions and possible names to be added include e.g. corretto, liberica, sapmachine, oracle-openjdk.

We should also consider the much anticipated appearance of Eclipse Adoptium related distributions, and the likelihood of multiple distributions being referred to from Eclipse Adoptium, using the Eclipse Adoptium build or quality mechanisms and/or infrastructure. One (but not the only one) of those Adoptium related distributions will be Eclipse Temurin which will likely, over time, replace the current adopt-hotspot name proposed here, but will probably be properly named and referred to as “temurin” or “eclipse-temurin” rather than “adopt” or “adopt-hotspot”. Additional distributions are expected there, including e.g. the newly announced Microsoft build of OpenJDK. (So “microsoft”? “microsoft-
adoptium”? “adoptium-microsoft”?). There may also (likely?) be something else, with a different name, that replaces the currently proposed adopt-openj9 (as AdoptOpenJDK itself transitions away and is displaced by Eclipse Temurin) which may be built and tested using Adoptium project materials, but will be unlikely to be using the Eclipse Temurin name.

@dmitry-shibanov dmitry-shibanov mentioned this pull request Apr 9, 2021
5 tasks
Copy link

@bmarwell bmarwell left a comment

Choose a reason for hiding this comment

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

Just a nit (thanks @dmitry-shibanov for pointing me here).
Otherwise you can also add fixes: #160 to the PR message now :)

docs/advanced-usage.md Outdated Show resolved Hide resolved
@maxim-lobanov
Copy link
Contributor

@MarcelCoding I have left one more comment. We need small trick for HotSpot to make sure that pre-cached versions will be consumed on Hosted runners.

@MarcelCoding
Copy link
Contributor Author

@maxim-lobanov Ok, I fixe it.

@MarcelCoding
Copy link
Contributor Author

and @bmarwell

@MarcelCoding
Copy link
Contributor Author

I don't did a force push, that you can see the changes better. I think you can squash this pr at the end. I added also some tests for the toolchain folder for adopt.

@maxim-lobanov
Copy link
Contributor

@MarcelCoding validated and it works as expected with cache.
@konradpabjan final look from your side after latest changes :) If you don't have concerns, we can proceed with merging / releasing on Monday

@karianna
Copy link

karianna commented Apr 11, 2021

Hi all - Martijn from AdoptOpenJDK here. With the shift to Eclipse Adoptium we won't be providing OpenJ9 based builds going forward (that'll come directly from IBM). I don't want to speak directly for IBM here, but if we could pause on that a few days so that they can chime in? CC @tellison

@maxim-lobanov
Copy link
Contributor

Hello @karianna , Yes, I think it is not a problem to wait a few days

@bmarwell
Copy link

Hi all - Martijn from AdoptOpenJDK here. With the shift to Eclipse Adoptium we won't be providing OpenJ9 based builds going forward (that'll come directly from IBM). I don't want to speak directly for IBM here, but if we could pause on that a few days so that they can chime in? CC @tellison

I do not think this will happen. In that case IBM would have informed me on my business mail. They know that we are using the adoptopenjdk.net API. Are you sure this hasn’t been forgotten to communicate to customers?

@MarcelCoding
Copy link
Contributor Author

@karianna isn't Openj9 in the eclipse foundation?

@karianna
Copy link

karianna commented Apr 12, 2021 via email

@karianna
Copy link

karianna commented Apr 12, 2021 via email

@mstoodle
Copy link

Here's some recent info about access to Eclipse OpenJ9 based JDKs : https://developer.ibm.com/blogs/ibm-joins-eclipse-adoptium-and-offers-free-certified-jdks-with-eclipse-openj9/

I'm afraid there's not much detail in there to help progress the OpenJ9 part of this PR. As far as I know, though, nothing is changing for the April update release coming very soon at AdoptOpenJDK. We'll be providing more details in advance of the release process migrating fully to Eclipse Adoptium. Stay tuned (but don't hold your breath :) )!

@konradpabjan
Copy link
Collaborator

Thanks for the info @karianna @mstoodle

Given that the existing APIs are not being switched over yet (it will probably take some time), I'm inclined to merge these changes in now. Any objections?

The OpenJ9 story is a bit ambiguous at the moment but I don't consider that to be a blocker. If we do need to make changes in the future then I'm sure we'll know well in advance and we'll have plenty of time to react.

@bmarwell
Copy link

Any plans merging this and releasing a v3 @maxim-lobanov?

@maxim-lobanov maxim-lobanov merged commit cbc183b into actions:main May 17, 2021
@maxim-lobanov
Copy link
Contributor

Merged PR and released version v2.1.0 Link. No need to bump major version since there are no breaking changes.
Going to update v2 tag to point to v2.1.0 tomorrow. Will be appreciate for any testing and confirmation that it works as expected.

@sehrope
Copy link

sehrope commented May 18, 2021

After updating the action version to @v2.1.0 I can confirm this worked with both adopt-openj9 and adopt-hotspot.

@maxim-lobanov
Copy link
Contributor

Updated v2 tag

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

9 participants