Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a module-info #40

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add a module-info #40

wants to merge 2 commits into from

Conversation

bowbahdoe
Copy link

@bowbahdoe bowbahdoe commented Aug 4, 2023

Addresses #39

Generated module-info looks like this

module kaitai.struct.runtime {
    exports io.kaitai.struct;
    exports io.kaitai.struct.annotations;

}

@bowbahdoe
Copy link
Author

bowbahdoe commented Feb 27, 2024

@generalmimon Is there anything I can do to help you get around to this?

I will send you a pizza.

@bowbahdoe
Copy link
Author

Bump

@generalmimon
Copy link
Member

@bowbahdoe I think the idea is good, the implementation looks probably fine. I'm just curious if moditect-maven-plugin in our case brings any advantage over just including the module-info (which you mentioned in #40 (comment) that moditect-maven-plugin generates) manually. I might be missing something, but it seems to me that the main selling point of moditect-maven-plugin is that it injects the "module info" to dependencies that might still be written for Java 8, but this project doesn't have any runtime dependencies at the moment other than TestNG, which is only a test dependency.

My other remark is that if #39 becomes a functionality that someone (i.e. you) depend on, we should test in CI whether it actually fulfills the purpose you proposed in #39 (i.e. if jlink relies on the module-info stuff, try to build a test runtime image using jlink and then run it to see if it really works, or something like that).

Otherwise, the module info generation performed by moditect-maven-plugin might work now (if you have tested it at least manually) and keep working for a while, but it's only a matter of time when it will break. This is the same point that @GreyCat made in kaitai-io/kaitai_struct_cpp_stl_runtime#48 (comment), and I fully agree with it:

My point on this is very simple: if somebody cares about support of specific OS/platform combination, let's reproduce it in CI. Otherwise, as development progresses, it's a matter of time when it will break again.

@bowbahdoe
Copy link
Author

Yeah so using the moditect-maven-plugin is akin to having the compiler not check import statements and just asserting them via a config file separately. This is not ideal and you generally would want the compiler to check it for you. Its just a compromise to not have to upgrade past Java 8.

If that taboo is broken then yeah - putting a module-info.java directly in the code is ideal. Then the java compiler checks it in the same way it checks String s = 123; and we probably don't need to add a special CI check to make sure it works. Its just something I have found socially hard to push.

Which, honestly, compiling for java 7 is already annoying. If i try to compile the project with a recent JDK I get

error: Source option 7 is no longer supported. Use 8 or later.

I can loop back to this and figure out the proper CI checks soon otherwise.

@generalmimon
Copy link
Member

generalmimon commented Sep 21, 2024

@bowbahdoe:

Its just a compromise to not have to upgrade past Java 8.

That's what I didn't know: if moditect-maven-plugin is required to support Java 8, then we really need it. I thought that perhaps if module-info.java is just an additional file, then it could be copied into the .jar archive and that would be it - Java 8 wouldn't care about this additional file but it would be relevant for Java 9 and later (that was my naive thinking without understanding how it works). But I see that it's not that simple to have support for Java 8 and Java 9+ modules at the same time and it either requires to build a multi-release JAR (https://stackoverflow.com/a/54130116/12940655), or maybe it doesn't (https://aseovic.medium.com/building-java-8-compatible-modular-jars-715b04299a7c, https://medium.com/nipafx-news/incremental-modularization-modular-jars-on-java-9-and-auto-updates-1176cd3a84d6#5709)?

Actually, could you check whether there are any upsides/downsides of doing what https://aseovic.medium.com/building-java-8-compatible-modular-jars-715b04299a7c or https://medium.com/nipafx-news/incremental-modularization-modular-jars-on-java-9-and-auto-updates-1176cd3a84d6#5709 suggest? The first article claims that it's possible to do what we want without multi-release JARs and that they are overkill for this purpose:

Multi-Release JARs in particular are a real pain, don't play well with either IDEs or Maven yet, and seem to be quite an overkill for something this simple.

It's an article from 2018, so the tooling issues might have disappeared since then, but this approach still seems worth at least considering. I personally don't know what's wrong with multi-release JARs (or if this criticism still applies in 2024), but if there's a simpler option that will work just fine, then why not use it. Simpler things usually work reliably and break less often.

Either way, I think that not only will we want to test in CI whether the jlink thing works, but we'll also need to try building a JAR in the CI (as we'll do when publishing a new version to Maven Central) and then testing if it can run on both Java 8 and the newer LTS versions like Java 11, 17 and 21. We've already had a case when the JAR we published to Maven Central was broken on Java 7 and Java 8 due to the combination of improper configuration and the JAR being built on JDK 11 (see #38 (comment)), and we wouldn't know this unless some good person reported it to us (#34). So I would like to prevent this from happening again.

You can use https://github.com/generalmimon/ks-java-runtime-test/blob/8e94a91e215bd8e4c41d9b29e2be37245d944a12/.github/workflows/maven.yml as an inspiration of what building the JAR and then testing it on different versions of Java can look like. It can probably be simpler, because we don't have to support building the JAR on JDK 8 anymore (I assume that it won't be possible to build the modular JAR using JDK 8 anyway, but that's fine). We can only build on one fixed JDK version (at the moment it probably makes the most sense to use JDK 17), but we should verify that the JAR we build this way can run everywhere.


On the topic of dropping support for old Java versions: I don't think we want to drop support for Java 8 right now. Java 8 has not reached end of life yet: Oracle provides Extended Support for Java 8 until the end of 2030, Azul Zulu will support Java 8 until 2030 too, Eclipse Temurin will continue to offer Java 8 until 2026, etc. And https://www.java.com/en/download/ still offers Java 8. Therefore, I assume there is still a significant use of Java 8 in production, so it makes no sense to drop its support.

Which, honestly, compiling for java 7 is already annoying. If i try to compile the project with a recent JDK I get

error: Source option 7 is no longer supported. Use 8 or later.

Dropping Java 7 is probably OK - unlike Java 8, Java 7 seems to be essentially dead now. Here's a graph from https://newrelic.com/resources/report/2024-state-of-the-java-ecosystem:

nr_chart_long_term_support

We can notice that the use of Java 8 is on the decline, but it's still used pretty much as heavily as Java 11 or Java 17. In contrast, the use of Java 7 seems to be negligible.

@generalmimon
Copy link
Member

Surprisingly, my initial intuition that it might be possible to just include the "module info" into the JAR and have Java 8 ignore it but enjoy the benefits of modules in Java 9+ might not be wrong after all. This article claims that you can do exactly that:

Just use jar --update

You can simply use jar --update to add the module descriptor to the JAR:

jar --update --file ${jar_file} module-info.class

This adds module-info.class to ${jar_file}. Note that --update does not perform any checks. This makes it easy to, accidentally or on purpose, create JARs whose module descriptor and class files do not agree, for example on required dependencies. Use with care!

Adding the module descriptor is not a problem for older JVMs because they actually ignore it. They only see other class files and because you build them for the correct version, everything just works.

While that is true for the JVM, it can not necessarily be said for other tools. Some trip over module-info.class and thus become useless for modular JARs. If you want to prevent that you have to create a multi-release JAR.

The question is whether it's a good idea to do that, considering that this non-standard approach might be a problem for other tools as they point out here. It seems that the multi-release JARs are the standard approach, so perhaps it doesn't make sense to avoid them after all. If we'll test in CI that we can build a multi-release JAR that runs smoothly on Java 8, 11, 17 and 21, then I have no problem with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants