Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v8]
> Please review a patch to add support for Markdown syntax in documentation > comments, as described in the associated JEP. > > Notable features: > > * support for `///` documentation comments in `JavaTokenizer` > * new module `jdk.internal.md` -- a private copy of the `commonmark-java` > library > * updates to `DocCommentParser` to treat `///` comments as Markdown > * updates to the standard doclet to render Markdown comments in HTML Jonathan Gibbons has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits: - Merge remote-tracking branch 'upstream/master' into 8298405.doclet-markdown-v3 - Merge with upstream/master - Merge with upstream/master - Merge remote-tracking branch 'upstream/master' into 8298405.doclet-markdown-v3 - Address review comments - Fix whitespace - Improve handling of embedded inline taglets - Customize support for Markdown headings - JDK-8298405: Support Markdown in Documentation Comments - Changes: https://git.openjdk.org/jdk/pull/16388/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=07 Stats: 21334 lines in 191 files changed: 20679 ins; 266 del; 389 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
Re: RFR: 8324723: GHA: Upgrade some actions to avoid deprecated Node 16
On Thu, 25 Jan 2024 14:48:40 GMT, Aleksey Shipilev wrote: > Current GHA runs produce lots of warnings: > > Node.js 16 actions are deprecated. Please update the following actions to use > Node.js 20: actions/cache@v3, actions/download-artifact@v3, > actions/upload-artifact@v3. For more information see: > https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/. > > We can upgrade to new actions to get the Node20. > > Release/migration notes: > https://github.com/actions/cache#whats-new > https://github.com/actions/upload-artifact/blob/main/docs/MIGRATION.md > https://github.com/actions/download-artifact/blob/main/docs/MIGRATION.md > https://github.com/actions/github-script#breaking-changes > > There is also msys2/setup-msys2, which was pinned by > [JDK-8310259](https://bugs.openjdk.org/browse/JDK-8310259). We need at least > 2.21.0 to get Node 20: > https://github.com/msys2/setup-msys2/blob/main/CHANGELOG.md. I think we can > unpin msys2 at this point. > > I don't think any of the migration problems outlined in those notes apply to > our workflows. > > Additional testing: > - [x] 3x GHA with cleaned caches > - [ ] 3x GHA with populated caches (default) Sure. I am now running multiple GHA iterations and trying to see if msys2 is stable enough to unpin. - PR Comment: https://git.openjdk.org/jdk/pull/17572#issuecomment-1911808258
Re: RFR: 8324723: GHA: Upgrade some actions to avoid deprecated Node 16
On Thu, 25 Jan 2024 14:48:40 GMT, Aleksey Shipilev wrote: > Current GHA runs produce lots of warnings: > > Node.js 16 actions are deprecated. Please update the following actions to use > Node.js 20: actions/cache@v3, actions/download-artifact@v3, > actions/upload-artifact@v3. For more information see: > https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/. > > We can upgrade to new actions to get the Node20. > > Release/migration notes: > https://github.com/actions/cache#whats-new > https://github.com/actions/upload-artifact/blob/main/docs/MIGRATION.md > https://github.com/actions/download-artifact/blob/main/docs/MIGRATION.md > https://github.com/actions/github-script#breaking-changes > > There is also msys2/setup-msys2, which was pinned by > [JDK-8310259](https://bugs.openjdk.org/browse/JDK-8310259). We need at least > 2.21.0 to get Node 20: > https://github.com/msys2/setup-msys2/blob/main/CHANGELOG.md. I think we can > unpin msys2 at this point. > > I don't think any of the migration problems outlined in those notes apply to > our workflows. > > Additional testing: > - [x] 3x GHA with cleaned caches > - [ ] 3x GHA with populated caches (default) Thanks for looking into this. - PR Comment: https://git.openjdk.org/jdk/pull/17572#issuecomment-1911801515
Re: RFR: 8324723: GHA: Upgrade some actions to avoid deprecated Node 16 [v2]
> Current GHA runs produce lots of warnings: > > Node.js 16 actions are deprecated. Please update the following actions to use > Node.js 20: actions/cache@v3, actions/download-artifact@v3, > actions/upload-artifact@v3. For more information see: > https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/. > > We can upgrade to new actions to get the Node20. > > Release/migration notes: > https://github.com/actions/cache#whats-new > https://github.com/actions/upload-artifact/blob/main/docs/MIGRATION.md > https://github.com/actions/download-artifact/blob/main/docs/MIGRATION.md > https://github.com/actions/github-script#breaking-changes > > There is also msys2/setup-msys2, which was pinned by > [JDK-8310259](https://bugs.openjdk.org/browse/JDK-8310259). We need at least > 2.21.0 to get Node 20: > https://github.com/msys2/setup-msys2/blob/main/CHANGELOG.md. I think we can > unpin msys2 at this point. > > I don't think any of the migration problems outlined in those notes apply to > our workflows. > > Additional testing: > - [x] 3x GHA with cleaned caches > - [ ] 3x GHA with populated caches (default) Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision: Also update github-script - Changes: - all: https://git.openjdk.org/jdk/pull/17572/files - new: https://git.openjdk.org/jdk/pull/17572/files/80390ae5..46545f82 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17572&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17572&range=00-01 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17572.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17572/head:pull/17572 PR: https://git.openjdk.org/jdk/pull/17572
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Fri, 19 Jan 2024 18:37:48 GMT, Jonathan Gibbons wrote:
>> Please review a patch to add support for Markdown syntax in documentation
>> comments, as described in the associated JEP.
>>
>> Notable features:
>>
>> * support for `///` documentation comments in `JavaTokenizer`
>> * new module `jdk.internal.md` -- a private copy of the `commonmark-java`
>> library
>> * updates to `DocCommentParser` to treat `///` comments as Markdown
>> * updates to the standard doclet to render Markdown comments in HTML
>
> Jonathan Gibbons has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains eight commits:
>
> - Merge with upstream/master
> - Merge with upstream/master
> - Merge remote-tracking branch 'upstream/master' into
> 8298405.doclet-markdown-v3
> - Address review comments
> - Fix whitespace
> - Improve handling of embedded inline taglets
> - Customize support for Markdown headings
> - JDK-8298405: Support Markdown in Documentation Comments
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 668:
> 666: * U+FFFC characters that were found in the original document.
> 667: */
> 668: Iterator replaceIter;
Suggestion:
final Iterator replaceIter;
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 732:
> 730: offsets.add(m.end());
> 731: }
> 732: sourceLineOffsets =
> offsets.stream().mapToInt(Integer::intValue).toArray();
Here's an alternative, which you might find better (or not):
Suggestion:
sourceLineOffsets = Stream.concat(Stream.of(0),
lineBreak.matcher(source).results()
.map(MatchResult::end)).mapToInt(Integer::intValue).toArray();
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 763:
> 761: *
> 762: * @param link the link node
> 763: */
Suggestion:
/**
* Visits a {@code Link} commonmark node.
*
* If the destination for the link begins with {@code code:}
* convert it to {@code {@link ...}} or {@code {@linkplain ...}} doc
tree node.
* {@code {@link ...}} will be used if the content (label) for
* the link is the same as the reference found after the {@code code:};
* otherwise, {@code {@linkplain ...}} will be used.
*
* The label will be left blank for {@code {@link ...}} nodes,
* implying that a default label should be used, based on the
* program element that was referenced.
*
* @param link the link node
*/
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 778:
> 776: copyTo(getEndPos(link.getLastChild()));
> 777:
> 778: // determine whether to use {@link... } or
> {@linkplain ...}
Nit:
Suggestion:
// determine whether to use {@link ... } or {@linkplain ...}
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 831:
> 829: /**
> 830: * {@return the position in the original comment for a position
> in {@code source},
> 831: * using {@link #replaceAdjustPos}}.
Suggestion:
* using {@link #replaceAdjustPos}}
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 840:
> 838:
> 839: /**
> 840: * Process a node and any children.
Suggestion:
* Processes a node and any children.
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 939:
> 937:
> 938: /**
> 939: * Flush any text in the {@code text} buffer, by creating a new
Suggestion:
* Flushes any text in the {@code text} buffer, by creating a new
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 950:
> 948: }
> 949:
> 950:
Suggestion:
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467870392
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467870182
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467643549
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467871256
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467876796
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467871714
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467872096
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467872597
Integrated: 8324659: GHA: Generic jtreg errors are not reported
On Thu, 25 Jan 2024 12:02:35 GMT, Aleksey Shipilev wrote: > When looking at [JDK-8324647](https://bugs.openjdk.org/browse/JDK-8324647), I > was surprised to see that GHA has this output: > > > Running test 'jtreg:test/lib-test:tier1' > /home/runner/work/jdk/jdk/test/lib-test/TEST.groups: group all: group > includes itself > Error: One or more groups are invalid > Finished running test 'jtreg:test/lib-test:tier1' > Test report is stored in > build/run-test-prebuilt/test-results/jtreg_test_lib_test_tier1 > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >>> jtreg:test/lib-test:tier1 1 0 0 1 << > == > TEST FAILURE > > > ...and yet, the tests are recorded as "green"! I think this is because our > error reporting code is tad buggy. AFAICS, `make test-prebuilt` does not > report a non-zero exit code on failure, and the external script looks for > `build/run-test-prebuilt/make-support/exit-with-error` to check for error. > But it does not _set_ the exit code on its own when the failure is > discovered, neither it sets `failure=true` on generic failure. > > I think at very minimum we should report `failure=true` on generic failure. I > thought about reporting non-zero exit code from the parsing script, but it > does not feel clean to return non-zero exit code from the _parsing script_. > Non-zero exit code from that script should signify the error in the script > itself. > > Additional testing: > - [x] Current GHA with lib-test failure (now red) > - [x] Current GHA with lib-test fix (now green) > - [x] GHA with artificial errors: > https://github.com/shipilev/jdk/actions/runs/7652797646/job/20854472816 This pull request has now been integrated. Changeset: c313d451 Author:Aleksey Shipilev URL: https://git.openjdk.org/jdk/commit/c313d451a513eb08de0b295c1ce66d0d849d2374 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8324659: GHA: Generic jtreg errors are not reported Reviewed-by: erikj, jwaters, stuefe - PR: https://git.openjdk.org/jdk/pull/17568
Integrated: 8324347: Enable "maybe-uninitialized" warning for FreeType 2.13.1
On Tue, 23 Jan 2024 00:58:27 GMT, Sergey Bylokhov wrote: > The next bug in freetype was fixed upstream and fix already merged to OpenJDK: > https://gitlab.freedesktop.org/freetype/freetype/-/issues/1245 > So now we can revert the workaround in the JDK: > https://bugs.openjdk.org/browse/JDK-8313576 > > Tested with "--with-freetype=bundled", "--with-freetype=system" and w/o > option on the system where the bug was reproduced. This pull request has now been integrated. Changeset: 781f368d Author:Sergey Bylokhov URL: https://git.openjdk.org/jdk/commit/781f368d421a94857929e4168974f43e890637d8 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod 8324347: Enable "maybe-uninitialized" warning for FreeType 2.13.1 Reviewed-by: erikj, azvegint, jwaters, aivanov - PR: https://git.openjdk.org/jdk/pull/17525
Re: RFR: 8324659: GHA: Generic jtreg errors are not reported
On Fri, 26 Jan 2024 08:42:36 GMT, Thomas Stuefe wrote: > Is this problem also in older releases? Yes, I think so. I'll backport it everywhere I can reach. - PR Comment: https://git.openjdk.org/jdk/pull/17568#issuecomment-1911680352
Re: RFR: 8324659: GHA: Generic jtreg errors are not reported
On Thu, 25 Jan 2024 12:02:35 GMT, Aleksey Shipilev wrote: > When looking at [JDK-8324647](https://bugs.openjdk.org/browse/JDK-8324647), I > was surprised to see that GHA has this output: > > > Running test 'jtreg:test/lib-test:tier1' > /home/runner/work/jdk/jdk/test/lib-test/TEST.groups: group all: group > includes itself > Error: One or more groups are invalid > Finished running test 'jtreg:test/lib-test:tier1' > Test report is stored in > build/run-test-prebuilt/test-results/jtreg_test_lib_test_tier1 > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >>> jtreg:test/lib-test:tier1 1 0 0 1 << > == > TEST FAILURE > > > ...and yet, the tests are recorded as "green"! I think this is because our > error reporting code is tad buggy. AFAICS, `make test-prebuilt` does not > report a non-zero exit code on failure, and the external script looks for > `build/run-test-prebuilt/make-support/exit-with-error` to check for error. > But it does not _set_ the exit code on its own when the failure is > discovered, neither it sets `failure=true` on generic failure. > > I think at very minimum we should report `failure=true` on generic failure. I > thought about reporting non-zero exit code from the parsing script, but it > does not feel clean to return non-zero exit code from the _parsing script_. > Non-zero exit code from that script should signify the error in the script > itself. > > Additional testing: > - [x] Current GHA with lib-test failure (now red) > - [x] Current GHA with lib-test fix (now green) > - [x] GHA with artificial errors: > https://github.com/shipilev/jdk/actions/runs/7652797646/job/20854472816 Ouch. Wow. +1. Is this problem also in older releases? - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17568#pullrequestreview-1845283123
Re: RFR: 8324659: GHA: Generic jtreg errors are not reported
On Thu, 25 Jan 2024 12:02:35 GMT, Aleksey Shipilev wrote: > When looking at [JDK-8324647](https://bugs.openjdk.org/browse/JDK-8324647), I > was surprised to see that GHA has this output: > > > Running test 'jtreg:test/lib-test:tier1' > /home/runner/work/jdk/jdk/test/lib-test/TEST.groups: group all: group > includes itself > Error: One or more groups are invalid > Finished running test 'jtreg:test/lib-test:tier1' > Test report is stored in > build/run-test-prebuilt/test-results/jtreg_test_lib_test_tier1 > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >>> jtreg:test/lib-test:tier1 1 0 0 1 << > == > TEST FAILURE > > > ...and yet, the tests are recorded as "green"! I think this is because our > error reporting code is tad buggy. AFAICS, `make test-prebuilt` does not > report a non-zero exit code on failure, and the external script looks for > `build/run-test-prebuilt/make-support/exit-with-error` to check for error. > But it does not _set_ the exit code on its own when the failure is > discovered, neither it sets `failure=true` on generic failure. > > I think at very minimum we should report `failure=true` on generic failure. I > thought about reporting non-zero exit code from the parsing script, but it > does not feel clean to return non-zero exit code from the _parsing script_. > Non-zero exit code from that script should signify the error in the script > itself. > > Additional testing: > - [x] Current GHA with lib-test failure (now red) > - [x] Current GHA with lib-test fix (now green) > - [x] GHA with artificial errors: > https://github.com/shipilev/jdk/actions/runs/7652797646/job/20854472816 LGTM - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/17568#pullrequestreview-1845265521
RFR: 8324723: GHA: Upgrade some actions to avoid deprecated Node 16
Current GHA runs produce lots of warnings: Node.js 16 actions are deprecated. Please update the following actions to use Node.js 20: actions/cache@v3, actions/download-artifact@v3, actions/upload-artifact@v3. For more information see: https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/. We can upgrade to new actions to get the Node20. Release/migration notes: https://github.com/actions/cache#whats-new https://github.com/actions/upload-artifact/blob/main/docs/MIGRATION.md https://github.com/actions/download-artifact/blob/main/docs/MIGRATION.md There is also msys2/setup-msys2, which was pinned by [JDK-8310259](https://bugs.openjdk.org/browse/JDK-8310259). We need at least 2.21.0 to get Node 20: https://github.com/msys2/setup-msys2/blob/main/CHANGELOG.md. I think we can unpin msys2 at this point. I don't think any of the migration problems outlined in those notes apply to our workflows. - Commit messages: - Correct msys2 version - Also msys2/setup-msys2 - Fix Changes: https://git.openjdk.org/jdk/pull/17572/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17572&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324723 Stats: 10 lines in 8 files changed: 0 ins; 1 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/17572.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17572/head:pull/17572 PR: https://git.openjdk.org/jdk/pull/17572
