On Thu, 7 Mar 2024 17:07:05 GMT, Magnus Ihse Bursie <i...@openjdk.org> 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 both of those only check the option works, not its output. This seems correct for something that is *internal* version data. To me, relying on a certain structure for this is something I would regard as a bug. ------------- PR Comment: https://git.openjdk.org/jdk/pull/18136#issuecomment-1984286305
