Add module-info via moditect-maven-plugin, compile to java 8

[rbygrave]

Removes Automatic-Module-Name manifest entry and replaces it with module-info via multi-version jar

[overheadhunter]

Since #19 is now closed, I want to make sure to emphasize the order to merge and release this PR before #21:

We would then get the maintainers to release that before we then merge this second PR which bumps to Java 9 11 and get the maintainers to release again a second time.

I believe it is important to have a Java 8 compatible modularized version of this library, as many downstream libs still target Java 8.

[Emily-Jiang]

I think other specs put this files under src/main/java (see here)

[Emily-Jiang]

Can we follow the same convention?

[overheadhunter]

yasson is already on Java 9. Java 8's javac would choke on this file, if it tried to compile it. Therefore, the multi-version layout is required.

[Ladicek]

Yea putting this into java9 (or module or something) is the right thing to do here, as we want to build the main source with Java 8.

[Ladicek]

I believe this is exactly what we want -- build the main source with Java 8, produce a Java 8 compatible JAR, but also include module-info.class.

[overheadhunter]

Are we sure, we don't need to set Multi-Release: true in the jar manifest? Is the module-info.class placed in the jar's root and still ignored by JREs < 9?

[Ladicek]

Pretty sure we don't have to set the Multi-Release attribute, because it is not a MR JAR (there's no META-INF/versions). The module-info.class is present in the root of the JAR and is version 53 (Java 9). All other classes are version 52 (Java 8).

To the best of my knowledge, Java 8 has no reason to load the module-info.class class file, if only because its name is not denotable in the language. (You may be able to handcraft some bytecode that refers to a class with the name module-info, and I have no idea what would happen then :-) )

[overheadhunter]

(You may be able to handcraft some bytecode that refers to a class with the name module-info, and I have no idea what would happen then :-) )

One should not access it, however some build tools seem to be too fancy to adhere specs: See google/guava#2970 (comment) (and the following discussion)

[rbygrave]

One should not access it, however ...

I note that Jackson has module-info without MR jar so I suspect problems with this approach have to be very rare indeed - but that all said, the more conservative and safe approach is to modify this PR back to use Multi-release Jar and I'm happy to do that, it's a one line change to this PR.

So, shall we go a touch more conservative and use MR jar?

Edit:
I've just pushed a commit that puts this back to MR jar as I suspect that is more likely to be desired/approved. Let me know if I should remove that and we instead do not want MR Jar.

Is everyone happy using MR jar? Any reason not to approve this PR?

[Ladicek]

Frankly, if there's a tool that finds and uses /module-info.class, that tool will most likely also find and use /META-INF/versions/9/module-info.class. I just don't see the need to make this a MR JAR.

[Ladicek]

That said, there's nothing wrong with this approach, and there's no extra complexity in the build, so if this is what everyone wants, I'm fine with it too.

[overheadhunter]

Frankly, if there's a tool that finds and uses /module-info.class, that tool will most likely also find and use /META-INF/versions/9/module-info.class. I just don't see the need to make this a MR JAR.

You're probably right. Also, Jackson (as mentioned above) strongly indicates that in the meantime most tools should be fixed.

But if MR-Jars were even slightly more forgiving, it might make a difference for other library vendors, that would otherwise not dare to update injection-api out of fear of breaking something downstream.

[rbygrave]

Maybe just looking for approvals then @Emily-Jiang @overheadhunter @Ladicek ?

[Emily-Jiang]

Pretty sure we don't have to set the Multi-Release attribute, because it is not a MR JAR (there's no META-INF/versions). The module-info.class is present in the root of the JAR and is version 53 (Java 9). All other classes are version 52 (Java 8).

To the best of my knowledge, Java 8 has no reason to load the module-info.class class file, if only because its name is not denotable in the language. (You may be able to handcraft some bytecode that refers to a class with the name module-info, and I have no idea what would happen then :-) )

IIRC, the file structure is pretty much MR jar layout. I found it is confusing not to state it is a MR jar but place module-info.class using the MR layout (my earlier comment reflected my confusion since I thought it was not a MR jar). I am not sure the module-info.class can be easily loaded by Java 9+ at all. I think it is safer to set as a MR jar and be done with it.

[Ladicek]

This PR originally produced a MR JAR. Then it wasn't a MR JAR (which is when I approved and also wrote that comment), and now it is a MR JAR again.

[rbygrave]

I think it is safer to set as a MR jar and be done with it.

Hi @Emily-Jiang , as @Ladicek has pointed out this produces a MR jar.

