Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the project to compile with OpenJDK 17, replacing the deprecated javah tool with javac -h for generating native headers. The changes upgrade build tools, CI configuration, and developer email information to support the modern JDK version.
Changes:
- Updated Maven plugins and dependencies to versions compatible with OpenJDK 17
- Migrated CI/CD configuration from OpenJDK 8 to OpenJDK 17
- Updated email address for developer Ronny Trommer
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pom.xml.in | Updated Maven plugins, changed execution environment to JavaSE-17, and replaced source/target with release configuration |
| m4 | Updated submodule commit reference |
| .circleci/config.yml | Updated CI images and JDK versions from 8 to 17, changed Debian from buster to bookworm, and updated repository URLs to HTTPS |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c81f42c to
664905e
Compare
Signed-off-by: Ronny Trommer <ronny@no42.org>
Signed-off-by: Ronny Trommer <ronny@no42.org>
Signed-off-by: Ronny Trommer <ronny@no42.org>
Signed-off-by: Ronny Trommer <ronny@no42.org>
Signed-off-by: Ronny Trommer <ronny@no42.org>
…:17.0-node Signed-off-by: Ronny Trommer <ronny@no42.org>
Signed-off-by: Ronny Trommer <ronny@no42.org>
Signed-off-by: Ronny Trommer <ronny@no42.org>
d784672 to
3ad229f
Compare
Signed-off-by: Ronny Trommer <ronny@no42.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| java.built: $(JAVA_FILES) pom.xml | ||
| -mkdir -p $(classdir) | ||
| $(JAVAC) $(JAVACFLAGS) -source $(JAVA_TARGET_VERSION) -target $(JAVA_TARGET_VERSION) -d $(classdir) $(JAVA_FILES) && touch java.built | ||
| $(JAVAC) $(JAVACFLAGS) --release $(JAVA_TARGET_VERSION) -h . -d $(classdir) $(JAVA_FILES) && touch java.built |
There was a problem hiding this comment.
java.built now uses javac -h, but the Makefile still generates org_opennms_protocols_icmp6_ICMPv6Socket.h via $(JAVAH) in the next rule. Since javah is removed in JDK 10+, this will break builds on JDK 17. Remove the $(JAVAH)-based rule and make the header target a byproduct of the javac -h invocation (and keep the dependency so C compilation waits for the header to exist).
| apt-get update | ||
| apt-get -y install openjdk-8-jdk-headless | ||
| rm -rf /etc/ssl/certs/java/cacerts | ||
| update-ca-certificates | ||
| apt-get -y install openjdk-17-jdk-headless |
There was a problem hiding this comment.
This script now runs update-ca-certificates, but ca-certificates is not installed anywhere in build-all.sh and apt-get install uses --no-install-recommends (so it may be missing on minimal Debian images). Install ca-certificates explicitly (or guard the call) so the Docker build doesn’t fail with update-ca-certificates: command not found.
| platform: | ||
| type: enum | ||
| enum: ["arm32v7", "arm64v8", "i386", "amd64"] | ||
| enum: ["arm64v8", "amd64"] |
There was a problem hiding this comment.
do-docker-build enumerates arm64v8, but the subsequent case statement matches arm64) (not arm64v8)), so PLATFORM will be empty for the arm build and the Docker run will fail. Align the enum values and the case patterns (e.g., use arm64v8) here or change the enum to arm64).
| package-path: ~/artifacts/rpm/jicmp6-*.armv7hl.rpm | ||
| - cloudsmith/publish: | ||
| allow-republish: true | ||
| cloudsmith-repository: << parameters.target_repository >> | ||
| package-format: rpm | ||
| package-distribution: any-distro/any-version |
There was a problem hiding this comment.
The build matrices remove arm32v7, but the publish step still tries to publish an armv7hl RPM (jicmp6-*.armv7hl.rpm). This artifact won’t exist anymore and will make the publish job fail; remove this publish entry or re-enable the arm32 build.
| package-path: ~/artifacts/rpm/jicmp6-*.armv7hl.rpm | |
| - cloudsmith/publish: | |
| allow-republish: true | |
| cloudsmith-repository: << parameters.target_repository >> | |
| package-format: rpm | |
| package-distribution: any-distro/any-version |
| parameters: | ||
| debian-version: | ||
| type: string | ||
| default: buster | ||
| default: bookworm | ||
| maven-version: |
There was a problem hiding this comment.
debian-version is now only defined (default bookworm) but no longer referenced anywhere else in the config. Either remove the parameter to avoid confusion, or wire it back into places that are currently hard-coded to bookworm (e.g., the Docker image tag).
mershad-manesh
left a comment
There was a problem hiding this comment.
the changes looks good to me. Should we create one that supports Java 21?
Migrate javah to javac -h to compile with modern JDK. The javah tool to generate native-headers is removed in OpenJDK 10+ and replaced with javac -h. See JEP 313.
JIRA: https://opennms.atlassian.net/browse/JICMP-27