-
Notifications
You must be signed in to change notification settings - Fork 531
Fix @AutoService classes not being loaded at runtime when using JDK23+ for compilation #11487
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
Conversation
This comment has been minimized.
This comment has been minimized.
|
Trying to reproduce here. (Without any enforcing rules, using a clean build with our current base images)
The problem was us having missed to add the auto-services annotation processor to the Maven compile plugin. See auto-services docs. Because of this mistake, the |
This reverts commit 952617e.
…tation processor #11487 Before JDK 23, the annotation processor for Auto Service was picked up automatically. With newer JDKs, the META-INF/services files were not generated, which made it impossible for the PID Factories to be picked up by the ServiceLoader at runtime. Following the documentation, we seem to have missed adding the annotation processor to the Maven Build Plugin. Adding this missing bit re-enables the auto-generation on newer JDKs. See also: https://github.com/google/auto/blob/bb4d48e4ca0bda0e75b3b67f6ff4f464db9304e4/service/README.md
|
@pdurbin this is ready for review and a very simple fix. IMHO this can be fast tracked, as it doesn't affect any code - just making sure the annotation processor works as intended. I tested this with JDK 17, 21 and 23, using |
|
The failing image builds are probably Docker Hub hickups. "error pulling image configuration: image config verification failed for digest" is not even remotely related to the changes of the build configuration (compile phase, not package phase). |
This comment has been minimized.
This comment has been minimized.
|
I tested with Java 21.0.4-tem and it worked fine. Thanks, @poikilotherm . This is really your PR now and I'd approve it if I could! 😄 I also tweaked the docs to say, "The recommended version is Java 17 because it's the version we test with." And in the dev guide I linked to the PR where we upgraded to Java 17: |
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
|
All tests passed successfully across JDK 17, 21, and 23. No issues found with service loader files or runtime behavior. The PR fix works as intended. Verified following JDKs (17.0.15-amzn, 21.0.7-amzn, 23.0.2-amzn), ensured that files were present. Merging. |
What this PR does / why we need it:
Both @JR-1991 and @ofahimIQSS have had trouble getting the containerized environment working because they had a version of Java that was too new. (For Jan it was Java 23, see discussion.) They're using our dev quickstart:
mvn -Pct clean package docker:runSpecial notes for your reviewer:
None. Merely adding the missing compile plugin config bits according to the auto-services docs.
Suggestions on how to test this:
mvn clean compile. Confirm all the necessary (3) files are present attarget/classes/META-INF/servicesmvn -Pct clean package docker:runand JDK 23 to verify it works now.Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope
Is there a release notes update needed for this change?:
Nope
Additional documentation:
None
We've been using the maven-enforcer-plugin plugin and its requireJavaVersion rule since d6c3f7a but it's set for "Java 17 or newer".In this pull request, I'm suggesting we set it to "Java 17 and not newer". See also "Version Range Specification" at https://maven.apache.org/enforcer/enforcer-rules/versionRanges.htmlI'm also simplifying the syntax because I don't think we need.0in there.Here's how the output looks (as of this pull request) when you try to use a version of Java that's too new:[ERROR] Detected JDK version 21.0.4 (JAVA_HOME=/Users/PDurbin/.sdkman/candidates/java/21.0.4-tem) is not in the allowed range [17,18].