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

[David Holmes]

Hi Magnus,

On 10/06/2015 10:13 PM, Magnus Ihse Bursie wrote:
> 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?

Given this is going to a staging repo I'm okay with deferring things - I 
just have a concern whether such things may be overlooked given that the 
integration with the staging repo will not be undergoing a final review.

I would prefer to see the Version.java.template doc comments corrected, 
even if the issue of whether to add and deprecate is deferred, but again 
as this is to a staging area I can let it slide for now.

I saw the fix to hotspot/src/share/vm/runtime/vmStructs.cpp.

Thanks,
David

>>
>> 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
>