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
It's Module Time #2453
base: master
Are you sure you want to change the base?
It's Module Time #2453
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for starting on this!
How do we test that the resulting modules work? Is that a new integration test?
exports org.jdbi.v3.core.generic; | ||
exports org.jdbi.v3.core.h2; | ||
exports org.jdbi.v3.core.interceptor; | ||
exports org.jdbi.v3.core.internal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be exporting .internal
packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just opened everything, you can change if you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there's a reason to keep it, let's not export internal
packages. All the rest I think are fine to export.
well it worked on my local ¯\(ツ)/¯ |
presumably the existing tests would work that out |
Ah it seems make run-tests doesn't fully compile everything |
it seems due to this peculiar
thing, a couple modules can't get modularized since the classes within are not exported. |
You should be able to run |
right now it seems I'll have to duplicate the classes in the core src/test and put them in the corresponding modules |
Could they be factored out into a reusable module? It'd be nice to avoid having to duplicate classes. |
So I've made a separate module for the test classes, but it seems the other ones cannot detect it
|
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:
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
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). |
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)
|
wait you can do that now? |
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 |
@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. |
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) |
The easiest way to add a new module is to clone an existing one. For example, you might |
doesn't work. |
SonarCloud Quality Gate failed. 0 Bugs No Coverage information Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
What error do you get? That is how I have done it in the past. |
when I try to use the new module I get this |
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 |
Strange. I will check out your branch and try myself... |
Hi @SentryMan , the problem is that the artifact you are trying to look for ( In the newly created |
knew it was something small that I missed |
some stuff that I looked at / might be interesting: |
Ah yeah, turns out having a dedicated test module complicates things a bit with the dependencies |
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. |
duplicating all the test files will surely work, but it's pretty unsightly. |
Fixes #2368
module-info
to most modules