[BUILD] update build to require Java 11+ for building about moditect HOT 23 OPEN

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.

from moditect.

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 🤷‍♂️

from moditect.

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. :)

from moditect.

@aalmiray it never worked on Java 8.

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

from moditect.

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).

from moditect.

@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.

from moditect.

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?

from moditect.

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.

from moditect.

from moditect.

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:

and here:

This is the actual root cause:

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

from moditect.

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).

from moditect.

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/

from moditect.

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.

from moditect.

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.

from moditect.

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.

from moditect.

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

from moditect.

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.

from moditect.

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.

from moditect.

Options:

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

from moditect.

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.

from moditect.

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...

from moditect.

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+.

from moditect.

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

from moditect.