-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8296546: Add @spec tags to API #11073
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
8296546: Add @spec tags to API #11073
Conversation
|
👋 Welcome back jjg! A progress list of the required criteria for merging this PR into |
|
@jonathan-gibbons The following labels will be automatically applied to this pull request:
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 pull request command. |
Webrevs
|
|
Hi Jon, When referencing an RFC, it might be good to keep the RFC number in the text link. For instance I see that java.net.URL now has this: http://cr.openjdk.java.net/~jjg/8296546/api.00/java.base/java/net/URL.html External Specifications You will see that two of the RFC links have the same text but link to different RFCs, which I am finding confusing. |
I agree and also to add that some RFCs have commas in their titles, the same separator used when there is more than one specification linked. Here's an example: http://cr.openjdk.java.net/~jjg/8296546/api.00/java.base/java/nio/channels/MulticastChannel.html |
|
I'm trying to understand what "fix-ups" will be needed if the automated patch is applied. In some cases, it looks the same spec will be linked from "See also" and "External Specifications", e.g. In other cases we will have inline refs and the same URL in the There will probably be a bit of cleanup to reflow some lines, e.g. StandardSocketOptions.java, as excessively long lines are problematic for side-by-side diffs. |
|
did you changed 420 files ? |
|
Mailing list message from Michael StJohns on security-dev: Daniel et al - Please avoid using ietf.org as the cite location for RFCs The preferred cite for RFCs is generally via Cf https://www.rfc-editor.org/info/rfc4180 - "Cite this RFC"? -
Note that the most stable cite might be the DOI cite, but the above is Mike On 11/10/2022 6:34 AM, Daniel Fuchs wrote: -------------- next part -------------- |
+1. |
On keeping RFC in the title, I'll go with the team preference. I note that not all spec authorities have such a well-defined naming/numbering scheme, so it does make the summary page a bit inconsistent. Also, the entries under "R" dominate the list, which may not be what you want. On the same text but linking to different RFCs: that's tantamount to a bug somewhere. The spec for |
I can change the doclet to use a bulleted list when any spec titles contain a comma. |
The utility I mentioned has the (optional) ability to remove When inline references are used, the wording is very rarely the primary title of the spec: it is more likely to be a word or phrase that makes sense in the context of the enclosing sentence. History: version 1 of this feature tried replacing inline links and |
|
/issue add JDK-8296546 |
|
@jonathan-gibbons This issue is referenced in the PR title - it will now be updated. |
I ran a custom utility that edited these files, yes. |
| * use instances for synchronization, or unpredictable behavior may | ||
| * occur. For example, in a future release, synchronization may fail. | ||
| * | ||
| * @spec https://www.unicode.org/reports/tr27 Unicode 3.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be removed, as the original link (explaining U+n notation) is broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naotoj The edits are driven by a script, using info about existing links in the same doc comment. If you don't think this reference is appropriate, it would be better to either remove the existing link (and I'll regenerate this patch) or else this patch goes through and you fix up both the existing link and the @spec tag afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way is fine with me. I will fix it up if you choose the latter.
| * @spec jdwp/jdwp-spec.html Java Debug Wire Protocol | ||
| * @spec jdwp/jdwp-transport.html Java Debug Wire Protocol Transport Interface (jdwpTransport) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http://cr.openjdk.java.net/~jjg/8296546/api.00/jdk.jdwp.agent/module-summary.html
The end result here is not very clean. You have the same two specs being referred to just a few lines apart, and the hyperlink titles are not even close to be the same, even though the links are the same. Maybe the "@see" section should be removed.
| @@ -104,6 +104,7 @@ | |||
| * </blockquote> | |||
| * | |||
| * | |||
| * @spec jpda/jpda.html Java Platform Debugger Architecture | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http://cr.openjdk.java.net/~jjg/8296546/api.00/jdk.jdi/module-summary.html
@spec and @see sections end up one right after the other with the same content, except the @see section has the preferred hyperlink title. Suggest you remove the @see section and also update @spec hyperlink title to include "(JPDA)", or update the actual title in the jpda.html doc so it includes "(JPDA)" in it and then rerun your tool.
| @@ -102,6 +102,7 @@ public Connection() {} | |||
| * @throws java.io.IOException | |||
| * If the length of the packet (as indictaed by the first | |||
| * 4 bytes) is less than 11 bytes, or an I/O error occurs. | |||
| * @spec jdwp/jdwp-spec.html Java Debug Wire Protocol | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Within this javadoc page the jdwp-spec.html references are titled "JDWP Specification", but these @spec references are titled "Java Debug Wire Protocol". I suggest making them more consistent. There is one more case below and this same issue also applies to TransportService.java. Perhaps the title in jdwp-spec.html should be updated. I think "Java Debug Wire Protocol (JDWP) Specification" would be good.
| @@ -76,6 +76,8 @@ | |||
| * method is used to accept a connection initiated by a | |||
| * target VM. | |||
| * | |||
| * @spec jdwp/jdwp-spec.html Java Debug Wire Protocol | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment for Connection.java.
It's not uncommon for a newer version of a RFC to change its number but keep its title. I see that the links in the class level API documentation both have the RFC number in their link text. Somehow that was stripped by your tool - possibly because it tried to extract some meta information from the linked page itself? |
LanceAndersen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Jon,
I only looked at the jar specific updates but there is some duplication leftovers.
It would probably be easier for the reviewers and for you if the PR could be broken out by areas into separate PRs
| * Manifest and Signature Specification</a> - The manifest format specification. | ||
| * </ul> | ||
| * | ||
| * @spec jar/jar.html JAR File Specification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 43 should be removed
| * <a href="{@docRoot}/../specs/jar/jar.html"> | ||
| * Manifest format specification</a>. | ||
| * | ||
| * @spec jar/jar.html JAR File Specification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 44 should be removed
| * and will be UTF8-encoded when written to the output stream. See the | ||
| * <a href="{@docRoot}/../specs/jar/jar.html">JAR File Specification</a> | ||
| * for more information about valid attribute names and values. | ||
| * @spec jar/jar.html JAR File Specification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 448 should be removed
| * <p>This map and its views have a predictable iteration order, namely the | ||
| * order that keys were inserted into the map, as with {@link LinkedHashMap}. | ||
| * | ||
| * @spec jar/jar.html JAR File Specification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 52 should be removed
| * <li>Adler-32 checksum is described in RFC 1950 (above) | ||
| * </ul> | ||
| * | ||
| * @spec https://www.ietf.org/rfc/rfc1951.html DEFLATE Compressed Data Format Specification version 1.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above references should be removed as they duplicate the @spec tags
Leaving out the non-public and non-exported classes would also reduce the PR size. |
|
Thanks for adding the RFC NNNN prefix to the RFC link. What is the purpose of editing non exported classes though, like those in the |
That was not intentional, and is a result of the scripted edit. I will look to revert those changes and/or change the tooling to ignore those packages. |
|
The java.base/net/, java.http/, java.naming/ changes look reasonable to me - though like Alan I wonder if it wouldn't be better to have an inline |
Believe me, I tried very hard to design and use an inline The general history of this work is:
|
| * @spec https://www.w3.org/TR/xml11 Extensible Markup Language (XML) 1.1 (Second Edition) | ||
| * @spec https://www.w3.org/TR/REC-xml-names Namespaces in XML 1.0 (Third Edition) | ||
| * @spec https://www.w3.org/TR/xml-names11 Namespaces in XML 1.1 (Second Edition) | ||
| * @spec https://www.w3.org/TR/xmlschema-1 XML Schema Part 1: Structures Second Edition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Jon,
I would agree with what Alan said earlier that the @see ref can be dropped. This particular class (XMLConstants.java [1]) is a good example for that argument: in the resulting javadoc, 5 specs were listed in the "External Specifications" section, 6 in "See Also:", and then they were listed again for each field. That's a lot of duplicates. Adding to the confusion was that the @SPEC and @see were not always the same, e.g. @SPEC XML 1.0.
points to the fifth edition while @see second.
A minor comment is that the '@SPEC's were rendered in one line while the @see refs a list. I would see the later is easier to read.
[1] http://cr.openjdk.java.net/~jjg/8296546/api.00/java.xml/javax/xml/XMLConstants.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The presentation of lists of @spec tags was fixed separately (JDK-8297802), and is incorporated into the latest docs that demo this work. The same algorithm is now used for both @see and @spec tags ... if the links are short and do not contain commas, they will be displayed as an inline list; otherwise, they will be displayed in a bulleted list.
JoeWang-Java
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specs for XSLT and XPath (many occurrences) need to point to specific version (e.g. 1.0) rather than the "cover page" (this is an issue in the original javadoc).
| * <p>All the fields in this class are read-only.</p> | ||
| * | ||
| * @spec https://www.w3.org/TR/xslt xslt cover page - W3C | ||
| * @see <a href="http://www.w3.org/TR/xslt#output"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pages for XSLT and XPath at W3C are organized differently from the days when this javadoc was created. The "latest version" now points to the "cover page". Could you change the spec to the following?
https://www.w3.org/TR/1999/REC-xslt-19991116 XSL Transformations (XSLT) Version 1.0
The @SPEC points to the general spec while @see also a specific section (similar situation as other classes in the package), if we want to keep @see ref here, it would be:
https://www.w3.org/TR/1999/REC-xslt-19991116#output
| * @spec https://www.w3.org/TR/xpath xpath cover page - W3C | ||
| * @author Norman Walsh | ||
| * @author Jeff Suttor | ||
| * @see <a href="http://www.w3.org/TR/xpath">XML Path Language (XPath) Version 1.0</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar situation as XSLT above, the latest version now points to "cover page". For this javadoc then, it needs to be:
https://www.w3.org/TR/1999/REC-xpath-19991116/ XML Path Language (XPath) Version 1.0
Unlike XSLT, the original @see ref also points to the spec generally (not a specific section), we could then drop it to keep just the @SPEC ref.
| * </dl> | ||
| * | ||
| * @spec jdwp/jdwp-spec.html Java Debug Wire Protocol | ||
| * @spec jni/index.html Java Native Interface Specification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that that bothers me a bit here is that the JNI and JDWP specs will be listed as "External Specifications" in the generated javadoc. This heading is appropriate for RFCs and other standards that we reference but seems misleading for specifications that are part of Java SE. Also if the existing table is removed then loose the word "Optional". Has there been any other concerns about this?
| * <li><a href="doc-files/FocusSpec.html">The AWT Focus Subsystem</a> | ||
| * <li><a href="doc-files/Modality.html">The AWT Modality</a> | ||
| * <li><a href="{@docRoot}/../specs/AWT_Native_Interface.html"> | ||
| * The Java AWT Native Interface (JAWT)</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only 1 of these 3 ?
| * </ul> | ||
| * | ||
| * @spec AWT_Native_Interface.html The Java AWT Native Interface Specification and Guide | ||
| * @since 1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if links to html we include in the javadoc should be really treated in the same manner as referecnes to externally defined specifactions ?
But I also wonder why only the native_interface spec was added and not the other two ?
| * | ||
| * @spec https://www.ietf.org/rfc/rfc1951.html RFC 1951: DEFLATE Compressed Data Format Specification version 1.3 | ||
| * @see #TAG_COMPRESSION | ||
| * @see <a href="https://tools.ietf.org/html/rfc1951">DEFLATE specification</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Mailing list message from Michael StJohns on compiler-dev: Hi - I need to repeat again.? Please avoid using www.ietf.org as the URL base The most general and correct form for referencing RFCs is Mike On 11/28/2022 6:27 PM, Phil Race wrote: |
|
Mailing list message from Daniel Fuchs on core-libs-dev: We hear you Mike :-) I have logged https://bugs.openjdk.org/browse/JDK-8297755 to update the best regards, -- daniel On 29/11/2022 03:14, Michael StJohns wrote:
|
|
Mailing list message from Jonathan Gibbons on compiler-dev: On 11/10/22 8:18 PM, Chris Plummer wrote:
Chris, The general problem is that we're starting from an inconsistent code base. Like you, I believe we should strive for consistency, especially between This patch is generated by tooling that understands specific families of As for `@see` tags, the tooling currently has the ability to remove a -- Jon |
|
Mailing list message from Jonathan Gibbons on core-libs-dev: Mike, Thank you for the additional info. In general, the intent of this patch is to leverage the existing links -- Jon On 11/28/22 7:14 PM, Michael StJohns wrote:
|
|
Mailing list message from Jonathan Gibbons on core-libs-dev: On 11/28/22 3:27 PM, Phil Race wrote:
Only one is a link outside of the overall api/ documentation hierarchy
The patch is generated by running a tool that detects existing links to
At this time yes, although the tooling does currently allow `@see` tags |
dfuch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed again the java.net, java.net.http, jdk.httpserver, java.naming, and javax.management changes - and I spotted a few places where the @spec duplicates an @see (noted below). I believe the duplicated @see should be removed now.
| * @spec https://www.rfc-editor.org/info/rfc919 RFC 919: Broadcasting Internet Datagrams | ||
| * @see <a href="http://www.ietf.org/rfc/rfc919.txt">RFC 929: | ||
| * Broadcasting Internet Datagrams</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This @see line should now be removed since it's referencing the exact same document.
| * @spec https://www.rfc-editor.org/info/rfc1122 RFC 1122: Requirements for Internet Hosts - Communication Layers | ||
| * @see <a href="http://www.ietf.org/rfc/rfc1122.txt">RFC 1122 | ||
| * Requirements for Internet Hosts -- Communication Layers</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark here: please remove the @see
| * @see <a href="http://www.ietf.org/rfc/rfc1323.txt">RFC 1323: TCP | ||
| * Extensions for High Performance</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the @see
| * @spec https://www.rfc-editor.org/info/rfc793 RFC 793: Transmission Control Protocol | ||
| * @see <a href="http://www.ietf.org/rfc/rfc793.txt">RFC 793: Transmission |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the @see
| * @spec https://www.rfc-editor.org/info/rfc1122 RFC 1122: Requirements for Internet Hosts - Communication Layers | ||
| * @see <a href="http://www.ietf.org/rfc/rfc1122.txt">RFC 1122: | ||
| * Requirements for Internet Hosts -- Communication Layers</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the @see
| * @spec https://www.rfc-editor.org/info/rfc2609 RFC 2609: Service Templates and Service: Schemes | ||
| * @spec https://www.rfc-editor.org/info/rfc3111 RFC 3111: Service Location Protocol Modifications for IPv6 | ||
| * @see <a | ||
| * href="http://www.ietf.org/rfc/rfc2609.txt">RFC 2609, | ||
| * "Service Templates and <code>Service:</code> Schemes"</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two @see should now be removed
|
@jonathan-gibbons This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
@jonathan-gibbons This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
Please review a "somewhat automated" change to insert
@spectags into doc comments, as appropriate, to leverage the recent new javadoc feature to generate a new page listing the references to all external specifications listed in the@spectags."Somewhat automated" means that I wrote and used a temporary utility to scan doc comments looking for HTML links to selected sites, such as
ietf.org,unicode.org,w3.org. These links may be in the main description of a doc comment, or in@seetags. For each link, the URL is examined, and "normalized", and inserted into the doc comment with a new@spectag, giving the link and tile for the spec."Normalized" means...
https:where possible (includes pretty much all cases)In addition, a "standard" title is determined for all specs, determined either from the content of the (main) spec page or from site index pages.
The net effect is (or should be) that all the changes are to just add new
@spectags, based on the links found in each doc comment. There should be no other changes to the doc comments, or to the implementation of any classes and interfaces.That being said, the utility I wrote does have additional abilities, to update the links that it finds (e.g. changing to use
https:etc,) but those features are not being used here, but could be used in followup PRs if component teams so desired. I did notice while working on this overall feature that many of our links do point to "outdated" pages, some with eye-catching notices declaring that the spec has been superseded. Determining how, when and where to update such links is beyond the scope of this PR.Going forward, it is to be hoped that component teams will maintain the underlying links, and the URLs in
@spectags, such that if references to external specifications are updated, this will include updating the@spectags.To see the effect of all these new
@spectags, see http://cr.openjdk.java.net/~jjg/8296546/api.00/In particular, see the new External Specifications page, which you can also find via the new link near the top of the Index pages.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11073/head:pull/11073$ git checkout pull/11073Update a local copy of the PR:
$ git checkout pull/11073$ git pull https://git.openjdk.org/jdk pull/11073/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 11073View PR using the GUI difftool:
$ git pr show -t 11073Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11073.diff