8216497: javadoc should auto-link to platform classes #171

hns · 2020-09-15T09:10:54Z

This pull request is identical with the RFR previously sent for the Mercurial repository:

https://mail.openjdk.java.net/pipermail/javadoc-dev/2020-August/001796.html

I'm copy-pasting the comments from the original RFR below.

Most of the new code is added to the Extern class where it fits in quite nicely and can use the existing supporting code for setting up external links.

The default behaviour is to generate links to docs.oracle.com for released versions and download.java.net for prereleases. Platform documentation URLs can be configured using the new --link-platform-properties option to provide a properties file with URLs pointing to to alternative locations. See the CSR linked above for more details on the new options.

The feature can be disabled using the --no-platform-link option, generating the same output as previously.

One problem I had to solve was how to handle the transition from prerelease versions to final releases, since the location of the documentation changes in this transition. For obvious reasons we don’t want to make that switch via code change at a time shortly before release.

The way it is done is that we determine if the current javadoc instance is a prerelease version as indicated by the Version returned by BaseConfiguration#getDocletVersion(), and then check whether the release/source version of the current javadoc execution uses the same (latest) version. This means that that only the latest version will ever generate prerelease links (e.g. running current javadoc 16 with source version 15 will generate links to the final release documentation) but I think this is acceptable.

Another issue I spent some time getting right was tests. New releases require a new element-list resource*), so tests have to pick up new releases. On the other hand, we don’t want hundreds of tests to fail when a new release is added, ideally there should be one test with a clear message. Because of this, when a release is encountered for which no element-list is available a warning is generated instead of an error, and the documentation is generated without platform links as if running with --no-platform-link option. This allows most existing tests to pass and just the new test to fail with a relatively clear message of what is wrong.

*) I also thought about generating the element-list for the current release at build time. It’s quite involved, and we still need to maintain element-lists for older versions, so I’m not sure it’s worth it.

For existing tests that check output affected by the new option I added the --no-platform-link option to disable the feature. Otherwise we’d have to update those tests with each new release (or else freeze the tests to use some static release or source version, which we don’t want either).

I updated the CSR to the new code. It also needs to be reviewed:
https://bugs.openjdk.java.net/browse/JDK-8251181

Thanks,
Hannes

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Testing

	Linux x64	Windows x64	macOS x64
Build	⏳ (1/3 running)	⏳ (2/2 running)	✔️ (2/2 passed)
Test (tier1)			⏳ (8/9 running)

Issue

JDK-8216497: javadoc should auto-link to platform classes

Reviewers

Erik Joelsson (@erikj79 - Reviewer) ⚠️ Review applies to 2aed84f
Jonathan Gibbons (@jonathan-gibbons - Reviewer) ⚠️ Review applies to 6009dd7

Contributors

Jan Lahoda <jlahoda@openjdk.org>

Download

$ git fetch https://git.openjdk.java.net/jdk pull/171/head:pull/171
$ git checkout pull/171

bridgekeeper · 2020-09-15T09:12:29Z

👋 Welcome back hannesw! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2020-09-15T09:13:38Z

@hns The following labels will be automatically applied to this pull request: build compiler core-libs javadoc.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label (add|remove) "label" command.

mlbridge · 2020-09-15T09:18:19Z

Webrevs

….sym historical data.

lahodaj · 2020-09-15T11:30:09Z

I think it would be awesome if we could generate (most of) the {element,package}-list-VERSION.txt from the ct.sym historical data at build time. This would (hopefully) help with long-term maintenance. I've started to sketch that here:
lahodaj@36c1510

Some comments on the attempt:
-in this PR, there is package-list-9.txt - should it be element-list-9.txt, and should it contain module names (dtto element-list-10.txt)?
-we may (for historical reasons) want to keep the lists for 7, 8, 9 and 10 (as the historical data in ct.sym do not exactly match what is in the package/element lists). It would be better to generate everything, but having a fixed list for some of the past versions would be better than having to create a new list for each new release.
-the patch does not yet generate the list for the current release, but that should be doable.

hns · 2020-09-15T12:55:35Z

Thanks for the suggestions and help, Jan!

I think it would be awesome if we could generate (most of) the {element,package}-list-VERSION.txt from the ct.sym historical data at build time. This would (hopefully) help with long-term maintenance. I've started to sketch that here:
lahodaj@36c1510

