Add Java module definitions #1028

A248 · 2025-09-15T21:33:44Z

Aims to solve #1012

I added Java module definitions where I thought appropriate. Several of the subprojects aren't designed for external consumption, such as test suites and benchmarks.

Packages named "internal" I treated as internal to chicory, i.e. not intended for external consumption.

Additionally, I did my best to check when types from a dependency were exposed in a module's public API, in order to use requires transitive appropriately. It's possible I missed a spot, but this will never be breaking and will result in at most a compiler warning (on chicory's side) or the need to write another requires (on the library user's side).

Checkstyle

Chicory was still using checkstyle 9.3, which is still the default version in the maven-checkstyle-plugin. Because this old checkstyle version was running into problems consuming module-info.java, I increased the version to the latest.

This required checking for breaking changes in checkstyle, which I did by looking at every release since 9.3. Checkstyle does not follow SemVer so you have to look at every release notes.

Terminology

For people reading this PR, the PR itself targets the Java Modules introduced in Java 9. They are a separate concept from WASM modules.

andreaTP

Thanks a lot @A248 !
Much appreciated!

Skimming this from the phone seems reasonable, I'll have a closer look tomorrow.
Meanwhile I have a question is it possible to test the functionality somehow?

A248 · 2025-09-15T22:31:19Z

Theoretically, yes. Some behavior is different on the module path: Java's ServiceLoader and ClassLoader.getResource(AsStream) are both affected by switching from an unnamed jar to a full module.

in practice, compilation will mostly ensure that the modules are declared properly. But if you want to spin up an integration test, you can always create a subproject that runs Chicory on the module path. This subproject can be created either as a plain Maven module, or using the maven-invoker-plugin.

Theoretically speaking though, there's no way to reach 100% coverage unless you literally fork all test runs and run them in a modular environment. That's why I rely on rg ServiceLoader and rg getResource when checking a codebase for the ability to use modules (using rg = ripgrep).

andreaTP · 2025-09-15T22:41:49Z

Thanks for the answer, I don't think we need 100% coverage, but an integration test to check that we are not badly messing up will be good...
I like the maven-invoker approach that's already used in this codebase, do you mind to add it to this PR?

andreaTP · 2025-09-17T10:33:08Z

@dmlloyd IIRC you played a bit with the module system, do you want to comment on this PR? 🙏

dmlloyd

The descriptors themselves look fine, but I would ensure there is testing from the start.

dmlloyd · 2025-09-17T12:39:40Z

pom.xml

+              <module name="BeforeExecutionExclusionFileFilter">
+                <property name="fileNamePattern" value="module\-info\.java$"/>
+              </module>


Why exactly is checkstyle failing on module-info?

module-info is wholly unsupported by checkstyle, so this is the recommended configuration (per their docs) to skip it.

checkstyle/checkstyle#8240

Makes sense. I'd put a comment in here to that effect though.

dmlloyd · 2025-09-17T12:40:35Z

pom.xml

          <compilerArgs>
            <arg>-XDcompilePolicy=simple</arg>
            <arg>--should-stop=ifError=FLOW</arg>
+            <arg>-Xlint:-module</arg>


Why are module warnings disabled, specifically?

In order to make "internal" packages stay internal to Chicory, I added qualified exports like exports ....internal to othermodule;.

However, this causes a warning message like "module not found". This happens because the downstream module is not present during module compilation. To work around this, we'd need to either disable the lint -- or to restructure Chicory in ways that are very unstandard for a Maven build (1)

Because of <failOnWarnings>, the module-related warning message was failing the build, so I disabled itt.

I'd add a comment here too.

pom.xml

A248 · 2025-09-17T16:35:54Z

Good news! Not only did the tests caught a couple bugs, but they showed that the "internal" packages need to be exported after al. This obviates the need for -Xlint:-module and makes the PR overall more clean.

All that considered, I don't have any other changes in mind to make to this PR, besides addressing your review concerns, of course.

andreaTP · 2025-09-18T12:24:36Z

compiler/pom.xml

+            <configuration>
+              <compilerArgs>
+                <arg>--add-modules</arg>
+                <arg>java.sql</arg>


this is surprising

wasm-corpus/pom.xml

It turns out that chicory was already using the latest checkstyle version, so this commit just ensures that checkstyle is really turned off on Java 11

andreaTP · 2025-09-18T14:29:44Z

I added a commit to disable checkstyle on Java 11, but now there is an issue with the modules.

Just to make sure I ask it: are those changes fully backward compatible?
E.g.: no changes are expected in user projects/code if they are not using the module system, correct?

andreaTP · 2025-09-29T11:28:46Z

Just to spell it out, I was waiting an iteration from you @A248 on this PR, let me know if you are going to have time to progress on it 🙂 and thanks again 🙏

A248 · 2025-10-09T21:10:16Z

Hi there, sorry for the delay. I took a look at the tests which were strangely failing on Java 11.

What's weird is that the approvaltests dependency jar contains .java source files alongside the classfiles. Javac is compiling these source files during test compilation. Why this only happens on Java 11 on the module path, but not in later Java versions, is beyond me.

Anyway, I'll hopefully get to the bottom of this and update this PR with the fix.

A248 · 2025-10-09T21:11:09Z

Potentially related: https://bugs.openjdk.org/browse/JDK-8338675 (javac modifies jar files when .java files are present inside them!)

andreaTP · 2025-10-09T21:27:39Z

@dmlloyd if you have a spare cycle and are interested in having a look it would be appreciated 🙏

dmlloyd · 2025-10-16T12:27:20Z

The build is failing for me locally, but per that JDK bug I'd try adding -implicit:none to the maven-compiler-plugin arguments to javac.

andreaTP reviewed Sep 15, 2025

View reviewed changes

dmlloyd reviewed Sep 17, 2025

View reviewed changes

A248 force-pushed the java-module-defs branch from eb1dbd3 to f27d73f Compare September 17, 2025 13:30

andreaTP force-pushed the java-module-defs branch from 09f2d4f to 7d5a86f Compare September 18, 2025 12:21

andreaTP reviewed Sep 18, 2025

View reviewed changes

compiler/pom.xml

<configuration>

<compilerArgs>

<arg>--add-modules</arg>

<arg>java.sql</arg>

Copy link

Collaborator

andreaTP Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is surprising

andreaTP reviewed Sep 18, 2025

View reviewed changes

wasm-corpus/pom.xml Show resolved Hide resolved

A248 added 7 commits September 18, 2025 14:53

Add module info definitions

ba136b3

Update build and tooling to skip module-related hurdles

a4a4853

Add requires on objectweb.asm.util

7ce6b9f

Add comment to Shader on why it works on module path

0831086

Add requires for zerofs

d18aaec

Don't duplicate checkstyle build configuration

40fdcb5

It turns out that chicory was already using the latest checkstyle version, so this commit just ensures that checkstyle is really turned off on Java 11

Enable tests on the module path + cleanup unneeded things

191f735

andreaTP force-pushed the java-module-defs branch from 7d5a86f to 191f735 Compare September 18, 2025 13:54

disable checkstyle on Java 11

a8b0033

Add Java module definitions #1028

Are you sure you want to change the base?

Add Java module definitions #1028

Uh oh!

Conversation

A248 commented Sep 15, 2025

Uh oh!

andreaTP left a comment

Choose a reason for hiding this comment

Uh oh!

A248 commented Sep 15, 2025

Uh oh!

andreaTP commented Sep 15, 2025

Uh oh!

andreaTP commented Sep 17, 2025

Uh oh!

dmlloyd left a comment

Choose a reason for hiding this comment

Uh oh!

dmlloyd Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

A248 Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

dmlloyd Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

dmlloyd Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

A248 Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

dmlloyd Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

A248 commented Sep 17, 2025

Uh oh!

andreaTP Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andreaTP commented Sep 18, 2025

Uh oh!

andreaTP commented Sep 29, 2025

Uh oh!

A248 commented Oct 9, 2025

Uh oh!

A248 commented Oct 9, 2025

Uh oh!

andreaTP commented Oct 9, 2025

Uh oh!

dmlloyd commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmlloyd commented Oct 16, 2025 •

edited

Loading