Add Java module definitions by A248 · Pull Request #1028 · dylibso/chicory

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

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

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

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

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


this is surprising

needed by approval tests apparently, we should make sure this configuration only apply during tests

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.

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-11-06T17:03:10Z

@A248 I want those fixes for the next release that is likely to happen soon, hence I took over this PR, I hope you don't mind.
Thanks a lot for your effort, LGTM!

A248 · 2025-11-11T13:07:28Z

All good! Thank you for taking over the PR.

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

Comment thread wasm-corpus/pom.xml

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

A248 and others added 12 commits November 6, 2025 10:57

Add module info definitions

9d09351

Update build and tooling to skip module-related hurdles

90f6afd

Add requires on objectweb.asm.util

540b34f

Add comment to Shader on why it works on module path

5578296

Add requires for zerofs

bef314b

Don't duplicate checkstyle build configuration

b4df08f

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

acfc706

disable checkstyle on Java 11

0c5f718

minor fixes and rebase

05fde32

Update build and tooling to skip module-related hurdles

16e15d5

Enable tests on the module path + cleanup unneeded things

f0e696c

disable module on Java 11

37c316e

andreaTP force-pushed the java-module-defs branch from 3a6a436 to 37c316e Compare November 6, 2025 16:14

dmlloyd approved these changes Nov 6, 2025

View reviewed changes

fix the android tests

b908177

andreaTP merged commit f17e081 into dylibso:main Nov 6, 2025
26 checks passed

andreaTP mentioned this pull request Nov 6, 2025

Add Java module info #1012

Closed

Marcono1234 mentioned this pull request Jul 17, 2026

Remove or restrict exports of internal packages in module-info bytecodealliance/endive#94

Closed

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

A248 commented Sep 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

andreaTP commented Nov 6, 2025

Uh oh!

Uh oh!

A248 commented Nov 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmlloyd commented Oct 16, 2025 •

edited

Loading