Integrated: 8326371: [BACKOUT] Clean up NativeCompilation.gmk and its newly created parts
On Wed, 21 Feb 2024 00:59:18 GMT, David Holmes wrote: > Revert "8325963: Clean up NativeCompilation.gmk and its newly created parts" > > This reverts commit 1bd91cdebee1e9ec78ecf185529923eef40ff89c due to code > signing failures on macOS. > > Thanks This pull request has now been integrated. Changeset: 14f9aba9 Author:David Holmes URL: https://git.openjdk.org/jdk/commit/14f9aba921c811eebc78d871aa24915412a19e14 Stats: 379 lines in 6 files changed: 120 ins; 124 del; 135 mod 8326371: [BACKOUT] Clean up NativeCompilation.gmk and its newly created parts Reviewed-by: mikael - PR: https://git.openjdk.org/jdk/pull/17938
Re: RFR: 8326371: [BACKOUT] Clean up NativeCompilation.gmk and its newly created parts
On Wed, 21 Feb 2024 00:59:18 GMT, David Holmes wrote: > Revert "8325963: Clean up NativeCompilation.gmk and its newly created parts" > > This reverts commit 1bd91cdebee1e9ec78ecf185529923eef40ff89c due to code > signing failures on macOS. > > Thanks Marked as reviewed by mikael (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17938#pullrequestreview-1891863168
Re: RFR: 8326371: [BACKOUT] Clean up NativeCompilation.gmk and its newly created parts
On Wed, 21 Feb 2024 01:12:40 GMT, Mikael Vidstedt wrote: >> Revert "8325963: Clean up NativeCompilation.gmk and its newly created parts" >> >> This reverts commit 1bd91cdebee1e9ec78ecf185529923eef40ff89c due to code >> signing failures on macOS. >> >> Thanks > > Marked as reviewed by mikael (Reviewer). Thanks for the review @vidmik ! - PR Comment: https://git.openjdk.org/jdk/pull/17938#issuecomment-1955638459
RFR: 8326371: [BACKOUT] Clean up NativeCompilation.gmk and its newly created parts
Revert "8325963: Clean up NativeCompilation.gmk and its newly created parts" This reverts commit 1bd91cdebee1e9ec78ecf185529923eef40ff89c due to code signing failures on macOS. Thanks - Commit messages: - Revert "8325963: Clean up NativeCompilation.gmk and its newly created parts" Changes: https://git.openjdk.org/jdk/pull/17938/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17938=00 Issue: https://bugs.openjdk.org/browse/JDK-8326371 Stats: 379 lines in 6 files changed: 120 ins; 124 del; 135 mod Patch: https://git.openjdk.org/jdk/pull/17938.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17938/head:pull/17938 PR: https://git.openjdk.org/jdk/pull/17938
Integrated: 8325963: Clean up NativeCompilation.gmk and its newly created parts
On Thu, 15 Feb 2024 14:13:19 GMT, Magnus Ihse Bursie wrote: > This is a follow-up to > [JDK-8325877](https://bugs.openjdk.org/browse/JDK-8325877). I was careful in > that bug not to change order of any lines, only split up the original file > into parts. > > In this PR, I use the new structure to improve the design. I collect the > functionality into three clearly separate phases, preparation, compilation > and linking. I use consistent naming to reveal whether a function just sets > up variables, or create output. I simplify and split up a few extra hard to > read functions. I move some code around so it is placed more logically. > > It can be hard to convince yourself that the changes are correct if you just > look at the end result. I have tried to make small and clear changes in each > separate commit, and to explain in the commit title what I am doing. I > recommend reviewing this PR [commit by > commit](https://github.com/openjdk/jdk/pull/17873/commits). This pull request has now been integrated. Changeset: 1bd91cde Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/1bd91cdebee1e9ec78ecf185529923eef40ff89c Stats: 389 lines in 6 files changed: 134 ins; 130 del; 125 mod 8325963: Clean up NativeCompilation.gmk and its newly created parts Reviewed-by: jwaters, erikj - PR: https://git.openjdk.org/jdk/pull/17873
Integrated: 8326368: compare.sh -2bins prints ugly errors on Windows
On Tue, 20 Feb 2024 20:01:40 GMT, Magnus Ihse Bursie wrote: > The logic in the compare script assumes that we compare two directories, and > so always have an `$OTHER`. If we are using `-2dirs`, this is not the case, > causing `cygpath` and the following `ls` to fail and print error messages > about missing paths. These are benign but annoying. This pull request has now been integrated. Changeset: 4d50ee63 Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/4d50ee63d6eebe73579f05214e6a0fc1b8ebad99 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod 8326368: compare.sh -2bins prints ugly errors on Windows Reviewed-by: erikj - PR: https://git.openjdk.org/jdk/pull/17936
Re: RFR: 8326368: compare.sh -2bins prints ugly errors on Windows
On Tue, 20 Feb 2024 20:01:40 GMT, Magnus Ihse Bursie wrote: > The logic in the compare script assumes that we compare two directories, and > so always have an `$OTHER`. If we are using `-2dirs`, this is not the case, > causing `cygpath` and the following `ls` to fail and print error messages > about missing paths. These are benign but annoying. Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17936#pullrequestreview-1891578612
Re: RFR: 8325963: Clean up NativeCompilation.gmk and its newly created parts [v3]
On Tue, 20 Feb 2024 19:02:12 GMT, Magnus Ihse Bursie wrote: >> This is a follow-up to >> [JDK-8325877](https://bugs.openjdk.org/browse/JDK-8325877). I was careful in >> that bug not to change order of any lines, only split up the original file >> into parts. >> >> In this PR, I use the new structure to improve the design. I collect the >> functionality into three clearly separate phases, preparation, compilation >> and linking. I use consistent naming to reveal whether a function just sets >> up variables, or create output. I simplify and split up a few extra hard to >> read functions. I move some code around so it is placed more logically. >> >> It can be hard to convince yourself that the changes are correct if you just >> look at the end result. I have tried to make small and clear changes in each >> separate commit, and to explain in the commit title what I am doing. I >> recommend reviewing this PR [commit by >> commit](https://github.com/openjdk/jdk/pull/17873/commits). > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Better fix for "macro" Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17873#pullrequestreview-1891576464
RFR: 8326368: compare.sh -2bins prints ugly errors on Windows
The logic in the compare script assumes that we compare two directories, and so always have an `$OTHER`. If we are using `-2dirs`, this is not the case, causing `cygpath` and the following `ls` to fail and print error messages about missing paths. These are benign but annoying. - Commit messages: - 8326368: compare.sh -2bins prints ugly errors on Windows Changes: https://git.openjdk.org/jdk/pull/17936/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17936=00 Issue: https://bugs.openjdk.org/browse/JDK-8326368 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17936.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17936/head:pull/17936 PR: https://git.openjdk.org/jdk/pull/17936
Re: RFR: JDK-8324774: Add DejaVu web fonts
On Tue, 30 Jan 2024 16:13:42 GMT, Hannes Wallnöfer wrote:
> This change adds the DejaVu web fonts that were previously maintained
> externally to the open repository so they are available both in JDK API
> documentation and any API documentation generated with the `javadoc` tool.
> All files added in this PR are the same as the ones previously maintained
> externally, with the exception of added license and name/version comments in
> `dejavu.css`.
>
> Copying of font files to the generated documentation is done by looking for
> font file names in `dejavu.css`, so font file names can be changed without
> changing the code. However, the font file list is hard-coded in
> `APITest.java`. `CheckLibraryVersions.java` is updated to make sure the name
> and version in the legal file matches the one in the stylesheet. Of course I
> also performed manual tests to make sure the font and legal files are copied
> to the output tree and used correctly in browsers.
>
> Once #17411 is integrated, `dejavu.css` should also be added to the list of
> files checked by the new "pass-through" test.
Some suggestions.
I think there should be an option to opt-out of using these fonts.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDoclet.java
line 470:
> 468: // Extract font file names from CSS file
> 469: URL cssURL =
> HtmlConfiguration.class.getResource(DocPaths.RESOURCES.resolve(cssPath).getPath());
> 470: InputStream in = cssURL.openStream();
Put this in try-with-resources
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDoclet.java
line 476:
> 474: try (BufferedReader reader = new BufferedReader(new
> InputStreamReader(in))) {
> 475: String line;
> 476: while ((line = reader.readLine()) != null) {
Consider using `BufferedReader.lines`
https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/io/BufferedReader.html#lines()
-
PR Review: https://git.openjdk.org/jdk/pull/17633#pullrequestreview-1891318432
PR Review Comment: https://git.openjdk.org/jdk/pull/17633#discussion_r1496355484
PR Review Comment: https://git.openjdk.org/jdk/pull/17633#discussion_r1496357502
Re: RFR: 8325963: Clean up NativeCompilation.gmk and its newly created parts [v3]
> This is a follow-up to > [JDK-8325877](https://bugs.openjdk.org/browse/JDK-8325877). I was careful in > that bug not to change order of any lines, only split up the original file > into parts. > > In this PR, I use the new structure to improve the design. I collect the > functionality into three clearly separate phases, preparation, compilation > and linking. I use consistent naming to reveal whether a function just sets > up variables, or create output. I simplify and split up a few extra hard to > read functions. I move some code around so it is placed more logically. > > It can be hard to convince yourself that the changes are correct if you just > look at the end result. I have tried to make small and clear changes in each > separate commit, and to explain in the commit title what I am doing. I > recommend reviewing this PR [commit by > commit](https://github.com/openjdk/jdk/pull/17873/commits). Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Better fix for "macro" - Changes: - all: https://git.openjdk.org/jdk/pull/17873/files - new: https://git.openjdk.org/jdk/pull/17873/files/449cbae2..17c7690b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17873=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17873=01-02 Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/17873.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17873/head:pull/17873 PR: https://git.openjdk.org/jdk/pull/17873
Re: RFR: 8325963: Clean up NativeCompilation.gmk and its newly created parts [v2]
On Tue, 20 Feb 2024 18:49:22 GMT, Erik Joelsson wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Clarify comment based on review > > make/common/NativeCompilation.gmk line 132: > >> 130: SetupNativeCompilation = $(NamedParamsMacroTemplate) >> 131: define SetupNativeCompilationBody >> 132: # In this functions, macros named Setup are just setting >> variables. > > "functions" is still grammatically wrong. Do you actually mean "this macro", > "these macros" or "this file"? I think there were a couple of other mentions > of "function" further down as well. Better now? - PR Review Comment: https://git.openjdk.org/jdk/pull/17873#discussion_r1496346965
Re: RFR: 8325963: Clean up NativeCompilation.gmk and its newly created parts [v2]
On Tue, 20 Feb 2024 18:34:04 GMT, Magnus Ihse Bursie wrote: >> This is a follow-up to >> [JDK-8325877](https://bugs.openjdk.org/browse/JDK-8325877). I was careful in >> that bug not to change order of any lines, only split up the original file >> into parts. >> >> In this PR, I use the new structure to improve the design. I collect the >> functionality into three clearly separate phases, preparation, compilation >> and linking. I use consistent naming to reveal whether a function just sets >> up variables, or create output. I simplify and split up a few extra hard to >> read functions. I move some code around so it is placed more logically. >> >> It can be hard to convince yourself that the changes are correct if you just >> look at the end result. I have tried to make small and clear changes in each >> separate commit, and to explain in the commit title what I am doing. I >> recommend reviewing this PR [commit by >> commit](https://github.com/openjdk/jdk/pull/17873/commits). > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Clarify comment based on review make/common/NativeCompilation.gmk line 132: > 130: SetupNativeCompilation = $(NamedParamsMacroTemplate) > 131: define SetupNativeCompilationBody > 132: # In this functions, macros named Setup are just setting > variables. "functions" is still grammatically wrong. Do you actually mean "this macro", "these macros" or "this file"? I think there were a couple of other mentions of "function" further down as well. - PR Review Comment: https://git.openjdk.org/jdk/pull/17873#discussion_r1496336036
Re: RFR: 8325963: Clean up NativeCompilation.gmk and its newly created parts [v2]
On Tue, 20 Feb 2024 17:13:12 GMT, Erik Joelsson wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Clarify comment based on review > > make/common/NativeCompilation.gmk line 132: > >> 130: SetupNativeCompilation = $(NamedParamsMacroTemplate) >> 131: define SetupNativeCompilationBody >> 132: # In this functions, helpers named Setup are just setting >> variables. > > Suggestion: > > # In this function, helpers named Setup are just setting variables. > > I think have tried to use the word "macro" rather than "function" in > makefiles as that's what they technically are. I was on the fence for this one. I know they are macros, but we use them like functions. But sure, I'll go with technically correct (after all, it's [the best kind of correct](https://www.youtube.com/watch?v=0ZEuWJ4muYc)). > make/common/native/Paths.gmk line 195: > >> 193: >> 194: >> >> 195: define RemoveSuperfluousOutputFiles > > This macro doesn't fit in either of the two categories mentioned at the top. > Perhaps worth clarifying that it's different here. It performs direct actions > on output files but also doesn't do it through a recipe. I left out the third category of macros that are neither Setup nor Create, since they can do all kind of strange stuff. But now I added a comment about it; hope you feel it's enough. - PR Review Comment: https://git.openjdk.org/jdk/pull/17873#discussion_r1496315088 PR Review Comment: https://git.openjdk.org/jdk/pull/17873#discussion_r1496316773
Re: RFR: 8325963: Clean up NativeCompilation.gmk and its newly created parts [v2]
> This is a follow-up to > [JDK-8325877](https://bugs.openjdk.org/browse/JDK-8325877). I was careful in > that bug not to change order of any lines, only split up the original file > into parts. > > In this PR, I use the new structure to improve the design. I collect the > functionality into three clearly separate phases, preparation, compilation > and linking. I use consistent naming to reveal whether a function just sets > up variables, or create output. I simplify and split up a few extra hard to > read functions. I move some code around so it is placed more logically. > > It can be hard to convince yourself that the changes are correct if you just > look at the end result. I have tried to make small and clear changes in each > separate commit, and to explain in the commit title what I am doing. I > recommend reviewing this PR [commit by > commit](https://github.com/openjdk/jdk/pull/17873/commits). Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Clarify comment based on review - Changes: - all: https://git.openjdk.org/jdk/pull/17873/files - new: https://git.openjdk.org/jdk/pull/17873/files/c77b88c2..449cbae2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17873=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17873=00-01 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17873.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17873/head:pull/17873 PR: https://git.openjdk.org/jdk/pull/17873
Re: RFR: 8325963: Clean up NativeCompilation.gmk and its newly created parts
On Thu, 15 Feb 2024 14:13:19 GMT, Magnus Ihse Bursie wrote: > This is a follow-up to > [JDK-8325877](https://bugs.openjdk.org/browse/JDK-8325877). I was careful in > that bug not to change order of any lines, only split up the original file > into parts. > > In this PR, I use the new structure to improve the design. I collect the > functionality into three clearly separate phases, preparation, compilation > and linking. I use consistent naming to reveal whether a function just sets > up variables, or create output. I simplify and split up a few extra hard to > read functions. I move some code around so it is placed more logically. > > It can be hard to convince yourself that the changes are correct if you just > look at the end result. I have tried to make small and clear changes in each > separate commit, and to explain in the commit title what I am doing. I > recommend reviewing this PR [commit by > commit](https://github.com/openjdk/jdk/pull/17873/commits). Overall this looks good. make/common/NativeCompilation.gmk line 132: > 130: SetupNativeCompilation = $(NamedParamsMacroTemplate) > 131: define SetupNativeCompilationBody > 132: # In this functions, helpers named Setup are just setting > variables. Suggestion: # In this function, helpers named Setup are just setting variables. I think have tried to use the word "macro" rather than "function" in makefiles as that's what they technically are. make/common/native/Paths.gmk line 195: > 193: > 194: > > 195: define RemoveSuperfluousOutputFiles This macro doesn't fit in either of the two categories mentioned at the top. Perhaps worth clarifying that it's different here. It performs direct actions on output files but also doesn't do it through a recipe. - PR Review: https://git.openjdk.org/jdk/pull/17873#pullrequestreview-1891021386 PR Review Comment: https://git.openjdk.org/jdk/pull/17873#discussion_r1496197461 PR Review Comment: https://git.openjdk.org/jdk/pull/17873#discussion_r1496208627
