RFR 8030011: Update Hotspot version string output

[Alejandro E Murillo]

On 4/21/2014 1:18 PM, Vladimir Kozlov wrote:
> Hi Alejandro,
>
> I don't think we need to rename make/hotspot_version file. It is still 
> used to set JVM's version string and not JDK's version.

>
> Next should be 2014 (I think David pointed it too but there is no new 
> webrev): HOTSPOT_VM_COPYRIGHT=Copyright 2013
correct. I haven't changed that yet
>
> If you pass major, micro etc numbers to avoid parsing you need to 
> verify that constructed from them string is equal to passed 
> HOTSPOT_RELEASE_VERSION.
yes, there's a test for that, which is run when I submit a full jprt job.
>
> Next assert message is not consistent with previous messages which use 
> "vm", I think it should be "vm" here too:
>
>   DEBUG_ONLY(assert_digits(vm_build_num, offset, "wrong JDK build 
> number"));
we do not have hotspot build number anymore, so I think we should not 
mention hotspot build numberls

>
> Abstract_VM_Version::jvm_version() should include micro version. See 
> JVM_GetVersionInfo() in jvm.cpp and jvm_version_info in 
> jdk/src/share/javavm/export/jvm.h.
I added that here, is that what you are referring?
http://cr.openjdk.java.net/~amurillo/9/8030011/src/share/vm/runtime/vmStructs.cpp.udiff.html
http://cr.openjdk.java.net/~amurillo/9/8030011/src/share/vm/runtime/vm_version.hpp.udiff.html
>
> Use corresponding test in jdk for testing of these changes:
>
> jdk/test/sun/misc/Version/Version.java
yeah, I run that test as part of jprt full builds,
That test handles both JDK and Hotspot express versions
>
> jvm.h: Next comment is not accurate:
>
> +    /* VM version string: JDK version string     */
>
> If we build VM separately (for example, in JPRT) VM version will not 
> be JDK version in which VM is installed. It will take numbers either 
> from passed make parameters or from make/hotspot_version. I think it 
> should say:
>
> +    /* VM version string follows the JDK release version naming 
> convention
> +     * <major>.<minor>.<micro>-bxx[-<identifier>][-<debug_flavor>]
>
> Based on your examples [-<identifier>][-<debug_flavor>] is still used 
> so it should be reflected in the comment.
Let me make that explicit.
>
> Don't remove next comments from vm_version.cpp but fix it ("follow the 
> JDK release"):
>
> -// HOTSPOT_RELEASE_VERSION must follow the release version naming 
> convention
> -// <major_ver>.<minor_ver>-b<nn>[-<identifier>][-<debug_target>]
ok
>
> You did not show default VM version example when VM is built locally 
> by engineer.
It will be similar to the hotspot only jprt build. There will be a mismatch,
I tested by dropping the resulting libjvm  into a promoted build, so it 
looks like this:

java version "1.9.0-ea"
Java(TM) SE Runtime Environment (build 1.9.0-ea-b01)
Java HotSpot(TM) 64-Bit Server VM (build 1.9.0-internal-debug, mixed mode)

>
> Please test that correct version string is constructed when you build 
> VM using make/build.sh, for example 'sh make/build.sh debug LP64=1'
Haven't tested that, thanks for pointing that out.
>
> Next comment change in buildtree.make is not correct because 
> HOTSPOT_RELEASE_VERSION make parameter does not include 
> HOTSPOT_BUILD_VERSION:
>
> -# HOTSPOT_RELEASE_VERSION - <major>.<minor>-b<nn> (11.0-b07)
> +# HOTSPOT_RELEASE_VERSION - JRE_RELEASE_VERSION plus 
> HOTSPOT_BUILD_VERSION
>
> see JPRT logs where HOTSPOT_BUILD_VERSION is set separately.
let me check that again
>
> I think next change in make/defs.make is not safe (removing make 
> parameter) due to complexity of our builds:
>
> -MAKE_ARGS += HOTSPOT_RELEASE_VERSION=$(HOTSPOT_RELEASE_VERSION)
I checked the code, and HOTSPOT_RELEASE_VERSION is only used in 
vm_version.cpp,
so I think is fine to remove it. Note that if we keep it there, since 
the JDK version string
sometimes might have time stamps, it may affect ccache,  that's why at 
some point they
separated the options for vm_version.cpp from the options for other 
components. See this
comment on vm,make:

# This is VERY important! The version define must only be supplied to vm_version.o
# If not, ccache will not re-use the cache at all, since the version string might contain
# a time and date.


>
>
> I know that windows build is mess. Please verify it carefully. For 
> example, you changed names JDK_*_VER to JDK_*_VERSION in def.make but 
> build.make uses them:
>
> JDK_VER=$(JDK_MINOR_VER),$(JDK_MICRO_VER),$(JDK_UPDATE_VER),$(JDK_BUILD_NUMBER) 
>
That was a typo. Note that I changed the left handside, so they were 
incorrectly reassigning those.
The places were JDK_MINOR_VER is used, the value is read from 
jdk_version (formerly hotspot_version)

Thanks a lot for the feedback!
Alejandro
>
>
> Regards,
> Vladimir
>
> On 4/21/14 10:13 AM, Alejandro E Murillo wrote:
>>
>> On 4/18/2014 6:50 PM, John Coomes wrote:
>>> Alejandro E Murillo (alejandro.murillo at oracle.com) wrote:
>>>> Please review this change to make the hotspot related output 
>>>> produced by
>>>> "java -version"
>>>> match the corresponding JDK output:
>>>>
>>>> webrev: http://cr.openjdk.java.net/~amurillo/9/8030011/
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8030011
>>>>
>>>> Note that we initially wanted to obtain more information from the repo
>>>> being built and add
>>>> it to the hotspot version output, but that will require changes in the
>>>> JPRT, so
>>>> we have decided to split the change in 2 bugs. One to make the
>>>> version match
>>>> (above webrev) and another one, an RFE, to add additional information
>>>> extracted from the repo.
>>> Generally looks good, but I agree with David that the long lines
>>> should be wrapped, and it might even be clearer to separate them into
>>> two variables, e.g.,
>>>
>>> JDK_VERSION_DEFS = -DJDK_MAJOR_VERSION="\"$(JDK_MAJOR_VERSION)\"" \
>>>            -DJDK_MINOR_VERSION="\"$(JDK_MINOR_VERSION)\"" \
>>>            ... other defs ...
>>> VERSION_DEFS  = -DHOTSPOT_RELEASE_VERSION="\"$(HS_BUILD_VER)\"" \
>>>         -DJRE_RELEASE_VERSION="\"$(JRE_RELEASE_VER)\"" \
>>>         $(JDK_VERSION_DEFS)
>>>
>>> Also the change to this part of vm.make differs between at least the
>>> solaris version and the linux version (didn't check bsd or windows).
>>> Please make them all consistent.
>>>
>>>> Note that in the current version of vm_version.cpp, there is no error
>>>> checking in product mode,
>>>> I was suggested to make that explicit.
>>> I like the additional error checking.  I think you could eliminate the
>>> 4 copies of similar code with a function that does the checks and sets
>>> the field, e.g.,
>>>
>>>      void set_version_field(int * version_field, const char *
>>> version_str,
>>>                const char * const assert_msg) {
>>>         if (version_str != NULL ...) {
>>>      DEBUG_ONLY(assert_digits(version_str, assert_msg));
>>>      *version_field = atoi(version_str);
>>>         }
>>>      }
>>>
>>> -John
>> Thanks John,
>> working on adding these changes and sanity testing
>>
>> Thanks
>>

-- 
Alejandro