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 Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17568#pullrequestreview-1844334099
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 221:
> 219: }
> 220: String code = t.getContent();
> 221: // handle the (unlikely) case of FFFC characters
> existing in the code
For consistency with the rest of the file:
Suggestion:
// handle the (unlikely) case of U+FFFC characters
existing in the code
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 230:
> 228: start = pos + 1;
> 229: }
> 230: sourceBuilder.append(code.substring(start));
If I understood this correctly, it could be achieved simpler:
Suggestion:
replacements.add(PLACEHOLDER);
start = pos + 1;
}
sourceBuilder.append(code);
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 353:
> 351: return (equal(desc2, tree.description))
> 352: ? tree
> 353: : m.at(tree.pos).newReturnTree(tree.inline,
> desc2).setEndPos(tree.getEndPos());
Don't we need to set end position here only if the tag is in its inline variant?
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 487:
> 485: }
> 486:
> 487: private static final String AUTOREF_PREFIX = "code:";
I wish the prefix were such that it could not be forged: for example,
automatically assigned, unique within a document comment.
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 543:
> 541: @Override
> 542: public LinkReferenceDefinition
> getLinkReferenceDefinition(String label) {
> 543: var l = label.replace("\\[\\]", "[]");
That `String.replace` looks suspicious. FWIW, when I removed that `replace`, no
tests failed.
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 559:
> 557: private boolean isReference(String s) {
> 558: try {
> 559: var ref = refParser.parse(s,
> ReferenceParser.Mode.MEMBER_OPTIONAL);
Suggestion:
refParser.parse(s, ReferenceParser.Mode.MEMBER_OPTIONAL);
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 771:
> 769: copyTo(getStartPos(link));
> 770: // push temporary value for {@code trees} while handling
> the content of the node
> 771: var saveTrees = trees;
"saveTrees": I see what you did there :-)
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 882:
> 880: private int getStartPos(Node node) {
> 881: var spans = node.getSourceSpans();
> 882: var firstSpan = spans.get(0);
Suggestion:
var firstSpan = spans.getFirst();
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 894:
> 892: private int getEndPos(Node node) {
> 893: var spans = node.getSourceSpans();
> 894: var lastSpan = spans.get(spans.size() - 1);
Suggestion:
var lastSpan = spans.getLast();
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465455477
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465591498
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465400705
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465628293
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465625839
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465626080
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1466197262
PR Review Comment:
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Tue, 23 Jan 2024 12:22:19 GMT, Pavel Rappo wrote:
>> 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.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
> line 1388:
>
>> 1386: * @see > href="https://spec.commonmark.org/0.30/#thematic-breaks;>Thematic Break
>> 1387: */
>> 1388: THEMATIC_BREAK(Pattern.compile("((\\+[ \t]*+){3,})|((-[
>> \t]*+){3,})|((_[ \t]*+){3,})")),
>
> Suggestion:
>
> /**
> * Thematic break: a line of * - _ interspersed with optional spaces
> and tabs
> * @see href="https://spec.commonmark.org/0.30/#thematic-breaks;>Thematic Break
> */
> THEMATIC_BREAK(Pattern.compile("((\*[ \t]*+){3,})|((-[
> \t]*+){3,})|((_[ \t]*+){3,})")),
To add to my earlier [comment], DocCommentParser recognises THEMATIC_BREAK
consisting of `-` as SETEXT_UNDERLINE. While it's inaccurate, it doesn't seem
important, as DCP's goal is to recognise and avoid Markdown, not process it.
[comment]: https://github.com/openjdk/jdk/pull/16388/files#r1462148038
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1466315132
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 Ah, I see a GHA failure similar to https://bugs.openjdk.org/browse/JDK-8318536 -- which is a nice example that current PR still works when there are failures :) - PR Comment: https://git.openjdk.org/jdk/pull/17568#issuecomment-1910475177
RFR: 8324659: GHA: Generic jtreg errors are not reported
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 - Commit messages: - Merge branch 'master' into JDK-83246590-gha-jtreg-failures - Report just the failure=true - Also set failure=true on generic error - Fix Changes: https://git.openjdk.org/jdk/pull/17568/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17568=00 Issue: https://bugs.openjdk.org/browse/JDK-8324659 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17568.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17568/head:pull/17568 PR: https://git.openjdk.org/jdk/pull/17568
Re: RFR: 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. Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17525#pullrequestreview-1843808498
Re: RFR: 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. Marked as reviewed by jwaters (Committer). - PR Review: https://git.openjdk.org/jdk/pull/17525#pullrequestreview-1843798687
Re: RFR: 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. Marked as reviewed by azvegint (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17525#pullrequestreview-1843779778
Re: RFR: 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. Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17525#pullrequestreview-1843752053
