[MJAVADOC-769] fix for transitive filename based modules

[hgschmie]

When a project depends on an artifact with a manifest entry for
Automatic-Module-Name which in turn depends on an artifact that uses the
filename to determine the module name, it will move the former onto the
module path and patch the latter into the main artifact. However, now
the direct dependency on the module path can no longer access the
classes that have been patched only into the main module and javadoc
generation fails.

As the JDK only differentiates between modules with a module descriptor
and "everything else" (modules with an automatic module entry in the
manifest and modules with file name based names), the javadoc plugin
should do the same.

This patch changes the treatment of dependencies with an
Automatic-Module-Name to match dependencies that use a filename based
module name. The plugin now patches all of those dependencies into the
main module and the build succeeds.

Includes an integration test.

Following this checklist to help us incorporate your
contribution quickly and easily:

• Make sure there is a JIRA issue filed
  for the change (usually before you start working on it). Trivial changes like typos do not
  require a JIRA issue. Your pull request should address just this issue, without
  pulling in other changes.
• Each commit in the pull request should have a meaningful subject line and body.
• Format the pull request title like [MJAVADOC-XXX] - Fixes bug in ApproximateQuantiles,
  where you replace MJAVADOC-XXX with the appropriate JIRA issue. Best practice
  is to use the JIRA issue title in the pull request title and in the first line of the
  commit message.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Run mvn clean verify -Prun-its to make sure basic checks pass. A more thorough check will
  be performed on your pull request automatically.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

• I hereby declare this contribution to be licensed under the Apache License Version 2.0, January 2004

[slawekjaranowski]

Should we have some of assertions on generate javadocs?
Does simply success build is enough?

[slawekjaranowski]

We build test-jar with tests classes - but java doc is executed only for main java code.

[hgschmie]

that is irrelevant. The tests are only executed to illustrate why the scopes in the pom make sense. The dependency scopes in the pom is what exhibits the problem that this patch fixes.

[slawekjaranowski]

Please consider to use goals:

• javadoc-no-fork and test-javadoc-no-fork

we not need jars with javadocs

[slawekjaranowski]

This execution should be before javadoc - it can have impact on test

[slawekjaranowski]

I did a test, in test project execute

mvn clean javadoc:3.5.0:javadoc - build pass
mvn clean package javadoc:3.5.0:javadoc - build failed

so I assume that existing project jar with Automatic-Module-Name in target directory can cause or trigger a problem
when I remove Automatic-Module-Name from test project - problem not occurs

Of course your fix resolve such issue

disclosure - I'm starting a journey with JPMS 😄

[hgschmie]

thanks for checking this. Do you have the exact error that you saw? Is it an open source project that I can check out?

[hgschmie]

I did a test, in test project execute

mvn clean javadoc:3.5.0:javadoc - build pass mvn clean package javadoc:3.5.0:javadoc - build failed

so I assume that existing project jar with Automatic-Module-Name in target directory can cause or trigger a problem when I remove Automatic-Module-Name from test project - problem not occurs

Of course your fix resolve such issue

disclosure - I'm starting a journey with JPMS 😄

I will hold off merging until I understand what fails here.

[hgschmie]

I am not sure that the behavior that you are observing is related to this PR.

With the current release (3.5.0):

When running "clean javadoc:javadoc", the javadoc plugin will consider your project to not use modules (the whole "automatic module name" thing is broken/iffy. JPMS itself only considers two different things: modules (which have a module-info.java) and "everything else". Maven tries to differentiate between modules, "things that have an automatic module name set" and "things that use filename based module names" which is IMHO wrong).

Anyway, in my project, if I use "clean javadoc:javadoc", this is what gets executed:

-classpath <... all the jars ...>
-encoding 'UTF-8'
-public -quiet --release 11
-sourcepath <... all the source folders ...>
... all the other stuff

however, when I run "clean package javadoc:javadoc", the javadoc plugin will see the jar which now contains a META-INF/MANIFEST.MF with an automatic module name and consider this project to be modularized (which is IMHO wrong but this is for another day to discuss) and executes

--add-modules ALL-MODULE-PATH
--module-path <the project jar>:<... all the jars that are either real modules or have an automatic module name...>
--patch-module <module-identifier>=<... all the source folders ...>:<... all the jars that are filename based modules ...>
... all the other stuff ...

