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

Update DEVELOPER_GUIDE.md instructions for JDK-11 #16533

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

reta
Copy link
Collaborator

@reta reta commented Oct 31, 2024

Description

Update DEVELOPER_GUIDE.md instructions for JDK-11, fixes the annoying deprecation failure:

Executing Gradle on JVM versions 16 and lower has been deprecated. This will fail with an error in Gradle 9.0. Use JVM 17 or greater to execute Gradle. Projects can continue to use older JVM versions via toolchains. Consult the upgrading guide for further information: https://docs.gradle.org/8.10.2/userguide/upgrading_version_8.html#minimum_daemon_jvm_version


> Configure project :
=======================================
OpenSearch Build Hamster says Hello!
  Gradle Version        : 8.10.2
  OS Info               : Linux 6.11.0-9-generic (amd64)
  JDK Version           : 11 (Ubuntu JDK)
  JAVA_HOME             : /usr/lib/jvm/java-11-openjdk-amd64
  Random Testing Seed   : AE535EC00EACC589
  In FIPS 140 mode      : false
=======================================

FAILURE: Build failed with an exception.

* What went wrong:
Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0

Related Issues

N/A (popped up in Slack from community member)

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Member

@andrross andrross left a comment

Choose a reason for hiding this comment

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

Lots of things out of date here. Some file references are now stale (buildSrc/version.properties). The comment about JDK 17 being the bundled JDK is wrong. Could we simplify this to say something to the effect of: "We recommend installing the latest LTS JDK and setting it as JAVA_HOME. To run the full suite of bwc tests you must also install and configure JAVA11_HOME and JAVA17_HOME."

Update: I'm not sure JDK 11 is required, but it seems JDK 17 is.
Update 2: Both JDK 11 and 17 are required for the bwc tests.

DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
@reta
Copy link
Collaborator Author

reta commented Nov 1, 2024

Lots of things out of date here. Some file references are now stale (buildSrc/version.properties). The comment about JDK 17 being the bundled JDK is wrong.

It is, was focusing on immediate "need" but you are 100% right, going over the whole document

@reta reta force-pushed the update.developers.guide branch 2 times, most recently from 07ede3d to 5373441 Compare November 1, 2024 13:23
@reta reta requested a review from andrross November 4, 2024 18:43

#### JDK 17
Copy link
Member

Choose a reason for hiding this comment

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

Should we add JDK17 back as @andrross called out we need 17 for bwc?

Copy link
Collaborator Author

@reta reta Nov 4, 2024

Choose a reason for hiding this comment

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

We don't use JDK-17 anywhere, for 2.x I will update it to JDK-21

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On the second thought, I think JDK-17 should probably stay there, thanks @owaiskazi19 !

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we could simplify the three "JDK" headings down to just this:

#### JDK

OpenSearch requires a minimum of JDK-11 in order to build. However, it is recommended to use a more recent LTS version of the JDK and set it to the JAVA_HOME environment variable. In addition, certain backward compatibility tests check out and compile the previous major version of OpenSearch, and therefore require installing JDK-11 and JDK-17 and setting the JAVA11_HOME and JAVA17_HOME environment variables.

By default, the test tasks use bundled JDK at runtime, which is configured in version catalog gradle/libs.versions.toml.

The existing text has a lot of detail that I think is maybe not necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm wondering if we could simplify the three "JDK" headings down to just this:

JDK

OpenSearch requires a minimum of JDK-11 in order to build. However, it is recommended to use a more recent LTS version of the JDK and set it to the JAVA_HOME environment variable. In addition, certain backward compatibility tests check out and compile the previous major version of OpenSearch, and therefore require installing JDK-11 and JDK-17 and setting the JAVA11_HOME and JAVA17_HOME environment variables.
By default, the test tasks use bundled JDK at runtime, which is configured in version catalog gradle/libs.versions.toml.

The existing text has a lot of detail that I think is maybe not necessary?

There are a few things here that do not play well (hence this pull request):

  • due to Gradle, to build with any JDK below 16, one needs to add -Dorg.gradle.warning.mode=none
  • due to Gradle, build with any JDK above 23 will fail (like 24-ea for example)

So I think we should be very explicit what we support or not, so I could introduce the build matrix instead. Also, the large chunk of this work will be throwaway very soon (after #16366).

If you don't mind, I would prefer to keep this guide exact (with respect to JDK versions) and get back to it once we level up to JDK-21.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that's fair, but what I don't like about the text in this PR is towards the beginning of this section it says:

This means you must have a JDK 11 installed with the environment variable JAVA_HOME referencing the path to Java home for your JDK 11 installation, e.g. JAVA_HOME=/usr/lib/jvm/jdk-11.

Only if you keep reading does it recommend to use Java 17 or 21. And nowhere does it mention the JAVA11_HOME and JAVA17_HOME environment variables which are required for bwc tests. I also don't love the snippets of gradle configuration which is destined to be out of date when things change anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Valid points, reworked these sections, thanks @andrross !

Signed-off-by: Andriy Redko <[email protected]>
DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
Co-authored-by: Andrew Ross <[email protected]>
Signed-off-by: Andriy Redko <[email protected]>
@reta reta requested a review from andrross November 8, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants