-
Notifications
You must be signed in to change notification settings - Fork 989
feat: Java 25 optimized vm options in startup scripts #9367
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
feat: Java 25 optimized vm options in startup scripts #9367
Conversation
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
-- Download platform specific goss wrappers in build.gradle instead of CI Signed-off-by: Usman Saleem <[email protected]>
…cker Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
…nymore Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
…cker Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR upgrades Besu's Docker images to use Java 25 (Eclipse Temurin JRE) instead of the previously embedded OpenJDK 21. Due to JEP-493 restrictions preventing jlink usage, the JRE is now copied directly from the Temurin base image. The PR also refactors Docker testing infrastructure by moving goss/dgoss downloads from CI workflows into the Gradle build script, enabling local test execution.
Key Changes:
- Upgraded Docker images from OpenJDK 21 to Eclipse Temurin JRE 25
- Added Java version detection and conditional JVM options for Java 25+ in startup scripts
- Moved goss/dgoss download logic from GitHub Actions workflows to Gradle build script
Reviewed Changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docker/Dockerfile | Updated to multi-stage build copying Temurin 25 JRE; removed OpenJDK 21 apt installation |
| ethereum/evmtool/src/main/docker/Dockerfile | Similar updates for evmtool image using Temurin 25 JRE |
| app/src/main/scripts/unixStartScript.txt | Added Java version detection to conditionally apply Java 25 JVM options |
| app/src/main/scripts/windowsStartScript.txt | Added Java version detection for Windows batch script |
| docker/test.sh | Refactored test execution with architecture detection and improved error handling |
| docker/tests/dgoss | Removed dgoss script (now downloaded by Gradle) |
| docker/tests/00/goss.yaml | Removed static Dockerfile tests (moved to runtime validation) |
| docker/tests/01/goss.yaml | Added runtime environment variable validation commands |
| docker/tests/README.md | Added documentation for running Docker tests |
| .github/workflows/*.yml | Removed manual goss installation steps from CI workflows |
| gradle/verification-metadata.xml | Added checksums for gradle-download-task plugin dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
docker/Dockerfile
Outdated
| ENV BESU_PID_PATH "/tmp/pid" | ||
| ENV BESU_RPC_HTTP_HOST="0.0.0.0" | ||
| ENV BESU_RPC_WS_HOST="0.0.0.0" | ||
| ENV BESU_GRAPHQL_HTTP_HOST="0.0.0.0" |
Copilot
AI
Oct 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Adding quotes around environment variable values is inconsistent with the previous unquoted style and unnecessary for simple values. While functionally equivalent, this creates inconsistency in the Dockerfile's style. Consider keeping the original unquoted format unless there's a specific reason for the change.
| ENV BESU_GRAPHQL_HTTP_HOST="0.0.0.0" | |
| ENV BESU_GRAPHQL_HTTP_HOST=0.0.0.0 |
| if %JAVA_VERSION% GEQ 25 ( | ||
| set DEFAULT_JVM_OPTS=@DEFAULT_JVM_OPTS_25@ | ||
| ) else if %JAVA_VERSION% GEQ 21 ( | ||
| set DEFAULT_JVM_OPTS=${defaultJvmOpts} |
Copilot
AI
Oct 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The placeholder @DEFAULT_JVM_OPTS_25@ appears to be a template variable that should be replaced during the build process, but ${defaultJvmOpts} uses a different placeholder format. This inconsistency could lead to confusion. Ensure both placeholders follow the same convention or document why they differ.
| set DEFAULT_JVM_OPTS=${defaultJvmOpts} | |
| set DEFAULT_JVM_OPTS=@DEFAULT_JVM_OPTS_21@ |
|
|
||
| # Add default JVM options here. You can also use JAVA_OPTS and ${optsEnvironmentVar} to pass JVM options to this script. | ||
| if [ "\$JAVA_VERSION" -ge 25 ]; then | ||
| DEFAULT_JVM_OPTS=@DEFAULT_JVM_OPTS_25@ |
Copilot
AI
Oct 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The placeholder @DEFAULT_JVM_OPTS_25@ uses a different format than ${defaultJvmOpts}. This inconsistency in placeholder syntax could indicate a template processing issue or incomplete implementation. Verify that both placeholders are correctly replaced during the build process.
| DEFAULT_JVM_OPTS=@DEFAULT_JVM_OPTS_25@ | |
| DEFAULT_JVM_OPTS=${defaultJvmOpts25} |
…cker Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
|
I think @fab-10 is also looking at that |
fab-10
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As agreed, we want to split in 2 PRs, so this PR should only focus on using the base image to fetch the JDK to included in our Docker image, and then we will apply the Java25 required tuning in a following PR
Signed-off-by: Usman Saleem <[email protected]>
This reverts commit 3ff68bf. Signed-off-by: Usman Saleem <[email protected]>
…cker Signed-off-by: Usman Saleem <[email protected]>
fab-10
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still waiting for some tests to be completed before we can proceed with this
…cker Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
PR description
Update Java 25 performance tuning vm options in startup scripts.
Added Java 25 tuneup options from #9346
Windows batch script verified against Java 21 and Java 25 on
cmd.Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests