Generally looks good, and is generally what I expected.
The change in the arg for checkExit is a pleasant surprise ;-) That
being said, ideally, in general, there should be a checkExit call after
each invocation of javadoc, but that's a pretty minor detail.
-- Jon
On 4/8/20 1:15 PM, Hannes Wallnoefer wrote:
Jon,
I pushed the changeset with separated source directories for modules
and packages, which worked fine.
Here are the changes in the test:
http://hg.openjdk.java.net/jdk/jdk/rev/3b557aef43c4#l3.13
Hannes
Am 08.04.2020 um 11:42 schrieb Hannes Wallnoefer
<hannes.wallnoe...@oracle.com <mailto:hannes.wallnoe...@oracle.com>>:
Thanks for the review, Jon.
I agree the test is weird. I had to check it does the right thing
with everything in the same directory.
I’ll rewrite it to use separate directories as you suggest.
Hannes
Am 07.04.2020 um 19:32 schrieb Jonathan Gibbons
<jonathan.gibb...@oracle.com <mailto:jonathan.gibb...@oracle.com>>:
Hannes,
The test is weird, and it took me a while to understand what I think
is going on. This predates your changes, but is still very confusing.
If I understand the code correctly, initModulesAndPackages creates
both packages and modules (which contain packages) in the same "src"
directory. The same "src" directory is then used as an argument for
both the module source path and source path, in different runs of
javadoc. This is confusing, at least to me, because you can put a
module on the source path and have javadoc recognize it as a module.
But, it seems to work in the test because the test is relying on the
naming conventions for qualified module names, and so it doesn't get
confused when looking for packages or modules. But, I'm mildly
surprised javac/javadoc doesn't complain at the "packages" found on
the module source path.
The test would be clearer if initModulesAndPackages wrote into two
separate directories, one for module source code and the other for
package source code. This would then affect/improve all the existing
@Test methods by making it clear which directory structure they are
reading source code from.
If this simple refactoring works, the review is Approved.
Otherwise, I'd like to understand the issues in more detail.
-- Jon
On 4/3/20 1:38 AM, Hannes Wallnoefer wrote:
I realised there’s a simpler solution to the problem. Instead of
using a Set to track packages with non-modular documentation we
can just return the module name that matches our internal model in
#checkLinkCompatibility.
I also changed the JBS summary to „javadoc fails to link to docs
with non-matching modularity“ so it covers both cases.
New Webrev: http://cr.openjdk.java.net/~hannesw/8240169/webrev.01/
Hannes
Am 02.04.2020 um 16:06 schrieb Hannes Wallnoefer
<hannes.wallnoe...@oracle.com <mailto:hannes.wallnoe...@oracle.com>>:
Please review:
JBS: https://bugs.openjdk.java.net/browse/JDK-8240169
Webrev: http://cr.openjdk.java.net/~hannesw/8240169/webrev.00/
This patch allows using external documentation even if it doesn’t
match the external library in terms of modularity, i.e.
non-modular documentation can be used for an modular library and
vice versa. Instead of showing an error a warning is issued. There
is still a warning if code and documentation do not match, but we
use the check to tweak reference lookup so that we are still able
to link to the appropriate documentation.
I think that all relevant cases for combinations of modular and
non-modular code are covered by existing tests (some of
which change with this patch obviouly).
TestLinkOptionWithAutomaticModule.java covers using a jar file as
automatic or unnamed module, both of which cases were
already supported as of JDK-8212233, so no changes there.
TestLinkOptionWithModule.java covers all combinations of modular
and non-modular code and documentation. For the two tests
that cover non-matching combinations, I changed the expected
return code to OK and added expected output with the link HTML.
The message about the mismatching documentation is still there,
but it is a warning instead of an error.
I also added the JBS id to the @bug tag in
TestLinkOptionWithModule.java as well as in
TestClassCrossReferences.java which is another test that contains
significant changes.
Thanks,
Hannes
