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.

Sign up for GitHub

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 an error message to better explain Java 11+ build requirement #2081

Merged
merged 3 commits into from
Nov 18, 2020
Merged

Add an error message to better explain Java 11+ build requirement #2081

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0712201
Add an error message to better explain Java 11+ build requirement
Nov 16, 2020
0427b11
Better wording
Nov 17, 2020
ae314da
Merge
Nov 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ plugins {
id "io.morethan.jmhreport" apply false
}

if (!JavaVersion.current().isJava11Compatible()) {
throw new GradleException("Java 11 or higher is required to build. " +
"Please download it from https://adoptopenjdk.net/. If you believe you already " +
Copy link
Contributor

Choose a reason for hiding this comment

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

I am against this endorsement. Albeit all the great work that AdoptOpenJDK did, I cannot recommend their distribution. At least as they are not TCK-compliant.

Copy link
Contributor Author

@anuraaga anuraaga Nov 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I mainly picked it because Gradle defaults to downloading it, and GitHub Actions has a plan to as well actions/setup-java#98. I don't know why, but I'm personally OK with any URL here as long as it's stable, which official OpenJDK ones aren't (old Java gets removed IIUC).

Copy link
Contributor

Choose a reason for hiding this comment

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

My go-to java distribution is Liberica. Another option is Zulu (which is default for setup-java as of now)

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer Adopt, but what about not including any concrete suggestion? Since this seems to be a point of conflict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked openjdk.java.net again for Java 11 but it seems to be as hidden as ever. So the most standard option doesn't seem great.

I have a personal preference for zulu, mainly because of the TCK thing but see the ecosystem consolidating on adopt (GitHub planning to switch from zulu to adopt does mean something), which is why I went with it here. Admittedly the TCK issue seems theoretical given they are in the end effectively the same codebase, and both have wide adoption.

I think a link is far more user friendly than no link. Given our Gradle build already automatically downloads adopt if Java 11 or 8 isn't installed, I am leaning strongly for not making a change to this url. I also wouldn't revert the change to using Gradle's toolchain feature to automatically download adopt because automatically downloading a missing JDK is way more user friendly than not.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also somewhat random but if someone can help with diffplug/spotless#724 than any Java would work with the build. I want to do it someday (cloned spotless repo :) but probably still will take some time. After that we can remove this message - if anyone has strong issues with the message I encourage them to help with the issue since this is OSS :D

Copy link
Contributor

Choose a reason for hiding this comment

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

How about mentioning several options, so it doesn't look like we're endorsing a single JDK distro?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok to include a link to AdoptOpenJDK. It's great to have a vendor neutral option. And this is only a recommendation for building the project (not for production usage where TCK compliance would be more of an influence).

To make clear that we don't specifically require AdoptOpenJDK:

Please download it from https://adoptopenjdk.net/.

==>

One option is to download it from https://adoptopenjdk.net/.

Copy link
Member

Choose a reason for hiding this comment

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

thought I had seen news on the AdoptOpenJDK TCK front, found it:

https://blog.adoptopenjdk.net/2020/06/adoptopenjdk-to-join-the-eclipse-foundation/

https://projects.eclipse.org/projects/adoptium/governance:

Scope:
Eclipse Adoptium provides runtime binaries that are high performance, enterprise-caliber, cross-platform, open-source licensed, Java SE TCK-tested and compliant for general use across the Java ecosystem

Copy link
Contributor

Choose a reason for hiding this comment

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

the real facepalm is the name "Adoptium"

"have it, please check that the JAVA_HOME environment variable is pointing at the " +
"Java 11 installation.")
}

ext {
opentelemetryProjects = subprojects - project(":opentelemetry-bom")
}
Expand Down