Re: RFR: 8330182: Start of release updates for JDK 24 [v9]
On Thu, 30 May 2024 18:39:19 GMT, Chen Liang wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update symbol files for JDK 23 build 25. > > src/jdk.compiler/share/data/symbols/jdk.incubator.foreign-N.sym.txt line 1: > >> 1: # > > Just curious, does CreateSymbols not track module migrations, now that > jdk.incubator.foreign is completely merged into java.base? When writing these symbol files, CreateSymbols does not keep track where a given package was in a given version, and puts packages to files based on the first version where the package was first introduced. Technically, it should not matter, as the assignment of classes/packages to modules is not based on the file from which the description of the class was read. So, so far, it didn't seem necessary to keep track of package movement for writing of these files. But it is feasible to do so, if the need arises. (When writing ct.sym, it keeps track of the exact package module for every version, of course.) - PR Review Comment: https://git.openjdk.org/jdk/pull/18787#discussion_r1621320340
Integrated: 8327476: Upgrade JLine to 3.26.1
On Wed, 6 Mar 2024 21:20:50 GMT, Jan Lahoda wrote: > This is a patch that: > a) upgrades the JLine inside the JDK to 3.25.1 > b) since the new version of JLine has a FFM backend, our custom native > backends are removed, and replaced with the FFM backend > > Some changes had to be made to the original JLine in order to fit into the > JDK. Most of them were already done for the previous version (repackaging, > avoiding non-ASCII characters, commenting out logging, adding ability to > modify to wrap the InputStream used by the terminal), and have only been > transferred to the new one. The main two new changes are: > - fixes to the FFM backend, so that it works on Linux and JDK 22. These have > been proposed to JLine itself: https://github.com/jline/jline3/pull/945 > - disabling the `NativeFileDescriptorCreator`, as I believe we don't need it, > and cannot make it work easily > > There's a full patch between the > `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content > of the corresponding sources of these original JLine sub-projects: > https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader > https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm > https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal > the patch is here: > https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff > > I've also cleaned the patch a little removing most of the changes for the > rename. The result is here: > https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff This pull request has now been integrated. Changeset: aaa90b30 Author:Jan Lahoda URL: https://git.openjdk.org/jdk/commit/aaa90b3005c85852971203ce6feb88e7091e167b Stats: 13181 lines in 144 files changed: 5784 ins; 5349 del; 2048 mod 8327476: Upgrade JLine to 3.26.1 Reviewed-by: ihse, vromero - PR: https://git.openjdk.org/jdk/pull/18142
Re: RFR: 8327476: Upgrade JLine to 3.26.1 [v7]
> This is a patch that: > a) upgrades the JLine inside the JDK to 3.25.1 > b) since the new version of JLine has a FFM backend, our custom native > backends are removed, and replaced with the FFM backend > > Some changes had to be made to the original JLine in order to fit into the > JDK. Most of them were already done for the previous version (repackaging, > avoiding non-ASCII characters, commenting out logging, adding ability to > modify to wrap the InputStream used by the terminal), and have only been > transferred to the new one. The main two new changes are: > - fixes to the FFM backend, so that it works on Linux and JDK 22. These have > been proposed to JLine itself: https://github.com/jline/jline3/pull/945 > - disabling the `NativeFileDescriptorCreator`, as I believe we don't need it, > and cannot make it work easily > > There's a full patch between the > `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content > of the corresponding sources of these original JLine sub-projects: > https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader > https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm > https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal > the patch is here: > https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff > > I've also cleaned the patch a little removing most of the changes for the > rename. The result is here: > https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 32 commits: - Merge branch 'master' into jline-upgrade-3.25.1 - Removing trailing whitespace. - Fixing test. - Cleanup. - Upgrade to 3.26.1 - Merge branch 'master' into jline-upgrade-3.25.1 - Merge branch 'master' into jline-upgrade-3.25.1 - Merge branch 'master' into jline-upgrade-3.25.1 - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 - Merge remote-tracking branch 'origin/native-access-modules1' into native-access-modules1 - ... and 22 more: https://git.openjdk.org/jdk/compare/23a72a1f...5d900a6e - Changes: https://git.openjdk.org/jdk/pull/18142/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18142=06 Stats: 13181 lines in 144 files changed: 5784 ins; 5349 del; 2048 mod Patch: https://git.openjdk.org/jdk/pull/18142.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18142/head:pull/18142 PR: https://git.openjdk.org/jdk/pull/18142
Re: RFR: 8327476: Upgrade JLine to 3.26.1 [v5]
On Fri, 3 May 2024 07:21:27 GMT, Jan Lahoda wrote: >> This is a patch that: >> a) upgrades the JLine inside the JDK to 3.25.1 >> b) since the new version of JLine has a FFM backend, our custom native >> backends are removed, and replaced with the FFM backend >> >> Some changes had to be made to the original JLine in order to fit into the >> JDK. Most of them were already done for the previous version (repackaging, >> avoiding non-ASCII characters, commenting out logging, adding ability to >> modify to wrap the InputStream used by the terminal), and have only been >> transferred to the new one. The main two new changes are: >> - fixes to the FFM backend, so that it works on Linux and JDK 22. These have >> been proposed to JLine itself: https://github.com/jline/jline3/pull/945 >> - disabling the `NativeFileDescriptorCreator`, as I believe we don't need >> it, and cannot make it work easily >> >> There's a full patch between the >> `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content >> of the corresponding sources of these original JLine sub-projects: >> https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader >> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm >> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal >> the patch is here: >> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff >> >> I've also cleaned the patch a little removing most of the changes for the >> rename. The result is here: >> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff > > Jan Lahoda has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 30 commits: > > - Fixing test. > - Cleanup. > - Upgrade to 3.26.1 > - Merge branch 'master' into jline-upgrade-3.25.1 > - Merge branch 'master' into jline-upgrade-3.25.1 > - Merge branch 'master' into jline-upgrade-3.25.1 > - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 > - Merge remote-tracking branch 'origin/native-access-modules1' into > native-access-modules1 > - Apply suggestions from code review > >Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> >Co-authored-by: Maurizio Cimadamore > <54672762+mcimadam...@users.noreply.github.com> > - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 > - ... and 20 more: https://git.openjdk.org/jdk/compare/130f71ca...01f95c42 Update: since JLine 3.26.1 was release, I tried to upgrade to that directly. I made a diff of the relevant files between JLine 3.25.1 and 3.26.1, and applied that diff to our code. Some additional cleanup was done as well, as suggested by Magnus. - PR Comment: https://git.openjdk.org/jdk/pull/18142#issuecomment-2092453108
Re: RFR: 8327476: Upgrade JLine to 3.26.1 [v6]
> This is a patch that: > a) upgrades the JLine inside the JDK to 3.25.1 > b) since the new version of JLine has a FFM backend, our custom native > backends are removed, and replaced with the FFM backend > > Some changes had to be made to the original JLine in order to fit into the > JDK. Most of them were already done for the previous version (repackaging, > avoiding non-ASCII characters, commenting out logging, adding ability to > modify to wrap the InputStream used by the terminal), and have only been > transferred to the new one. The main two new changes are: > - fixes to the FFM backend, so that it works on Linux and JDK 22. These have > been proposed to JLine itself: https://github.com/jline/jline3/pull/945 > - disabling the `NativeFileDescriptorCreator`, as I believe we don't need it, > and cannot make it work easily > > There's a full patch between the > `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content > of the corresponding sources of these original JLine sub-projects: > https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader > https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm > https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal > the patch is here: > https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff > > I've also cleaned the patch a little removing most of the changes for the > rename. The result is here: > https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Removing trailing whitespace. - Changes: - all: https://git.openjdk.org/jdk/pull/18142/files - new: https://git.openjdk.org/jdk/pull/18142/files/01f95c42..9f6509a9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18142=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18142=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18142.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18142/head:pull/18142 PR: https://git.openjdk.org/jdk/pull/18142
Re: RFR: 8327476: Upgrade JLine to 3.25.1 [v5]
> This is a patch that: > a) upgrades the JLine inside the JDK to 3.25.1 > b) since the new version of JLine has a FFM backend, our custom native > backends are removed, and replaced with the FFM backend > > Some changes had to be made to the original JLine in order to fit into the > JDK. Most of them were already done for the previous version (repackaging, > avoiding non-ASCII characters, commenting out logging, adding ability to > modify to wrap the InputStream used by the terminal), and have only been > transferred to the new one. The main two new changes are: > - fixes to the FFM backend, so that it works on Linux and JDK 22. These have > been proposed to JLine itself: https://github.com/jline/jline3/pull/945 > - disabling the `NativeFileDescriptorCreator`, as I believe we don't need it, > and cannot make it work easily > > There's a full patch between the > `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content > of the corresponding sources of these original JLine sub-projects: > https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader > https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm > https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal > the patch is here: > https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff > > I've also cleaned the patch a little removing most of the changes for the > rename. The result is here: > https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 30 commits: - Fixing test. - Cleanup. - Upgrade to 3.26.1 - Merge branch 'master' into jline-upgrade-3.25.1 - Merge branch 'master' into jline-upgrade-3.25.1 - Merge branch 'master' into jline-upgrade-3.25.1 - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 - Merge remote-tracking branch 'origin/native-access-modules1' into native-access-modules1 - Apply suggestions from code review Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> Co-authored-by: Maurizio Cimadamore <54672762+mcimadam...@users.noreply.github.com> - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 - ... and 20 more: https://git.openjdk.org/jdk/compare/130f71ca...01f95c42 - Changes: https://git.openjdk.org/jdk/pull/18142/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18142=04 Stats: 13181 lines in 144 files changed: 5785 ins; 5349 del; 2047 mod Patch: https://git.openjdk.org/jdk/pull/18142.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18142/head:pull/18142 PR: https://git.openjdk.org/jdk/pull/18142
Integrated: 8331027: JDK's ct.sym file packs corrupted module-info classes
On Wed, 1 May 2024 11:18:36 GMT, Jan Lahoda wrote: > When writing the `ModuleMainClass` attribute, `CreateSymbols` uses an UTF8 > entry (incorrect), instead of a Class_info (correct). This patch fixes that. This pull request has now been integrated. Changeset: 8771015d Author: Jan Lahoda URL: https://git.openjdk.org/jdk/commit/8771015d7e3c4349ad58b58150a49217b1ffb902 Stats: 238 lines in 4 files changed: 206 ins; 10 del; 22 mod 8331027: JDK's ct.sym file packs corrupted module-info classes Reviewed-by: asotona - PR: https://git.openjdk.org/jdk/pull/19031
Re: RFR: 8331027: JDK's ct.sym file packs corrupted module-info classes [v2]
On Thu, 2 May 2024 07:03:20 GMT, Adam Sotona wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Removing unnecessary directory cleanup. > > test/langtools/tools/javac/platform/createsymbols/CreateSymbolsTestImpl.java > line 1104: > >> 1102: CreateSymbols.EXTENSION = ".class"; >> 1103: >> 1104: deleteRecursively(ctSym); > > Is double deletion needed for some kind of race condition safety? I don't think this is needed. Thanks for noticing, removed. - PR Review Comment: https://git.openjdk.org/jdk/pull/19031#discussion_r1587724055
Re: RFR: 8331027: JDK's ct.sym file packs corrupted module-info classes [v2]
> When writing the `ModuleMainClass` attribute, `CreateSymbols` uses an UTF8 > entry (incorrect), instead of a Class_info (correct). This patch fixes that. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Removing unnecessary directory cleanup. - Changes: - all: https://git.openjdk.org/jdk/pull/19031/files - new: https://git.openjdk.org/jdk/pull/19031/files/bde7257c..369f0bf0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19031=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19031=00-01 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19031.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19031/head:pull/19031 PR: https://git.openjdk.org/jdk/pull/19031
RFR: 8331027: JDK's ct.sym file packs corrupted module-info classes
When writing the `ModuleMainClass` attribute, `CreateSymbols` uses an UTF8 entry (incorrect), instead of a Class_info (correct). This patch fixes that. - Commit messages: - 8331027: JDK's ct.sym file packs corrupted module-info classes Changes: https://git.openjdk.org/jdk/pull/19031/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19031=00 Issue: https://bugs.openjdk.org/browse/JDK-8331027 Stats: 240 lines in 4 files changed: 208 ins; 10 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/19031.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19031/head:pull/19031 PR: https://git.openjdk.org/jdk/pull/19031
Re: RFR: 8327476: Upgrade JLine to 3.25.1 [v4]
> This is a patch that: > a) upgrades the JLine inside the JDK to 3.25.1 > b) since the new version of JLine has a FFM backend, our custom native > backends are removed, and replaced with the FFM backend > > Some changes had to be made to the original JLine in order to fit into the > JDK. Most of them were already done for the previous version (repackaging, > avoiding non-ASCII characters, commenting out logging, adding ability to > modify to wrap the InputStream used by the terminal), and have only been > transferred to the new one. The main two new changes are: > - fixes to the FFM backend, so that it works on Linux and JDK 22. These have > been proposed to JLine itself: https://github.com/jline/jline3/pull/945 > - disabling the `NativeFileDescriptorCreator`, as I believe we don't need it, > and cannot make it work easily > > There's a full patch between the > `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content > of the corresponding sources of these original JLine sub-projects: > https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader > https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm > https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal > the patch is here: > https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff > > I've also cleaned the patch a little removing most of the changes for the > rename. The result is here: > https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 26 commits: - Merge branch 'master' into jline-upgrade-3.25.1 - Merge branch 'master' into jline-upgrade-3.25.1 - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 - Merge remote-tracking branch 'origin/native-access-modules1' into native-access-modules1 - Apply suggestions from code review Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> Co-authored-by: Maurizio Cimadamore <54672762+mcimadam...@users.noreply.github.com> - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 - Reflecting review feedback. - Updating copyright headers. - Re-enabling the exec provider. - Cleanup. - ... and 16 more: https://git.openjdk.org/jdk/compare/e5e7cd20...c8097592 - Changes: https://git.openjdk.org/jdk/pull/18142/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18142=03 Stats: 12892 lines in 142 files changed: 5623 ins; 5297 del; 1972 mod Patch: https://git.openjdk.org/jdk/pull/18142.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18142/head:pull/18142 PR: https://git.openjdk.org/jdk/pull/18142
Re: RFR: 8327476: Upgrade JLine to 3.25.1 [v3]
> This is a patch that: > a) upgrades the JLine inside the JDK to 3.25.1 > b) since the new version of JLine has a FFM backend, our custom native > backends are removed, and replaced with the FFM backend > > Some changes had to be made to the original JLine in order to fit into the > JDK. Most of them were already done for the previous version (repackaging, > avoiding non-ASCII characters, commenting out logging, adding ability to > modify to wrap the InputStream used by the terminal), and have only been > transferred to the new one. The main two new changes are: > - fixes to the FFM backend, so that it works on Linux and JDK 22. These have > been proposed to JLine itself: https://github.com/jline/jline3/pull/945 > - disabling the `NativeFileDescriptorCreator`, as I believe we don't need it, > and cannot make it work easily > > There's a full patch between the > `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content > of the corresponding sources of these original JLine sub-projects: > https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader > https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm > https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal > the patch is here: > https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff > > I've also cleaned the patch a little removing most of the changes for the > rename. The result is here: > https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 25 commits: - Merge branch 'master' into jline-upgrade-3.25.1 - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 - Merge remote-tracking branch 'origin/native-access-modules1' into native-access-modules1 - Apply suggestions from code review Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> Co-authored-by: Maurizio Cimadamore <54672762+mcimadam...@users.noreply.github.com> - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 - Reflecting review feedback. - Updating copyright headers. - Re-enabling the exec provider. - Cleanup. - Fixing test. - ... and 15 more: https://git.openjdk.org/jdk/compare/27a03e0d...fe8d5a42 - Changes: https://git.openjdk.org/jdk/pull/18142/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18142=02 Stats: 12893 lines in 142 files changed: 5623 ins; 5298 del; 1972 mod Patch: https://git.openjdk.org/jdk/pull/18142.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18142/head:pull/18142 PR: https://git.openjdk.org/jdk/pull/18142
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v4]
On Thu, 7 Mar 2024 21:53:07 GMT, Jan Lahoda wrote: >> Currently, JDK modules load by the bootstrap and platform ClassLoaders are >> automatically granted the native access. I am working on an upgrade of JLine >> inside the `jdk.internal.le` module, and I would like to replace the current >> native bindings with FFM-based bindings (which are now somewhat provided by >> JLine). But, for that, native access is needed for the `jdk.internal.le` >> module. We could possibly move the module to the platform ClassLoader, but >> it seems it might be better to have more control over which modules have the >> native access. >> >> This patch introduces an explicit list of modules that will automatically be >> granted the native access. Note this patch is not yet intended to change the >> end behavior - the list of modules granted native access is supposed to be >> the same as modules in the boot and platform ClassLoaders. > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Adjusting javadoc as suggested. Thanks for all the feedback and comments! I've filled: https://bugs.openjdk.org/browse/JDK-8327686 to cover the cleanup of the modules with native access enabled, and: https://bugs.openjdk.org/browse/JDK-8327688 to allow to configure the modules with native access at link time. I can try to take a look at either of those, if desired. - PR Comment: https://git.openjdk.org/jdk/pull/18106#issuecomment-1985530868
Re: RFR: 8327476: Upgrade JLine to 3.25.1 [v2]
> This is a patch that: > a) upgrades the JLine inside the JDK to 3.25.1 > b) since the new version of JLine has a FFM backend, our custom native > backends are removed, and replaced with the FFM backend > > Some changes had to be made to the original JLine in order to fit into the > JDK. Most of them were already done for the previous version (repackaging, > avoiding non-ASCII characters, commenting out logging, adding ability to > modify to wrap the InputStream used by the terminal), and have only been > transferred to the new one. The main two new changes are: > - fixes to the FFM backend, so that it works on Linux and JDK 22. These have > been proposed to JLine itself: https://github.com/jline/jline3/pull/945 > - disabling the `NativeFileDescriptorCreator`, as I believe we don't need it, > and cannot make it work easily > > There's a full patch between the > `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content > of the corresponding sources of these original JLine sub-projects: > https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader > https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm > https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal > the patch is here: > https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff > > I've also cleaned the patch a little removing most of the changes for the > rename. The result is here: > https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 24 commits: - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 - Merge remote-tracking branch 'origin/native-access-modules1' into native-access-modules1 - Apply suggestions from code review Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> Co-authored-by: Maurizio Cimadamore <54672762+mcimadam...@users.noreply.github.com> - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 - Reflecting review feedback. - Updating copyright headers. - Re-enabling the exec provider. - Cleanup. - Fixing test. - Fixing Crl-C/VINTR handling on Linux. - ... and 14 more: https://git.openjdk.org/jdk/compare/e772e781...d6c2072f - Changes: https://git.openjdk.org/jdk/pull/18142/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18142=01 Stats: 13023 lines in 150 files changed: 5727 ins; 5307 del; 1989 mod Patch: https://git.openjdk.org/jdk/pull/18142.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18142/head:pull/18142 PR: https://git.openjdk.org/jdk/pull/18142
Integrated: 8327218: Add an ability to specify modules which should have native access enabled
On Mon, 4 Mar 2024 13:52:13 GMT, Jan Lahoda wrote: > Currently, JDK modules load by the bootstrap and platform ClassLoaders are > automatically granted the native access. I am working on an upgrade of JLine > inside the `jdk.internal.le` module, and I would like to replace the current > native bindings with FFM-based bindings (which are now somewhat provided by > JLine). But, for that, native access is needed for the `jdk.internal.le` > module. We could possibly move the module to the platform ClassLoader, but it > seems it might be better to have more control over which modules have the > native access. > > This patch introduces an explicit list of modules that will automatically be > granted the native access. Note this patch is not yet intended to change the > end behavior - the list of modules granted native access is supposed to be > the same as modules in the boot and platform ClassLoaders. This pull request has now been integrated. Changeset: 27a03e0d Author:Jan Lahoda URL: https://git.openjdk.org/jdk/commit/27a03e0dc3e08094aebc3524f68617f7e7fb5c5d Stats: 132 lines in 9 files changed: 106 ins; 9 del; 17 mod 8327218: Add an ability to specify modules which should have native access enabled Co-authored-by: Maurizio Cimadamore Reviewed-by: mcimadamore, erikj, alanb, ihse - PR: https://git.openjdk.org/jdk/pull/18106
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v4]
> Currently, JDK modules load by the bootstrap and platform ClassLoaders are > automatically granted the native access. I am working on an upgrade of JLine > inside the `jdk.internal.le` module, and I would like to replace the current > native bindings with FFM-based bindings (which are now somewhat provided by > JLine). But, for that, native access is needed for the `jdk.internal.le` > module. We could possibly move the module to the platform ClassLoader, but it > seems it might be better to have more control over which modules have the > native access. > > This patch introduces an explicit list of modules that will automatically be > granted the native access. Note this patch is not yet intended to change the > end behavior - the list of modules granted native access is supposed to be > the same as modules in the boot and platform ClassLoaders. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Adjusting javadoc as suggested. - Changes: - all: https://git.openjdk.org/jdk/pull/18106/files - new: https://git.openjdk.org/jdk/pull/18106/files/e30e4529..6af399ee Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18106=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18106=02-03 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18106.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18106/head:pull/18106 PR: https://git.openjdk.org/jdk/pull/18106
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v3]
On Thu, 7 Mar 2024 09:30:41 GMT, Alan Bateman wrote:
>> Jan Lahoda has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - Merge remote-tracking branch 'origin/native-access-modules1' into
>> native-access-modules1
>> - Reflecting review feedback.
>> - Updating copyright headers.
>
> src/java.base/share/classes/java/lang/ModuleLayer.java line 891:
>
>> 889: * {@code false} otherwise
>> 890: */
>> 891: boolean addEnableNativeAccess(String name) {
>
> Do you mind changing the method description to "Updates the module with the
> given name in this layer to allow access to restricted methods"? This will be
> keep it more consistent with the exiting methods.
>
> Also "was present" in the return description hints that it may not now be
> present. A module layer is immutable so it can just say that it returns true
> if the module is in this layer.
Adjusted here:
https://github.com/openjdk/jdk/pull/18106/commits/6af399ee4a3e908cb7c6b983b9747310e23a888e
please let me know if further/other changes/adjustment are desirable.
Thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1516878109
Re: RFR: 8327476: Upgrade JLine to 3.25.1
On Thu, 7 Mar 2024 12:11:19 GMT, ExE Boss wrote: >> This is a patch that: >> a) upgrades the JLine inside the JDK to 3.25.1 >> b) since the new version of JLine has a FFM backend, our custom native >> backends are removed, and replaced with the FFM backend >> >> Some changes had to be made to the original JLine in order to fit into the >> JDK. Most of them were already done for the previous version (repackaging, >> avoiding non-ASCII characters, commenting out logging, adding ability to >> modify to wrap the InputStream used by the terminal), and have only been >> transferred to the new one. The main two new changes are: >> - fixes to the FFM backend, so that it works on Linux and JDK 22. These have >> been proposed to JLine itself: https://github.com/jline/jline3/pull/945 >> - disabling the `NativeFileDescriptorCreator`, as I believe we don't need >> it, and cannot make it work easily >> >> There's a full patch between the >> `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content >> of the corresponding sources of these original JLine sub-projects: >> https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader >> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm >> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal >> the patch is here: >> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff >> >> I've also cleaned the patch a little removing most of the changes for the >> rename. The result is here: >> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff > > src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/AbstractPty.java > line 214: > >> 212: return descriptor; >> 213: } >> 214: } > > Since this is a part of the JDK, > `jdk.internal.access.JavaIOFileDescriptorAccess` can be used instead of > reflection. As this originates in the JLine itself, I would rather avoid doing changes that are unnecessary. Because as a consequence, we would need to re-apply the changes for every upgrade. I believe this code is practically not used in our context, so I don't think it really matters to make it nice. - PR Review Comment: https://git.openjdk.org/jdk/pull/18142#discussion_r1516856805
RFR: 8327476: Upgrade JLine to 3.25.1
This is a patch that: a) upgrades the JLine inside the JDK to 3.25.1 b) since the new version of JLine has a FFM backend, our custom native backends are removed, and replaced with the FFM backend Some changes had to be made to the original JLine in order to fit into the JDK. Most of them were already done for the previous version (repackaging, avoiding non-ASCII characters, commenting out logging, adding ability to modify to wrap the InputStream used by the terminal), and have only been transferred to the new one. The main two new changes are: - fixes to the FFM backend, so that it works on Linux and JDK 22. These have been proposed to JLine itself: https://github.com/jline/jline3/pull/945 - disabling the `NativeFileDescriptorCreator`, as I believe we don't need it, and cannot make it work easily There's a full patch between the `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content of the corresponding sources of these original JLine sub-projects: https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal the patch is here: https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff I've also cleaned the patch a little removing most of the changes for the rename. The result is here: https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff - Depends on: https://git.openjdk.org/jdk/pull/18106 Commit messages: - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 - Re-enabling the exec provider. - Cleanup. - Fixing test. - Fixing Crl-C/VINTR handling on Linux. - Cleanup. - Enabling native access for jdk.internal.le. - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 - Ctrl-C should not kill JShell. - ... and 8 more: https://git.openjdk.org/jdk/compare/e30e4529...d6c2072f Changes: https://git.openjdk.org/jdk/pull/18142/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18142=00 Issue: https://bugs.openjdk.org/browse/JDK-8327476 Stats: 12893 lines in 142 files changed: 5623 ins; 5298 del; 1972 mod Patch: https://git.openjdk.org/jdk/pull/18142.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18142/head:pull/18142 PR: https://git.openjdk.org/jdk/pull/18142
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 18:54:55 GMT, Alan Bateman wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Apply suggestions from code review
>>
>> Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>> Co-authored-by: Maurizio Cimadamore
>> <54672762+mcimadam...@users.noreply.github.com>
>
> src/java.base/share/classes/java/lang/ModuleLayer.java line 896:
>
>> 894: return nameToModule.get(name);
>> 895: }
>> 896:
>
> What would you think about replacing this with addEnableNativeAccess(String
> name) so it can be called by JLA. addEnableNativeAccess. The reason is that
> the JLA methods are usually just calls to some non-public method but the
> changes mean mean there is "core" in the JLA method that is not easy to find.
I've tried to that here:
https://github.com/openjdk/jdk/pull/18106/commits/e17cd3722724cbc6aa298f7b789c6574554af6ea
> src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 812:
>
>> 810: }
>> 811:
>> 812: private static void addEnableNativeAccess(ModuleLayer layer,
>> Set moduleNames, boolean shouldWarn) {
>
> The private methods in this class have a short comment to summarise what they
> do.
I've tried to add a comment here:
https://github.com/openjdk/jdk/pull/18106/commits/e17cd3722724cbc6aa298f7b789c6574554af6ea
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1515171359
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1515171099
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v3]
> Currently, JDK modules load by the bootstrap and platform ClassLoaders are > automatically granted the native access. I am working on an upgrade of JLine > inside the `jdk.internal.le` module, and I would like to replace the current > native bindings with FFM-based bindings (which are now somewhat provided by > JLine). But, for that, native access is needed for the `jdk.internal.le` > module. We could possibly move the module to the platform ClassLoader, but it > seems it might be better to have more control over which modules have the > native access. > > This patch introduces an explicit list of modules that will automatically be > granted the native access. Note this patch is not yet intended to change the > end behavior - the list of modules granted native access is supposed to be > the same as modules in the boot and platform ClassLoaders. Jan Lahoda has updated the pull request incrementally with three additional commits since the last revision: - Merge remote-tracking branch 'origin/native-access-modules1' into native-access-modules1 - Reflecting review feedback. - Updating copyright headers. - Changes: - all: https://git.openjdk.org/jdk/pull/18106/files - new: https://git.openjdk.org/jdk/pull/18106/files/6127044d..e30e4529 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18106=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18106=01-02 Stats: 36 lines in 9 files changed: 11 ins; 9 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/18106.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18106/head:pull/18106 PR: https://git.openjdk.org/jdk/pull/18106
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Wed, 6 Mar 2024 09:19:44 GMT, Alan Bateman wrote: >> Many of these modules do not need native access in the current >> implementation. Should this list be trimmed to list the ones that need >> native access in the current implementation? > >> Many of these modules do not need native access in the current >> implementation. > > In addition this will eventually need jlink support. I view the change to > ModuleBootstrap initialiser to use ModuleLoaderMap.nativeAccessModules() as > very temporary. It may include many standard/JDK modules that aren't in the > image. In addition we'll need some way to grant native access at link-time. > The workaround for the latter right now is to configure default options. > Many of these modules do not need native access in the current > implementation. Should this list be trimmed to list the ones that need native > access in the current implementation? Not sure if I know enough to do the pruning, so I was hoping that could be done separately (I'd file a bug as Alan suggests). But I can try to prune the list, if you prefer. - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1514640785
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 13:58:50 GMT, Jaikiran Pai wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Apply suggestions from code review
>>
>> Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>> Co-authored-by: Maurizio Cimadamore
>> <54672762+mcimadam...@users.noreply.github.com>
>
> src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 817:
>
>> 815: JLA.addEnableNativeAccessToAllUnnamed();
>> 816: } else if (!JLA.addEnableNativeAccess(layer, name) &&
>> shouldWarn) {
>> 817: warnUnknownModule(ENABLE_NATIVE_ACCESS, name);
>
> Should we instead warn irrespective of whether or not it's user configured
> native access module name or a module name configured in JDK's build
> configuration? Or are the build misconfiguration in the
> `NATIVE_ACCESS_MODULES` expected to be caught much earlier in some other
> place?
For normal "full" JDK build, all the modules with pre-set native access should
be present, and the warning might make sense there. But, the user may jlink a
smaller version of the platform, or use `--limit-modules`, and that may cause
some of the modules are not present. So I don't think it is realistic to
produce a warning here for the modules with pre-set native access.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1514633918
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 14:14:15 GMT, Jaikiran Pai wrote: >>> to make the intention clear as well as reduce the chances of missing some >>> boot or platform module in this NATIVE_ACCESS_MODULES? >> >> The list to be be granted native access is a subset, it shouldn't be granted >> all modules mapped to the boot or platform class loaders. > > I see. I then misunderstood a part of the PR description. I believe we could technically reuse the list for boot/platform modules. But, the intent here is to provide more control over the modules with native access, not only being able to add to the list, but also remove from the list. So, to me, it seemed better to have an explicit list, from/to which we can remove/add modules easily. - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1513107379
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 12:41:18 GMT, Jan Lahoda wrote: >> Currently, JDK modules load by the bootstrap and platform ClassLoaders are >> automatically granted the native access. I am working on an upgrade of JLine >> inside the `jdk.internal.le` module, and I would like to replace the current >> native bindings with FFM-based bindings (which are now somewhat provided by >> JLine). But, for that, native access is needed for the `jdk.internal.le` >> module. We could possibly move the module to the platform ClassLoader, but >> it seems it might be better to have more control over which modules have the >> native access. >> >> This patch introduces an explicit list of modules that will automatically be >> granted the native access. Note this patch is not yet intended to change the >> end behavior - the list of modules granted native access is supposed to be >> the same as modules in the boot and platform ClassLoaders. > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Apply suggestions from code review > > Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> > Co-authored-by: Maurizio Cimadamore > <54672762+mcimadam...@users.noreply.github.com> src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 806: > 804: /** > 805: * Process the --enable-native-access option to grant access to > restricted methods to selected modules. > 806: * Also add Enable native access from JDK modules. Suggestion: - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1512761636
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
> Currently, JDK modules load by the bootstrap and platform ClassLoaders are > automatically granted the native access. I am working on an upgrade of JLine > inside the `jdk.internal.le` module, and I would like to replace the current > native bindings with FFM-based bindings (which are now somewhat provided by > JLine). But, for that, native access is needed for the `jdk.internal.le` > module. We could possibly move the module to the platform ClassLoader, but it > seems it might be better to have more control over which modules have the > native access. > > This patch introduces an explicit list of modules that will automatically be > granted the native access. Note this patch is not yet intended to change the > end behavior - the list of modules granted native access is supposed to be > the same as modules in the boot and platform ClassLoaders. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Apply suggestions from code review Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> Co-authored-by: Maurizio Cimadamore <54672762+mcimadam...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/18106/files - new: https://git.openjdk.org/jdk/pull/18106/files/924e1aa0..6127044d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18106=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18106=00-01 Stats: 3 lines in 3 files changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18106.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18106/head:pull/18106 PR: https://git.openjdk.org/jdk/pull/18106
RFR: 8327218: Add an ability to specify modules which should have native access enabled
Currently, JDK modules load by the bootstrap and platform ClassLoaders are automatically granted the native access. I am working on an upgrade of JLine inside the `jdk.internal.le` module, and I would like to replace the current native bindings with FFM-based bindings (which are now somewhat provided by JLine). But, for that, native access is needed for the `jdk.internal.le` module. We could possibly move the module to the platform ClassLoader, but it seems it might be better to have more control over which modules have the native access. This patch introduces an explicit list of modules that will automatically be granted the native access. Note this patch is not yet intended to change the end behavior - the list of modules granted native access is supposed to be the same as modules in the boot and platform ClassLoaders. - Commit messages: - Explicitly listing the modules that should get native access. - Native access modules-1 Changes: https://git.openjdk.org/jdk/pull/18106/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18106=00 Issue: https://bugs.openjdk.org/browse/JDK-8327218 Stats: 120 lines in 9 files changed: 103 ins; 9 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/18106.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18106/head:pull/18106 PR: https://git.openjdk.org/jdk/pull/18106
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v18]
On Tue, 6 Feb 2024 01:36:58 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:
>
> 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.
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 1:
> 1: /*
This transformer seems to break positions of the `RawTextTree`.
For javadoc like:
/// Markdown test
///
/// @author testAuthor
without this transformer, taking the start and end positions of the
`RawTextTree` and taking the text between these positions will lead to:
`[Markdown test, testAuthor]`. With this transfomer, it leads to `[Markdown
test, Markdown t]`, which is clearly suspicious.
Testcase:
/*
* Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
/*
* @test
* @bug 999
* @summary XXX
* @run main/othervm --limit-modules jdk.compiler
MarkdownTransformerPositionTest
* @run main MarkdownTransformerPositionTest
*/
import com.sun.source.doctree.DocCommentTree;
import com.sun.source.doctree.RawTextTree;
import com.sun.source.tree.*;
import com.sun.source.util.*;
import java.net.URI;
import java.util.*;
import javax.lang.model.element.Element;
import javax.tools.*;
public class MarkdownTransformerPositionTest {
public static void main(String... args) throws Exception {
String source = """
/// Markdown test
///
/// @author testAuthor
public class Test {
}
""";
JavaCompiler comp = ToolProvider.getSystemJavaCompiler();
JavacTask task = (JavacTask)comp.getTask(null, null, null, null, null,
Arrays.asList(new JavaSource(source)));
CompilationUnitTree cu = task.parse().iterator().next();
task.analyze();
DocTrees trees = DocTrees.instance(task);
List rawSpans = new ArrayList<>();
TreePath clazzTP = new TreePath(new TreePath(cu),
cu.getTypeDecls().get(0));
Element clazz = trees.getElement(clazzTP);
DocCommentTree docComment = trees.getDocCommentTree(clazz);
new DocTreeScanner() {
@Override
public Void visitRawText(RawTextTree node, Void p) {
int start = (int)
trees.getSourcePositions().getStartPosition(cu, docComment, node);
int end = (int) trees.getSourcePositions().getEndPosition(cu,
docComment, node);
rawSpans.add(source.substring(start, end));
return super.visitRawText(node, p);
}
}.scan(docComment, null);
List expectedRawSpans = List.of("Markdown test", "testAuthor");
if (!expectedRawSpans.equals(rawSpans)) {
throw new AssertionError("Incorrect raw text spans, should be: " +
expectedRawSpans + ", but is: " +
rawSpans);
}
System.err.println("Test result: success, boot modules: " +
ModuleLayer.boot().modules());
}
static class JavaSource extends SimpleJavaFileObject {
private final String source;
public JavaSource(String source) {
super(URI.create("myfo:/Test.java"), JavaFileObject.Kind.SOURCE);
Re: RFR: 8323675: Race in jdk.javadoc-gendata [v2]
On Thu, 18 Jan 2024 07:34:31 GMT, Magnus Ihse Bursie wrote:
>> In [JDK-8318913](https://bugs.openjdk.org/browse/JDK-8318913), the
>> symbolgenerator started to look at current sources as well. This means that
>> the gensrc stage needs to be completed before this is run. A dependency was
>> added for jdk.compiler-gendata, but unfortunately the same tool is run also
>> in jdk.javadoc-gendata, where no such safeguard was created.
>>
>> The result is that the build can fail intermittently with:
>>
>> .../module-info.java:77: error: module not found on module source path
>> module java.base {
>> ^
>> error: cannot access module-info
>> cannot resolve modules
>> Exception in thread "main" java.lang.AssertionError
>> at
>> jdk.compiler.interim/com.sun.tools.javac.util.Assert.error(Assert.java:155)
>> at
>> jdk.compiler.interim/com.sun.tools.javac.util.Assert.checkNonNull(Assert.java:62)
>> at
>> jdk.compiler.interim/com.sun.tools.javac.comp.Modules.allModules(Modules.java:1225)
>> at
>> jdk.compiler.interim/com.sun.tools.javac.comp.Modules.getObservableModule(Modules.java:1450)
>> at
>> jdk.compiler.interim/com.sun.tools.javac.model.JavacElements.getModuleElement(JavacElements.java:144)
>> at
>> jdk.compiler.interim/com.sun.tools.javac.model.JavacElements.getModuleElement(JavacElements.java:89)
>> at
>> build.tools.symbolgenerator.JavadocElementList.main(JavadocElementList.java:98)
>> Compiling up to 2 files for BUILD_BREAKITERATOR_BASE
>> Compiling up to 2 files for BUILD_BREAKITERATOR_LD
>> make[3]: *** [.../_element_lists.marker] Error 1
>> Gendata.gmk:74: recipe for target '.../_element_lists.marker' failed
>
> Magnus Ihse Bursie has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Let jdk.javadoc-gendata only depend on GENSRC_TARGETS
With my very limited understanding of the targets, this look good to me. Thanks!
-
Marked as reviewed by jlahoda (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17402#pullrequestreview-1833768517
Re: RFR: 8323675: Race in jdk.javadoc-gendata
On Wed, 17 Jan 2024 19:14:01 GMT, Erik Joelsson wrote: >> Thanks, in that case I think both of these should be depending on >> `$(GENSRC_TARGETS)` instead. > > I take that back, jdk.javadoc-gendata should depend on `$(GENSRC_TARGETS)`, > jdk.compiler-gendata still needs `$(JAVA_TARGETS)` as the recipe for that > target seems to actually use compiled classes. Uh, sorry for not being precise. I was talking of the `JavadocElementList`/`jdk.javadoc-gendata` - that does not use classfiles, only sources. `jdk.compiler-gendata`/`ct.sym` generation surely does use (all) the new JDK classfiles. Sorry for the confusion. - PR Review Comment: https://git.openjdk.org/jdk/pull/17402#discussion_r1456389328
Re: RFR: 8323675: Race in jdk.javadoc-gendata
On Tue, 16 Jan 2024 18:53:23 GMT, Erik Joelsson wrote: >> I think it might operate on both, but I'm not sure. But my thinking was that >> JAVA_TARGETS depended on GENSRC_TARGETS, which in turn would make sure that >> all source code was properly generated. >> >> Am I missing something? > > Unnecessarily depending on too much hurts build performance. I would like > @lahodaj to help us understand what the tool actually needs. This tool only uses sources, it will not use the compiled classfiles. So, running it at a point where all sources are generated should be sufficient. Most important are generated module-infos, but other sources may in some cases be significant as well. - PR Review Comment: https://git.openjdk.org/jdk/pull/17402#discussion_r1456176633
Integrated: 8321224: ct.sym for JDK 22 contains references to internal modules
On Sun, 3 Dec 2023 21:31:42 GMT, Jan Lahoda wrote: > As part of: > https://github.com/openjdk/jdk/pull/16505 > > there are new symbol files for JDK 22, and @jddarcy noted the content looks > weird. > > I was investigating, and found a few problems, some introduced by > https://github.com/openjdk/jdk/commit/fc314740e947b2338ab9e4d4fce0c4f52de56c4b, > some pre-existing. > > Note that > https://github.com/openjdk/jdk/commit/fc314740e947b2338ab9e4d4fce0c4f52de56c4b > changed the way we generate `ct.sym` - it now contains `.sig` files also for > the current JDK, not only for the past versions. Before this patch, `ct.sym` > only contained a list of permitted modules for the current JDK, and the > classfiles themselves were read from `lib/modules`. > > The problems (and their solution) I've attempted to tackle here: > - since > https://github.com/openjdk/jdk/commit/fc314740e947b2338ab9e4d4fce0c4f52de56c4b, > the ct.sym accidentally includes all modules for the current release, > including non-API/undocumented modules. The proposed solution is to pass the > documented modules together with their transitive dependencies as a parameter > when constructing ct.sym, then use them to only generate data for these > modules. > - there are tiny differences between the data that are in `ct.sym` and in > the `.sym.txt` historical files, mostly around annotations. The `.sym.txt` > files contain references to internal annotations (like `@PreviewFeature`), > which are stripped or converted before the `.sig` file is written into > `ct.sym`. When generating historical data for JDK N, we run `CreateSymbols` > on JDK N, reading the JDK N classfiles, and producing `.sym.txt`. This is > done using `--release N`, so that we read the correct span of modules. Sadly, > since now `--release N` will always use the `.sig` files, we are loosing a > little bit of information esp. around the annotations. The proposal here is > to use `--release N` to read the list of modules to cover, and `--source N` > to read the actual real classfiles from `lib/modules`. This should basically > revert the behavior to the previous state. > - this is an existing issue, but one that needs to be fixed. Sealed types > are not written and read properly - writing was not storing them, and reading > permitted subclasses was happening too late, not on the `header` line. Note > that when fixing this, we now must store some of the non-exported elements, > which are reachable through the permitted subclasses, so that casting works > as expected. Also, since the historical record is incorrect here, I re-run > the generator for JDK 17-21 (as sealed c... This pull request has now been integrated. Changeset: 18c79227 Author:Jan Lahoda URL: https://git.openjdk.org/jdk/commit/18c7922781536366be93b2478251e32e261d06bb Stats: 26870 lines in 194 files changed: 6101 ins; 20256 del; 513 mod 8321224: ct.sym for JDK 22 contains references to internal modules Reviewed-by: darcy, vromero, asotona, ihse - PR: https://git.openjdk.org/jdk/pull/16942
Re: RFR: 8321224: ct.sym for JDK 22 contains references to internal modules [v3]
> As part of: > https://github.com/openjdk/jdk/pull/16505 > > there are new symbol files for JDK 22, and @jddarcy noted the content looks > weird. > > I was investigating, and found a few problems, some introduced by > https://github.com/openjdk/jdk/commit/fc314740e947b2338ab9e4d4fce0c4f52de56c4b, > some pre-existing. > > Note that > https://github.com/openjdk/jdk/commit/fc314740e947b2338ab9e4d4fce0c4f52de56c4b > changed the way we generate `ct.sym` - it now contains `.sig` files also for > the current JDK, not only for the past versions. Before this patch, `ct.sym` > only contained a list of permitted modules for the current JDK, and the > classfiles themselves were read from `lib/modules`. > > The problems (and their solution) I've attempted to tackle here: > - since > https://github.com/openjdk/jdk/commit/fc314740e947b2338ab9e4d4fce0c4f52de56c4b, > the ct.sym accidentally includes all modules for the current release, > including non-API/undocumented modules. The proposed solution is to pass the > documented modules together with their transitive dependencies as a parameter > when constructing ct.sym, then use them to only generate data for these > modules. > - there are tiny differences between the data that are in `ct.sym` and in > the `.sym.txt` historical files, mostly around annotations. The `.sym.txt` > files contain references to internal annotations (like `@PreviewFeature`), > which are stripped or converted before the `.sig` file is written into > `ct.sym`. When generating historical data for JDK N, we run `CreateSymbols` > on JDK N, reading the JDK N classfiles, and producing `.sym.txt`. This is > done using `--release N`, so that we read the correct span of modules. Sadly, > since now `--release N` will always use the `.sig` files, we are loosing a > little bit of information esp. around the annotations. The proposal here is > to use `--release N` to read the list of modules to cover, and `--source N` > to read the actual real classfiles from `lib/modules`. This should basically > revert the behavior to the previous state. > - this is an existing issue, but one that needs to be fixed. Sealed types > are not written and read properly - writing was not storing them, and reading > permitted subclasses was happening too late, not on the `header` line. Note > that when fixing this, we now must store some of the non-exported elements, > which are reachable through the permitted subclasses, so that casting works > as expected. Also, since the historical record is incorrect here, I re-run > the generator for JDK 17-21 (as sealed c... Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision: - Removing unneeded exports. - Merge branch 'master' into JDK-8321224 - Reflecting review feedback - keeping CT_TRANSITIVE_MODULES separate from CT_MODULES. - Removing trailing whitespace. - Ensuring qualified packages are recorded properly. - Fixing tests. - Adding a way to dump the API. - Adding missing file. - 8321224: ct.sym for JDK 22 contains references to internal modules - Changes: - all: https://git.openjdk.org/jdk/pull/16942/files - new: https://git.openjdk.org/jdk/pull/16942/files/87927789..ce981117 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16942=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16942=01-02 Stats: 56068 lines in 1520 files changed: 31819 ins; 18328 del; 5921 mod Patch: https://git.openjdk.org/jdk/pull/16942.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16942/head:pull/16942 PR: https://git.openjdk.org/jdk/pull/16942
Re: RFR: 8321224: ct.sym for JDK 22 contains references to internal modules [v2]
On Mon, 4 Dec 2023 11:39:04 GMT, Magnus Ihse Bursie wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Reflecting review feedback - keeping CT_TRANSITIVE_MODULES separate from >> CT_MODULES. > > make/modules/jdk.compiler/Gendata.gmk line 41: > >> 39: CT_MODULES := $(filter-out $(MODULES_FILTER), $(DOCS_MODULES)) >> 40: CT_EXTRA_MODULES := jdk.unsupported >> 41: CT_TRANSITIVE_MODULES := $(call FindTransitiveIndirectDepsForModules, >> $(CT_MODULES)) $(CT_MODULES) > > Now the CT_TRANSITIVE_MODULES will include all CT_MODULES. I am pretty sure > the intention was to keep them separated, for clarity, but to act on both > CT_MODULES and CT_TRANSITIVE_MODULES. See e.g. the foreach on line 43. (This > change will also make that foreach process CT_MODULES twice.) > > Also, just for code readability, can you move the hard-coded list of extra > modules to after the programmatic definitions of CT_MODULES and transitive > modules, perhaps even with an empty line in between? > > I think a better solution is to change your new line to: > > $(ECHO) $(CT_MODULES) $(CT_TRANSITIVE_MODULES) $(CT_EXTRA_MODULES) > >$(SUPPORT_OUTPUTDIR)/symbols/included-modules Thanks for the comment! I tried to act on it here: https://github.com/openjdk/jdk/pull/16942/commits/879277891708533b0b56dbe36b20760c62708bbf - PR Review Comment: https://git.openjdk.org/jdk/pull/16942#discussion_r1414515956
Re: RFR: 8321224: ct.sym for JDK 22 contains references to internal modules [v2]
> As part of: > https://github.com/openjdk/jdk/pull/16505 > > there are new symbol files for JDK 22, and @jddarcy noted the content looks > weird. > > I was investigating, and found a few problems, some introduced by > https://github.com/openjdk/jdk/commit/fc314740e947b2338ab9e4d4fce0c4f52de56c4b, > some pre-existing. > > Note that > https://github.com/openjdk/jdk/commit/fc314740e947b2338ab9e4d4fce0c4f52de56c4b > changed the way we generate `ct.sym` - it now contains `.sig` files also for > the current JDK, not only for the past versions. Before this patch, `ct.sym` > only contained a list of permitted modules for the current JDK, and the > classfiles themselves were read from `lib/modules`. > > The problems (and their solution) I've attempted to tackle here: > - since > https://github.com/openjdk/jdk/commit/fc314740e947b2338ab9e4d4fce0c4f52de56c4b, > the ct.sym accidentally includes all modules for the current release, > including non-API/undocumented modules. The proposed solution is to pass the > documented modules together with their transitive dependencies as a parameter > when constructing ct.sym, then use them to only generate data for these > modules. > - there are tiny differences between the data that are in `ct.sym` and in > the `.sym.txt` historical files, mostly around annotations. The `.sym.txt` > files contain references to internal annotations (like `@PreviewFeature`), > which are stripped or converted before the `.sig` file is written into > `ct.sym`. When generating historical data for JDK N, we run `CreateSymbols` > on JDK N, reading the JDK N classfiles, and producing `.sym.txt`. This is > done using `--release N`, so that we read the correct span of modules. Sadly, > since now `--release N` will always use the `.sig` files, we are loosing a > little bit of information esp. around the annotations. The proposal here is > to use `--release N` to read the list of modules to cover, and `--source N` > to read the actual real classfiles from `lib/modules`. This should basically > revert the behavior to the previous state. > - this is an existing issue, but one that needs to be fixed. Sealed types > are not written and read properly - writing was not storing them, and reading > permitted subclasses was happening too late, not on the `header` line. Note > that when fixing this, we now must store some of the non-exported elements, > which are reachable through the permitted subclasses, so that casting works > as expected. Also, since the historical record is incorrect here, I re-run > the generator for JDK 17-21 (as sealed c... Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Reflecting review feedback - keeping CT_TRANSITIVE_MODULES separate from CT_MODULES. - Changes: - all: https://git.openjdk.org/jdk/pull/16942/files - new: https://git.openjdk.org/jdk/pull/16942/files/c65eb44b..87927789 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16942=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16942=00-01 Stats: 6 lines in 1 file changed: 3 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16942.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16942/head:pull/16942 PR: https://git.openjdk.org/jdk/pull/16942
RFR: 8321224: ct.sym for JDK 22 contains references to internal modules
As part of: https://github.com/openjdk/jdk/pull/16505 there are new symbol files for JDK 22, and @jddarcy noted the content looks weird. I was investigating, and found a few problems, some introduced by https://github.com/openjdk/jdk/commit/fc314740e947b2338ab9e4d4fce0c4f52de56c4b, some pre-existing. Note that https://github.com/openjdk/jdk/commit/fc314740e947b2338ab9e4d4fce0c4f52de56c4b changed the way we generate `ct.sym` - it now contains `.sig` files also for the current JDK, not only for the past versions. Before this patch, `ct.sym` only contained a list of permitted modules for the current JDK, and the classfiles themselves were read from `lib/modules`. The problems (and their solution) I've attempted to tackle here: - since https://github.com/openjdk/jdk/commit/fc314740e947b2338ab9e4d4fce0c4f52de56c4b, the ct.sym accidentally includes all modules for the current release, including non-API/undocumented modules. The proposed solution is to pass the documented modules together with their transitive dependencies as a parameter when constructing ct.sym, then use them to only generate data for these modules. - there are tiny differences between the data that are in `ct.sym` and in the `.sym.txt` historical files, mostly around annotations. The `.sym.txt` files contain references to internal annotations (like `@PreviewFeature`), which are stripped or converted before the `.sig` file is written into `ct.sym`. When generating historical data for JDK N, we run `CreateSymbols` on JDK N, reading the JDK N classfiles, and producing `.sym.txt`. This is done using `--release N`, so that we read the correct span of modules. Sadly, since now `--release N` will always use the `.sig` files, we are loosing a little bit of information esp. around the annotations. The proposal here is to use `--release N` to read the list of modules to cover, and `--source N` to read the actual real classfiles from `lib/modules`. This should basically revert the behavior to the previous state. - this is an existing issue, but one that needs to be fixed. Sealed types are not written and read properly - writing was not storing them, and reading permitted subclasses was happening too late, not on the `header` line. Note that when fixing this, we now must store some of the non-exported elements, which are reachable through the permitted subclasses, so that casting works as expected. Also, since the historical record is incorrect here, I re-run the generator for JDK 17-21 (as sealed classes where finalized in JDK 17), changes are included here, and are the bulk of the changes of this patch. - when trying to find an existing class header when processing a new class header, it might have happened that an older existing version was picked up. This then caused repetition of the header in the new version. Proposed fix is to first look at the baseline existing header when searching for existing headers, and also a bit more careful computation of baseline. As part of testing, I've written a simple program (inside tests), that prints all elements in exported packages using the `PrintingProcessor`, please see `PrintCTSymContent.java`. And printed the content for JDK 17-21 before this patch: https://cr.openjdk.org/~jlahoda/8321224/diff.00/orig/ after this patch: https://cr.openjdk.org/~jlahoda/8321224/diff.00/new/ and made a diff: https://cr.openjdk.org/~jlahoda/8321224/diff.00/diff/ Looking at the diff, it seems that (as expected) it is basically addition of `sealed`&`permits` and addition of package private classes (forced by `permits`). Symbol files generated for JDK 22 are here: https://github.com/lahodaj/jdk/commit/06606d544cb63f007b1d4eb5196f3ca6ffd24bed#diff-0029c8ea954c5b329126d760dc307cd3235abbe4e9f0530dc301141e33746d37 There's one quirk in the current historical record files: as the `jdk/internal/foreign/` package existed in the `jdk.incubator.foreign` module, and was moved to `java.base`, the generator still puts the classes in this package into `jdk.incubator.foreign-.sym.txt`. This is a quirk only in the way the `.sym.txt` files are written, when `ct.sym` is written, the `.sig` files are generated into the correct module. I may try to fix this for the future, but it does not seem to be blocker. (We need to include classes in this package due get the correct permitted subclasses handling.) - Commit messages: - Removing trailing whitespace. - Ensuring qualified packages are recorded properly. - Fixing tests. - Adding a way to dump the API. - Adding missing file. - 8321224: ct.sym for JDK 22 contains references to internal modules Changes: https://git.openjdk.org/jdk/pull/16942/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16942=00 Issue: https://bugs.openjdk.org/browse/JDK-8321224 Stats: 26876 lines in 194 files changed: 6106 ins; 20256 del; 514 mod Patch: https://git.openjdk.org/jdk/pull/16942.diff Fetch: git fetch
Integrated: 8318913: The module-infos for --release data do not contain pre-set versions
On Fri, 27 Oct 2023 11:35:54 GMT, Jan Lahoda wrote:
> Consider a simple module, like:
>
> module test {}
>
>
> And compile it with JDK 22 and JDK 21 using:
> javac --release 21
>
> The results of the compilations will differ: when compiling with JDK 21, the
> mandated java.base dependency will get a version, possibly like
> "21-internal". When compiling with JDK 22, the version of the java.base
> dependency will be empty.
>
> This is a) because `module-info.class`es in `ct.sym` do not have any module
> version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather
> `lib/modules`, which may contain a range of version specifiers.
>
> This patch does two changes:
> a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a
> simple version. For `--release N`, the version is `N`.
> b) tweaks the whole build so that `ct.sym` is used always for `--release`, a
> `lib/modules` is never used. I.e. the appropriate classfiles are copied into
> `ct.sym`. This not only allows for a general approach to module versions, but
> simplifies the `--release` handling in javac, and should enable future
> improvements. This is, however, a relatively big change.
>
> The use of `lib/modules` for `--release ` was made to improve build
> performance, but the build has been updated since this has been introduced,
> so the slowdown caused by rebuilding `ct.sym` should be much lower now.
>
> With these changes, compiling with `--release N` should record the same
> dependency versions in `module-info` on JDK N and JDK N + 1.
This pull request has now been integrated.
Changeset: fc314740
Author:Jan Lahoda
URL:
https://git.openjdk.org/jdk/commit/fc314740e947b2338ab9e4d4fce0c4f52de56c4b
Stats: 730 lines in 14 files changed: 470 ins; 184 del; 76 mod
8318913: The module-infos for --release data do not contain pre-set versions
Co-authored-by: Erik Joelsson
Reviewed-by: vromero, ihse
-
PR: https://git.openjdk.org/jdk/pull/16400
Re: RFR: 8318913: The module-infos for --release data do not contain pre-set versions [v2]
On Fri, 10 Nov 2023 00:42:43 GMT, Magnus Ihse Bursie wrote: > The build changes almost look good. Just remove the extra added lines at the > end. > > I think this point too still remains: > > > 1. TransitiveDependencies seems to be unused now. I assume this is > > intended. But maybe the java file can be removed? Sorry it took so long. I believe these have been reflected in the patch now: - removal of TransitiveDependencies: https://github.com/openjdk/jdk/pull/16400/commits/ac8fb43dfa660bdfdf34cd7f119b0823cf391357 - removal of the extra lines: https://github.com/openjdk/jdk/pull/16400/commits/70ab16d078750c066a07afc88f569e8b45af83e4 - PR Comment: https://git.openjdk.org/jdk/pull/16400#issuecomment-1819628827
Re: RFR: 8318913: The module-infos for --release data do not contain pre-set versions [v4]
> Consider a simple module, like:
>
> module test {}
>
>
> And compile it with JDK 22 and JDK 21 using:
> javac --release 21
>
> The results of the compilations will differ: when compiling with JDK 21, the
> mandated java.base dependency will get a version, possibly like
> "21-internal". When compiling with JDK 22, the version of the java.base
> dependency will be empty.
>
> This is a) because `module-info.class`es in `ct.sym` do not have any module
> version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather
> `lib/modules`, which may contain a range of version specifiers.
>
> This patch does two changes:
> a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a
> simple version. For `--release N`, the version is `N`.
> b) tweaks the whole build so that `ct.sym` is used always for `--release`, a
> `lib/modules` is never used. I.e. the appropriate classfiles are copied into
> `ct.sym`. This not only allows for a general approach to module versions, but
> simplifies the `--release` handling in javac, and should enable future
> improvements. This is, however, a relatively big change.
>
> The use of `lib/modules` for `--release ` was made to improve build
> performance, but the build has been updated since this has been introduced,
> so the slowdown caused by rebuilding `ct.sym` should be much lower now.
>
> With these changes, compiling with `--release N` should record the same
> dependency versions in `module-info` on JDK N and JDK N + 1.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Reflecting review comment - removing unnecessary lines.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/16400/files
- new: https://git.openjdk.org/jdk/pull/16400/files/ac8fb43d..70ab16d0
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=16400=03
- incr: https://webrevs.openjdk.org/?repo=jdk=16400=02-03
Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/16400.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16400/head:pull/16400
PR: https://git.openjdk.org/jdk/pull/16400
Re: RFR: 8318913: The module-infos for --release data do not contain pre-set versions [v3]
> Consider a simple module, like:
>
> module test {}
>
>
> And compile it with JDK 22 and JDK 21 using:
> javac --release 21
>
> The results of the compilations will differ: when compiling with JDK 21, the
> mandated java.base dependency will get a version, possibly like
> "21-internal". When compiling with JDK 22, the version of the java.base
> dependency will be empty.
>
> This is a) because `module-info.class`es in `ct.sym` do not have any module
> version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather
> `lib/modules`, which may contain a range of version specifiers.
>
> This patch does two changes:
> a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a
> simple version. For `--release N`, the version is `N`.
> b) tweaks the whole build so that `ct.sym` is used always for `--release`, a
> `lib/modules` is never used. I.e. the appropriate classfiles are copied into
> `ct.sym`. This not only allows for a general approach to module versions, but
> simplifies the `--release` handling in javac, and should enable future
> improvements. This is, however, a relatively big change.
>
> The use of `lib/modules` for `--release ` was made to improve build
> performance, but the build has been updated since this has been introduced,
> so the slowdown caused by rebuilding `ct.sym` should be much lower now.
>
> With these changes, compiling with `--release N` should record the same
> dependency versions in `module-info` on JDK N and JDK N + 1.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Removing TransitiveDependencies, as suggested.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/16400/files
- new: https://git.openjdk.org/jdk/pull/16400/files/f3be97d6..ac8fb43d
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=16400=02
- incr: https://webrevs.openjdk.org/?repo=jdk=16400=01-02
Stats: 123 lines in 1 file changed: 0 ins; 123 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/16400.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16400/head:pull/16400
PR: https://git.openjdk.org/jdk/pull/16400
Re: RFR: 8318913: The module-infos for --release data do not contain pre-set versions [v2]
> Consider a simple module, like:
>
> module test {}
>
>
> And compile it with JDK 22 and JDK 21 using:
> javac --release 21
>
> The results of the compilations will differ: when compiling with JDK 21, the
> mandated java.base dependency will get a version, possibly like
> "21-internal". When compiling with JDK 22, the version of the java.base
> dependency will be empty.
>
> This is a) because `module-info.class`es in `ct.sym` do not have any module
> version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather
> `lib/modules`, which may contain a range of version specifiers.
>
> This patch does two changes:
> a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a
> simple version. For `--release N`, the version is `N`.
> b) tweaks the whole build so that `ct.sym` is used always for `--release`, a
> `lib/modules` is never used. I.e. the appropriate classfiles are copied into
> `ct.sym`. This not only allows for a general approach to module versions, but
> simplifies the `--release` handling in javac, and should enable future
> improvements. This is, however, a relatively big change.
>
> The use of `lib/modules` for `--release ` was made to improve build
> performance, but the build has been updated since this has been introduced,
> so the slowdown caused by rebuilding `ct.sym` should be much lower now.
>
> With these changes, compiling with `--release N` should record the same
> dependency versions in `module-info` on JDK N and JDK N + 1.
Jan Lahoda has updated the pull request incrementally with four additional
commits since the last revision:
- Fixing tests.
- Merge remote-tracking branch 'erikj/pull/16400' into JDK-8318913
- Build fixes
- Include pre-release version in the module version.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/16400/files
- new: https://git.openjdk.org/jdk/pull/16400/files/522d58d6..f3be97d6
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=16400=01
- incr: https://webrevs.openjdk.org/?repo=jdk=16400=00-01
Stats: 86 lines in 8 files changed: 49 ins; 9 del; 28 mod
Patch: https://git.openjdk.org/jdk/pull/16400.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16400/head:pull/16400
PR: https://git.openjdk.org/jdk/pull/16400
Re: RFR: 8318913: The module-infos for --release data do not contain pre-set versions
On Fri, 27 Oct 2023 11:35:54 GMT, Jan Lahoda wrote:
> Consider a simple module, like:
>
> module test {}
>
>
> And compile it with JDK 22 and JDK 21 using:
> javac --release 21
>
> The results of the compilations will differ: when compiling with JDK 21, the
> mandated java.base dependency will get a version, possibly like
> "21-internal". When compiling with JDK 22, the version of the java.base
> dependency will be empty.
>
> This is a) because `module-info.class`es in `ct.sym` do not have any module
> version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather
> `lib/modules`, which may contain a range of version specifiers.
>
> This patch does two changes:
> a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a
> simple version. For `--release N`, the version is `N`.
> b) tweaks the whole build so that `ct.sym` is used always for `--release`, a
> `lib/modules` is never used. I.e. the appropriate classfiles are copied into
> `ct.sym`. This not only allows for a general approach to module versions, but
> simplifies the `--release` handling in javac, and should enable future
> improvements. This is, however, a relatively big change.
>
> The use of `lib/modules` for `--release ` was made to improve build
> performance, but the build has been updated since this has been introduced,
> so the slowdown caused by rebuilding `ct.sym` should be much lower now.
>
> With these changes, compiling with `--release N` should record the same
> dependency versions in `module-info` on JDK N and JDK N + 1.
Thanks for the comments so far. I have locally added the pre-release text to
the module versions and merged recent Erik's changes. I'll push the updated
version in a few hours.
-
PR Comment: https://git.openjdk.org/jdk/pull/16400#issuecomment-1803606278
Re: RFR: 8318913: The module-infos for --release data do not contain pre-set versions
On Tue, 7 Nov 2023 15:36:46 GMT, Mark Reinhold wrote: > Will this patch preserve the pre-release identifier (e.g., `-ea` or > `-internal`), which was the behavior prior to JDK 22? If not, then it’s an > incompatible change. Hm, no, the current version will always use the specification version. Should be easy to add the pre-release identifier, if desired. Probably the most difficult would be to tweak the tests. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/16400#issuecomment-1799660275
RFR: 8318913: The module-infos for --release data do not contain pre-set versions
Consider a simple module, like:
module test {}
And compile it with JDK 22 and JDK 21 using:
javac --release 21
The results of the compilations will differ: when compiling with JDK 21, the
mandated java.base dependency will get a version, possibly like "21-internal".
When compiling with JDK 22, the version of the java.base dependency will be
empty.
This is a) because `module-info.class`es in `ct.sym` do not have any module
version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather
`lib/modules`, which may contain a range of version specifiers.
This patch does two changes:
a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a simple
version. For `--release N`, the version is `N`.
b) tweaks the whole build so that `ct.sym` is used always for `--release`, a
`lib/modules` is never used. I.e. the appropriate classfiles are copied into
`ct.sym`. This not only allows for a general approach to module versions, but
simplifies the `--release` handling in javac, and should enable future
improvements. This is, however, a relatively big change.
The use of `lib/modules` for `--release ` was made to improve build
performance, but the build has been updated since this has been introduced, so
the slowdown caused by rebuilding `ct.sym` should be much lower now.
With these changes, compiling with `--release N` should record the same
dependency versions in `module-info` on JDK N and JDK N + 1.
-
Commit messages:
- Test cleanup.
- Merge branch 'master' into JDK-8318913
- Fixing tests.
- Retain packages for extra classes.
- Handle permitted subtypes.
- Fix for writing empty parameter names.
- Fixing tests.
- Fixing tests.
- Cleanup
- Fixing dependencies
- ... and 7 more: https://git.openjdk.org/jdk/compare/667cca9d...522d58d6
Changes: https://git.openjdk.org/jdk/pull/16400/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=16400=00
Issue: https://bugs.openjdk.org/browse/JDK-8318913
Stats: 554 lines in 11 files changed: 429 ins; 58 del; 67 mod
Patch: https://git.openjdk.org/jdk/pull/16400.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16400/head:pull/16400
PR: https://git.openjdk.org/jdk/pull/16400
Re: RFR: JDK-8306584: Start of release updates for JDK 22 [v3]
On Tue, 30 May 2023 22:16:08 GMT, Joe Darcy wrote: >> Time to get JDK 22 underway... > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 24 commits: > > - Merge branch 'master' into JDK-8306584 > - Merge branch 'master' into JDK-8306584 > - Merge branch 'master' into JDK-8306584 > - Sync in symbol changes for JDK 21 build 24. > - Merge branch 'master' into JDK-8306584 > - Merge branch 'master' into JDK-8306584 > - Merge branch 'master' into JDK-8306584 > - Minor test fixes. > - Merge branch 'master' into JDK-8306584 > - Update symbol files to JDK 21 b23. > - ... and 14 more: https://git.openjdk.org/jdk/compare/cb40db05...376dbe26 javac and symbol related changes seem reasonable to me. Two minor comments added for consideration. src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassFile.java line 127: > 125: V64(64, 0), // JDK 20 > 126: V65(65, 0), // JDK 21 > 127: V66(66, 0); // JDK 22 A very small nit/suggestion - it should be possible to do: V66(66, 0), //JDK 22 ; (i.e. ending the enum constant with `,`, and putting the semicolon on a new line.) This way adding a new constant would mean just one line addition, no modification. (The same could be done for other enums.) test/langtools/tools/javac/versions/Versions.java line 93: > 91: TWENTY(false,"64.0", "20", Versions::checksrc20), > 92: TWENTY_ONE(false,"65.0", "21", Versions::checksrc21), > 93: TWENTY_TWO(false,"66.0", "22", Versions::checksrc21); Should there be `checksrc22` instead of `checksrc21`? Or is that done later? - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13567#pullrequestreview-1453854784 PR Review Comment: https://git.openjdk.org/jdk/pull/13567#discussion_r1212101629 PR Review Comment: https://git.openjdk.org/jdk/pull/13567#discussion_r1212114188
Integrated: JDK-8306983: Do not invoke external programs when switch terminal to raw mode on selected platforms
On Thu, 27 Apr 2023 11:21:04 GMT, Jan Lahoda wrote: > To support JShell and other usecases, the JDK uses JLine, which provides > line-editing functionality inside a terminal. > > JLine has several ways to work with the terminal, based on various additional > libraries and tools. Most of them are not directly usable inside the JDK, so > currently, on non-Windows platforms, the JLine inside the JDK will use > external programs, like `stty`, to inspect the terminal's properties and to > switch the terminal to raw mode. > > This is slightly problematic, as the external programs might be missing, and > while this is not that big problem for JShell, it might be a problem for > other potential uses of JLine, like using it inside `System.console()`. > > So, the proposal here is to call the corresponding native methods directly, > on selected platforms for now (Linux and Mac OS/X), instead of invoking the > external programs. On Windows, this has always been the case, we are using a > custom implementation of the interface that maps native and Java function for > JNA there, and the proposal is to do the same here. We take the appropriate > mapping interface for JNA, and provide hand-written implementation for it, > using JNI. > > The Windows implementation is mostly unchanged, the changes are mostly > non-Windows only. The overview of the changes is: > - `LastErrorException` is moved from the Windows-specific code to the > platform-neutral code, as it is used by all the native implementations. This > is the only change that directly affects the Windows-specific code > - `unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaNativePty.java` > and > `unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaTerminalProvider.java` > are created based on the corresponding files from JLine. In JLine, these > files directly call platform-specific implementations, but in this patch, the > platform specific code is commented out, and replaced with calls to > `JDKNativePty`, which is a new platform-specific class, that delegates to the > actual platform-specific implementations. Note the `JnaNativePty.java` and > `JnaTerminalProvider.java` already exist in the Windows part, but have > different pieces of code commented out, which makes them substantially > different. > - for Linux and Mac OS/X, there are platform-specific implementations based > on the corresponding code from JLine, with the hand-written JNI-based > implementation of the JNA-based existing interfaces. They also have an > implementation of `JDKNativePty`, which just delegates to the given > platform's `"NativePty"` imple... This pull request has now been integrated. Changeset: a9705196 Author:Jan Lahoda URL: https://git.openjdk.org/jdk/commit/a9705196cea7d6f468b76b1cfff561352ee0b6b2 Stats: 2145 lines in 23 files changed: 2097 ins; 40 del; 8 mod 8306983: Do not invoke external programs when switch terminal to raw mode on selected platforms Co-authored-by: Adam Sotona Reviewed-by: erikj, vromero, bpb - PR: https://git.openjdk.org/jdk/pull/13687
Re: RFR: JDK-8306983: Do not invoke external programs when switch terminal to raw mode on selected platforms [v3]
On Thu, 11 May 2023 15:15:02 GMT, Vicente Romero wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Reflecting review feedback. > > src/jdk.internal.le/linux/classes/jdk/internal/org/jline/terminal/impl/jna/linux/CLibrary.java > line 2: > >> 1: /* >> 2: * Copyright (c) 2002-2020, the original author or authors. > > 2023 only? This is the original header from JLine, which we generally keep. > src/jdk.internal.le/linux/classes/jdk/internal/org/jline/terminal/impl/jna/linux/CLibrary.java > line 16: > >> 14: import java.util.List; >> 15: >> 16: //import com.sun.jna.LastErrorException; > > there is a lot of commented code in this file, not sure if it has a purpose > or just left overs? I've intentionally left the original JLine code commented here and on other places, to see what the code was doing originally, and to possibly aid future merges of new versions of JLine. - PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1191427112 PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1191428160
Re: RFR: JDK-8306983: Do not invoke external programs when switch terminal to raw mode on selected platforms [v3]
> To support JShell and other usecases, the JDK uses JLine, which provides > line-editing functionality inside a terminal. > > JLine has several ways to work with the terminal, based on various additional > libraries and tools. Most of them are not directly usable inside the JDK, so > currently, on non-Windows platforms, the JLine inside the JDK will use > external programs, like `stty`, to inspect the terminal's properties and to > switch the terminal to raw mode. > > This is slightly problematic, as the external programs might be missing, and > while this is not that big problem for JShell, it might be a problem for > other potential uses of JLine, like using it inside `System.console()`. > > So, the proposal here is to call the corresponding native methods directly, > on selected platforms for now (Linux and Mac OS/X), instead of invoking the > external programs. On Windows, this has always been the case, we are using a > custom implementation of the interface that maps native and Java function for > JNA there, and the proposal is to do the same here. We take the appropriate > mapping interface for JNA, and provide hand-written implementation for it, > using JNI. > > The Windows implementation is mostly unchanged, the changes are mostly > non-Windows only. The overview of the changes is: > - `LastErrorException` is moved from the Windows-specific code to the > platform-neutral code, as it is used by all the native implementations. This > is the only change that directly affects the Windows-specific code > - `unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaNativePty.java` > and > `unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaTerminalProvider.java` > are created based on the corresponding files from JLine. In JLine, these > files directly call platform-specific implementations, but in this patch, the > platform specific code is commented out, and replaced with calls to > `JDKNativePty`, which is a new platform-specific class, that delegates to the > actual platform-specific implementations. Note the `JnaNativePty.java` and > `JnaTerminalProvider.java` already exist in the Windows part, but have > different pieces of code commented out, which makes them substantially > different. > - for Linux and Mac OS/X, there are platform-specific implementations based > on the corresponding code from JLine, with the hand-written JNI-based > implementation of the JNA-based existing interfaces. They also have an > implementation of `JDKNativePty`, which just delegates to the given > platform's `"NativePty"` imple... Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Reflecting review feedback. - Changes: - all: https://git.openjdk.org/jdk/pull/13687/files - new: https://git.openjdk.org/jdk/pull/13687/files/4cf8f67e..edeba637 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13687=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13687=01-02 Stats: 63 lines in 2 files changed: 24 ins; 24 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/13687.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13687/head:pull/13687 PR: https://git.openjdk.org/jdk/pull/13687
Re: RFR: JDK-8306983: Do not invoke external programs when switch terminal to raw mode on selected platforms [v2]
On Tue, 9 May 2023 20:49:16 GMT, Brian Burkhalter wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Adjusting build script as suggested on the review.
>
> src/jdk.internal.le/linux/native/lible/CLibrary.cpp line 183:
>
>> 181: JNIEXPORT jint JNICALL
>> Java_jdk_internal_org_jline_terminal_impl_jna_linux_CLibraryImpl_isatty
>> 182: (JNIEnv *, jobject, jint fd) {
>> 183: return isatty(fd);
>
> Do we care if the native `isatty()` returns zero? Or is this dealt with
> somewhere else?
This method is only used to determine with the fd is attached to a terminal
(returns 1) or not (return 0). The reasons why it is not attached to a terminal
are not really important. The value is used here:
https://github.com/lahodaj/jdk/blob/4cf8f67e43f442a5c48cd30349740ac2cb638d6e/src/jdk.internal.le/unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaNativePty.java#L186
> src/jdk.internal.le/macosx/native/lible/CLibrary.cpp line 187:
>
>> 185: JNIEXPORT jint JNICALL
>> Java_jdk_internal_org_jline_terminal_impl_jna_osx_CLibraryImpl_isatty
>> 186: (JNIEnv *, jobject, jint fd) {
>> 187: return isatty(fd);
>
> Do we care if the native `isatty()` returns zero? Or is this dealt with
> somewhere else?
This method is only used to determine with the fd is attached to a terminal
(returns 1) or not (return 0). The reasons why it is not attached to a terminal
are not really important. The value is used here:
https://github.com/lahodaj/jdk/blob/4cf8f67e43f442a5c48cd30349740ac2cb638d6e/src/jdk.internal.le/unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaNativePty.java#L186
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1190739079
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1190741351
Re: RFR: JDK-8306983: Do not invoke external programs when switch terminal to raw mode on selected platforms [v2]
> To support JShell and other usecases, the JDK uses JLine, which provides > line-editing functionality inside a terminal. > > JLine has several ways to work with the terminal, based on various additional > libraries and tools. Most of them are not directly usable inside the JDK, so > currently, on non-Windows platforms, the JLine inside the JDK will use > external programs, like `stty`, to inspect the terminal's properties and to > switch the terminal to raw mode. > > This is slightly problematic, as the external programs might be missing, and > while this is not that big problem for JShell, it might be a problem for > other potential uses of JLine, like using it inside `System.console()`. > > So, the proposal here is to call the corresponding native methods directly, > on selected platforms for now (Linux and Mac OS/X), instead of invoking the > external programs. On Windows, this has always been the case, we are using a > custom implementation of the interface that maps native and Java function for > JNA there, and the proposal is to do the same here. We take the appropriate > mapping interface for JNA, and provide hand-written implementation for it, > using JNI. > > The Windows implementation is mostly unchanged, the changes are mostly > non-Windows only. The overview of the changes is: > - `LastErrorException` is moved from the Windows-specific code to the > platform-neutral code, as it is used by all the native implementations. This > is the only change that directly affects the Windows-specific code > - `unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaNativePty.java` > and > `unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaTerminalProvider.java` > are created based on the corresponding files from JLine. In JLine, these > files directly call platform-specific implementations, but in this patch, the > platform specific code is commented out, and replaced with calls to > `JDKNativePty`, which is a new platform-specific class, that delegates to the > actual platform-specific implementations. Note the `JnaNativePty.java` and > `JnaTerminalProvider.java` already exist in the Windows part, but have > different pieces of code commented out, which makes them substantially > different. > - for Linux and Mac OS/X, there are platform-specific implementations based > on the corresponding code from JLine, with the hand-written JNI-based > implementation of the JNA-based existing interfaces. They also have an > implementation of `JDKNativePty`, which just delegates to the given > platform's `"NativePty"` implementation. > - the `JdkConsoleProviderImpl.java` has been tweaked to not allow the > implementation based on the external programs, and gracefully falling back to > the standard implementation. JShell is kept unchanged. > > The reasons for this organization are: limiting duplication, mostly adhering > to the JDK's platform specific build (which is different from the normal > JLine's platform-neutral build) and limiting the difference between standard > JLine and JLine inside JDK, to help future upgrades. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Adjusting build script as suggested on the review. - Changes: - all: https://git.openjdk.org/jdk/pull/13687/files - new: https://git.openjdk.org/jdk/pull/13687/files/f14e7fb5..4cf8f67e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13687=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13687=00-01 Stats: 3 lines in 1 file changed: 2 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13687.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13687/head:pull/13687 PR: https://git.openjdk.org/jdk/pull/13687
RFR: JDK-8306983: Do not invoke external programs when switch terminal to raw mode on selected platforms
To support JShell and other usecases, the JDK uses JLine, which provides line-editing functionality inside a terminal. JLine has several ways to work with the terminal, based on various additional libraries and tools. Most of them are not directly usable inside the JDK, so currently, on non-Windows platforms, the JLine inside the JDK will use external programs, like `stty`, to inspect the terminal's properties and to switch the terminal to raw mode. This is slightly problematic, as the external programs might be missing, and while this is not that big problem for JShell, it might be a problem for other potential uses of JLine, like using it inside `System.console()`. So, the proposal here is to call the corresponding native methods directly, on selected platforms for now (Linux and Mac OS/X), instead of invoking the external programs. On Windows, this has always been the case, we are using a custom implementation of the interface that maps native and Java function for JNA there, and the proposal is to do the same here. We take the appropriate mapping interface for JNA, and provide hand-written implementation for it, using JNI. The Windows implementation is mostly unchanged, the changes are mostly non-Windows only. The overview of the changes is: - `LastErrorException` is moved from the Windows-specific code to the platform-neutral code, as it is used by all the native implementations. This is the only change that directly affects the Windows-specific code - `unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaNativePty.java` and `unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaTerminalProvider.java` are created based on the corresponding files from JLine. In JLine, these files directly call platform-specific implementations, but in this patch, the platform specific code is commented out, and replaced with calls to `JDKNativePty`, which is a new platform-specific class, that delegates to the actual platform-specific implementations. Note the `JnaNativePty.java` and `JnaTerminalProvider.java` already exist in the Windows part, but have different pieces of code commented out, which makes them substantially different. - for Linux and Mac OS/X, there are platform-specific implementations based on the corresponding code from JLine, with the hand-written JNI-based implementation of the JNA-based existing interfaces. They also have an implementation of `JDKNativePty`, which just delegates to the given platform's `"NativePty"` implementation. - the `JdkConsoleProviderImpl.java` has been tweaked to not allow the implementation based on the external programs, and gracefully falling back to the standard implementation. JShell is kept unchanged. The reasons for this organization are: limiting duplication, mostly adhering to the JDK's platform specific build (which is different from the normal JLine's platform-neutral build) and limiting the difference between standard JLine and JLine inside JDK, to help future upgrades. - Commit messages: - Removing tabs - Adding handling of errors in the jni code. - Permit dumb terminals. - Only create JLine-based Console when the direct terminal access passes. - Attempting to minimize the diff between JDK and vanilla JLine on Mac. - Reorganizing the native classes. - mac impl - Using direct native calls instead of stty on Linux. Changes: https://git.openjdk.org/jdk/pull/13687/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13687=00 Issue: https://bugs.openjdk.org/browse/JDK-8306983 Stats: 2144 lines in 23 files changed: 2096 ins; 40 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/13687.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13687/head:pull/13687 PR: https://git.openjdk.org/jdk/pull/13687
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v2]
On Fri, 28 Apr 2023 13:01:33 GMT, Jan Lahoda wrote: >> I see that logic in JavaCompiler - I wonder if that's just the way it is, or >> if there's a deeper reason as to why the sourcefile is set on the toplevel >> unit *after* parsing (I don't think I can see any, in which case that might >> be changed if that makes the rest of the code simpler). @lahodaj what do you >> think? > > I believe we were discussing this some time ago, and there were some > problems. I don't recall the exact details, but I'll try to look into this > later. I've sketched this: https://github.com/lahodaj/jdk/commit/efe55f7d354ed7bbf91077d058823d682db501b9 I don't have too strong opinion here, there might be more cleanup possible after a change like this, and might be a bit cleaner outside of the parser; but forces the parser to work with files which is somewhat less clean. - PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180673509
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v3]
On Thu, 27 Apr 2023 18:21:56 GMT, Jim Laskey wrote:
>> Add flexible main methods and anonymous main classes to the Java language.
>
> Jim Laskey has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 14 commits:
>
> - Merge branch 'master' into 8306112
> - PreviewFeatures.isEnabled()
> - Clean up isPreview
> - Missing exception
> - Corrections
> - Update VM.java
> - Clean up testing
> - Update TestJavacTaskScanner.java
> - Merge branch 'master' into 8306112
> - Clean up
> - ... and 4 more: https://git.openjdk.org/jdk/compare/96cdf93b...53a5321d
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line
3933:
> 3931: if
> (Feature.ANONYMOUS_MAIN_CLASSES.allowedInSource(source) &&
> 3932: !isDeclaration() &&
> 3933: (token.kind == VOID || token.kind ==
> IDENTIFIER)) {
These checks (`token.kind == VOID || token.kind == IDENTIFIER`) for token kind
will fail to parse fields of primitive types (or methods with primitive types
as a return type)? Either the tests must include the primitive types, or
(maybe) just this test can be avoided completely?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180481474
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v3]
On Thu, 27 Apr 2023 18:21:56 GMT, Jim Laskey wrote:
>> Add flexible main methods and anonymous main classes to the Java language.
>
> Jim Laskey has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 14 commits:
>
> - Merge branch 'master' into 8306112
> - PreviewFeatures.isEnabled()
> - Clean up isPreview
> - Missing exception
> - Corrections
> - Update VM.java
> - Clean up testing
> - Update TestJavacTaskScanner.java
> - Merge branch 'master' into 8306112
> - Clean up
> - ... and 4 more: https://git.openjdk.org/jdk/compare/96cdf93b...53a5321d
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 64:
> 62:
> 63: for (Method method : refc.getDeclaredMethods()) {
> 64: int argc = method.getParameterCount();
Nit: unused variable?
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 147:
> 145: }
> 146:
> 147: return mainClass.getMethod("main", String[].class);
Nit: could return `mainMethod` here, correct?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Enter.java line 453:
> 451: }
> 452: if (!SourceVersion.isIdentifier(simplename) ||
> SourceVersion.isKeyword(simplename)) {
> 453: log.error(null, Errors.BadFileName(simplename));
Suggestion:
log.error(tree.pos(), Errors.BadFileName(simplename));
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Enter.java line 462:
> 460: for (JCTree def : tree.defs) {
> 461: if (def.hasTag(Tag.PACKAGEDEF)) {
> 462: log.error(null,
> Errors.AnonymousMainClassShouldNotHavePackageDeclaration);
Suggestion:
log.error(def.pos(),
Errors.AnonymousMainClassShouldNotHavePackageDeclaration);
src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/Main.java line 438:
> 436: Class appClass = Class.forName(mainClassName, true, cl);
> 437: Method main = MainMethodFinder.findMainMethod(appClass);
> 438: if (!PreviewFeatures.isEnabled() && (!isStatic(main) ||
> !isPublic(main))) {
It seems one can run a main method without parameters without
`--enable-preview` using this codepath. That is presumably not intended.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java line 627:
> 625: }
> 626:
> 627: public boolean isAnonymousMainClass() {
This method seems a bit confusing to me. I believe the fields and methods will
be stripped from `defs` if/when the wrapping class is created, which will mean
this method will start to return `false`, no? It overall does not seem like a
generally useful predicate.
(If I understand this correctly, if we created the wrapping class in parser
neither of these two methods would be needed.)
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java line 640:
> 638: // Find anonymous main class.
> 639: for (JCTree def : defs) {
> 640: if (def.hasTag(CLASSDEF)) {
Nit, in cases like this, we lately tend to write `def instanceof JCClassDecl
cls`, although we understand this way to check the AST is only safe inside
javac.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180360702
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180360546
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180378753
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180378611
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180375691
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180389813
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180392123
Re: RFR: JDK-8306112 Implementation of JEP 445: Flexible Main Methods and Anonymous Main Classes (Preview) [v2]
On Thu, 27 Apr 2023 20:34:44 GMT, Maurizio Cimadamore wrote: >> The source file name is not available until after parsing. > > I see that logic in JavaCompiler - I wonder if that's just the way it is, or > if there's a deeper reason as to why the sourcefile is set on the toplevel > unit *after* parsing (I don't think I can see any, in which case that might > be changed if that makes the rest of the code simpler). @lahodaj what do you > think? I believe we were discussing this some time ago, and there were some problems. I don't recall the exact details, but I'll try to look into this later. - PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1180377062
Integrated: JDK-8304537: Ant-based langtools build fails after JDK-8015831 Add lint check for calling overridable methods from a constructor
On Mon, 20 Mar 2023 14:50:01 GMT, Jan Lahoda wrote: > `this-escape` lint is disabled for jdk.compiler, so it should be disabled for > the ant-based langtools-only build as well. This pull request has now been integrated. Changeset: c4df9b5f Author: Jan Lahoda URL: https://git.openjdk.org/jdk/commit/c4df9b5f176672617f29bd253f01df2ea81dac36 Stats: 14 lines in 6 files changed: 5 ins; 6 del; 3 mod 8304537: Ant-based langtools build fails after JDK-8015831 Add lint check for calling overridable methods from a constructor Reviewed-by: vromero, erikj - PR: https://git.openjdk.org/jdk/pull/13102
Re: RFR: JDK-8304537: Ant-based langtools build fails after JDK-8015831 Add lint check for calling overridable methods from a constructor [v2]
> `this-escape` lint is disabled for jdk.compiler, so it should be disabled for > the ant-based langtools-only build as well. Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8304537 - Suppressing using @SW rather than by command line configuration. - JDK-8304537: Ant-based langtools build fails after JDK-8015831 Add lint check for calling overridable methods from a constructor - Changes: - all: https://git.openjdk.org/jdk/pull/13102/files - new: https://git.openjdk.org/jdk/pull/13102/files/0939eaa7..603a60e5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13102=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13102=00-01 Stats: 382 lines in 73 files changed: 168 ins; 82 del; 132 mod Patch: https://git.openjdk.org/jdk/pull/13102.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13102/head:pull/13102 PR: https://git.openjdk.org/jdk/pull/13102
Re: RFR: JDK-8304537: Ant-based langtools build fails after JDK-8015831 Add lint check for calling overridable methods from a constructor
On Mon, 20 Mar 2023 14:50:01 GMT, Jan Lahoda wrote: > `this-escape` lint is disabled for jdk.compiler, so it should be disabled for > the ant-based langtools-only build as well. Thanks, Archie. So, I've: - added a few more @SuppressWarnings, as needed - removed the `this-escape` disablement from `jdk.compiler`, `jdk.jdeps` and `jdk.jshell` (because all cases should be suppressed using @SuppressWarnings, I hope) - removed the `this-escape` disablement from the langtools build config - PR: https://git.openjdk.org/jdk/pull/13102
RFR: JDK-8304537: Ant-based langtools build fails after JDK-8015831 Add lint check for calling overridable methods from a constructor
`this-escape` lint is disabled for jdk.compiler, so it should be disabled for the ant-based langtools-only build as well. - Commit messages: - JDK-8304537: Ant-based langtools build fails after JDK-8015831 Add lint check for calling overridable methods from a constructor Changes: https://git.openjdk.org/jdk/pull/13102/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13102=00 Issue: https://bugs.openjdk.org/browse/JDK-8304537 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13102.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13102/head:pull/13102 PR: https://git.openjdk.org/jdk/pull/13102
Integrated: 8303355: The Depend plugin does fully recompile when primitive type changes
On Wed, 1 Mar 2023 10:12:41 GMT, Jan Lahoda wrote: > The OpenJDK build is using a Plugin called Depend to avoid building Java code > unnecessarily. It has two parts, one is checking module APIs (and forces > rebuild of dependent modules if a dependency changes), and second takes > modified files in a module, and attempts to detect whether it is enough to > recompile these files, or if the whole module must be rebuilt. > > There's a bug in the second part, it does not track changes in primitive > types, so e.g. a change of a type of a public field form `int` to `long` does > not cause recompile. > > This patch fixes that by visiting the primitive types, and including them in > the hash computed for the given file. > > There's also another problem - if a module is being recompiled from scratch > due to a change in a file hash, the new/updated file hashes are written > immediately. And if the compilation consequently fails, the state is not > compared to the original hash, but to the hash from the broken compilation, > which may lead to missing compilation of some files. > > This patch moves the code to write the new hashes to the end of compilation, > and only do that when there were no compile-time errors during the > compilation. This pull request has now been integrated. Changeset: dbb562d3 Author:Jan Lahoda URL: https://git.openjdk.org/jdk/commit/dbb562d3b128094cb5bca55237e1331e83526adb Stats: 113 lines in 2 files changed: 77 ins; 23 del; 13 mod 8303355: The Depend plugin does fully recompile when primitive type changes Reviewed-by: erikj, vromero - PR: https://git.openjdk.org/jdk/pull/12801
Re: RFR: 8303355: The Depend plugin does fully recompile when primitive type changes [v2]
On Wed, 1 Mar 2023 11:09:17 GMT, Vicente Romero wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Adjusting DependTest, as suggested on the review
>
> make/jdk/src/classes/build/tools/depend/Depend.java line 885:
>
>> 883: @Override
>> 884: public Void visitPrimitiveType(PrimitiveTypeTree node, Void p) {
>> 885: update(node.getPrimitiveTypeKind().name());
>
> shouldn't `DependTest` include a test to stress this new code?
Test updated:
https://github.com/openjdk/jdk/pull/12801/commits/de84439d64f6882d64f657fee0f6c55b2f717a9c
Thanks!
-
PR: https://git.openjdk.org/jdk/pull/12801
Re: RFR: 8303355: The Depend plugin does fully recompile when primitive type changes [v2]
> The OpenJDK build is using a Plugin called Depend to avoid building Java code > unnecessarily. It has two parts, one is checking module APIs (and forces > rebuild of dependent modules if a dependency changes), and second takes > modified files in a module, and attempts to detect whether it is enough to > recompile these files, or if the whole module must be rebuilt. > > There's a bug in the second part, it does not track changes in primitive > types, so e.g. a change of a type of a public field form `int` to `long` does > not cause recompile. > > This patch fixes that by visiting the primitive types, and including them in > the hash computed for the given file. > > There's also another problem - if a module is being recompiled from scratch > due to a change in a file hash, the new/updated file hashes are written > immediately. And if the compilation consequently fails, the state is not > compared to the original hash, but to the hash from the broken compilation, > which may lead to missing compilation of some files. > > This patch moves the code to write the new hashes to the end of compilation, > and only do that when there were no compile-time errors during the > compilation. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Adjusting DependTest, as suggested on the review - Changes: - all: https://git.openjdk.org/jdk/pull/12801/files - new: https://git.openjdk.org/jdk/pull/12801/files/63cd6884..de84439d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12801=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12801=00-01 Stats: 50 lines in 2 files changed: 46 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/12801.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12801/head:pull/12801 PR: https://git.openjdk.org/jdk/pull/12801
RFR: 8303355: The Depend plugin does fully recompile when primitive type changes
The OpenJDK build is using a Plugin called Depend to avoid building Java code unnecessarily. It has two parts, one is checking module APIs (and forces rebuild of dependent modules if a dependency changes), and second takes modified files in a module, and attempts to detect whether it is enough to recompile these files, or if the whole module must be rebuilt. There's a bug in the second part, it does not track changes in primitive types, so e.g. a change of a type of a public field form `int` to `long` does not cause recompile. This patch fixes that by visiting the primitive types, and including them in the hash computed for the given file. There's also another problem - if a module is being recompiled from scratch due to a change in a file hash, the new/updated file hashes are written immediately. And if the compilation consequently fails, the state is not compared to the original hash, but to the hash from the broken compilation, which may lead to missing compilation of some files. This patch moves the code to write the new hashes to the end of compilation, and only do that when there were no compile-time errors during the compilation. - Commit messages: - 8303355: The Depend plugin does fully recompile when primitive type changes Changes: https://git.openjdk.org/jdk/pull/12801/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12801=00 Issue: https://bugs.openjdk.org/browse/JDK-8303355 Stats: 63 lines in 1 file changed: 31 ins; 23 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/12801.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12801/head:pull/12801 PR: https://git.openjdk.org/jdk/pull/12801
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v10]
On Fri, 13 Jan 2023 22:48:59 GMT, Archie L. Cobbs wrote:
>> This PR adds a new lint warning category `this-escape`.
>>
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
>> allow the JDK to continue to compile with `-Xlint:all`.
>>
>> A 'this' escape warning is generated for a constructor `A()` in a class `A`
>> when the compiler detects that the following situation is _in theory
>> possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate
>> source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could
>> get invoked, perhaps indirectly
>>
>> In the above scenario, `B.foo()` would execute before `A()` has returned and
>> before `B()` has performed any initialization. To the extent `B.foo()`
>> accesses any fields of `B` - all of which are still uninitialized - it is
>> likely to function incorrectly.
>>
>> Note, when determining if a 'this' escape is possible, the compiler makes no
>> assumptions about code outside of the current compilation unit. It doesn't
>> look outside of the current source file to see what might actually happen
>> when a method is invoked. It does follow method and constructors within the
>> current compilation unit, and applies a simplified
>> union-of-all-possible-branches data flow analysis to see where 'this' could
>> end up.
>>
>> From my review, virtually all of the warnings generated in the various JDK
>> modules are valid warnings in the sense that a 'this' escape, as defined
>> above, is really and truly possible. However, that doesn't imply that any
>> bugs were found within the JDK - only that the possibility of a certain type
>> of bug exists if certain superclass constructors are used by someone,
>> somewhere, someday.
>>
>> For several "popular" classes, this PR also adds `@implNote`'s to the
>> offending constructors so that subclass implementors are made aware of the
>> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>>
>> More details and a couple of motivating examples are given in an included
>> [doc
>> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
>> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
>> for some background.
>>
>> Ideally, over time the owners of the various modules would review their
>> `@SuppressWarnings("this-escape")` annotations and determine which other
>> constructors also warranted such an `@implNote`.
>>
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch
>> of different JDK modules. My apologies for that. Adding these annotations
>> was determined to be the more conservative approach, as compared to just
>> excepting `this-escape` from various module builds globally.
>>
>> **Patch Navigation Guide**
>>
>> * Non-trivial compiler changes:
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>>
>> * Javadoc additions of `@implNote`:
>>
>> * `src/java.base/share/classes/java/io/PipedReader.java`
>> * `src/java.base/share/classes/java/io/PipedWriter.java`
>> * `src/java.base/share/classes/java/lang/Throwable.java`
>> * `src/java.base/share/classes/java/util/ArrayDeque.java`
>> * `src/java.base/share/classes/java/util/EnumMap.java`
>> * `src/java.base/share/classes/java/util/HashSet.java`
>> * `src/java.base/share/classes/java/util/Hashtable.java`
>> * `src/java.base/share/classes/java/util/LinkedList.java`
>> * `src/java.base/share/classes/java/util/TreeMap.java`
>> * `src/java.base/share/classes/java/util/TreeSet.java`
>>
>> * New unit tests
>> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>>
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Fix bug where field initializer warnings could be incorrectly suppressed.
> - Consolidate all the unit tests that generate warnings into one.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
line 293:
> 291: return !currentClass.sym.isFinal() &&
> 292: !(currentClass.sym.isSealed() &&
>
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v11]
On Mon, 16 Jan 2023 23:22:00 GMT, Archie L. Cobbs wrote:
>> This PR adds a new lint warning category `this-escape`.
>>
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
>> allow the JDK to continue to compile with `-Xlint:all`.
>>
>> A 'this' escape warning is generated for a constructor `A()` in a class `A`
>> when the compiler detects that the following situation is _in theory
>> possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate
>> source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could
>> get invoked, perhaps indirectly
>>
>> In the above scenario, `B.foo()` would execute before `A()` has returned and
>> before `B()` has performed any initialization. To the extent `B.foo()`
>> accesses any fields of `B` - all of which are still uninitialized - it is
>> likely to function incorrectly.
>>
>> Note, when determining if a 'this' escape is possible, the compiler makes no
>> assumptions about code outside of the current compilation unit. It doesn't
>> look outside of the current source file to see what might actually happen
>> when a method is invoked. It does follow method and constructors within the
>> current compilation unit, and applies a simplified
>> union-of-all-possible-branches data flow analysis to see where 'this' could
>> end up.
>>
>> From my review, virtually all of the warnings generated in the various JDK
>> modules are valid warnings in the sense that a 'this' escape, as defined
>> above, is really and truly possible. However, that doesn't imply that any
>> bugs were found within the JDK - only that the possibility of a certain type
>> of bug exists if certain superclass constructors are used by someone,
>> somewhere, someday.
>>
>> For several "popular" classes, this PR also adds `@implNote`'s to the
>> offending constructors so that subclass implementors are made aware of the
>> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>>
>> More details and a couple of motivating examples are given in an included
>> [doc
>> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
>> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
>> for some background.
>>
>> Ideally, over time the owners of the various modules would review their
>> `@SuppressWarnings("this-escape")` annotations and determine which other
>> constructors also warranted such an `@implNote`.
>>
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch
>> of different JDK modules. My apologies for that. Adding these annotations
>> was determined to be the more conservative approach, as compared to just
>> excepting `this-escape` from various module builds globally.
>>
>> **Patch Navigation Guide**
>>
>> * Non-trivial compiler changes:
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>>
>> * Javadoc additions of `@implNote`:
>>
>> * `src/java.base/share/classes/java/io/PipedReader.java`
>> * `src/java.base/share/classes/java/io/PipedWriter.java`
>> * `src/java.base/share/classes/java/lang/Throwable.java`
>> * `src/java.base/share/classes/java/util/ArrayDeque.java`
>> * `src/java.base/share/classes/java/util/EnumMap.java`
>> * `src/java.base/share/classes/java/util/HashSet.java`
>> * `src/java.base/share/classes/java/util/Hashtable.java`
>> * `src/java.base/share/classes/java/util/LinkedList.java`
>> * `src/java.base/share/classes/java/util/TreeMap.java`
>> * `src/java.base/share/classes/java/util/TreeSet.java`
>>
>> * New unit tests
>> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>>
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with three
> additional commits since the last revision:
>
> - Remove unused type variable on method visitScoped().
> - Remove expression type filtering; it doesn't seem to be needed.
> - Clean up unused import.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
line 908:
> 906: public void visitAssert(JCAssert tree) {
> 907: scan(tree.cond);
> 908: refs.discardExprs(depth);
Should also handle
Re: RFR: JDK-8296149: Start of release updates for JDK 21 [v3]
On Sat, 3 Dec 2022 23:10:48 GMT, Joe Darcy wrote: >> Usual start-of-release updates. Symbol updates in initial version reflect >> JDK 20 build 21. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 16 additional commits since > the last revision: > > - Merge branch 'master' into JDK-8296149 > - Merge branch 'master' into JDK-8296149 > - Update symbol information for JDK 20 build 26. > - Merge branch 'master' into JDK-8296149 > - Merge branch 'master' into JDK-8296149 > - Fix failed HotSpot regression tests. > - Merge branch 'master' into JDK-8296149 > - Update symbol information to JDK 20 build 24. > - Merge branch 'master' into JDK-8296149 > - Merge branch 'master' into JDK-8296149 > - ... and 6 more: https://git.openjdk.org/jdk/compare/ea5d4b3a...515002c7 javac changes look fine to me. - Marked as reviewed by jlahoda (Reviewer). PR: https://git.openjdk.org/jdk/pull/10924
Re: RFR: 8294368: Java incremental builds broken on Windows after JDK-8293116
On Tue, 4 Oct 2022 16:19:24 GMT, Jorn Vernee wrote: > This patch fixes incremental builds on Windows. > > There are 2 parts to this: > 1. the build system needs to run the paths in the modified file list through > fixpath. I've added a `convert` mode to `fixpath.sh` for that. There's an > extra target for generating the file with fixed paths. On non-windows > platforms this is just a simple `cp` of the file. > 2. the dependency plugin of `javac` was using string-based path comparison. > But, the paths fed by the build system and the paths used internally by javac > could be in slightly different formats, meaning that files were not detected > properly as changed. I switched to `Path`-based comparison instead and that > fixes the issue. > > Testing: > tested this manually by doing the following: > 1. `make clean` > 2. `make images` > 3. put garbage in one of the files in `java.base` > 4. `make images` (incremental) > 5. verify that the build reported an error > 6. verify the contents of > `\jdk\modules\java.base_the.java.base_batch.modfiles.fixed` > 7. revert the changes of 3, and do the same for another file > 8. `make images` (incremental) > 9. verify that the build reported an error > 10. verify the contents of > `\jdk\modules\java.base_the.java.base_batch.modfiles.fixed` > 11. remove garbage from file modified by 9 again > 12. `make images` (incremental) > 13. verify that build succeeds as in 2 > > I've tested the build on Windows and Linux (WSL) using the above steps. Thanks a lot for fixing this, and sorry for trouble! - Marked as reviewed by jlahoda (Reviewer). PR: https://git.openjdk.org/jdk/pull/10560
Re: RFR: 8293116: Incremental JDK build could be sped up [v6]
> Currently, when doing a small change inside a module that does not affect the > API of the module, the build system will skip rebuild of the dependent > modules. If there's a change that affects the module's API, the dependent > modules are recompiled. So far, this seems to work reasonably. > > But, for large modules, like `java.base`, any change to any of the module's > sources will cause a full rebuild of that module, which may take tens of > seconds. This patch strives to improve that, by: > -parsing only sources that were modified first, > -computing an "API" hash for them, > -if there is not a significant change in the file(s), only recompile the > files, if there are significant changes to the files, recompile the whole > module, as before > > There is no attempt made to determine the minimal set of files that needs to > be reparsed, all module's files are reparsed when the internal API check > fails. This is in line with the existing code to skip the build of dependent > modules. > > Sadly, this is overall a bit more difficult to do, so the implementation uses > some javac internals. The existing build-only `Depend` plugin is enhanced so > that parsing of the initial files is redirected to go through the plugin, and > it decides parses either only the modified files, or all files. This > redirection requires a change to `main/JavaCompiler`, which is not in the > boot JDK, so reflection is currently used to install the redirection. Once > the new classes are in a boot JDK, we could clean up, and avoid the > reflection. > > On my laptop, doing: > > touch src/java.base/share/classes/java/lang/Object.java ; time make > > can take well over 30s without this patch. With this patch, it takes <5s for > me. Changes to e.g. only method bodies should typically not require rebuild > of the whole module. > > The caveats of this patch include: > -it is more involved than the existing plugin > -it only speeds up only incremental Java compilation, so the other tasks the > build system is doing when running `make`, like timestamp checks or starting > the build server, are still happening. This may slow down builds on platforms > with slow I/O > -for modules that are part of the interim toolchain (like jdk.compiler), the > interim build is not using this new approach, and it will likely be slow. We > might try to improve once the required hooks are part of the boot JDK > -as the check for modified "internal" API cannot use attribution (as it runs > too early), it tries to be safe, and will mark some changes as "internal API > changes", even if they in fact don't affect the file's API. This may be > especially true for some changes to imports. > -it is necessary to store an (internal) API hash for every file, in addition > to one for every module which is stored currently Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Adding LOG_LEVEL based debugging to the Depend Plugin. - Merge branch 'master' into JDK-8293116 - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8293116 - Regenerating modfiles as necessary. - Adding comment. - Reflecting review feedback. - Reflecting review feedback. - 8293116: Incremental JDK build could be sped up - Changes: - all: https://git.openjdk.org/jdk/pull/10104/files - new: https://git.openjdk.org/jdk/pull/10104/files/c3455660..661f9072 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10104=05 - incr: https://webrevs.openjdk.org/?repo=jdk=10104=04-05 Stats: 75927 lines in 1333 files changed: 36116 ins; 31717 del; 8094 mod Patch: https://git.openjdk.org/jdk/pull/10104.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10104/head:pull/10104 PR: https://git.openjdk.org/jdk/pull/10104
Re: RFR: 8293116: Incremental JDK build could be sped up [v5]
On Fri, 9 Sep 2022 11:41:08 GMT, Jan Lahoda wrote:
>> Currently, when doing a small change inside a module that does not affect
>> the API of the module, the build system will skip rebuild of the dependent
>> modules. If there's a change that affects the module's API, the dependent
>> modules are recompiled. So far, this seems to work reasonably.
>>
>> But, for large modules, like `java.base`, any change to any of the module's
>> sources will cause a full rebuild of that module, which may take tens of
>> seconds. This patch strives to improve that, by:
>> -parsing only sources that were modified first,
>> -computing an "API" hash for them,
>> -if there is not a significant change in the file(s), only recompile the
>> files, if there are significant changes to the files, recompile the whole
>> module, as before
>>
>> There is no attempt made to determine the minimal set of files that needs to
>> be reparsed, all module's files are reparsed when the internal API check
>> fails. This is in line with the existing code to skip the build of dependent
>> modules.
>>
>> Sadly, this is overall a bit more difficult to do, so the implementation
>> uses some javac internals. The existing build-only `Depend` plugin is
>> enhanced so that parsing of the initial files is redirected to go through
>> the plugin, and it decides parses either only the modified files, or all
>> files. This redirection requires a change to `main/JavaCompiler`, which is
>> not in the boot JDK, so reflection is currently used to install the
>> redirection. Once the new classes are in a boot JDK, we could clean up, and
>> avoid the reflection.
>>
>> On my laptop, doing:
>>
>> touch src/java.base/share/classes/java/lang/Object.java ; time make
>>
>> can take well over 30s without this patch. With this patch, it takes <5s for
>> me. Changes to e.g. only method bodies should typically not require rebuild
>> of the whole module.
>>
>> The caveats of this patch include:
>> -it is more involved than the existing plugin
>> -it only speeds up only incremental Java compilation, so the other tasks the
>> build system is doing when running `make`, like timestamp checks or starting
>> the build server, are still happening. This may slow down builds on
>> platforms with slow I/O
>> -for modules that are part of the interim toolchain (like jdk.compiler), the
>> interim build is not using this new approach, and it will likely be slow. We
>> might try to improve once the required hooks are part of the boot JDK
>> -as the check for modified "internal" API cannot use attribution (as it runs
>> too early), it tries to be safe, and will mark some changes as "internal API
>> changes", even if they in fact don't affect the file's API. This may be
>> especially true for some changes to imports.
>> -it is necessary to store an (internal) API hash for every file, in addition
>> to one for every module which is stored currently
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Regenerating modfiles as necessary.
I ran some tests (in a semi-automated fashion), results seem reasonable:
## Non-API change in java.base:
sed -e 's/^}//* */}/' src/java.base/share/classes/java/lang/Object.java
time make
result:
Building target 'default (exploded-image)' in configuration
'linux-x86_64-server-release'
Compiling up to 3192 files for java.base
[stderr] compiling module: java.base, all Java inputs: 3192, modified files
(Java or non-Java): 1, full recompile: false, non-Java modified files:
Stopping sjavac server
Finished building target 'default (exploded-image)' in configuration
'linux-x86_64-server-release'
real0m3,414s
user0m5,188s
sys 0m1,328s
## Non-API change in java.compiler:
sed -e 's/^}//* */}/'
src/java.compiler/share/classes/javax/lang/model/util/ElementFilter.java
time make
result:
Building target 'default (exploded-image)' in configuration
'linux-x86_64-server-release'
Compiling up to 127 files for BUILD_java.compiler.interim
Compiling up to 127 files for java.compiler
[stderr] compiling module: java.compiler, all Java inputs: 127, modified files
(Java or non-Java): 1, full recompile: false, non-Java modified files:
Stopping sjavac server
Finished building target 'default (exploded-image)' in configuration
'linux-x86_64-server-release'
real0m15,530s
user0m22,948s
sys 0m2,337s
## Package-private method added to java.base:
sed -e 's/^}/static void testMethod() {} }/'
src/java.compiler/share/classes/javax/lang/model/util/ElementFilter.java
time make
result:
Building tar
Re: RFR: 8293116: Incremental JDK build could be sped up [v5]
> Currently, when doing a small change inside a module that does not affect the > API of the module, the build system will skip rebuild of the dependent > modules. If there's a change that affects the module's API, the dependent > modules are recompiled. So far, this seems to work reasonably. > > But, for large modules, like `java.base`, any change to any of the module's > sources will cause a full rebuild of that module, which may take tens of > seconds. This patch strives to improve that, by: > -parsing only sources that were modified first, > -computing an "API" hash for them, > -if there is not a significant change in the file(s), only recompile the > files, if there are significant changes to the files, recompile the whole > module, as before > > There is no attempt made to determine the minimal set of files that needs to > be reparsed, all module's files are reparsed when the internal API check > fails. This is in line with the existing code to skip the build of dependent > modules. > > Sadly, this is overall a bit more difficult to do, so the implementation uses > some javac internals. The existing build-only `Depend` plugin is enhanced so > that parsing of the initial files is redirected to go through the plugin, and > it decides parses either only the modified files, or all files. This > redirection requires a change to `main/JavaCompiler`, which is not in the > boot JDK, so reflection is currently used to install the redirection. Once > the new classes are in a boot JDK, we could clean up, and avoid the > reflection. > > On my laptop, doing: > > touch src/java.base/share/classes/java/lang/Object.java ; time make > > can take well over 30s without this patch. With this patch, it takes <5s for > me. Changes to e.g. only method bodies should typically not require rebuild > of the whole module. > > The caveats of this patch include: > -it is more involved than the existing plugin > -it only speeds up only incremental Java compilation, so the other tasks the > build system is doing when running `make`, like timestamp checks or starting > the build server, are still happening. This may slow down builds on platforms > with slow I/O > -for modules that are part of the interim toolchain (like jdk.compiler), the > interim build is not using this new approach, and it will likely be slow. We > might try to improve once the required hooks are part of the boot JDK > -as the check for modified "internal" API cannot use attribution (as it runs > too early), it tries to be safe, and will mark some changes as "internal API > changes", even if they in fact don't affect the file's API. This may be > especially true for some changes to imports. > -it is necessary to store an (internal) API hash for every file, in addition > to one for every module which is stored currently Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Regenerating modfiles as necessary. - Changes: - all: https://git.openjdk.org/jdk/pull/10104/files - new: https://git.openjdk.org/jdk/pull/10104/files/6c1a83b2..c3455660 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10104=04 - incr: https://webrevs.openjdk.org/?repo=jdk=10104=03-04 Stats: 26 lines in 2 files changed: 14 ins; 9 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/10104.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10104/head:pull/10104 PR: https://git.openjdk.org/jdk/pull/10104
Re: RFR: 8293116: Incremental JDK build could be sped up [v3]
On Thu, 8 Sep 2022 13:54:02 GMT, Erik Joelsson wrote: >> I looked up what kind of dependencies we are passing in through `DEPEND`. >> None, it turns out. It is not used. So that is not a worry. We might >> consider removing it. Sending in exceptional dependencies like that is never >> a good design decision. If it ever happens again that this would be the only >> acceptable solution, we can re-add the `DEPEND` argument. >> >> But that basically mean that Jan's patch might be okay, after all. `DEPENDS` >> does not exist, and I want to remove it. `EXTRA_DEPS` is only modified if we >> change the `depend` javac plugin. I am willing to sacrifice automatic >> recompilation of all Java classes if that is changed. > > Ok I was wrong about what EXTRA_DEPS was used for, I read the code too > hastily. However, the dependency declaration for the pubapi files from other > modules is added in make/CompileJavaModules.gmk like this: > > > $($(MODULE)_COMPILE_TARGET): $(foreach d, $(call FindDepsForModule, > $(MODULE)), \ > $(call SetupJavaCompilationApiTarget, $d, \ > $(if $($d_BIN), $($d_BIN), $(JDK_OUTPUTDIR)/modules/$d))) > > > So this still doesn't change my main point. The pubapi files only appear on > the dependency declaration for `$($(MODULE)_COMPILE_TARGET)`. This means that > with the current patch, we will not recompile the full module when a parent > module has a change to the pubapi. The Depend plugin needs to see the pubapi > files in `$$?` for that to happen. > > One could also make the case for using DEPEND when adding such a dependency > instead of referencing `$($(MODULE)_COMPILE_TARGET)` in explicit rules. It > would be more inline with more recent design patterns. Hmm, how about this? https://github.com/openjdk/jdk/pull/10104/commits/c3455660310bea8a33b1b5b5cb05c792f5d825a2 that seems to work (hopefully), although I am not sure if that is not breaking some overall concept in the build. - PR: https://git.openjdk.org/jdk/pull/10104
Re: RFR: 8293116: Incremental JDK build could be sped up [v3]
On Tue, 6 Sep 2022 14:33:10 GMT, Magnus Ihse Bursie wrote: >>> The `Depend` plugin will do a full build if a non-Java file is present in >>> the list, which I hope should lead to a more reliable recompilation for >>> some complex changes among the sources. >> >> Ah, I had missed that and that's clever! That also means there is a reason >> to generate MODFILES in the actual compile recipe instead of the separate >> rule as I suggested earlier. For this to be "safe" we need `$$?` to really >> contain all the updated prereqs, otherwise we may miss compiling everything >> when something external actually should make us do just that. > > ... and also that we *must* include the vardeps file, since any change there > should trigger a complete rebuild. > > The logic here seems correct, but apparently somewhat hard to fully > understand correctly. Maybe a few lines of comments summarizing the important > points raised here would be a good thing? > ... and also that we _must_ include the vardeps file, since any change there > should trigger a complete rebuild. > > The logic here seems correct, but apparently somewhat hard to fully > understand correctly. Maybe a few lines of comments summarizing the important > points raised here would be a good thing? How about: https://github.com/openjdk/jdk/pull/10104/commits/6c1a83b28ee72a99bf24e7f66c96d1294ad485dd Thanks, Jan - PR: https://git.openjdk.org/jdk/pull/10104
Re: RFR: 8293116: Incremental JDK build could be sped up [v4]
> Currently, when doing a small change inside a module that does not affect the > API of the module, the build system will skip rebuild of the dependent > modules. If there's a change that affects the module's API, the dependent > modules are recompiled. So far, this seems to work reasonably. > > But, for large modules, like `java.base`, any change to any of the module's > sources will cause a full rebuild of that module, which may take tens of > seconds. This patch strives to improve that, by: > -parsing only sources that were modified first, > -computing an "API" hash for them, > -if there is not a significant change in the file(s), only recompile the > files, if there are significant changes to the files, recompile the whole > module, as before > > There is no attempt made to determine the minimal set of files that needs to > be reparsed, all module's files are reparsed when the internal API check > fails. This is in line with the existing code to skip the build of dependent > modules. > > Sadly, this is overall a bit more difficult to do, so the implementation uses > some javac internals. The existing build-only `Depend` plugin is enhanced so > that parsing of the initial files is redirected to go through the plugin, and > it decides parses either only the modified files, or all files. This > redirection requires a change to `main/JavaCompiler`, which is not in the > boot JDK, so reflection is currently used to install the redirection. Once > the new classes are in a boot JDK, we could clean up, and avoid the > reflection. > > On my laptop, doing: > > touch src/java.base/share/classes/java/lang/Object.java ; time make > > can take well over 30s without this patch. With this patch, it takes <5s for > me. Changes to e.g. only method bodies should typically not require rebuild > of the whole module. > > The caveats of this patch include: > -it is more involved than the existing plugin > -it only speeds up only incremental Java compilation, so the other tasks the > build system is doing when running `make`, like timestamp checks or starting > the build server, are still happening. This may slow down builds on platforms > with slow I/O > -for modules that are part of the interim toolchain (like jdk.compiler), the > interim build is not using this new approach, and it will likely be slow. We > might try to improve once the required hooks are part of the boot JDK > -as the check for modified "internal" API cannot use attribution (as it runs > too early), it tries to be safe, and will mark some changes as "internal API > changes", even if they in fact don't affect the file's API. This may be > especially true for some changes to imports. > -it is necessary to store an (internal) API hash for every file, in addition > to one for every module which is stored currently Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Adding comment. - Changes: - all: https://git.openjdk.org/jdk/pull/10104/files - new: https://git.openjdk.org/jdk/pull/10104/files/23e3b756..6c1a83b2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10104=03 - incr: https://webrevs.openjdk.org/?repo=jdk=10104=02-03 Stats: 9 lines in 1 file changed: 9 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/10104.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10104/head:pull/10104 PR: https://git.openjdk.org/jdk/pull/10104
Re: RFR: 8293116: Incremental JDK build could be sped up [v3]
On Mon, 5 Sep 2022 12:19:38 GMT, Magnus Ihse Bursie wrote: > I think you really mean `$$($1_SRCS)` here, since the vardeps file is a build > internal file that javac do not and should not care about. While I'm sure it > will ignore it, the code here will be clearer if we don't pass it around when > we shouldn't. Hm, but `$$($1_SRCS)` contains all the source files of the given module, modified and unmodified, correct? The point of this patch is that we send the list of modified files to the `Depend` plugin (all sources are still passed to the javac invocation), and the plugin decides whether or not all files should be compiled, or only the modified files should be compiled. The plugin could theoretically do the timestamp check from the sources, but it is more difficult to do, and the make has (presumably) already done that in a predictable way. The `Depend` plugin will do a full build if a non-Java file is present in the list, which I hope should lead to a more reliable recompilation for some complex changes among the sources. Note that the modfiles are not sent to javac as such - all sources are still sent to javac. The modfiles are sent to the `Depend` plugin using a side channel. - PR: https://git.openjdk.org/jdk/pull/10104
Re: RFR: 8293116: Incremental JDK build could be sped up [v3]
On Thu, 1 Sep 2022 16:48:48 GMT, Erik Joelsson wrote: > Oh right, ListPathsSafely takes the data to write by reference, not by value > (as in you supply the name of a variable that contains the data, not the data > itself). I doubt `?` can be sent into an eval as a variable name, but I may > be wrong. Perhaps you could extract the value into another variable first in > a separate line in the recipe, something like this: > > ``` > $$(eval $1_MODFILES := $$?) > $$(eval $$(call ListPathsSafely, $1_MODFILES, $$($1_MODFILELIST))) > ``` > > (note that I redefined $1_MODFILES to be the list of modified files and > introduced a new variable $1_MODFILELIST for the filename where we print it) Thanks! Done in: https://github.com/openjdk/jdk/pull/10104/commits/23e3b75679908d26b76cc291a8efba06879c0d83 - PR: https://git.openjdk.org/jdk/pull/10104
Re: RFR: 8293116: Incremental JDK build could be sped up [v3]
> Currently, when doing a small change inside a module that does not affect the > API of the module, the build system will skip rebuild of the dependent > modules. If there's a change that affects the module's API, the dependent > modules are recompiled. So far, this seems to work reasonably. > > But, for large modules, like `java.base`, any change to any of the module's > sources will cause a full rebuild of that module, which may take tens of > seconds. This patch strives to improve that, by: > -parsing only sources that were modified first, > -computing an "API" hash for them, > -if there is not a significant change in the file(s), only recompile the > files, if there are significant changes to the files, recompile the whole > module, as before > > There is no attempt made to determine the minimal set of files that needs to > be reparsed, all module's files are reparsed when the internal API check > fails. This is in line with the existing code to skip the build of dependent > modules. > > Sadly, this is overall a bit more difficult to do, so the implementation uses > some javac internals. The existing build-only `Depend` plugin is enhanced so > that parsing of the initial files is redirected to go through the plugin, and > it decides parses either only the modified files, or all files. This > redirection requires a change to `main/JavaCompiler`, which is not in the > boot JDK, so reflection is currently used to install the redirection. Once > the new classes are in a boot JDK, we could clean up, and avoid the > reflection. > > On my laptop, doing: > > touch src/java.base/share/classes/java/lang/Object.java ; time make > > can take well over 30s without this patch. With this patch, it takes <5s for > me. Changes to e.g. only method bodies should typically not require rebuild > of the whole module. > > The caveats of this patch include: > -it is more involved than the existing plugin > -it only speeds up only incremental Java compilation, so the other tasks the > build system is doing when running `make`, like timestamp checks or starting > the build server, are still happening. This may slow down builds on platforms > with slow I/O > -for modules that are part of the interim toolchain (like jdk.compiler), the > interim build is not using this new approach, and it will likely be slow. We > might try to improve once the required hooks are part of the boot JDK > -as the check for modified "internal" API cannot use attribution (as it runs > too early), it tries to be safe, and will mark some changes as "internal API > changes", even if they in fact don't affect the file's API. This may be > especially true for some changes to imports. > -it is necessary to store an (internal) API hash for every file, in addition > to one for every module which is stored currently Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Reflecting review feedback. - Changes: - all: https://git.openjdk.org/jdk/pull/10104/files - new: https://git.openjdk.org/jdk/pull/10104/files/f1e19e8b..23e3b756 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10104=02 - incr: https://webrevs.openjdk.org/?repo=jdk=10104=01-02 Stats: 26 lines in 3 files changed: 16 ins; 4 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/10104.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10104/head:pull/10104 PR: https://git.openjdk.org/jdk/pull/10104
Re: RFR: 8293116: Incremental JDK build could be sped up [v2]
On Thu, 1 Sep 2022 13:25:19 GMT, Erik Joelsson wrote: >> Is there some recommendation on how to generate what is needed? Which is: >> >> "-XDmodifiedFiles= " >> >> >> I was looking at the `WriteFile` rule, and it is not clear to be what is the >> problem there, but I'm not very knowledgeable of the build system. >> >> I was trying to use `ListPathsSafely`, but wasn't able to find a way that >> would be working. >> >> Thanks! > > Before make 4.0, there was no `$(file ...)` function in make. That meant that > if you needed to write something to a file directly from the makefile, you > had to do something like `$(shell echo "foo" > /my/file)`. This works ok as > long as the amount of text you need to write is small enough, but when > writing long lists of filenames, you will eventually hit OS limits on command > line length (which are different on different OSes, most notably much shorter > on Windows). Our solution to this was ListPathsSafely, which breaks up long > lists of path names into manageable chunks (and also compresses parts of the > path using sed). > > In make 4.0, the $(file) function was introduced, which makes this so much > easier. Since it was also more performant, I implemented a 4.0 version of > ListPathsSafely. However, we haven't changed the build to require make >4.0, > so we have to keep both implementations around. > > The WriteFile macro was only intended writing smaller files, so doesn't have > any of the command splitting functionality of ListPathsSafely. > > So to answer your question, no there isn't a good way to achieve what you are > asking for today, not without modifying or extending the macros. One could > imagine adding a flag to ListPathsSafely that made it use space instead of > newline as separator. Another solution could be to have Depend.java accept > something like > > "-XDmodifiedFilesFile=/path/to/file" > > where the file is a list as ListPathsSafely would create it. > > We could also consider requiring make 4.0, but that will need some > socializing first. I can change `Depend` for this, that is easy, but it is not clear how to write the file. I've tried the naive `$$(eval $$(call ListPathsSafely, $$?, $$($1_MODFILES)))` at the end of `$$($1_FILELIST):` but that does not seem to write the modified files into the file - it produces an empty file. Is there a way for me to write the modified files into the file? Thanks. - PR: https://git.openjdk.org/jdk/pull/10104
Re: RFR: 8293116: Incremental JDK build could be sped up [v2]
On Wed, 31 Aug 2022 20:02:46 GMT, Erik Joelsson wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Reflecting review feedback. > > make/common/JavaCompilation.gmk line 451: > >> 449: # $$($1_VARDEPS_FILE) is used as dependency to track changes in set >> of >> 450: # list of files. >> 451: $$($1_COMPILE_TARGET): $$($1_SRCS) $$($1_DEPENDS) \ > > What's the reason for inlining this recipe into the main compilation recipe? > I would think generating the modfiles file in the FILELIST rule would help > reduce the noise from non source files in the prerequisites list (as the old > FILELIST rule had a lot less prerequisites listed). Keeping this recipe > separate also limits regeneration of the FILELIST. I've tried to fix this in: https://github.com/openjdk/jdk/pull/10104/commits/f1e19e8b636ddf08500a6401ce1488265f7b9446 > make/common/JavaCompilation.gmk line 456: > >> 454: $$(call LogWarn, Compiling up to $$(words $$($1_SRCS)) >> files for $1) >> 455: $$(eval $$(call ListPathsSafely, $1_SRCS, >> $$($1_FILELIST))) >> 456: $$(call WriteFile, "-XDmodifiedInputs=$$? ", >> $$@.modfiles) > > I'm not sure WriteFile is safe to use here. `$?` could potentially contain > all files in `$1_SRCS`, which is why we use ListPathsSafely in the first > place. This is only a problem if using make <4.0, but as long as that's not > prohibited, we need to keep it working without the file function. Is there some recommendation on how to generate what is needed? Which is: "-XDmodifiedFiles= " I was looking at the `WriteFile` rule, and it is not clear to be what is the problem there, but I'm not very knowledgeable of the build system. I was trying to use `ListPathsSafely`, but wasn't able to find a way that would be working. Thanks! - PR: https://git.openjdk.org/jdk/pull/10104
Re: RFR: 8293116: Incremental JDK build could be sped up [v2]
> Currently, when doing a small change inside a module that does not affect the > API of the module, the build system will skip rebuild of the dependent > modules. If there's a change that affects the module's API, the dependent > modules are recompiled. So far, this seems to work reasonably. > > But, for large modules, like `java.base`, any change to any of the module's > sources will cause a full rebuild of that module, which may take tens of > seconds. This patch strives to improve that, by: > -parsing only sources that were modified first, > -computing an "API" hash for them, > -if there is not a significant change in the file(s), only recompile the > files, if there are significant changes to the files, recompile the whole > module, as before > > There is no attempt made to determine the minimal set of files that needs to > be reparsed, all module's files are reparsed when the internal API check > fails. This is in line with the existing code to skip the build of dependent > modules. > > Sadly, this is overall a bit more difficult to do, so the implementation uses > some javac internals. The existing build-only `Depend` plugin is enhanced so > that parsing of the initial files is redirected to go through the plugin, and > it decides parses either only the modified files, or all files. This > redirection requires a change to `main/JavaCompiler`, which is not in the > boot JDK, so reflection is currently used to install the redirection. Once > the new classes are in a boot JDK, we could clean up, and avoid the > reflection. > > On my laptop, doing: > > touch src/java.base/share/classes/java/lang/Object.java ; time make > > can take well over 30s without this patch. With this patch, it takes <5s for > me. Changes to e.g. only method bodies should typically not require rebuild > of the whole module. > > The caveats of this patch include: > -it is more involved than the existing plugin > -it only speeds up only incremental Java compilation, so the other tasks the > build system is doing when running `make`, like timestamp checks or starting > the build server, are still happening. This may slow down builds on platforms > with slow I/O > -for modules that are part of the interim toolchain (like jdk.compiler), the > interim build is not using this new approach, and it will likely be slow. We > might try to improve once the required hooks are part of the boot JDK > -as the check for modified "internal" API cannot use attribution (as it runs > too early), it tries to be safe, and will mark some changes as "internal API > changes", even if they in fact don't affect the file's API. This may be > especially true for some changes to imports. > -it is necessary to store an (internal) API hash for every file, in addition > to one for every module which is stored currently Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Reflecting review feedback. - Changes: - all: https://git.openjdk.org/jdk/pull/10104/files - new: https://git.openjdk.org/jdk/pull/10104/files/778a6719..f1e19e8b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10104=01 - incr: https://webrevs.openjdk.org/?repo=jdk=10104=00-01 Stats: 14 lines in 1 file changed: 6 ins; 2 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/10104.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10104/head:pull/10104 PR: https://git.openjdk.org/jdk/pull/10104
RFR: 8293116: Incremental JDK build could be sped up
Currently, when doing a small change inside a module that does not affect the API of the module, the build system will skip rebuild of the dependent modules. If there's a change that affects the module's API, the dependent modules are recompiled. So far, this seems to work reasonably. But, for large modules, like `java.base`, any change to any of the module's sources will cause a full rebuild of that module, which may take tens of seconds. This patch strives to improve that, by: -parsing only sources that were modified first, -computing an "API" hash for them, -if there is not a significant change in the file(s), only recompile the files, if there are significant changes to the files, recompile the whole module, as before There is no attempt made to determine the minimal set of files that needs to be reparsed, all module's files are reparsed when the internal API check fails. This is in line with the existing code to skip the build of dependent modules. Sadly, this is overall a bit more difficult to do, so the implementation uses some javac internals. The existing build-only `Depend` plugin is enhanced so that parsing of the initial files is redirected to go through the plugin, and it decides parses either only the modified files, or all files. This redirection requires a change to `main/JavaCompiler`, which is not in the boot JDK, so reflection is currently used to install the redirection. Once the new classes are in a boot JDK, we could clean up, and avoid the reflection. On my laptop, doing: touch src/java.base/share/classes/java/lang/Object.java ; time make can take well over 30s without this patch. With this patch, it takes <5s for me. Changes to e.g. only method bodies should typically not require rebuild of the whole module. The caveats of this patch include: -it is more involved than the existing plugin -it only speeds up only incremental Java compilation, so the other tasks the build system is doing when running `make`, like timestamp checks or starting the build server, are still happening. This may slow down builds on platforms with slow I/O -for modules that are part of the interim toolchain (like jdk.compiler), the interim build is not using this new approach, and it will likely be slow. We might try to improve once the required hooks are part of the boot JDK -as the check for modified "internal" API cannot use attribution (as it runs too early), it tries to be safe, and will mark some changes as "internal API changes", even if they in fact don't affect the file's API. This may be especially true for some changes to imports. -it is necessary to store an (internal) API hash for every file, in addition to one for every module which is stored currently - Commit messages: - 8293116: Incremental JDK build could be sped up Changes: https://git.openjdk.org/jdk/pull/10104/files Webrev: https://webrevs.openjdk.org/?repo=jdk=10104=00 Issue: https://bugs.openjdk.org/browse/JDK-8293116 Stats: 543 lines in 6 files changed: 513 ins; 6 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/10104.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10104/head:pull/10104 PR: https://git.openjdk.org/jdk/pull/10104
