Skip to content
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

Fix Java module descriptor configuration #138

Merged
merged 1 commit into from
May 30, 2022

Conversation

Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented May 28, 2022

Resolves hopefully #120

The workaround needed for the moditect-gradle-plugin issue is a bit brittle, but it seems to work for the current Lingua setup.
Only the regular JAR is a Multi-Release JAR with module descriptor; the created -with-dependencies.jar is not a proper Multi-Release JAR, but that probably does not matter because I think you would normally not use JARs with dependencies as modules.

Also adds a test to verify that the modular JAR can be used by using the incubating Gradle Test Suite feature.

One issue with the moditect-gradle-plugin is that it does not seem to detect incorrect configuration of the module-info.java file, such as missing requires or exports of a non-existent package (though the test suite added by this PR would hopefully catch that). An alternative might be to perform regular compilation of the module-info.java file, similar to what is described in this StackOverflow question but that would require some adjustments to create a Multi-Release JAR.

Comment on lines +13 to +14
// requires com.squareup.moshi;
// requires com.squareup.moshi.kotlin;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moshi already has an Automatic-Module-Name in the latest version on master, but it appears that version has not been released yet.

@@ -0,0 +1,15 @@
module com.github.pemistahl.lingua {
exports com.github.pemistahl.lingua.api;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might also need to export the ...api.io package, in case that is public API.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, the io package is necessary as well. I will merge your PR and make the final adjustments myself.

}

// Module tests require Java 9 or newer to compile and execute
if (JavaVersion.current().isJava9Compatible) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not ideal that this test won't be executed if a user builds the project with JDK 8. It seems to be possible to use a Gradle Toolchain for compilation and execution, but configuration would be a bit cumbersome at the moment because the test suite does not seem to support that directly yet for compilation (would have to specify it separately for the compile task). Should I try to implement that anyway?

Alternatively in the future it could be considered to specify a toolchain for the complete build to make it more reproducible, regardless of which JDK version a user has installed. But that would require changes to the GitHub workflow.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I try to implement that anyway?

No, you don't have to do that. I'm happy with the current solution already.

@@ -98,6 +99,27 @@ tasks.test {
maxParallelForks = 1
}

// Suppress warnings about incubating test suites feature
@Suppress("UnstableApiUsage")
testing {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the test suite feature is stable it might be reasonable to convert the other test definitions to test suites as well.

@pemistahl
Copy link
Owner

Awesome work @Marcono1234. Thank you so much for your help. :) I don't have much knowledge about Java 9 modules yet so I'm very grateful for any help in this regard. 👍

@pemistahl pemistahl merged commit ae953cd into pemistahl:main May 30, 2022
@Marcono1234 Marcono1234 deleted the modular-jar branch May 30, 2022 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants