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

David Holmes david.holmes at oracle.com
Thu Jun 11 04:52:53 UTC 2015

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 DEFAULTVERSIONXXX 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 -DVERSIONXX=$(VERSIONXX) definitions be taken from the VERSIONCFLAGS 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 VERSIONCFLAGS. 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. JDKBUILDNUMBER 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 staticfield(AbstractVMVersion, vmminorversion, _int) _ 1240 staticfield(AbstractVMVersion, _vmsecurityversion, 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

More information about the hotspot-dev mailing list