-
Notifications
You must be signed in to change notification settings - Fork 39
Fix module path in JDK 24 and older #101
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
base: master
Are you sure you want to change the base?
Conversation
Note that the JLink test only works for JDK 25 and above
- Added integration test for classpath usage - Test using value extractor in main - Remove service provider entries in module-info to avoid optional dependency issue in JDK 24 and older
src/it/jakarta-jar-classpath/pom.xml
Outdated
| <version>3.6.2</version> | ||
| <executions> | ||
| <execution> | ||
| <id>run-jlink-main</id> |
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.
Rename (or remove) this since there isn't any jlink here.
| <version>3.6.2</version> | ||
| <executions> | ||
| <execution> | ||
| <id>run-jlink-main</id> |
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.
Rename (or remove) this since there isn't any jlink here.
|
|
||
| exports org.openapitools.jackson.nullable; | ||
|
|
||
| provides com.fasterxml.jackson.databind.Module with org.openapitools.jackson.nullable.JsonNullableModule; |
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.
Does this one need to be removed? It's not related to javax/jakarta, so I don't think that it should be a problem here (unless I'm misunderstanding the root problem).
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.
I was testing some other things and forgot to add it back :)
|
|
||
| provides com.fasterxml.jackson.databind.Module with org.openapitools.jackson.nullable.JsonNullableModule; | ||
| provides javax.validation.valueextraction.ValueExtractor with org.openapitools.jackson.nullable.JsonNullableValueExtractor; | ||
| provides jakarta.validation.valueextraction.ValueExtractor with org.openapitools.jackson.nullable.JsonNullableJakartaValueExtractor; |
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.
Maybe leave this in, but commented out that it can be re-added when either of the below are true.
- Baseline JDK is 25 (to avoid telling users that they can build with 25+, and still use something lower).
- Javax is dropped, so that there isn't a need for anything optional.
Just a suggestion, but not a problem either way.
This PR removes the SPI entries for
ValueExtractors, since they are optional dependencies and there is a bug that was fixed in JDK 25 (https://bugs.openjdk.org/browse/JDK-8347915).Changes:
mvn wrapper:wrappersince I had some problems with it. I do not know what the problem was, but it was fixed by updating.integration-testMaven profileJsonNullableJakartaValueExtractorJsonNullableJakartaValueExtractor--module-pathJsonNullableJakartaValueExtractor--class-pathJsonNullableJakartaValueExtractorvia theServiceLoadercloses #100