#53 Add option to specify JVM version for add-module-info. Fixes #53 #54
Conversation
There was a problem hiding this comment.
@beikov LGTM overall, thanks a lot! Just one remark inline. And could you also add a description of the new option to the goal's section in README.md (which is the authorative documentation for ModiTect for the time being)?
| Manifest manifest = new Manifest(); | ||
| manifest.getMainAttributes().put( Attributes.Name.MANIFEST_VERSION, "1.0" ); | ||
| if ( mrJar ) { | ||
| manifest.getMainAttributes().put( Attributes.Name.MULTI_RELEASE, "true" ); |
There was a problem hiding this comment.
Should we add this attribute to amended JARs (if not present yet) also in the actual Maven goal, if jvmVersion is given?
There was a problem hiding this comment.
Not sure what you mean. Could you please be more specific?
There was a problem hiding this comment.
Note that we can't always add this attribute because of https://bugs.openjdk.java.net/browse/JDK-8193802
There was a problem hiding this comment.
What I meant is, is this attribute needed for a JAR to be properly recognized as MR JAR? Or will the JVM also load class files from META-INF/versions/... if that attribute is not present?
About the referenced JDK bug, I'm not quite clear what is triggering it?
There was a problem hiding this comment.
The JVM needs this attribute to be set, otherwise classes located in META-INF/versions aren't considered. If there is no content in META-INF/versions and the attribute is set, Java 9 and 10 JVMs will throw the NPE described in the issue when reading such a JAR.
There was a problem hiding this comment.
Ok, so the add-module-info goal should set the attribute then if the jvmVersion is given and the patched JAR hasn't been an MR JAR before.
There was a problem hiding this comment.
Ok I'll add that in a second.
|
The AddModuleInfo now marks the JAR as Multi-Release JAR if a specific jvm version is selected. I also added documentation about that configuration element. |
|
Excellent; rebased and applied. Thanks, @beikov! I'll try and do another Beta release later this week. |
|
Thanks :) |
Fixes #53