FasterXML / jackson-core

Core part of Jackson that defines Streaming API as well as basic shared abstractions
Apache License 2.0
2.25k stars 781 forks source link

Extra module-info.class in 2.15.1 #1027

Closed tbnguyen1407 closed 1 year ago

tbnguyen1407 commented 1 year ago

problem

There are extra module-info.class in META-INF/versions

This fails JPMS build with error:

module-info.java:29: error: module not found: com.fasterxml.jackson.core
    requires com.fasterxml.jackson.core;

Those other module-info.class files were not present in 2.15.0

jackson-core-module-info-java

workaround

Manually deleting extra module-info.class (except versions/9) the build can succeed.

pjfanning commented 1 year ago

We probably need to change the maven shade plugin config to exclude the module-info classes coming from FastDoubleParser.

cowtowncoder commented 1 year ago

Fixed for inclusion in 2.15.2.

Wonder if there's any way this could be tested -- probably only jackson-integration-tests or something.

pjfanning commented 1 year ago

@cowtowncoder I wrote https://github.com/pjfanning/jackson-jpms-check to test this. I tried a few approaches but this Gradle project is the only way that I got it to work. It uses a Gradle plugin developed by @jjohannes

If you adjust the build.gradle to use v2.15.1, the test fails. You need to use Java 9+.

The test passes with v2.15.2-SNAPSHOT.

cowtowncoder commented 1 year ago

@pjfanning Cool. Note that jackson-integration-tests actually does some Gradle testing (see src/test/java/com/fasterxml/jackson/integtest/gradle/GradleTest.java) so perhaps it'd be possible to integrate it there. Wrt JDK 9 we could either have separate test dirs (like what jackson-databind does) or just increase baseline to JDK 11.

Alternatively it'd be fine if there was a way to trigger tests to run as cron (like jackson-integration-tests has) and send updates on failures to whoever is interested -- I'd want to know of breakages for example?

stklcode commented 1 year ago

I have several projects using plain Maven to build an artifact with JPMS module descriptor depending on Jackson modules, like

module-info.java

module com.example.testme {
    /* ... *\
    requires com.fasterxml.jackson.databind;
}

failing the compile stage with 2.15.1 (passing with 2.15.0 and 2.15.2-SNAPSHOT) So I’d say there should be several ways to test this running any build tool with JDK9+.

If the test are triggered from JUnit just add s skip condition on the Java version, s.t. the specific tests cases are not executed on JDK 8. Should be preferred over dropping JDK 8 completely.

pjfanning commented 1 year ago

@cowtowncoder https://github.com/FasterXML/jackson-integration-tests is not a maven multi-module project. If it was refactored to become a multi-module project, a new maven submodule could be added that tests the JPMS modules of the core jackson projects.

Or we could just clone https://github.com/pjfanning/jackson-jpms-check into the fasterxml org and set up a github workflow that runs those tests with a few Java 9+ java versions.

cowtowncoder commented 1 year ago

Worth noting, too, that build is done on JDK 8 which is not module-aware; module-info is really an add-on that is "alien" for most of the Jackson components at this point. Once we move to JDK 11 or beyond this will become easier for sure.

cowtowncoder commented 1 year ago

@cowtowncoder https://github.com/FasterXML/jackson-integration-tests is not a maven multi-module project. If it was refactored to become a multi-module project, a new maven submodule could be added that tests the JPMS modules of the core jackson projects.

True, true.

Or we could just clone https://github.com/pjfanning/jackson-jpms-check into the fasterxml org and set up a github workflow that runs those tests with a few Java 9+ java versions.

I think it's fine for it to exist on its own -- all I care about is to have visibility, maybe get notified on failures automatically (and run on schedule). While in perfect world it'd be nice to have cascading triggers from builds, I think daily runs work quite well to catch relatively rare (?) failures like module info breakages.

cowtowncoder commented 1 year ago

Fixed: included in 2.15.2 that was just released