In your case, the second one succeeds and the first one fails.

What the patch does is that only "real" modules (the ones with a module descriptor) end up on the module path and everything else ends up being patched. e.g. for my project, this is how javadoc is called without the patch:

 --add-modules ALL-MODULE-PATH
 --module-path '/Users/henning/.m2/repository/com/google/errorprone/error_prone_annotations/2.21.1/error_prone_annotations-2.21.1.jar: \
   /Users/henning/.m2/repository/io/leangen/geantyref/geantyref/1.3.14/geantyref-1.3.14.jar: \
   /Users/henning/.m2/repository/jakarta/annotation/jakarta.annotation-api/2.1.1/jakarta.annotation-api-2.1.1.jar: \
   /Users/henning/.m2/repository/org/antlr/antlr4-runtime/4.13.0/antlr4-runtime-4.13.0.jar: \
   /Users/henning/.m2/repository/org/checkerframework/checker-qual/3.37.0/checker-qual-3.37.0.jar: \
   /Users/henning/.m2/repository/org/immutables/value/2.9.3/value-2.9.3.jar: \
   /Users/henning/.m2/repository/org/slf4j/slf4j-api/1.7.36/slf4j-api-1.7.36.jar: \
   /Users/henning/code/jdbi/core/target/jdbi3-core-3.41.1-SNAPSHOT.jar'
 --patch-module org.jdbi.v3.core='/Users/henning/code/jdbi/core/src/main/java: \
   /Users/henning/code/jdbi/core/target/generated-sources/antlr4: \
   /Users/henning/code/jdbi/core/target/generated-sources/annotations: \
   /Users/henning/.m2/repository/org/inferred/freebuilder/2.8.0/freebuilder-2.8.0.jar'

and with the patch:

--add-modules ALL-MODULE-PATH
 --module-path '/Users/henning/code/jdbi/core/target/jdbi3-core-3.41.1-SNAPSHOT.jar: \
   /Users/henning/.m2/repository/jakarta/annotation/jakarta.annotation-api/2.1.1/jakarta.annotation-api-2.1.1.jar'
 --patch-module org.jdbi.v3.core='/Users/henning/code/jdbi/core/src/main/java: \
   /Users/henning/code/jdbi/core/target/generated-sources/antlr4: \
   /Users/henning/code/jdbi/core/target/generated-sources/annotations: \
   /Users/henning/.m2/repository/com/google/errorprone/error_prone_annotations/2.21.1/error_prone_annotations-2.21.1.jar: \
   /Users/henning/.m2/repository/io/leangen/geantyref/geantyref/1.3.14/geantyref-1.3.14.jar: \
   /Users/henning/.m2/repository/org/antlr/antlr4-runtime/4.13.0/antlr4-runtime-4.13.0.jar: \
   /Users/henning/.m2/repository/org/checkerframework/checker-qual/3.37.0/checker-qual-3.37.0.jar: \
   /Users/henning/.m2/repository/org/immutables/value/2.9.3/value-2.9.3.jar: \
   /Users/henning/.m2/repository/org/inferred/freebuilder/2.8.0/freebuilder-2.8.0.jar: \
   /Users/henning/.m2/repository/org/slf4j/slf4j-api/1.7.36/slf4j-api-1.7.36.jar'

which may or may not make a difference in your case. But I am fairly certain that the failure that you observe with clean javadoc:javadoc vs. clean package javadoc:javadoc is caused by one not using modules at all and the other one is using the module path.

If you run the javadoc process with the <debug> flag set to true and share your options file from the target/apidocs folder, I can take a look and say more. Also, the actual error message would be helpful.

[hgschmie]

This is the slightly longer (and a bit more winded) explanation of what that patch does:

consider a project that spits out a module. This can be a defined module or an automatic module; it does not actually matter. What is important is that the maven tool chain considers it something where it will use the module path.

let's consider this dependency graph: project --> automatic module with manifest entry --> automatic module with file name based module id (If that is too abstract for you, you can call those jdbi3-guice -> guice-5.1.0 -> javax.inject-1).

• jdbi3-guice is a defined module (it contains a module-info.java file)
• guice-5.1.0 is an automatic module that contains an entry in META-INF/MANIFEST.MF
• javax.inject-1 is an automatic module that has no JPMS information, so it is treated as a filename based module.

