Skip to content

Work on #701, try to use module-info.java natively #702

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

Merged
merged 9 commits into from
Jan 12, 2025

Conversation

cowtowncoder
Copy link
Member

No description provided.

@cowtowncoder cowtowncoder marked this pull request as ready for review January 12, 2025 00:39
@cowtowncoder cowtowncoder merged commit ee6e748 into master Jan 12, 2025
3 checks passed
@cowtowncoder cowtowncoder deleted the tatu/3.0/701-use-module-info branch January 12, 2025 00:41
@ppkarwasz
Copy link

Shouldn't this also replace:

    requires tools.jackson.core;
    requires tools.jackson.databind;

with

    requires transitive tools.jackson.databind;

Since XmlMapper extends ObjectMapper, a module that reads tools.jackson.databind.xml also needs to read tools.jackson.databind. Specifying a requirement on tools.jackson.core is probably not necessary, since it will be implied by transitivity.

Personally I am not a big fan of writing module-info.java files by hand, since it is easy to miss the transitive modifier. Binary analysis tools like Moditect or BND do a better job here.

@cowtowncoder
Copy link
Member Author

Yes, probably transitive makes sense here, although I suspect that most if not all use of XML module will include dependency to jackson-databind anyway.

I don't have much experience with use of Moditect or BND outside of Maven plug-ins so I am probably missing something. The main challenge I have is that IDEs (well, at least Eclipse) have pretty clunky support still, making manual edits harder than usual IDE-supported Java refactoring.

@ppkarwasz
Copy link

Yes, probably transitive makes sense here, although I suspect that most if not all use of XML module will include dependency to jackson-databind anyway.

I have seen two philosophies: some users only add jackson-databind-xml and don't include anything else. These would profit from the transitive modifier. Personally I like to add directly all the libraries that I use in my code.

I don't have much experience with use of Moditect or BND outside of Maven plug-ins so I am probably missing something. The main challenge I have is that IDEs (well, at least Eclipse) have pretty clunky support still, making manual edits harder than usual IDE-supported Java refactoring.

BND flawlessly determines the transitive modifier (and its uses OSGi counterpart). In the Log4j project we moved away from module-info.java exactly because IDEs (especially Eclipse) didn't support it well. Now we only have problems with the generated module-info.class. 😉

@cowtowncoder
Copy link
Member Author

If I may ask: how do you use BND? I have only used it via "bundle" packaging mechanism, although somehow that seems to exclude module-info.java. I think I'd be most interested in one-off generation probably (I recall Moditect plug-in allowing this, which is how Jackson components' module-info.java were bootstrapped).

@ppkarwasz
Copy link

We have a common bnd-maven-plugin configuration in logging-parent (see lines 817-894) that we use in all modules to have a coherent OSGi and JPMS descriptor.

For JPMS the interesting part is the -jpms-module-info directive, which enables the generation of a module-info.class file. The generated file is perfect, unless you have optional dependencies or dependencies that are not even automatic modules. In those cases you need to make some adjustments to the config (see log4j-core POM file). We found out for example that optional/provided Maven dependencies + transitive modifier cause a lot of problems to users, since the compiler looks for those dependencies at compile-time.

The disadvantage of generating the module-info.class file is that we run unit tests on the classpath, instead of the module path, but arguably the module path does not provide any advantages for unit testing.

@cowtowncoder
Copy link
Member Author

The disadvantage of generating the module-info.class file is that we run unit tests on the classpath, instead of the module path, but arguably the module path does not provide any advantages for unit testing.

To be honest my biggest/only real problems with module-info.java are with unit tests.
That's a real PITA.

And somehow only having src/main/java/module-info.java with no src/test/main/java/module-info.java does not seem to disable JPMS processing so access problems make this approach unfeasible. Directly generating module-info.class might by-pass that.

One last question: do you often (need to) inspect generated module-info.class? And if so, what tool do you use?

@ppkarwasz
Copy link

To be honest my biggest/only real problems with module-info.java are with unit tests. That's a real PITA.

We use the classpath for unit tests by setting useModulePath to false in both the Maven Compiler and Surefire plugins. Since unit tests break encapsulation anyway by accessing package-private methods, in my opinion it doesn't make sense to run them with JPMS.

Running unit tests on the classpath doesn't validate the JPMS descriptor, so we have some external Maven modules that do black-box testing in a JPMS environment (e.g. log4j-samples-jlink).

And somehow only having src/main/java/module-info.java with no src/test/main/java/module-info.java does not seem to disable JPMS processing so access problems make this approach unfeasible. Directly generating module-info.class might by-pass that.

Yes, the problem is that each tool (Gradle, Maven, IDEs) has a different convention on how to specify a module-info.java descriptor for unit tests. @rfscholte proposed a working group to uniformize module path handling in tools some time ago. I don't know what is the current status of the proposal.

One last question: do you often (need to) inspect generated module-info.class? And if so, what tool do you use?

I run jar -d -f <jarfile> from time to time and bnd print <jarfile> (from the BND cli tools).
Honestly we don't run it often enough, so we get user reports like apache/logging-log4j2#2929 from time to time. That is why nowadays we run the log4j-samples-jlink application above regularly. JLink is a good way to test Java modularization, because it removes undeclared JRE modules. This way we can even catch problems like missing try...catch around references to java.sql.

@cowtowncoder
Copy link
Member Author

Whoa! This is such good knowledge and would have saved me time earlier wrt conversion. I was puzzled by unit test oddities. Although I think I got them mostly resolved, save for Kotlin module where I just need to disable module path.
Then again, plain Maven works more reliably than Eclipse so maybe should considering just disabling for other projects too...

@ppkarwasz
Copy link

Although I think I got them mostly resolved, save for Kotlin module where I just need to disable module path. Then again, plain Maven works more reliably than Eclipse so maybe should considering just disabling for other projects too...

Looking at the Kotlin compiler options, it doesn't seem to have any option dealing with module path.

@cowtowncoder
Copy link
Member Author

Alas! Ok, that is important to know at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants