It's Module Time

[SentryMan]

Fixes #2368

• Adds module-info to most modules

[stevenschlansker]

Thank you for starting on this!

How do we test that the resulting modules work? Is that a new integration test?

[stevenschlansker]

Should we be exporting .internal packages?

[SentryMan]

I just opened everything, you can change if you want

[stevenschlansker]

Unless there's a reason to keep it, let's not export internal packages. All the rest I think are fine to export.

[SentryMan]

well it worked on my local ¯\(ツ)/¯

[SentryMan]

How do we test that the resulting modules work? Is that a new integration test?

presumably the existing tests would work that out

[SentryMan]

Ah it seems make run-tests doesn't fully compile everything

[SentryMan]

it seems due to this peculiar

        <dependency>
            <groupId>org.jdbi</groupId>
            <artifactId>jdbi3-core</artifactId>
            <classifier>tests</classifier>
            <scope>test</scope>
        </dependency>

thing, a couple modules can't get modularized since the classes within are not exported.

[stevenschlansker]

You should be able to run make install to build and check everything.

[SentryMan]

right now it seems I'll have to duplicate the classes in the core src/test and put them in the corresponding modules

[stevenschlansker]

Could they be factored out into a reusable module? It'd be nice to avoid having to duplicate classes.

[SentryMan]

So I've made a separate module for the test classes, but it seems the other ones cannot detect it

 Could not resolve dependencies for project org.jdbi:jdbi3-caffeine-cache:jar:3.40.1-SNAPSHOT: The following artifacts could not be resolved: org.jdbi:jdbi3-core-test:jar:3.40.1-SNAPSHOT (absent): Could not find artifact org.jdbi:jdbi3-core-test:jar:3.40.1-SNAPSHOT in snapshots-repo (https://oss.sonatype.org/content/repositories/snapshots) -

[hgschmie]

First: Thank you for starting this! We need to modularize Jdbi fully if only for the javadocs. :-)

You seem to have encountered all the problems that I found as well when trying to do so. Here is what I tried:

• opening internal packages only to other jdbi modules
• opening some of the test packages separate from the main packages
• introducing different identifiers for the test jars

There is additional confusion with "maven module" vs "JPMS module" in all of the Maven documentation and the idea of "test vs. compile" is something that basically does not exist for maven. Having separate module-info for the test and the main jar is cool, but it requires separate identifiers.

JPMS tooling is generally immature (six years after JPMS came out!) and a lot of manual work is required.

Happy to collaborate on this. Few comments: run make in the main folder shows you all the options in the make driven build. There are additional docs in the CONTRIBUTING.md file in the root folder.

make run-tests only runs the tests. Someone (@stevenschlansker :-) ) asked to split "run-tests" (only running tests) and "tests" (compiling and running tests) because they kept doing "make tests" and complained that the tests were not compiled....

The gold standard to testing a change is to do "make install run-tests", same as the ci does (it does it in two steps for separate JDKs for building and testing).

[SentryMan]

from a JPMS perspective, I figured out how to get it all working, the main issue I have now is getting the reusable test maven module to be detected by the other maven modules. When I duplicate the classes manually it works, but it's pretty unsightly.

I get this on cli. (jdbi3-core-test is something I added)

Could not find artifact org.jdbi:jdbi3-core-test:jar:3.40.1-SNAPSHOT in snapshots-repo (https://oss.sonatype.org/content/repositories/snapshots) -

[SentryMan]

Having separate module-info for the test and the main jar is cool, but it requires separate identifiers.

wait you can do that now?

[SentryMan]

in any case, I find myself a little stumped by the separate test module bit not being found, shall I just duplicate the classes to make it work

[stevenschlansker]

@SentryMan , I don't feel great about merging it with duplicated classes, but if you keep the duplication in its own commit, then we can make forward progress and get everything else working. Once this is ready to merge, we can decide if we go with duplicated classes or figure out how to fix it otherwise.

[SentryMan]

it seems then that the prereq for this change is replacing all instances of

        <dependency>
            <groupId>org.jdbi</groupId>
            <artifactId>jdbi3-core</artifactId>
            <classifier>tests</classifier>
            <scope>test</scope>
        </dependency>

with a dedicated module. If you can show me the steps of how to add a jdbi maven module I can do it. (I tried adding a new module to the root pom.xml but no dice)

[stevenschlansker]

The easiest way to add a new module is to clone an existing one. For example, you might mkdir core-testing && cp core/pom.xml core-testing/. Then, edit the pom to update everything, and then add <module>core-testing</module> to the /pom.xml

[SentryMan]

The easiest way to add a new module is to clone an existing one. For example, you might mkdir core-testing && cp core/pom.xml core-testing/. Then, edit the pom to update everything, and then add <module>core-testing</module> to the /pom.xml

doesn't work.

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
3 Security Hotspots
12 Code Smells

No Coverage information
5.2% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[stevenschlansker]

What error do you get? That is how I have done it in the past.

[SentryMan]

Failed to execute goal on project jdbi3-caffeine-cache: 
Could not resolve dependencies for project org.jdbi:jdbi3-caffeine-cache:jar:3.40.1-SNAPSHOT:
The following artifacts could not be resolved: org.jdbi:jdbi3-core-test:jar:3.40.1-SNAPSHOT (absent): 
Could not find artifact org.jdbi:jdbi3-core-test:jar:3.40.1-SNAPSHOT in snapshots-repo 
(https://oss.sonatype.org/content/repositories/snapshots) -> [Help 1]

when I try to use the new module I get this

[stevenschlansker]

What command did you run, and from what directory? Either you need to build the whole project from the root, so that the module is included in the reactor build, or you need to run mvn install on your new module to put it in your local maven repo.

[SentryMan]

[stevenschlansker]

Strange. I will check out your branch and try myself...

[stevenschlansker]

Hi @SentryMan , the problem is that the artifact you are trying to look for (org.jdbi:jdbi3-core-test:jar:3.40.1-SNAPSHOT) is not the same as the new module you added (org.jdbi.internal:jdbi3-core-test:jar:3.40.1-SNAPSHOT)

In the newly created core-test/pom.xml, there is no groupId declared, so it inherits it from the <parent>. This new module should be internal, so just update your dependency to add the .internal to the groupId and hopefully it works.

[SentryMan]

knew it was something small that I missed

[hgschmie]

some stuff that I looked at / might be interesting:

• https://stackoverflow.com/questions/53477690/properly-implementing-java-modules-in-a-maven-build-with-inter-module-test-depen
• sample code from that SO: https://github.com/jhenriquez418/java-cross-module-testing
• the moditect tooling: https://github.com/moditect/moditect

[SentryMan]

Ah yeah, turns out having a dedicated test module complicates things a bit with the dependencies

[hgschmie]

Hey @SentryMan. I summarized our current state of things with modules here: https://github.com/jdbi/jdbi/blob/master/JPMS-SUPPORT.md

I am slowly trying to work the tool chain up to support real modules in the code base. There is a lot of work to be done before we can go fully modularized, especially around testing and javadocs.

[SentryMan]

duplicating all the test files will surely work, but it's pretty unsightly.