Re: RFR: 8327389: Remove use of HOTSPOT_BUILD_USER
On Thu, 7 Mar 2024 19:53:35 GMT, Andrew John Hughes wrote: > Also, it might be worth repeating one of my long-standing wishes: that the > version string should not be hard-coded into the build, but e.g. stored as a > string in the `release` file, and read from there. If we did that, the cost > of changing the version string would be negligible, and we wouldn't need to > worry as much about it. It would also be simple to compare different builds > which end up with the same bits since they are built from the same sources, > but by different version flags (e.g. -ea vs GA). (In fact, we'd turn a -ea > build into a GA just by updating the version string, so we'd know for sure we > are publishing what we tested.) Why not store the version string inside HotSpot, and have it as the one source of truth for the version string so it doesn't need to be hardcoded in other places? A text file seems too easy to modify to set the version string to a rubbish value - PR Comment: https://git.openjdk.org/jdk/pull/18136#issuecomment-1985087876
Re: RFR: JDK-8298405: Implement JEP 467: Markdown Documentation Comments [v47]
> 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: add test cases for links to anchors - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/fc76e8c7..8dfd9e08 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=46 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=45-46 Stats: 60 lines in 1 file changed: 60 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
Re: RFR: 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&pr=18106&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18106&range=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: 8327218: Add an ability to specify modules which should have native access enabled [v3]
On Wed, 6 Mar 2024 21:16:25 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 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. Marked as reviewed by mcimadamore (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18106#pullrequestreview-1923612749
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
Re: RFR: 8327389: Remove use of HOTSPOT_BUILD_USER
On Wed, 6 Mar 2024 11:57:27 GMT, Andrew John Hughes wrote: > The HotSpot code has inserted the value of `HOTSPOT_BUILD_USER` into the > internal version string since before the JDK was open-sourced, so the > reasoning for this is not in the public history. See > https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/abstract_vm_version.cpp#L284 > > ~~~ > $ /usr/lib/jvm/java-21-openjdk/bin/java -Xinternalversion > OpenJDK 64-Bit Server VM (21.0.2+13) for linux-amd64 JRE (21.0.2+13), built > on 2024-01-19T16:39:52Z by "mockbuild" with gcc 8.5.0 20210514 (Red Hat > 8.5.0-20) > ~~~ > > It is not clear what value this provides. By default, the value is set to the > system username, but it can be set to a static value using > `--with-build-user`. > > In trying to compare builds, this is a source of difference if the builds are > produced under different usernames and `--with-build-user` is not set to the > same value. > > It may also be a source of information leakage from the host system. It is > not clear what value it provides to the end user of the build. > > This patch proposes removing it altogether, but at least knowing why it needs > to be there would be progress from where we are now. We could also consider a > middle ground of only setting it for "adhoc" builds, those without set > version information, as is the case with the overall version output. > > With this patch: > > ~~~ > $ ~/builder/23/images/jdk/bin/java -Xinternalversion > OpenJDK 64-Bit Server VM (fastdebug 23-internal-adhoc.andrew.jdk) for > linux-amd64 JRE (23-internal-adhoc.andrew.jdk), built on 2024-03-06T00:23:37Z > with gcc 13.2.1 20230826 > > $ ~/builder/23/images/jdk/bin/java -XX:ErrorHandlerTest=1 > > $ grep '^vm_info' > /localhome/andrew/projects/openjdk/upstream/jdk/hs_err_pid1119824.log > vm_info: OpenJDK 64-Bit Server VM (fastdebug 23-internal-adhoc.andrew.jdk) > for linux-amd64 JRE (23-internal-adhoc.andrew.jdk), built on > 2024-03-06T00:23:37Z with gcc 13.2.1 20230826 > ~~~ > > The above are from a fastdebug build but I also built a product build with no > issues. I agree with Andrew. We aren't removing the username from adhoc builds, just the copy in the hotspot internal version string, so the concern about that part seems baseless to me. I don't see the connection to the `git describe` discussion, I think that should move to a separate thread. I definitely don't think we need to add a fake HOTSPOT_BUILD_USER to the _internal_ hotspot version string. Lets just get rid of it by accepting this PR. - PR Comment: https://git.openjdk.org/jdk/pull/18136#issuecomment-1984415570
Re: RFR: 8327389: Remove use of HOTSPOT_BUILD_USER
On Thu, 7 Mar 2024 17:32:02 GMT, Frederic Thevenet wrote: > Also, it might be worth repeating one of my long-standing wishes: that the > version string should not be hard-coded into the build, but e.g. stored as a > string in the `release` file, and read from there. If we did that, the cost > of changing the version string would be negligible, and we wouldn't need to > worry as much about it. It would also be simple to compare different builds > which end up with the same bits since they are built from the same sources, > but by different version flags (e.g. -ea vs GA). (In fact, we'd turn a -ea > build into a GA just by updating the version string, so we'd know for sure we > are publishing what we tested.) This certainly sounds like it has the potential to solve a lot of these kind of problems. I would point out that, if you can flip the EA status in the text file, someone could also easily masquerade a build as something completely different from what it is. However, it is already possible to create a build like this via the `configure` options so it's really only a slight change in accessibility. For example, if I specify `--with-version-feature=11`, I can produce: ~~~ $ /home/andrew/builder/fake11/images/jdk/bin/java -version openjdk version "11-internal" 2024-09-17 OpenJDK Runtime Environment (fastdebug build 11-internal-adhoc.andrew.jdk) OpenJDK 64-Bit Server VM (fastdebug build 11-internal-adhoc.andrew.jdk, mixed mode, sharing) $ /home/andrew/builder/fake11/images/jdk/bin/java -Xinternalversion OpenJDK 64-Bit Server VM (fastdebug 11-internal-adhoc.andrew.jdk) for linux-amd64 JRE (11-internal-adhoc.andrew.jdk), built on 2024-03-07T19:41:08Z with gcc 13.2.1 20230826 ~~~ despite the fact that the source code I've built is actually an in-development JDK 23. All we'd be doing is moving that into a text file. - PR Comment: https://git.openjdk.org/jdk/pull/18136#issuecomment-1984317603
Re: RFR: 8327389: Remove use of HOTSPOT_BUILD_USER
On Thu, 7 Mar 2024 17:07:05 GMT, Magnus Ihse Bursie wrote: > There is an inherent conflict with creating a version string that is very > much up-to-date and includes ephemeral build data, and creating a robustly > reproducible build. If this patch were to remove HOTSPOT_BUILD_USER, and we > would later on store `git describe` in the version string, I'm not sure we > actually gained anything in terms of reproducability. :-( It's worth remembering that there are essentially two potential forms of version string at play; those for "adhoc" builds and those for release builds, which depends on whether `NO_DEFAULT_VERSION_PARTS` ends up being set to `true` or not. This change does nothing to remove the username used in the "adhoc" version of `--with-version-opt` i.e. `adhoc.$USERNAME.$basedirname`: >From the patched build: ~~~ $ /home/andrew/builder/23/images/jdk/bin/java -version openjdk version "23-internal" 2024-09-17 OpenJDK Runtime Environment (fastdebug build 23-internal-adhoc.andrew.jdk) OpenJDK 64-Bit Server VM (fastdebug build 23-internal-adhoc.andrew.jdk, mixed mode, sharing) $ /home/andrew/builder/23/images/jdk/bin/java -Xinternalversion OpenJDK 64-Bit Server VM (fastdebug 23-internal-adhoc.andrew.jdk) for linux-amd64 JRE (23-internal-adhoc.andrew.jdk), built on 2024-03-06T00:23:37Z with gcc 13.2.1 20230826 ~~~ What we aim to get rid of is the duplicate username copy which trickles into the `-Xinternalversion` via `HOTSPOT_BUILD_USER`. From an unpatched build of 22: ~~~ $ /home/andrew/build/openjdk22/bin/java -Xinternalversion OpenJDK 64-Bit Server VM (22.0.1-internal-adhoc.andrew.jdk) for linux-amd64 JRE (22.0.1-internal-adhoc.andrew.jdk), built on 2024-03-05T19:52:35Z by "andrew" with gcc 13.2.1 20230826 ~~~ The only difference between the two is my username is there twice, instead of thrice. The problem with the `HOTSPOT_BUILD_USER` one is that it also turns up when you do a build for release: ~~~ $ /usr/lib/jvm/java-21-openjdk/bin/java -Xinternalversion OpenJDK 64-Bit Server VM (21.0.2+13) for linux-amd64 JRE (21.0.2+13), built on 2024-01-19T16:39:52Z by "mockbuild" with gcc 8.5.0 20210514 (Red Hat 8.5.0-20) ~~~ Because `NO_DEFAULT_VERSION_PARTS` has been set, the two instances of the username from the adhoc build string are gone. However, the one from `HOTSPOT_BUILD_USER` being set to `mockbuild` still remains. Regarding the conflict between very accurate version information and reproducibility, it is possible to support both scenarios by having "adhoc" builds provide one and release builds the other. The problem with `HOTSPOT_BUILD_USER` as it stands is it ends up in both. I don't see an issue with including `git describe` output in adhoc builds. It may well not even be a problem for reproducibility in release builds as they are likely built from a set tag without local changes or even from a bare source tree without repository information at all. It does seem like a separate issue from this PR though. > 3. We can agree to disagree about reproducibility limits, but accept that > HOTSPOT_BUILD_USER is silly, and accept this PR, but open a separate JBS > issue to discuss if adding `git describe` is desirable, and if it can be done > in an opt-in manner, with the understanding that it might not be added if we > can't agree on it. > > > My personal preference would be 3) above, but I am willing to accept any of > the paths. > > @gnu-andrew and @shipilev, what do you think? I recon you are the one most > opinionated about this. I would go for 3. As I say, I don't see why this change would depend on a `git describe` change. Would you still prefer to hardcode a value for `HOTSPOT_BUILD_USER` or remove it altogether? I have a slight preference for the cleanliness of removing it altogether, especially as this is early in the 23 development cycle, so there is some time to see how it pans out. Alternatively, we can hardcode it to `unknown` (the current default if `HOTSPOT_BUILD_USER is undefined). Not only is this accurate (we don't know who the user was as we haven't recorded the information) but it is already a possible value for compatibility. Changing to this would simply be a matter of removing only the `#ifndef` in `abstract_vm_version.cpp` and leaving the actual version string alone. Looking further ahead, it would be preferable for us to backport this to 21, as that is where we are trying to achieve comparability between Red Hat & Temurin builds, but there is no rush for that and we could even hardcode it only in the backport. As regards testing, I have built this in release and fastdebug builds and checked the output where it is used in `-Xinternalversion` and the `vm info` line of a crash dump. Like David, I can't see any tests that pertain to this. The only ones that mention `-Xinternalversion` are `test/hotspot/jtreg/runtime/CommandLine/TestNullTerminatedFlags.java` and `test/hotspot/jtreg/sanity/BasicVMTest.java` and
Re: RFR: 8327389: Remove use of HOTSPOT_BUILD_USER
On Thu, 7 Mar 2024 17:11:18 GMT, Magnus Ihse Bursie wrote: > Also, it might be worth repeating one of my long-standing wishes: that the > version string should not be hard-coded into the build, but e.g. stored as a > string in the `release` file, and read from there. If we did that, the cost > of changing the version string would be negligible, and we wouldn't need to > worry as much about it. It would also be simple to compare different builds > which end up with the same bits since they are built from the same sources, > but by different version flags (e.g. -ea vs GA). (In fact, we'd turn a -ea > build into a GA just by updating the version string, so we'd know for sure we > are publishing what we tested.) +1 This would be a great step in making comparability (beyond reproducibility) of builds of OpenJDK at lot simpler. - PR Comment: https://git.openjdk.org/jdk/pull/18136#issuecomment-1984065312
Re: RFR: 8327389: Remove use of HOTSPOT_BUILD_USER
On Wed, 6 Mar 2024 11:57:27 GMT, Andrew John Hughes wrote: > The HotSpot code has inserted the value of `HOTSPOT_BUILD_USER` into the > internal version string since before the JDK was open-sourced, so the > reasoning for this is not in the public history. See > https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/abstract_vm_version.cpp#L284 > > ~~~ > $ /usr/lib/jvm/java-21-openjdk/bin/java -Xinternalversion > OpenJDK 64-Bit Server VM (21.0.2+13) for linux-amd64 JRE (21.0.2+13), built > on 2024-01-19T16:39:52Z by "mockbuild" with gcc 8.5.0 20210514 (Red Hat > 8.5.0-20) > ~~~ > > It is not clear what value this provides. By default, the value is set to the > system username, but it can be set to a static value using > `--with-build-user`. > > In trying to compare builds, this is a source of difference if the builds are > produced under different usernames and `--with-build-user` is not set to the > same value. > > It may also be a source of information leakage from the host system. It is > not clear what value it provides to the end user of the build. > > This patch proposes removing it altogether, but at least knowing why it needs > to be there would be progress from where we are now. We could also consider a > middle ground of only setting it for "adhoc" builds, those without set > version information, as is the case with the overall version output. > > With this patch: > > ~~~ > $ ~/builder/23/images/jdk/bin/java -Xinternalversion > OpenJDK 64-Bit Server VM (fastdebug 23-internal-adhoc.andrew.jdk) for > linux-amd64 JRE (23-internal-adhoc.andrew.jdk), built on 2024-03-06T00:23:37Z > with gcc 13.2.1 20230826 > > $ ~/builder/23/images/jdk/bin/java -XX:ErrorHandlerTest=1 > > $ grep '^vm_info' > /localhome/andrew/projects/openjdk/upstream/jdk/hs_err_pid1119824.log > vm_info: OpenJDK 64-Bit Server VM (fastdebug 23-internal-adhoc.andrew.jdk) > for linux-amd64 JRE (23-internal-adhoc.andrew.jdk), built on > 2024-03-06T00:23:37Z with gcc 13.2.1 20230826 > ~~~ > > The above are from a fastdebug build but I also built a product build with no > issues. Also, it might be worth repeating one of my long-standing wishes: that the version string should not be hard-coded into the build, but e.g. stored as a string in the `release` file, and read from there. If we did that, the cost of changing the version string would be negligible, and we wouldn't need to worry as much about it. It would also be simple to compare different builds which end up with the same bits since they are built from the same sources, but by different version flags (e.g. -ea vs GA). (In fact, we'd turn a -ea build into a GA just by updating the version string, so we'd know for sure we are publishing what we tested.) - PR Comment: https://git.openjdk.org/jdk/pull/18136#issuecomment-1984028842
Re: RFR: 8327389: Remove use of HOTSPOT_BUILD_USER
On Wed, 6 Mar 2024 11:57:27 GMT, Andrew John Hughes wrote: > The HotSpot code has inserted the value of `HOTSPOT_BUILD_USER` into the > internal version string since before the JDK was open-sourced, so the > reasoning for this is not in the public history. See > https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/abstract_vm_version.cpp#L284 > > ~~~ > $ /usr/lib/jvm/java-21-openjdk/bin/java -Xinternalversion > OpenJDK 64-Bit Server VM (21.0.2+13) for linux-amd64 JRE (21.0.2+13), built > on 2024-01-19T16:39:52Z by "mockbuild" with gcc 8.5.0 20210514 (Red Hat > 8.5.0-20) > ~~~ > > It is not clear what value this provides. By default, the value is set to the > system username, but it can be set to a static value using > `--with-build-user`. > > In trying to compare builds, this is a source of difference if the builds are > produced under different usernames and `--with-build-user` is not set to the > same value. > > It may also be a source of information leakage from the host system. It is > not clear what value it provides to the end user of the build. > > This patch proposes removing it altogether, but at least knowing why it needs > to be there would be progress from where we are now. We could also consider a > middle ground of only setting it for "adhoc" builds, those without set > version information, as is the case with the overall version output. > > With this patch: > > ~~~ > $ ~/builder/23/images/jdk/bin/java -Xinternalversion > OpenJDK 64-Bit Server VM (fastdebug 23-internal-adhoc.andrew.jdk) for > linux-amd64 JRE (23-internal-adhoc.andrew.jdk), built on 2024-03-06T00:23:37Z > with gcc 13.2.1 20230826 > > $ ~/builder/23/images/jdk/bin/java -XX:ErrorHandlerTest=1 > > $ grep '^vm_info' > /localhome/andrew/projects/openjdk/upstream/jdk/hs_err_pid1119824.log > vm_info: OpenJDK 64-Bit Server VM (fastdebug 23-internal-adhoc.andrew.jdk) > for linux-amd64 JRE (23-internal-adhoc.andrew.jdk), built on > 2024-03-06T00:23:37Z with gcc 13.2.1 20230826 > ~~~ > > The above are from a fastdebug build but I also built a product build with no > issues. I am okay with either (3) or (4). That is, I agree `HOTSPOT_BUILD_USER` is silly. I think we can survive without `git describe` thing for a while too. - PR Comment: https://git.openjdk.org/jdk/pull/18136#issuecomment-1984028130
Re: RFR: 8327389: Remove use of HOTSPOT_BUILD_USER
On Wed, 6 Mar 2024 18:01:00 GMT, Aleksey Shipilev wrote: > > I agree that it is good to get rid of this. However, the reason it has > > stuck around for so long is, eh, that it has been around for so long, so it > > is a bit unclear what or who could be relying on this. > > The example I know is to get a signal that test infrastructures actually > picked up my ad-hoc binary build, not some other build from somewhere else. > Maybe we should revisit the `git describe` hash idea we kicked along for a > while now: https://bugs.openjdk.org/browse/JDK-8274980?focusedId=14452017 -- > which would cover that case too. That bug was resolved with a different patch; presumably it conflated several issues. There is an inherent conflict with creating a version string that is very much up-to-date and includes ephemeral build data, and creating a robustly reproducible build. If this patch were to remove HOTSPOT_BUILD_USER, and we would later on store `git describe` in the version string, I'm not sure we actually gained anything in terms of reproducability. :-( As I see it, given this PR, we have a few different options: 1) We can agree that stable and robust reproducibility trumps any automatic inclusion of build situation metadata, and accept this PR and drop the idea of using `git describe` 2) We can agree that reproducibility has its limits, and we need to include (technically irrelevant) metadata in builds to facilitate development. This means dropping this PR since it is used to verify that the correct build was tested, and possibly also adding `git describe` later on. 3) We can agree to disagree about reproducibility limits, but accept that HOTSPOT_BUILD_USER is silly, and accept this PR, but open a separate JBS issue to discuss if adding `git describe` is desirable, and if it can be done in an opt-in manner, with the understanding that it might not be added if we can't agree on it. 4) We can agree to disagree about reproducibility limits, but accept that HOTSPOT_BUILD_USER is silly, and accept this PR, but require that we first implement a substitute functionality using `git describe`, before we can get rid of HOTSPOT_BUILD_USER. My personal preference would be 3) above, but I am willing to accept any of the paths. @gnu-andrew and @shipilev, what do you think? I recon you are the one most opinionated about this. - PR Comment: https://git.openjdk.org/jdk/pull/18136#issuecomment-1984021748
Re: RFR: 8327476: Upgrade JLine to 3.25.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 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/18142#discussion_r1516053385
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v3]
On Wed, 6 Mar 2024 21:16:25 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 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.
Marked as reviewed by alanb (Reviewer).
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.
-
PR Review: https://git.openjdk.org/jdk/pull/18106#pullrequestreview-1921919290
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1515834537
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Wed, 6 Mar 2024 21:22:40 GMT, Jan Lahoda wrote:
>> 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
Thanks. I view these changes to ModuleBootstrap to be temporary. We'll need to
create a few follow on issues in JBS, one of which is to update jlink so that
we have the exact set of modules in the run-time image that have been granted
native access.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1515838474
