RFR: JDK-8085822 JEP 223: New Version-String Scheme (initial integration)

[Magnus Ihse Bursie]

On 2015-06-10 11:58, David Holmes wrote:
> Hi Magnus,
>
> Generally looks good - a few comments/queries below.

In general, I believe most issues you found are valid. :-) However, as I 
said before in this thread, I'd like to see them resolved in the needed 
follow-up patches. The source code changes in Hotspot and JDK are 
inadequate to fully implement JEP-223, so these areas will need to be 
rewritten anyhow. Are you okay with resolving these issues later?

>
> On 9/06/2015 11:52 PM, Magnus Ihse Bursie wrote:
>> Here's an updated webrev, which fixes the typos that were pointed out by
>> reviewers:
>> http://cr.openjdk.java.net/~ihse/JDK-8085822-JEP-223-initial-patch/webrev.02/ 
>>
>
> common/autoconf/version-numbers
>
> Shouldn't there be a DEFAULT_VERSION_XXX for each of the XXX parts of 
> the version info? (Even if all zeroes presently.)
So that's a tricky one. :-& The question here is, I think, does it make 
sense to update version-numbers when we do a security (9.0.1) or minor 
(9.1.0) release? Looking historically, the version-numbers file have not 
been changed for the 8u family. The only change was going from 8 to 9 
when creating the new jdk9 forest. That was what I based my decition to 
only have the major number in the file. (The rest of the version string 
is set by configure flags when building, in Oracle case by the RE team.)

If it makes sense to put the minor/security/patch numbers here, I won't 
oppose it, but I guess we have until the first such release to figure 
out what's best, and that likely includes discussion with RE and 
possibly other consumers in the community (RedHat etc).

>
> ---
>
> Looking at hotspot changes I can't convince myself that the new 
> streamlined "version" variables will capture things like "fastdebug". 
> Will that filter through via configure variables?

The "fastdebug" label has been handled as a less valued token in the 
JEP-223 process. Right now there's no mention of it at all in the 
version string proposal in the JEP. As I understand it, Iris is about to 
propose an amendment which will just make fastdebug be a part of the OPT 
string. I'm not entirely happy with that myself, but that's the way it's 
decided. So wherever you can see the OPT string, you'll see the debug 
level tag.

Currently, however, that's not how it is implemented in this patch. 
Since no decision was made on this (and it's still not formally 
decided), I had to pick a route to go forward. The current patch will 
instead put the "fastdebug" label as part of the PRE string (that's the 
reason for the pre-base and pre-debuglevel arguments to configure). If 
the planned changes goes into the JEP, this'll change to make the 
debuglevel tag a part of the OPT string instead.

> ---
>
> make/*/makefiles/vm.make
>
> Shouldn't the -DVERSION_XX=$(VERSION_XX) definitions be taken from the 
> VERSION_CFLAGS in spec.gmk? (Or are you still allowing for a 
> stand-alone hotspot build?)
The hotspot JEP-223 work initially made by Alejandro kept support for 
stand-alone hotspot builds. I made additional changes on top of that, 
which still tried to cater for stand-alone builds. At this very moment 
I'm not sure if stand-alone hotspot builds are supposed to work or not, 
but I've tried to not change the situation with this patch.

There are two future changes coming down the pipe: One is the additional 
JEP-223 work needed for Hotspot. I know Alejandro had plans for 
redesigning the API between Hotspot and the JDK, so no JDK version 
strings should be compiled into the JVM, but all of it extracted during 
an API in runtime. That would make most (all?) of the makefile changes 
in hotspot irrelevant. However, that implementation is not even started, 
so it's needed for the time being.

The second change is the build-infra hotspot makefile rewrite. When that 
happens, stand-alone builds will definitely disappear, and if Hotspot 
still needs make support for version strings, then the logical choice is 
to use VERSION_CFLAGS.

Ok?

>
> ---
>
> hotspot/src/share/vm/prims/jvm.h
> jdk/src/java.base/share/native/include/jvm.h
>
> I think this comment is way out of date:
>
>  /* Build number is available only for RE build (i.e. JDK_BUILD_NUMBER 
> is set to bNN)
>    * It will be zero for internal builds.
>   */
>
> and can be completely removed. Even if still true, mention of an "RE 
> build" has no place in OpenJDK sources.
Yep, agree. Follow-up patch, right?

>
> ---
>
> hotspot/src/share/vm/runtime/java.cpp
>
> I think a lot of the modified code is obsolete post Hotspot Express - 
> which makes it hard to determine if the changes make sense.
Yep, agree. Follow-up patch, right?
>
> ---
>
> hotspot/src/share/vm/runtime/vmStructs.cpp
>
> Please fix the alignment (3 extra spaces on modified line):
>
> 1239   static_field(Abstract_VM_Version, _vm_minor_version, 
> int)                                   \
> 1240   static_field(Abstract_VM_Version, 
> _vm_security_version,                             int)                  \

I was about to say "follow-up patch", but ugly indentation really sucks 
so I can fix this. Ok if I skip redoing the review for that?

>
> ---
>
> hotspot/test/runtime/6981737/Test6981737.java
>
> This test is really only valid for the new version scheme now, so it 
> should probably be rewritten - and in that case it really isn't 
> testing 6981737 but should be replaced by a new test for the new 
> version format.
Yep, agree. Follow-up patch, right?
>
> ---
>
> jdk/src/java.base/share/classes/sun/misc/Version.java.template
>
> This comment is nonsensical:
>
>       /**
> !      * Returns the security version of the running JVM if it's 1.6 
> or newer
>        * or any RE VM build. It will return 0 if it's an internal 1.5 or
>        * 1.4.x build.
>        * @since 1.6
>        */
>
> as security version does not exist pre 9. Normally you should be 
> adding a new method and deprecating the old one. The new one is @since 9.
Yep, agree. Follow-up patch, right?
>
>      /**
> !      * Returns the security version of the running JDK.
>        * @since 1.6
>        */
>
> Ditto: @since 9 (but again old should be deprecated and new method added)
>
> 253     /**
>  254      * Returns the build number of the running JDK if it's a RE 
> build
>  255      * It will return 0 if it's an internal build.
>
> As with jvm.h this seems obsolete commentary these days - not only RE 
> builds define a build number.
Yep, agree. Follow-up patch, right?

/Magnus

>
> Thanks,
> David