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

Merged
merged 3 commits into from Nov 18, 2020

Conversation

anuraaga
Copy link
Contributor

No description provided.

build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
@Oberon00
Copy link
Member

Partial "fix" for #2080

build.gradle Outdated
@@ -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

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"

@carlosalberto
Copy link
Contributor

Not a strong opinion on the default recommendation - the actual error message will be super helpful though.

Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

Thanks!

@jkwatson jkwatson merged commit 5528fe8 into open-telemetry:master Nov 18, 2020
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

7 participants