I agree files should be generated dynamically. I knew about the sym files but wasn't sure how to go about it. Thanks a lot for stepping in and helping out, it's very much appreciated!

Some comments on the attempt:
-in this PR, there is package-list-9.txt - should it be element-list-9.txt, and should it contain module names (dtto element-list-10.txt)?

Javadoc in 9 still uses the old package-centric layout (package-list and no module names in URL paths). It only became fully module-aware in 10.

-we may (for historical reasons) want to keep the lists for 7, 8, 9 and 10 (as the historical data in ct.sym do not exactly match what is in the package/element lists). It would be better to generate everything, but having a fixed list for some of the past versions would be better than having to create a new list for each new release.
-the patch does not yet generate the list for the current release, but that should be doable.

I definitely agree. I'll work on a new version that generates as much of the lists as possible.

erikj79

Build changes look good.

openjdk · 2020-09-15T12:56:51Z

@hns This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8216497: javadoc should auto-link to platform classes

Co-authored-by: Jan Lahoda <jlahoda@openjdk.org>
Reviewed-by: erikj, jjg

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

04ca660: 8253874: [JVMCI] added test omitted in 8252881
49128a1: 8253475: Javadoc clean up in HttpExchange and HttpServer

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

hns · 2020-09-15T13:12:26Z

Converted to draft as @lahodaj has offered to enhance the feature as described in the comments above.

…r versions < 11

jonathan-gibbons · 2020-09-18T16:44:34Z

/csr

openjdk · 2020-09-18T16:45:28Z

@jonathan-gibbons this pull request will not be integrated until the CSR request JDK-8251181 for issue JDK-8216497 has been approved.

Automatically generate the elements-list data from the ct.sym for releases 11+, including the current release under development

openjdk · 2020-09-21T10:44:38Z

⚠️ @hns This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

openjdk · 2020-09-22T08:45:31Z

@hns Could not parse Jan Lahoda jan.lahoda@oracle.com as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

/contributor add @openjdk-bot
/contributor add duke
/contributor add J. Duke <duke@openjdk.org>

hns · 2020-09-22T08:49:59Z

/contributor add @lahodaj

openjdk · 2020-09-22T08:50:22Z

@hns
Contributor Jan Lahoda <jlahoda@openjdk.org> successfully added.

jonathan-gibbons

Generally excellent. Some feedback inline.

jonathan-gibbons · 2020-09-22T17:14:56Z

...avadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/standard.properties

+    <url>
+doclet.usage.link-platform-properties.description=\
+    Link to platform documentation URLs declared in properties file at <url>
+
 doclet.usage.excludedocfilessubdir.parameters=\
    <name>:..


3 dots for ellipsis? 2 dots is "parent directory"

...avadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/standard.properties

jonathan-gibbons · 2020-09-22T17:17:26Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/BaseOptions.java

@@ -185,6 +190,13 @@
     */
    private boolean noDeprecated = false;

