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
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.
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"));
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.
Use corresponding test in jdk for testing of these changes:
jdk/test/sun/misc/Version/Version.java
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.
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>]
You did not show default VM version example when VM is built locally by
engineer.
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'
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.
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 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)
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.muri...@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
