restructure for modularization. build with jdk11, target 8 #150
Conversation
There was a problem hiding this comment.
I think you could improve the build setup. I'm a little bit confused why you split the build into two maven modules, for the core and tests. My guess is that would only make testing in a modular setting more difficult. Did you run into issues trying to access test libraries?
net.sf.joptsimple/pom.xml
Outdated
| <artifactId>maven-compiler-plugin</artifactId> | ||
| <configuration> | ||
| <source>1.8</source> | ||
| <target>1.8</target> |
There was a problem hiding this comment.
Since there is already a JDK 11 build requirement, you can use the release option to ensure compatibility with Java 8. The --release flag can easily replace animal-sniffer and is very rock-solid.
For example:
<configuration>
<release>8</release> <!-- Instead of source/target -->
</configuration>There was a problem hiding this comment.
Nice -- thanks! Didn't grasp the release config.
net.sf.joptsimple/pom.xml
Outdated
|
|
||
| <plugin> | ||
| <groupId>org.moditect</groupId> | ||
| <artifactId>moditect-maven-plugin</artifactId> |
There was a problem hiding this comment.
The moditect plugin works, but I do not think it is the most bulletproof way to create a module descriptor. Moditect essentially has to duplicate all of the module-related logic in javac.
The maven-compiler-plugin supports compiling the descriptor as shown here: https://maven.apache.org/plugins/maven-compiler-plugin/examples/module-info.html . Just remove the toolchains from that example and it works fine - it compiles the main classes with the intended JDK compatibility and the module descriptor separately. While you can still make mistakes writing the module descriptor, just as you can with moditect, many of those mistakes will become compile-time errors.
Some similar discussion on the use of moditect versus other build architecture can be found here: java9-modularity/gradle-modules-plugin#72
There was a problem hiding this comment.
OK, great! I'll apply this learning to some of my other side projects also.
Nah, just figured I'd work toward having the tests know only about the exported stuff from the main module. Not quite there yet ... I'll implement your other suggestions first. Thanks for the feedback! I'm learning a lot by undertaking this modularization. |
|
@A248 I appreciate the time you took to offer feedback on this PR. Quick question: it's great that the maven-compiler-plugin can be made to compile a module-info.java file though we may be targeting JDK 8 for the bytecode. Should I be making a multi-release JAR, with the module-info tucked under |
|
I don't think a multi-release JAR is necessary. Creating a MRJAR adds a lot of build complexity with very little gain. They also do not play well with IDEs. On the other hand, they provide only marginal benefits for compatibility. The same tools which distaste the module descriptor neither like MRJARs and will often fail on the module descriptor regardless of where it is placed. This is the case with old versions of Jetty and the Android Gradle Plugin. If you needed to target Android Studio and JDK 6 maybe a MRJAR would be a better choice, since those platforms will have incredibly ancient software. Much less so for JDK 8 users. Also, sorry if I seem a little snappy, that's just how I sound some times. You sound a lot friendlier by comparison. |
|
@A248 Thanks again for your feedback. I've updated the configs and some tests in the process. |
@A248 No worries, I didn't get that sense at all. Appreciate the learning opportunities! At your convenience, have a look at recent updates to this PR. I think we're converging on a satisfactory solution. |
There was a problem hiding this comment.
It all looks good to me. I'm sure you are aware the module name you chose is different from the previous Automatic-Module-Name, but that's fine since 6.0 is a new major release.
Also, you are dropping OSGI support, which I personally know little to nothing about.
|
@A248 Thanks! re: OSGi -- you and me both. I suppose OSGi and JPMS metadata can coexist in a JAR, not sure how to get there just yet. |
No description provided.