To explain that a little more, this is compiling to Java 8 bytecode. We can't use the JDK8 javac to compile module-info.java obviously. Instead this uses the moditect-maven-plugin to compile module-info.java and put it into the jar creating a MR jar. The <jvmVersion>9</jvmVersion> configuration option is what specifies to make it a MR jar with the result that the jar contains /META-INF/versions/9/module-info.class (Multi-release jar containing java 9 bytecode for module-info.class).

Personally, I have used the moditect-maven-plugin a lot myself as it's a really nice and simple way to add proper module-info to projects that still need to compile to java 8. Let me know if you have any questions.

Thanks, Rob.

[rbygrave]

@pzygielo done, title updated.

[Emily-Jiang]

I think it is safer to set as a MR jar and be done with it.

Hi @Emily-Jiang , as @Ladicek has pointed out this produces a MR jar.

To explain that a little more, this is compiling to Java 8 bytecode. We can't use the JDK8 javac to compile module-info.java obviously. Instead this uses the moditect-maven-plugin to compile module-info.java and put it into the jar creating a MR jar. The <jvmVersion>9</jvmVersion> configuration option is what specifies to make it a MR jar with the result that the jar contains /META-INF/versions/9/module-info.class (Multi-release jar containing java 9 bytecode for module-info.class).

Personally, I have used the moditect-maven-plugin a lot myself as it's a really nice and simple way to add proper module-info to projects that still need to compile to java 8. Let me know if you have any questions.

Thanks, Rob.

Thank you @rbygrave for your detail explanation! Let me relay to see whether I understood this correctly. You said it is not a MR jar because there is no META-INF/module-info.class and we only have /META-INF/versions/9/module-info.class. It does not need to be marked as a MR jar as there is no duplicated module-info.class. For Java 9+, JVM should find module-info.class despite it is under META-INF/versions/9/module-info.class. If I understood correctly, this does make sense.

[rbygrave]

we only have /META-INF/versions/9/module-info.class.

There is only one module-info.class in the jar and yes it is located at /META-INF/versions/9/module-info.class

It does not need to be marked as a MR jar

In the resulting /META-INF/MANIFEST.MF it contains the Multi-Release: true entry. So I think it is more accurate to say that it IS marked as a MR jar. It is a MR jar with the manifest containing Multi-Release: true plus it has /META-INF/versions/9/module-info.class

That is, if we pull this branch and mvn clean package and look at the resulting target/jakarta.inject-api-2.0.1-SNAPSHOT.jar we see that the manifest is as expected with the Multi-Release: true entry plus we see the /META-INF/versions/9/module-info.class

as there is no duplicated module-info.class.

Correct, there is no duplicated module-info class @Emily-Jiang

[Emily-Jiang]

we only have /META-INF/versions/9/module-info.class.

There is only one module-info.class in the jar and yes it is located at /META-INF/versions/9/module-info.class

It does not need to be marked as a MR jar

In the resulting /META-INF/MANIFEST.MF it contains the Multi-Release: true entry. So I think it is more accurate to say that it IS marked as a MR jar. It is a MR jar with the manifest containing Multi-Release: true plus it has /META-INF/versions/9/module-info.class

This is what I thought as a MR jar. Then I read @Ladicek 's comments , which he hinted he approved with a non-MR jar approach. I might completely misunderstood what it meant. However, I am happy with the final approach.

That is, if we pull this branch and mvn clean package and look at the resulting target/jakarta.inject-api-2.0.1-SNAPSHOT.jar we see that the manifest is as expected with the Multi-Release: true entry plus we see the /META-INF/versions/9/module-info.class

as there is no duplicated module-info.class.

Correct, there is no duplicated module-info class @Emily-Jiang

In summary, I am happy with the fact of being a MR jar and the entry of /META-INF/versions/9/module-info.class as I think this class will be loaded by Java 9+ with that marker.

[Emily-Jiang]

LGTM! Thank you @rbygrave for your contribution!

[rbygrave]

Hi Scott @starksm64 , I am looking to find out what the next step is to get this PR merged (and ideally release 2.0.1 with a module-info to maven central). I see you have merged commits so perhaps you can help me / let me know what the next step is?

Thanks, Rob.

[rbygrave]

Hi @Cousjava I am trying to determine the next step to getting this PR merged. Are you able to help?

Thanks, Rob.

[Emily-Jiang]

I'm a committer on this project and will merge this PR.

[rbygrave]

FYI: Adding cross reference to issue #17