[MJAVADOC-620] Do not ignore JARs w/o module info when building classpath #35
Conversation
…path When locationManager.resolvePaths() processes JARs that do not define a module name, module-name-guessing is triggered. The last resort is to use the JAR's file name to derive a module name. The heuristic is to determine where a version number starts, for which the regex is used: "-(\\d+(\\.|$))" See https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/module/ModuleFinder.html#of(java.nio.file.Path...) For the remaining prefix, all non-alphanumeric characters (this includes dashes) are replaced by dots. When splitting at the dots, every part must be a legal Java identifier. Now, when a Maven JAR uses the version "1-SNAPSHOT", the JAR is named <artifactId>-1-SNAPSHOT.jar, so the version suffix is not recognized by the heuristic (the number is neither followed by a dot, nor is this the end of the file name without extension), so trying to guess the module name eventually fails with an error (FindException, "1" is not a valid Java identifier). There are other situations in which trying to derive module information from a JAR leads to errors, e.g. when the top-level package is used ("unnamed package not allowed in module"). The code in maven-javadoc-plugin checks all returned classpath elements whether they contain module information. Now, in the result returned by locationManager.resolvePaths(), all JARs for that no module information could be derived are not contained in getClasspathElements(), but only in getPathExceptions(). When a JAR path is mapped to a FindException, it does not make sense to look for module information, but it *does* make sense to still add these JARs to the classpath. Ignoring them, as before, is what breaks backwards-compatibility with non-module JARs and causes bug MJAVADOC-620. The fix is to add all such JARs without module information to the JavaDoc classpath.
|
Looks like you removed the PR-template, please restore. And this issue probably deserves an integration test, the template has the instructions for it. |
|
Sorry, it did not occur to me the template was meant to keep. I followed the instructions step by step and only kept the license declaration. If you really think it makes sense that every single PR contains the instructions how to author PRs, I can restore the template text. |
The first case in which the module info retrieval fails is a JAR containing a class in the top-level package. This is the originally reported case in the JIRA ticket MJAVADOC-620. The FindException has a root cause that says that the usage of top-level packages is not allowed in modules. For the test, it is important to build the JAR in a Maven module that is *not* part of the Reactor. Otherwise, javadoc:aggregate would use the sources of class Test directly and the bug would not occur. This has been achieved by *not* adding the test project maven-MJAVADOC620-jar as a <module> to the main project. Instead, in invoker.properties, the first project is built using option -f. Note: The artifactId may not be maven-MJAVADOC-620-jar (with a dash before the issue number), because then, the module info retrieval fails for another reason, namely not being able to derive a module name. This is tested by an upcoming second integration test. Without the fix, maven-MJAVADOC620-jar-1.0-SNAPSHOT.jar is not added to the classpath and building the JavaDoc fails, because class Test is not found. With the fix, maven-MJAVADOC620-jar-1.0-SNAPSHOT.jar is added to the classpath and building the JavaDoc succeeds.
The second case in which the module info retrieval fails is a JAR with a file name that contains a number *not* followed by a dot. This is the case we experienced, because we often use 1-SNAPSHOT as the working version, resulting in a JAR file name like "foo-1-SNAPSHOT.jar". The FindException has a root cause that says that the module name derived from the file name, "foo.1.SNAPSHOT", is invalid because "1" is not a valid Java identifier. Compared to the first integration test, the version number of the test project has been changed from "1.0" to just "1". To test one failure cause at a time, the test class Test has been moved from the top-level package into package "somepackage". Without the fix, maven-MJAVADOC620-jar-1-SNAPSHOT.jar is not added to the classpath and building the JavaDoc fails, because class Test is not found. With the fix, maven-MJAVADOC620-jar-1-SNAPSHOT.jar is added to the classpath and building the JavaDoc succeeds.
|
Here you go: Not one, but two integration tests that test both scenarios described in the JIRA ticket, with extensive commit messages. |
|
Anything more you need? Any questions? |
|
I just need time to review |
|
Sounds good. I need to test this but I did something similar in a branch see 15419fd |
There was a problem hiding this comment.
I don't understand what's going on with the integration test stuff, but the actual code change is what I would have come up with to fix MJAVADOC-609, which might be the same issue as MJAVADOC-620. So, I think this looks good and should be merged (assuming tests pass).
|
Thanks everybody! |
|
Which version of maven-javadoc-plugin contains this fix? |
|
@ctubbsii no release yet I hope soon in the coming weeks |
When
locationManager.resolvePaths()processes JARs that do not define a module name, module-name-guessing is triggered. The last resort is to use the JAR's file name to derive a module name. The heuristic is to determine where a version number starts, for which the regex is used:"-(\\d+(\\.|$))"See JavaDoc of ModuleFinder.of().
For the remaining prefix, all non-alphanumeric characters (this includes dashes) are replaced by dots. When splitting at the dots, every part must be a legal Java identifier.
Now, when a Maven JAR uses the version
1-SNAPSHOT, the JAR is named<artifactId>-1-SNAPSHOT.jar, so the version suffix is not recognized by the heuristic (the number is neither followed by a dot, nor is this the end of the file name without extension), so trying to guess the module name eventually fails with an error (FindException, "1" is not a valid Java identifier).There are other situations in which trying to derive module information from a JAR leads to errors, e.g. when the top-level package is used ("unnamed package not allowed in module").
The code in
maven-javadoc-pluginchecks all returnedclasspathelements whether they contain module information. Now, in the result returned bylocationManager.resolvePaths(), all JARs for that no module information could be derived are not contained ingetClasspathElements(), but only ingetPathExceptions(). When a JAR path is mapped to aFindException, it does not make sense to look for module information, but it does make sense to still add these JARs to the classpath.Ignoring them, as before, is what breaks backwards-compatibility with non-module JARs and causes bug MJAVADOC-620.
The fix is to add all such JARs without module information to the JavaDoc
classpath.Filled-in Template:
Following this checklist to help us incorporate your
contribution quickly and easily:
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. --> MJAVADOC-620
[MJAVADOC-XXX] - Fixes bug in ApproximateQuantiles,where you replace
MJAVADOC-XXXwith the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean verifyto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
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 licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.