-
Notifications
You must be signed in to change notification settings - Fork 64
JOHNZON-305 : Java Modules #99
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
JOHNZON-305 : Java Modules #99
Conversation
rmannibucau
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.
Think we should align info style+fix the naming for the asf
| * under the License. | ||
| */ | ||
| module johnzon.jaxrs { | ||
| requires java.xml; |
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.
This part is optional
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| module johnzon.core { |
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.
Please use org.apache in the naming
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.
sure no problem, so for this one I'll update it to be org.apache.johnzon.core and the same convention for the others
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.
done
johnzon-jsonb/pom.xml
Outdated
| <artifactId>maven-bundle-plugin</artifactId> | ||
| <configuration> | ||
| <instructions> | ||
| <Automatic-Module-Name>johnzon.jsonb</Automatic-Module-Name> |
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.
To drop if we use module-info?
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.
yes using a module-info file is a better option if possible.
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.
Well module-info or automatic name but all modules should have the same option (it is the key). module-info has some pitfalls and will break some integration - why we dont have it yet - but since we move to 2.0 very soon I guess door can be opened now if preferred but clearly a single solution for the full project 🙏 .
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.
ok for this PR I'll keep it to simply using the Automatic-Module-Name approach. With the goal of moving over to using a module-info file in the future.
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.
done
now that Java 11 is the minimum adding proper support for JPMS using a module-info.java instead of specifying Automatic-Module-Name is preferable. Unfortunately this will involve a fair amount of refactoring so this PR is a mix of both approaches.
As mentioned in JOHNZON-305, getting the Automatic-Module-Name configured using maven-jar-plugin was not working due to the bundle plugin. I ended up setting the auto module name using the maven-bundle-plugin itself in
mostall cases.The core module is done properly with a module-info.java but adding that to all the maven modules won't work without a fairly large PR. I'd like to know if there's interest in merging this before making further changes.In the projects that I've done previously with JPMS I've generally gone with the approach of putting unit tests into a different package name to prevent the split packages problem. For example, all the tests under org.apache.johnzon.core I'd change the package to something like ut.johnzon.core. That way a module-info.java can be defined in the main source and the test source. I noticed there are some tests in Johnzon that rely on test classes being in the same package as source files to get access to package private code. That would need some re-work.
Getting johnzon.core working with JPMS was fairly straight forward. I have a module info locally for johnzon.mapper that essentially works when compiling the main source but could not include it in the PR due to JsonGeneratorCloseTest using test code in the org.apache.johnzon.core package. That would be along the lines of: