[BUILD] update build to require Java 11+ for building

[bmarwell]

replace maven.compiler.source and maven.compiler.target with maven.compiler.release (still pointing to 8), which brings better API checks, Javadoc linting, compilation performance, etc.

PR inc

[aalmiray]

ModiTect must remain Java8 compatible as that's kind of the point of configuring the plugin in a build that has not migrated past Java8.

[bmarwell]

ModiTect must remain Java8 compatible as that's kind of the point of configuring the plugin in a build that has not migrated past Java8.

with maven.compiler.release (still pointing to 8),

I am not sure what you mean 🤷‍♂️

[bmarwell]

Oh. I was moving everything to the Java11 folder, because I saw the ToolProvider:

This won't work, but maven.compiler.source nor .target won't tell you. That's a runtime exception.

I will create a test where I create a module info for snakeyaml using Java 8 to create a showcase. :)

[bmarwell]

@aalmiray it never worked on Java 8.

https://github.com/bmarwell/moditect-java8example/actions/runs/7836458897/job/21384161711

[bmarwell]

So,, an actual fix would be to use my PR as a base (#229) and rework parts of it in a way that for Java 8 no module / ToolProvider import is being used. Maybe split into ModuleInfo and ModuleInfoSource generator.

Or we could drop Java 8 support as no-one ever used it (or complained about it).

[gunnarmorling]

@bmarwell, not all functionality of ModiTect can be used with Java 8, but some can, and that's one of its key features. See the README:

Note that moduleInfoSource and moduleInfoFile can be used on Java 8, allowing to add a Java 9 module descriptor to your JAR also if you did not move to Java 9 for your own build yet. moduleInfo can only be used on Java 9 or later.

In that build you are using moduleInfo, so it is expected that it doesn't work with Java 8. Please do not increase the baseline of ModiTect to Java 11, as it would defeat the purpose of this feature.

[bmarwell]

Please do not increase the baseline of ModiTect to Java 11, as it would defeat the purpose of this feature.

Where do I do that?

[bmarwell]

Module info source doesn't work on 8, because the same class imports Java 9 classes. It never worked.

In the title I explicitly mention to require Java 11 only for building. And in the first comment I said: use "release=8".

I'm not sure how to phrase it otherwise.

I really think it's clear I'm not changing any runtime requirements here, nor any existing functionality.

As my demo shows, moduleInfoSource never worked on Java 8, because of the Java 9 imports. You don't have an IT for this, nad you were using neither the release parameter nor the animal sniffer plugin. It's untested. The readme is wrong, sadly. Or maybe it worked in a previous version.

The commits I did only moved the classes with Java 9+ code to the Java11 folder. This way you can get compile errors.

If you have any questions, let me know.

[gunnarmorling]

Where do I do that?

This was in reply to "Or we could drop Java 8 support".

Module info source doesn't work on 8, because the same class imports Java 9 classes. It never worked.

Have you actually tried it? The imports of a class don't matter for its compatibility (they do not even exist from the perspective of a class file). The actual code path at runtime matters, and if a certain feature doesn't trigger the loading of a Java 9+ class, then it will work with Java 8. If this doesn't work any more, it is a regression which should be fixed. I have used this functionality of ModiTect on Java 8 in the past.

[bmarwell]

Have you actually tried it? The imports of a class don't matter for its compatibility (they do not even exist from the perspective of a class file). The actual code path at runtime matters, and if a certain feature doesn't trigger the loading of a Java 9+ class, then it will work with Java 8.

https://github.com/bmarwell/moditect-java8example/actions/runs/7836458897/job/21384161711

If this doesn't work any more, it is a regression which should be fixed. I have used this functionality of ModiTect on Java 8 in the past.

I haven't look through the git history of that file, but ToolProvider (Java 9) is used in the constructor:
here:

moditect/core/src/main/java/org/moditect/commands/GenerateModuleList.java

Line 47 in be7d707

this.jdeps = ToolProvider

and here:

moditect/core/src/main/java/org/moditect/commands/GenerateModuleInfo.java

Line 93 in be7d707

String autoModuleNameForInputJar = DependencyDescriptor.getAutoModuleNameFromInputJar(inputJar, null);

and

moditect/core/src/main/java/org/moditect/commands/GenerateModuleInfo.java

Line 123 in be7d707

Optional<ToolProvider> jdeps = ToolProvider.findFirst("jdeps");

This is the actual root cause:

moditect/core/src/main/java/org/moditect/model/DependencyDescriptor.java

Line 49 in be7d707

return ModuleFinder.of(path)

This is also the code path I see in my demo project in the exception:

Caused by: java.lang.ClassNotFoundException: java.lang.module.FindException
    at org.codehaus.plexus.classworlds.strategy.SelfFirstStrategy.loadClass (SelfFirstStrategy.java:50)
    at org.codehaus.plexus.classworlds.realm.ClassRealm.unsynchronizedLoadClass (ClassRealm.java:271)
    at org.codehaus.plexus.classworlds.realm.ClassRealm.loadClass (ClassRealm.java:247)
    at org.codehaus.plexus.classworlds.realm.ClassRealm.loadClass (ClassRealm.java:239)
    at org.moditect.commands.GenerateModuleInfo.<init> (GenerateModuleInfo.java:93)
    at org.moditect.mavenplugin.generate.ModuleInfoGenerator.generateModuleInfo (ModuleInfoGenerator.java:158)
    at org.moditect.mavenplugin.generate.ModuleInfoGenerator.generateModuleInfo (ModuleInfoGenerator.java:94)
    at org.moditect.mavenplugin.add.AddModuleInfoMojo.getModuleInfoSource (AddModuleInfoMojo.java:372)
    at org.moditect.mavenplugin.add.AddModuleInfoMojo.execute (AddModuleInfoMojo.java:190)
    at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:12

I really think it would make sense to create a new Code Path where no Java 9+ classes are even imported. This, create a ModuleInfoFromSourceGenerator and compile that (maybe even to Java 8, I talked about it with @nipafx a while ago). Just rip out this part of the functionality from existing classes.

Apart from this, I find it much nicer to throw specific exceptions with a helpful message:
https://github.com/moditect/moditect/pull/229/files#diff-836f6fc417d0bd5e752a2e6cd01849a2fc8ba29710d70df90813e855f53b795cR23

[gunnarmorling]

I haven't look through the git history of that file, but ToolProvider (Java 9) is used in the constructor [...]

All these references shouldn't matter for the case where we just compile a module descriptor (see here) either from an external source file (moduleInfoFile option) or an inline verbatim descriptor (moduleInfoSource).

I just tried the latter with Java 8, and it works as expected.

I find it much nicer to throw specific exceptions with a helpful message

Yes, agreed. Not sure though whether it's worth the hassle of moving to a multi-release JAR, though. How is the devexp with that in IDEs for us these days? Does Eclipse support it well, or would there be lots of "duplicate source file" errors? If so, then I'd suggest to just add a manual version check at a suitable place (probably the constructors of the command classes in the core module).

[bmarwell]

I just tried the latter with Java 8, and it works as expected.

Can you tell me what's wrong with this project then?

https://github.com/bmarwell/moditect-java8example/

[gunnarmorling]

See above:

See the README:

Note that moduleInfoSource and moduleInfoFile can be used on Java 8, allowing to add a Java 9 module descriptor to your JAR also if you did not move to Java 9 for your own build yet. moduleInfo can only be used on Java 9 or later.

In that build you are using moduleInfo, so it is expected that it doesn't work with Java 8.

[bmarwell]

In that build you are using moduleInfo, so it is expected that it doesn't work with Java 8.

Yup. Just created a PR to my repo with the fix. Thanks.

I think it is worth investigating where #229 goes (sorry for a bit of a mess in the commits… will squash later).

Not sure why the vert.x test doesn't find the META-INF/java9 dir, though. Will investigate next week. I honestly don't think MR-Jars are a hazzle at all.

[gunnarmorling]

I honestly don't think MR-Jars are a hazzle at all.

As said before, my main question is around dev-exp for the ModiTect maintainers: how does this look like in IDEs, how can the different variants be tested, etc. In general, it helps to discuss these things based on actual facts an observations as much as possible, rather than assumptions. The next question is: what is the actual advantage we perceive from doing this change? Is it actually going to be any simpler than just putting version checks into the four command classes, as suggested before?

Note I don't mean to push back on this just because I don't want to change things. I'd like to make sure though we understand what problem we actually want to solve, and whether any solution we come up with is advantageous in terms of effort/additional complexity vs. gain. What I wouldn't like us to do is moving to MR JARs just for the sake of exploring this technique.

[bmarwell]

Fair point!

Yes, IDE support could be better. But IntelliJ told me (recently on twitter) they are working on IDE support.

The benefits are:

• you can't accidentally use methods for a specific version, which could happen now
• no version checking needed, the JVM will do this for you. In my example there are version-dependent error messages already.
• I (personally) find them easier to maintain, because you can concentrate on the logic without querying for the Java version.

Disadvantages:

• No full IDE support
• Not that common

[gunnarmorling]

Ok, gotcha. Thanks for clarifying! So tbh. if the choice is between doing nothing and keep things as they are (after all, nothing actually is broken here), do a four line change across those command classes, or doing this substantial change here with its implications on our productivity, I am clearly favoring options one or two.

[bmarwell]

Can see why. This also means, we can never use the animal sniffer plugin nor the --release javac parameter.

For this reason, I'd love to see tests running on a Java 8 JVM. That makes the four line change a bit longer, but I honestly don't mind.

[aalmiray]

Options:

• additional test module with toolchains targeting Java 8
• conditional tests that only run when Java 8 is available

[bmarwell]

Options:

* additional test module with toolchains targeting Java 8

* conditional tests that only run when Java 8 is available

Currently, these options will not work (easily), as the license-format plugin MUST be run and requires 11+. A configured skip won't help here, obviously, we would need a parent update.

Otherwise a new integration-tests directory which is a new maven project (I personally don't like that option either).

The easiest way in terms of "lines needed to configure a Java 8 test" is (in my opinion) to combine toolchains with the invoker-plugin. Drawback: Worse IDE support and still a bit to set up.

[antoinesd]

hello, just came to this issue after proposing #237 that requires Java 11 at minimum since it is required by Jakarta EE 10 and Weld.
I get the point with Java 8 support but, since it has entered in extend support 2 years ago, I'm pretty sure that people stuck in Java 8 won't see Moditect as an helper to move to 9+. These guys are not worried by multiple CVE in 8 or are willing to pay a lot of money to not update, so they probably don't give a **** of JPMS ;-).
To be Honest I see Moditect more as a tool to build POC in order to convince (or help) libs and frameworks developer to adopt JPMS in their dev that are already using Java 11+. Weld and other Jakarta EE / Microprofile implementation are a good example...

[aalmiray]

AFAICT the build currently requires Java 9 for running. The maven plugin targets Java 8 for most operations, some operations do require running the target build with Java 9+.

[bmarwell]

It is hard to say for sure without using the --release parameter imho.