Re: RFR: 8324243: Fix GCC 14 build [v3]
On Sat, 3 Feb 2024 10:07:26 GMT, Sam James wrote: >> This fixes building with GCC 14: >> * ~Cherry-pick a fix from Harfbuzz upstream~ >> * Apply other `-Wcalloc-transposed-args` fixes to the JDK sources >> >> -Wcalloc-transposed-args errors out with GCC 14 as the OpenJDK build uses >> -Werror. >> >> The calloc prototype is: >> >> void *calloc(size_t nmemb, size_t size); >> >> >> So, just swap the number of members and size arguments to match the >> prototype, as >> we're initialising 1 struct of size `sizeof(struct ...)`. GCC then sees >> we're not >> doing anything wrong. > > Sam James has updated the pull request incrementally with one additional > commit since the last revision: > > whitespace Marked as reviewed by prr (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17506#pullrequestreview-1871602651
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v27]
On Thu, 8 Feb 2024 23:02:18 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 incrementally with one > additional commit since the last revision: > > added test case in TestMarkdown.java for handling of `@deprecated` tag Re: https://github.com/openjdk/jdk/pull/16388#discussion_r1483182361 See https://github.com/openjdk/jdk/pull/17782 - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-1935112305
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v27]
> 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 incrementally with one additional commit since the last revision: added test case in TestMarkdown.java for handling of `@deprecated` tag - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/cb070451..3b1350b2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16388=26 - incr: https://webrevs.openjdk.org/?repo=jdk=16388=25-26 Stats: 97 lines in 1 file changed: 91 ins; 2 del; 4 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: JDK-8298405: Support Markdown in Documentation Comments [v26]
On Thu, 8 Feb 2024 20:58:24 GMT, Jonathan Gibbons wrote:
>> Ping.
>
> I believe this is substantially covered in line 226-227. See the third call
> to `test` in the following group of lines:
>
>
> for (String src : sources) {
> test(src);
> test(src.replaceAll("@Deprecated", "/** @deprecated */"));
> test(src.replaceAll("deprecated", "notDeprecated2") // change
> class name
> .replaceAll("@Deprecated", "/// @deprecated\n"));
> }
>
>
> * The first call, `test(src)`, runs all the test cases, using the
> `@Deprecated` annotation by default. In these test cases, the name of the
> element encodes whether it is expected that the element is deprecated or not.
>
> * The second call, `test(src.replaceAll("deprecated", "notDeprecated2")`,
> runs the test cases again, after changing the annotation to a traditional
> (`/** ...*/`) comment containing the `@deprecated` tag. This is a
> long-standing call, and tests the legacy behavior of `@deprecated` appearing
> in a traditional doc comment. Effectively, it tests whether a `/**
> @deprecated */` comment has equivalent behavior to a `@Deprecated` comment.
>
> * The third call is new to this PR and the functionality to support Markdown
> comments. It makes two changes to the source for the test cases, before
> running them:
>1. as in the previous test, the annotations are replaced by a comment
> containing `@deprecated` -- except that this time, the comment is a new-style
> `/// ... ` comment instead of a traditional `/** ... */` comment, and ...
>2. because we do not expect `/// @deprecated` to indicate deprecation, we
> need to change the expected result for each test case, which we do by
> changing the element name for the test case. The change is the first call to
> replace to avoid false positives after changing the doc comment. The change
> uses a new name, `notDeprecated2`, in which `notDeprecated` encodes the
> expected deprecation status, and the `2` is to differentiate the declarations
> from other declarations in the case case that already use the name
> `notDeprecated`.
While the underlying mechanism in javac for indicating deprecation is the same
for all, I accept this is primarily a test for generated class files. I can add
a javadoc test for basic behavior of `@deprecated` in Markdown comments.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1483628179
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v26]
> 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 incrementally with one additional commit since the last revision: amend comment in test - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/c891fe9a..cb070451 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16388=25 - incr: https://webrevs.openjdk.org/?repo=jdk=16388=24-25 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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: JDK-8298405: Support Markdown in Documentation Comments [v25]
On Thu, 8 Feb 2024 15:37:06 GMT, Pavel Rappo wrote:
>> Ping. I do believe that it's important to have such a test.
>
> Ping.
I believe this is substantially covered in line 226-227. See the third call to
`test` in the following group of lines:
for (String src : sources) {
test(src);
test(src.replaceAll("@Deprecated", "/** @deprecated */"));
test(src.replaceAll("deprecated", "notDeprecated2") // change
class name
.replaceAll("@Deprecated", "/// @deprecated\n"));
}
* The first call, `test(src)`, runs all the test cases, using the `@Deprecated`
annotation by default. In these test cases, the name of the element encodes
whether it is expected that the element is deprecated or not.
* The second call, `test(src.replaceAll("deprecated", "notDeprecated2")`, runs
the test cases again, after changing the annotation to a traditional (`/**
...*/`) comment containing the `@deprecated` tag. This is a long-standing call,
and tests the legacy behavior of `@deprecated` appearing in a traditional doc
comment. Effectively, it tests whether a `/** @deprecated */` comment has
equivalent behavior to a `@Deprecated` comment.
* The third call is new to this PR and the functionality to support Markdown
comments. It makes two changes to the source for the test cases, before
running them:
1. as in the previous test, the annotations are replaced by a comment
containing `@deprecated` -- except that this time, the comment is a new-style
`/// ... ` comment instead of a traditional `/** ... */` comment, and ...
2. because we do not expect `/// @deprecated` to indicate deprecation, we
need to change the expected result for each test case, which we do by changing
the element name for the test case. The change is the first call to replace to
avoid false positives after changing the doc comment. The change uses a new
name, `notDeprecated2`, in which `notDeprecated` encodes the expected
deprecation status, and the `2` is to differentiate the declarations from other
declarations in the case case that already use the name `notDeprecated`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1483582480
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v25]
> 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 incrementally with one additional commit since the last revision: improve comments on negative test case - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/5fc415b6..c891fe9a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16388=24 - incr: https://webrevs.openjdk.org/?repo=jdk=16388=23-24 Stats: 12 lines in 1 file changed: 6 ins; 0 del; 6 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: JDK-8298405: Support Markdown in Documentation Comments [v24]
> 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 incrementally with one additional commit since the last revision: remove commented-out code from DocCommentTester - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/92b5e7a5..5fc415b6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16388=23 - incr: https://webrevs.openjdk.org/?repo=jdk=16388=22-23 Stats: 11 lines in 1 file changed: 0 ins; 11 del; 0 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: JDK-8298405: Support Markdown in Documentation Comments [v23]
> 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 incrementally with one additional commit since the last revision: clean up imports in ModuleGenerator test file - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/63dd8bf4..92b5e7a5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16388=22 - incr: https://webrevs.openjdk.org/?repo=jdk=16388=21-22 Stats: 18 lines in 1 file changed: 7 ins; 9 del; 2 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: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Fri, 26 Jan 2024 18:14:05 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.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/CommentUtils.java
> line 629:
>
>> 627: public DocCommentTree parse(URI uri, String text) {
>> 628: return trees.getDocCommentTree(new SimpleJavaFileObject(
>> 629: uri, JavaFileObject.Kind.HTML) {
>
> Was it a bug before?
I would describe it as having been a "latent bug, waiting to happen".
Previously, all file objects were regarded as containing HTML content, and so
there was no need to use the parameter, although maybe it would have been good
to check it.
Now, file objects can be HTML or Markdown, and so we do need to use the
parameter.
In the case here, the method is used on strings specified in command-line
options, which are specified to be in HTML format. Yes, we might want to
change that, but that would be a different RFE separate from the work in this
PR. In that future evolution, I would suggest adding a `JavaFileObject.Kind`
parameter to parse and/or inferring the kind from the tail of the path in the
`uri` parameter.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1483458107
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Fri, 26 Jan 2024 18:10:18 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.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/RawHtml.java
> line 145:
>
>> 143: }
>> 144:
>> 145: Pattern tag = Pattern.compile("<(?[A-Za-z0-9]+)(\\s|>)");
>
> I'm not sure I grok this pattern; what's up with `\\s`?
The match for a tag is one of
* `<` _tag-name_ `>`
* `<` _tag-name_ _whitespace_
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1483444658
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v10]
On Thu, 8 Feb 2024 07:44:18 GMT, Magnus Ihse Bursie wrote: >> Similar to [JDK-8318696](https://bugs.openjdk.org/browse/JDK-8318696), we >> should use -D_FILE_OFFSET_BITS=64, and not -D_LARGEFILE64_SOURCE in the JDK >> native libraries. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Once more, remove AIX dirent64 et al defines And the smaller file system ID is not a problem, I guess? Do you want me to remove the redefines? - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1934691860
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v22]
On Thu, 11 Jan 2024 15:07:33 GMT, Pavel Rappo wrote: >> test/langtools/tools/javac/classfiles/attributes/deprecated/DeprecatedTest.java >> line 26: >> >>> 24: /* >>> 25: * @test >>> 26: * @bug 8042261 8298405 >> >> This comment is not for this line, but for two compiler tests for >> `@deprecated` javadoc tag. It seemed quite contextual place to put it. >> >> Did I miss it, or you are planning to add a javadoc test that verifies that >> `@deprecated` appearing in a `///` comment has no [special meaning] it has >> in classic `/** */` comments? >> >> [special meaning]: >> https://github.com/openjdk/jdk/blob/128363bf3b57dfa05b3807271b47851733c1afb9/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java#L1639-L1653 > > Ping. I do believe that it's important to have such a test. Ping. >> test/langtools/tools/javac/doctree/DocCommentTester.java line 1012: >> >>> 1010: //} >>> 1011: //return null; >>> 1012: //} >> >> Debugging leftover? > > If you want to leave it for debugging you can make it private and uncomment. Ping. >> test/langtools/tools/javac/doctree/MarkdownTest.java line 555: >> >>> 553: // block tags: empty >>> 554: //] >>> 555: //*/ >> >> Just to clarify: it is supposed to be commented out, right? If uncommented, >> this test fails with a slightly different error. > > Please update the DocComment printout in that commented out test: the actual > content is different. It would be nice if the test were passing at least at > the moment of its initial commit. > > Here's what I see locally: > > > Expect: > DocComment[DOC_COMMENT, pos:0 > firstSentence: 1 > Summary[SUMMARY, pos:4 > summary: 1 > Erroneous[ERRONEOUS, pos:14, prefPos:37 > code: compiler.err.dc.unterminated.inline.tag > body: abc_`|_def}|_rest_`more` > ] > ] > body: empty > block tags: empty > ] > > Found: > DocComment[DOC_COMMENT, pos:0 > firstSentence: 1 > Summary[SUMMARY, pos:1 > summary: 1 > Erroneous[ERRONEOUS, pos:11, prefPos:32 > code: compiler.err.dc.unterminated.inline.tag > body: abc_`|def}|rest_`more` > ] > ] > body: empty > block tags: empty > ] Ping. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1483179667 PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1483179302 PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1483178073
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v22]
On Wed, 8 Nov 2023 17:17:46 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 28 commits: >> >> - Merge remote-tracking branch 'upstream/master' into >> 8298405.doclet-markdown-v3 # Please enter a commit message to explain why >> this merge is necessary, # especially if it >>merges an updated upstream into a topic branch. # # Lines starting with >> '#' will be ignored, and an empty message aborts # the commit. >> - add test case contributed by @lahodaj >> - initial fix for source offset problem >> - remove unused imports >> - First pass at remove DocCommentTransformer from the public API. >> >>It is still declared internally, and installed by default, using the >> service-provider mechanism. >>If the standard impl is not available, an identity transformer is used. >> - updates for "first sentence" issues and tests >> - add info about provenance of `jdk.internal.md` module >> - MarkdownTransformer: tweak doc comments >> - MarkdownTransformer: change `Lower.replaceIter` to be `private final` >> - MarkdownTransformer: use suggested text for using streams >> - ... and 18 more: https://git.openjdk.org/jdk/compare/18e24d06...63dd8bf4 > > test/langtools/jdk/javadoc/tool/sampleapi/lib/sampleapi/generator/ModuleGenerator.java > line 49: > >> 47: import sampleapi.util.PoorDocCommentTable; >> 48: >> 49: import static >> com.sun.tools.javac.parser.Tokens.Comment.CommentStyle.JAVADOC; > > To clarify, the change to this file is that you removed two unused imports, > right? Ping. Maybe if it is not essential, we could remove that change, to keep PR more focused. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1483182361
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v22]
On Wed, 7 Feb 2024 18:14:24 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 28 commits: > > - Merge remote-tracking branch 'upstream/master' into > 8298405.doclet-markdown-v3 # Please enter a commit message to explain why > this merge is necessary, # especially if it >merges an updated upstream into a topic branch. # # Lines starting with > '#' will be ignored, and an empty message aborts # the commit. > - add test case contributed by @lahodaj > - initial fix for source offset problem > - remove unused imports > - First pass at remove DocCommentTransformer from the public API. > >It is still declared internally, and installed by default, using the > service-provider mechanism. >If the standard impl is not available, an identity transformer is used. > - updates for "first sentence" issues and tests > - add info about provenance of `jdk.internal.md` module > - MarkdownTransformer: tweak doc comments > - MarkdownTransformer: change `Lower.replaceIter` to be `private final` > - MarkdownTransformer: use suggested text for using streams > - ... and 18 more: https://git.openjdk.org/jdk/compare/18e24d06...63dd8bf4 Jon, here are a few reminders and misc comments. - PR Review: https://git.openjdk.org/jdk/pull/16388#pullrequestreview-1846272367
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.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlConfiguration.java
line 329:
> 327: if (doclint == null) {
> 328: var trees = docEnv.getDocTrees();
> 329: if (trees.getDocCommentTreeTransformer()== null) {
Suggestion:
if (trees.getDocCommentTreeTransformer() == null) {
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/RawHtml.java
line 145:
> 143: }
> 144:
> 145: Pattern tag = Pattern.compile("<(?[A-Za-z0-9]+)(\\s|>)");
I'm not sure I grok this pattern; what's up with `\\s`?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/CommentUtils.java
line 629:
> 627: public DocCommentTree parse(URI uri, String text) {
> 628: return trees.getDocCommentTree(new SimpleJavaFileObject(
> 629: uri, JavaFileObject.Kind.HTML) {
Was it a bug before?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Env.java line 139:
> 137: this.types = types;
> 138:
> 139: if (this.trees.getDocCommentTreeTransformer()== null) {
Suggestion:
if (this.trees.getDocCommentTreeTransformer() == null) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467988621
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467977203
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467980609
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467982493
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v10]
On Thu, 8 Feb 2024 09:03:10 GMT, Joachim Kern wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Once more, remove AIX dirent64 et al defines > > And also `#define statvfs statvfs64` is not necessary with the same > explanation as for the `opendir` defines above -- sorry again. > The very only difference between statvfs and statvfs64 is that the filesystem > ID field in statvfs has a width of 8 Bytes, while in statvfs64 it has a width > of 16 Bytes. > @JoKern65 So what about `#define fstatvfs fstatvfs64`? Is that still needed? > If so, I could not be bothered to make another change. Otherwise, we can get > rid of _all_ AIX 64-bit redefines, and then I'll (probably) do it. Same as `statvfs`. Only the file system ID field is smaller. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1934275624
Integrated: 8325444: GHA: JDK-8325194 causes a regression
On Wed, 7 Feb 2024 19:50:51 GMT, Christoph Langer wrote: > The > [change](https://github.com/openjdk/jdk/commit/d1c82156ba6ede4b798ac15f935289cfcc99d1a0) > for [JDK-8325194](https://bugs.openjdk.org/browse/JDK-8325194) broke > get-jtreg because the way to determine the build jdk was not correct. > > I then tried to use the bootstrap JDK with the correct wiring but it turned > out that the build of JTReg fails with the current JDK, see > [here](https://github.com/RealCLanger/jdk/actions/runs/7820130826/job/21334120478). > > So I propose to fix it by going to the originally proposed solution using > `$JAVA_HOME_17_`. This pull request has now been integrated. Changeset: 3c91b59e Author:Christoph Langer URL: https://git.openjdk.org/jdk/commit/3c91b59ef9c992718d73f2fc9fa50ad2ead78208 Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod 8325444: GHA: JDK-8325194 causes a regression Reviewed-by: gdams, shade, ihse - PR: https://git.openjdk.org/jdk/pull/17756
Re: RFR: 8325444: GHA: JDK-8325194 causes a regression
On Wed, 7 Feb 2024 19:50:51 GMT, Christoph Langer wrote: > The > [change](https://github.com/openjdk/jdk/commit/d1c82156ba6ede4b798ac15f935289cfcc99d1a0) > for [JDK-8325194](https://bugs.openjdk.org/browse/JDK-8325194) broke > get-jtreg because the way to determine the build jdk was not correct. > > I then tried to use the bootstrap JDK with the correct wiring but it turned > out that the build of JTReg fails with the current JDK, see > [here](https://github.com/RealCLanger/jdk/actions/runs/7820130826/job/21334120478). > > So I propose to fix it by going to the originally proposed solution using > `$JAVA_HOME_17_`. Okay, whatever. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17756#pullrequestreview-1870266919
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v10]
On Thu, 8 Feb 2024 09:03:10 GMT, Joachim Kern wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Once more, remove AIX dirent64 et al defines > > And also `#define statvfs statvfs64` is not necessary with the same > explanation as for the `opendir` defines above -- sorry again. > The very only difference between statvfs and statvfs64 is that the filesystem > ID field in statvfs has a width of 8 Bytes, while in statvfs64 it has a width > of 16 Bytes. @JoKern65 So what about `#define fstatvfs fstatvfs64`? Is that still needed? If so, I could not be bothered to make another change. Otherwise, we can get rid of *all* AIX 64-bit redefines, and then I'll (probably) do it. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1934144258
Re: RFR: 8325444: GHA: JDK-8325194 causes a regression
On Thu, 8 Feb 2024 07:31:15 GMT, Magnus Ihse Bursie wrote: >> The >> [change](https://github.com/openjdk/jdk/commit/d1c82156ba6ede4b798ac15f935289cfcc99d1a0) >> for [JDK-8325194](https://bugs.openjdk.org/browse/JDK-8325194) broke >> get-jtreg because the way to determine the build jdk was not correct. >> >> I then tried to use the bootstrap JDK with the correct wiring but it turned >> out that the build of JTReg fails with the current JDK, see >> [here](https://github.com/RealCLanger/jdk/actions/runs/7820130826/job/21334120478). >> >> So I propose to fix it by going to the originally proposed solution using >> `$JAVA_HOME_17_`. > > I don't like this approach. There must be better ways to achieve this, like > inputting the correct value as input to the action, or setting it as a global > gh variable. Where does even the `$JAVA_HOME_17_arm64` come from? Is it > provided by GH? I don't recall setting variables with that weird case style > in our GHA files. OK, I'm ready to integrate. Waiting for blessing from @magicus. - PR Comment: https://git.openjdk.org/jdk/pull/17756#issuecomment-1934128826
Re: RFR: 8325444: GHA: JDK-8325194 causes a regression
On Wed, 7 Feb 2024 19:50:51 GMT, Christoph Langer wrote: > The > [change](https://github.com/openjdk/jdk/commit/d1c82156ba6ede4b798ac15f935289cfcc99d1a0) > for [JDK-8325194](https://bugs.openjdk.org/browse/JDK-8325194) broke > get-jtreg because the way to determine the build jdk was not correct. > > I then tried to use the bootstrap JDK with the correct wiring but it turned > out that the build of JTReg fails with the current JDK, see > [here](https://github.com/RealCLanger/jdk/actions/runs/7820130826/job/21334120478). > > So I propose to fix it by going to the originally proposed solution using > `$JAVA_HOME_17_`. Noting the discussion here: https://github.com/actions/runner-images/discussions/9266 GitHub are considering removing the architecture prefix - PR Comment: https://git.openjdk.org/jdk/pull/17756#issuecomment-1933883736
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Tue, 6 Feb 2024 08:05:14 GMT, Matthias Baesken wrote: >>> I hope finally the AIX part of this PR is done. >> >> Thanks for the AIX related effort ; I put it again into our internal >> build/test queue. > >> >> Thanks for the AIX related effort ; I put it again into our internal >> build/test queue. > > With the latest commit the build again fails on AIX with this error > > /jdk/src/java.base/unix/native/libnio/ch/UnixFileDispatcherImpl.c:381:27: > error: incompatible pointer types passing 'struct statvfs64 *' to parameter > of type 'struct statvfs *' [-Werror,-Wincompatible-pointer-types] > result = fstatvfs(fd, _stat); > ^~ > /usr/include/sys/statvfs.h:102:42: note: passing argument to parameter here > extern int fstatvfs(int, struct statvfs *); > Well, well... The code at least looks cleaner without these AIX defines, so I > really hope that this is the end of the AIX saga, at the `n+1`th time. > @MBaesken Can you rerun AIX testing with the latest commit? Latest commit looks still good on AIX. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1933652124
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v10]
On Thu, 8 Feb 2024 07:44:18 GMT, Magnus Ihse Bursie wrote: >> Similar to [JDK-8318696](https://bugs.openjdk.org/browse/JDK-8318696), we >> should use -D_FILE_OFFSET_BITS=64, and not -D_LARGEFILE64_SOURCE in the JDK >> native libraries. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Once more, remove AIX dirent64 et al defines And also `#define statvfs statvfs64` is not necessary with the same explanation as for the `opendir` defines above -- sorry again. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1933630674
Re: RFR: 8325444: GHA: JDK-8325194 causes a regression
On Wed, 7 Feb 2024 19:50:51 GMT, Christoph Langer wrote: > The > [change](https://github.com/openjdk/jdk/commit/d1c82156ba6ede4b798ac15f935289cfcc99d1a0) > for [JDK-8325194](https://bugs.openjdk.org/browse/JDK-8325194) broke > get-jtreg because the way to determine the build jdk was not correct. > > I then tried to use the bootstrap JDK with the correct wiring but it turned > out that the build of JTReg fails with the current JDK, see > [here](https://github.com/RealCLanger/jdk/actions/runs/7820130826/job/21334120478). > > So I propose to fix it by going to the originally proposed solution using > `$JAVA_HOME_17_`. I would prefer us to unbreak GHA for everyone first, and then look for the cleaner solutions to this problem. Current fix, while ugly, is essentially what we did before: using the runner image var. The `JAVA_HOME_17_arm64` is documented here for `macos-14` runner image: https://github.com/actions/runner-images/blob/main/images/macos/macos-14-arm64-Readme.md - PR Comment: https://git.openjdk.org/jdk/pull/17756#issuecomment-1933625985
Re: RFR: 8325444: GHA: JDK-8325194 causes a regression
On Thu, 8 Feb 2024 07:40:50 GMT, Christoph Langer wrote:
> > I don't like this approach. There must be better ways to achieve this, like
> > inputting the correct value as input to the action, or setting it as a
> > global gh variable. Where does even the `$JAVA_HOME_17_arm64` come from? Is
> > it provided by GH? I don't recall setting variables with that weird case
> > style in our GHA files.
>
> I think (as @gdams told me) these are paths to the Java installation in the
> Github Runners which happen to be there.
>
> I agree, to avoid the if statement, we could properly set it as input to
> get-jtreg.
>
> We probably could also use the setup-java action here - however, this will
> then lead to downloading another JDK, not sure if this is a performance
> concern.
@magicus I looked a bit more into the alternatives. So, regarding using the
correct value as input to the action, I actually don't see that it buys us
anything. We'd have switch statements further up when invoking the actions and
we'll likely have even more of them.
And as for setup-java, this also would only set up a `JAVA_HOME_{{
MAJOR_VERSION }}_{{ ARCHITECTURE }}` environment which we'd have to reference
then. So no gain.
Unless you have some great other idea, I don't see how we could resolve this
more elegantly.
-
PR Comment: https://git.openjdk.org/jdk/pull/17756#issuecomment-1933615018
Re: RFR: 8325444: GHA: JDK-8325194 causes a regression
On Wed, 7 Feb 2024 19:50:51 GMT, Christoph Langer wrote: > The > [change](https://github.com/openjdk/jdk/commit/d1c82156ba6ede4b798ac15f935289cfcc99d1a0) > for [JDK-8325194](https://bugs.openjdk.org/browse/JDK-8325194) broke > get-jtreg because the way to determine the build jdk was not correct. > > I then tried to use the bootstrap JDK with the correct wiring but it turned > out that the build of JTReg fails with the current JDK, see > [here](https://github.com/RealCLanger/jdk/actions/runs/7820130826/job/21334120478). > > So I propose to fix it by going to the originally proposed solution using > `$JAVA_HOME_17_`. Looks good. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17756#pullrequestreview-1869529331