+    /**
+     * Argument for command-line option {@code --no-platform-link}.


minor: would "--no-platform-links" (plural) be a better name for the option?

jonathan-gibbons · 2020-09-22T17:19:17Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/BaseOptions.java

@@ -411,6 +432,14 @@ public boolean process(String opt, List<String> args) {
                    }
                },

+                new Option(resources, "--no-platform-link") {


Repeating preceding comment: would --no-platform-links (plural) be a better name?

jonathan-gibbons · 2020-09-22T17:22:49Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Extern.java

+     *
+     * @param linkPlatformProperties path or URL to properties file containing
+     *                               platform documentation URLs, or null
+     * @param reporter The {@code DocErrorReporter} used to report errors.


(pedantic) param descriptions are typically phrases (no initial capital, no trailing period)
In this case, your two @param descriptions are inconsistent

jonathan-gibbons · 2020-09-22T17:24:19Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Extern.java

+            if (isUrl(linkPlatformProperties)) {
+                inputStream = toURL(linkPlatformProperties).openStream();
+            } else {
+                inputStream = DocFile.createFileForInput(configuration, linkPlatformProperties).openInputStream();
+            }
+            try (inputStream) {
+                props.load(inputStream);
+            }
+            url = props.getProperty("doclet.platform.docs." + version);


Similar to other file-or-url arguments: good!

As a possibly-later cleanup, should we have a single utility method somewhere (in this class) to open a stream on a file-or-url?

jonathan-gibbons · 2020-09-22T17:26:29Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Extern.java

+     */
+    private int getSourceVersionNumber() {
+        SourceVersion sourceVersion = configuration.docEnv.getSourceVersion();
+        // TODO it would be nice if this was provided by SourceVersion


File an RFE for SourceVersion

jonathan-gibbons · 2020-09-22T17:30:19Z

test/langtools/jdk/javadoc/doclet/testAnnotationTypes/TestAnnotationTypes.java

@@ -46,6 +46,7 @@ public static void main(String... args) throws Exception {
    public void test() {
        javadoc("-d", "out-1",
                "-sourcepath", testSrc,
+                "--no-platform-link",


I see lots of instances of no-platform-link in this and subsequent tests. JavadocTester does have the concept of default options, although that may be more for the behavior after executing javadoc and not for the options given to javadoc itself. Is it worth supporting default javadoc options, since that the default can be disabled for specific tests?

I can't really find how JavadocTester uses or supports default options. My concern with this would be that it would make JavadocTester less transparent and intuitive to use, as you'd have to be aware what the default options are.

mlbridge · 2020-09-23T11:52:22Z

Mailing list message from Hannes Wallnoefer on javadoc-dev:

Switching to reply by email to javadoc-dev only in order to not spam all the other mailing lists with each comment/reply.

Am 22.09.2020 um 19:37 schrieb Jonathan Gibbons <jjg at openjdk.java.net>:

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/standard.properties line 350:

348:
349: doclet.usage.excludedocfilessubdir.parameters=\
350: <name>:..

3 dots for ellipsis? 2 dots is "parent directory"

That is outside the scope of this changeset. Should I include the fix regardless?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/standard.properties line 384:

382:
383: doclet.usage.no-platform-link.description=\
384: Do not generate links to platform documentation

Suggest:
Do not generate links to the platform documentation

Changed in next commit.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/BaseOptions.java line 194:

192:
193: /**
194: * Argument for command-line option {@code --no-platform-link}.

minor: would "--no-platform-links" (plural) be a better name for the option?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/BaseOptions.java line 435:

433: },
434:
435: new Option(resources, "--no-platform-link") {

Repeating preceding comment: would `--no-platform-links` (plural) be a better name?

Yes, I think `--no-platform-links` is a better name, although it will require some changes in tests and CSR.
I guess it helped to convince me that you asked twice :)

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Extern.java line 236:

234: * @param linkPlatformProperties path or URL to properties file containing
235: * platform documentation URLs, or null
236: * @param reporter The {@code DocErrorReporter} used to report errors.

(pedantic) param descriptions are typically phrases (no initial capital, no trailing period)
In this case, your two `@param` descriptions are inconsistent

Fixed.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Extern.java line 323:

321: props.load(inputStream);
322: }
323: url = props.getProperty("doclet.platform.docs." + version);

Similar to other file-or-url arguments: good!

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Extern.java line 340:

338: private int getSourceVersionNumber() {
339: SourceVersion sourceVersion = configuration.docEnv.getSourceVersion();
340: // TODO it would be nice if this was provided by SourceVersion

File an RFE for `SourceVersion`

Will do.

test/langtools/jdk/javadoc/doclet/testAnnotationTypes/TestAnnotationTypes.java line 49:

47: javadoc("-d", "out-1",
48: "-sourcepath", testSrc,
49: "--no-platform-link",

I see lots of instances of `no-platform-link` in this and subsequent tests. `JavadocTester` does have the concept of
default options, although that may be more for the behavior after executing javadoc and not for the options given to
javadoc itself. Is it worth supporting default javadoc options, since that the default can be disabled for specific
tests?

I answered that one in a separate comment on github already, to recap: I couldn?t find the default options mechanism
in JavadocTester, and I?m afraid it will be detrimental for transparency and usability of the class.

Hannes

jonathan-gibbons

I agree with the judgement call to not introduce default javadoc options. It was just a suggestion to consider, and I agree it would make the calls less intuitively obvious, for the short term gain of editing fewer tests here. It also helped to realize that the support default platform links does not involve any network access.

FWIW, the precedent in JavadocTester that I was referrng to is setAutomaticCheckLinks, setAutomaticCheckAccessibility, but those are about default actions after javadoc has been run, and not about default methods.

hns · 2020-10-07T13:06:51Z

/integrate

openjdk · 2020-10-07T13:09:09Z

@hns Since your change was applied there have been 2 commits pushed to the master branch:

04ca660: 8253874: [JVMCI] added test omitted in 8252881
49128a1: 8253475: Javadoc clean up in HttpExchange and HttpServer

Your commit was automatically rebased without conflicts.

Pushed as commit 1e8e543.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8216497: javadoc should auto-link to platform classes

2aed84f

openjdk bot added the rfr Pull request is ready for review label Sep 15, 2020

openjdk bot added javadoc javadoc-dev@openjdk.org build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org labels Sep 15, 2020

Attempting to (mostly) generate the javadoc release manifests from ct…

36c1510

….sym historical data.

erikj79 approved these changes Sep 15, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 15, 2020

hns marked this pull request as draft September 15, 2020 13:10

openjdk bot removed the rfr Pull request is ready for review label Sep 15, 2020

Generating current release list for javadoc; using hardcoded lists fo…

5be834b

…r versions < 11

openjdk bot added the csr Pull request needs approved CSR before integration label Sep 18, 2020

Merge pull request #1 from lahodaj/JDK-8216497

6d659ae

Automatically generate the elements-list data from the ct.sym for releases 11+, including the current release under development

hns marked this pull request as ready for review September 21, 2020 12:51

openjdk bot added the rfr Pull request is ready for review label Sep 21, 2020

jonathan-gibbons reviewed Sep 22, 2020

View reviewed changes

Rename --no-platform-link option plus minor cleanup

6009dd7

openjdk bot removed the ready Pull request is ready to be integrated label Sep 23, 2020

jonathan-gibbons approved these changes Sep 23, 2020

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Sep 29, 2020

hns added 2 commits October 7, 2020 13:48

Merge branch 'master' into JDK-8216497

2f74741

Fix test failure

991daef

openjdk bot closed this Oct 7, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 7, 2020

8216497: javadoc should auto-link to platform classes #171

8216497: javadoc should auto-link to platform classes #171

hns commented Sep 15, 2020 •

edited by openjdk bot

bridgekeeper bot commented Sep 15, 2020

openjdk bot commented Sep 15, 2020

mlbridge bot commented Sep 15, 2020 •

edited

lahodaj commented Sep 15, 2020

hns commented Sep 15, 2020

erikj79 left a comment

openjdk bot commented Sep 15, 2020 •

edited

hns commented Sep 15, 2020

jonathan-gibbons commented Sep 18, 2020

openjdk bot commented Sep 18, 2020

openjdk bot commented Sep 21, 2020

openjdk bot commented Sep 22, 2020

hns commented Sep 22, 2020

openjdk bot commented Sep 22, 2020

jonathan-gibbons left a comment

jonathan-gibbons Sep 22, 2020

jonathan-gibbons Sep 22, 2020

jonathan-gibbons Sep 22, 2020

jonathan-gibbons Sep 22, 2020

jonathan-gibbons Sep 22, 2020

jonathan-gibbons Sep 22, 2020

jonathan-gibbons Sep 22, 2020

jonathan-gibbons Sep 22, 2020

hns Sep 23, 2020

mlbridge bot commented Sep 23, 2020

jonathan-gibbons left a comment

hns commented Oct 7, 2020

openjdk bot commented Oct 7, 2020

8216497: javadoc should auto-link to platform classes #171

8216497: javadoc should auto-link to platform classes #171

Conversation

hns commented Sep 15, 2020 • edited by openjdk bot

Progress

Testing

Issue

Reviewers

Contributors

Download

bridgekeeper bot commented Sep 15, 2020

openjdk bot commented Sep 15, 2020

mlbridge bot commented Sep 15, 2020 • edited

Webrevs

lahodaj commented Sep 15, 2020

hns commented Sep 15, 2020

erikj79 left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 15, 2020 • edited

hns commented Sep 15, 2020

jonathan-gibbons commented Sep 18, 2020

openjdk bot commented Sep 18, 2020

openjdk bot commented Sep 21, 2020

openjdk bot commented Sep 22, 2020

hns commented Sep 22, 2020

openjdk bot commented Sep 22, 2020

jonathan-gibbons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Sep 23, 2020

jonathan-gibbons left a comment

Choose a reason for hiding this comment

hns commented Oct 7, 2020

openjdk bot commented Oct 7, 2020

hns commented Sep 15, 2020 •

edited by openjdk bot

mlbridge bot commented Sep 15, 2020 •

edited

openjdk bot commented Sep 15, 2020 •

edited