To run code that uses these three modules, it needs to do --module-path jdbi3-guice.jar:guice-5.1.0.jar:javax.inject-1.jar. This will work fine. The automatic modules open all their packages to the module path; there are no overlaps and the jdbi3-guice module can consume all the pieces that it needs.

When the maven toolchain is trying to set up javadoc generation, it will inspect all three modules and come to the following conclusion:

• jdbi3-guice --> ModuleNameSource.MODULEDESCRIPTOR
• guice-5.1.0 --> ModuleNameSource.MANIFEST
• javax.inject-1 --> ModuleNameSource.FILENAME

(this is buried deep in the plexus-java component)

Now, the javadoc plugin sees this and creates module and classpath:

--add-modules ALL-MODULE-PATH
--module-path 'jdbi/guice/target/jdbi3-guice-3.41.0.jar:m2repo/guice-5.1.0.jar'
--patch-module org.jdbi.v3.guice='m2repo/javax.inject-1.jar'

because it treats the ModuleNameSource.MANIFEST different from ModuleNameSource.FILENAME

So, everything cool? No. What happens now is that a class in the main module (InternalBindingProvider) implements a class in the guice module (com.google.inject.Provider) which in turn extends a class from javax.inject-1 (javax.inject.Provider). But the javax.inject-1 jar has only been patched into the jdbi3-guice component. So javadoc can not find it when the guice class says "I extend that" and that leads to:

InternalImportBindingBuilder.java:88: error: cannot access Provider
    static final class InternalBindingProvider<T> implements Provider<T> {
                 ^
  class file for javax.inject.Provider not found
1 error

so the right fix is one of two things:

• move all dependencies of the main artifact onto the module path. This sounds nice in theory but reality is that this only works for defined modules (those that have a module-info.java). Many of the other ones (especially the ones that simply tack an automatic module name into the jar after compile time) have dependencies with overlapping packages (e.g. jsr305 as the biggest offender) and short of inspecting every jar and keeping a list of exports, this option leads to more problems than actual solutions. It is, however, the right solution but, unless the rest of the tooling starts to differentiate between defined modules and automatic modules, not a viable solution.
• patch all the automatic modules that are dependencies for the main artifact into the main artifact. IAW, don't differentiate between the two types of automatic modules (manifest and filename based).

And that is what this patch does: It removes the differentiation between manifest and filename based dependencies.

A more complete fix would do this:

• plot the complete dependency graph
• start from the root. Any dependency that is a defined module "infects" its full downstream tree. All of those dependencies need to go onto the module path
• look at the remainder of the graph. Anything here needs to be patched into the main module.

E.g.

artifact --+--- defined module a --+-- defined module b-- +-- automatic module 1 --+ automatic module 2
                 |
                 +--- automatic module 4 --+-- automatic module 5
                 |
                 +--- automatic module 6 --+-- defined module c -+-- automatic module 5

• defined module a goes onto the module path. As does defined module b, automatic module 1 and automatic module 2
• defined module c goes onto the module path. This infects automatic module 5.
• automatic module 4 gets patched into the main module. its dependency has been forced onto the module path
• automatic module 6 gets patched into the main module.

So the result is

--module-path defined module a:defined module b:defined module c:automatic module 1:automatic module 2:automatic module 5
-- patch-module main.module=automatic module 4:automatic module 6

Or in the case above, the chain main artifact --> guice (automatic module) -> javax.inject (automatic module) leads to both guice and javax.inject being patched into main artifact.

If guice ever wises up and provides a module descriptor, it would be this scenario: main artifact --> guice (defined module) -> javax.inject (automatic module)

Which would force guice onto the module path and its dependency (javax.inject) as well. That works as well as patching both. What does not work is move one of them onto the module path and patch the other one into the main module.

I would implement that, except that AbstractJavadocMojo.java is a brittle 6,100 lines (!) abomination littered with special cases, exceptions and dependencies on other components of unclear function. Also most mojos in the plugin share functionality here, all the way to the aggregation jar target. I did not dare to touch this and so I did the minimal invasive patch.

No, seriously, this thing needs to be refactored before any serious work can be done. Or better yet, thrown away and start over (maven-javadoc-next-plugin anyone?)