On 4/10/2014 8:11 PM, David Holmes wrote:
On 11/04/2014 1:42 AM, Alejandro E Murillo wrote:
Hi David, thanks for the feedback, see below
On 4/9/2014 8:38 PM, David Holmes wrote:
Hi Alejandro,

Given we have to maintain the JDK version information in two places
(top level repo and hotspot repo) wouldn't it have been simpler to
keep hotspot_version file and HOTSPOT_RELEASE_VERSION and simply set
the major/minor/build values to match those of the JDK version?
The file is still used, just renamed as only jdk info is in there.
HOTSPOT_RELEASE_VERSION was also kept, we just don't get the
major,minor,micro and build number
from it anymore, mainly it  is now set very similar to the
JDK_RELEASE_VERSION (plus other values),
and that format it's not fixed as it used to be for hotspot express.
Those values (major,minor,micro and build number)
are already defined in the makefiles, so we just pass them to
vm_version.cpp, so no more parsing in it.

I still think the overall changes could have been much smaller but okay.
The change actually is not that big, what happens is that due to the makefile structure the exact same change has to be replicated on each platform. Then, some of the changes themselves are actually comment changes, to remove references to the hsx hs version, and/or removal of lines of code that had been removed in some platform (Solaris) but not in
others; also renaming  some macros to better reflect what they are for.
I also fixed some typos on macros names that were incorrect already, like these:
http://cr.openjdk.java.net/~amurillo/9/8030011/make/windows/makefiles/defs.make.wdiff.html

In a nutshell I just added the ability to get major,minor,micro and build number available to vm_version.cpp, in addition to hotspot_release_version, so that we don't have parse that to obtain those. And of course, adding additional testing of those values that is minimal right now. The other changes are related to now having to handle the micro_version, which was ignored with the old hotspot_version, but that has to be done regardless of how the values were obtained.

The change is actually simple, compared to understanding how the makefiles work and how
vm_version receives the values.

Alejandro


Thanks,
David
-----



That aside, in jdk_version file hotspot copyright should now be 2014
will fix that

*/vm.make:

This line is way too long.

! VM_VER_DEFS   = -DHOTSPOT_RELEASE_VERSION="\"$(HS_BUILD_VER)\""
-DJRE_RELEASE_VERSION="\"$(JRE_RELEASE_VER)\""
-DJDK_MAJOR_VERSION="\"$(JDK_MAJOR_VERSION)\""
-DJDK_MINOR_VERSION="\"$(JDK_MINOR_VERSION)\""
-DJDK_MICRO_VERSION="\"$(JDK_MICRO_VERSION)\""
-DJDK_BUILD_NUMBER="\"$(JDK_BUILD_NUMBER)\""
ok

Not clear why we suddenly need defines for all the component pieces
either. You could have kept the HS_XXX variables, just adding the
micro part.
as mentioned above,  the micro part was actually added to
HOTSPOT_RELEASE_VERSION,
we just don't get those values by parsing it, so we just pass those
values to the vm_version.cpp,
since they are already defined in the makefiles. The format of the JDK
version is not that fixed.

Thanks
Alejandro

David


On 10/04/2014 10:15 AM, Alejandro E Murillo 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.

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.

Release Engineering did some testing and I also ran several cases with
full and just hotspot JPRT builds.

Here is a summary of how the new output compares to the old one:

(1)  Release Engineering builds (9-dev):

from jdk9-dev build:
java version "1.9.0-ea"
Java(TM) SE Runtime Environment (build
1.9.0-ea-langtools-nightly-h257-20140328-b07-b00)
Java HotSpot(TM) 64-Bit Server VM (build
1.9.0-ea-langtools-nightly-h257-20140328-b07-b00, mixed mode)

compared with what we currently have
java version "1.9.0-ea"
Java(TM) SE Runtime Environment (build
1.9.0-ea-langtools-nightly-h247-20140326-b06-b00)
Java HotSpot(TM) 64-Bit Server VM (build 25.0-b62, mixed mode)

(2)  Release Engineering builds (jdk9):

java version "1.9.0-ea"
Java(TM) SE Runtime Environment (build 1.9.0-ea-b07)
Java HotSpot(TM) Server VM (build 1.9.0-ea-b07, mixed mode)

compared with what we currently have
java version "1.9.0-ea"
Java(TM) SE Runtime Environment (build 1.9.0-ea-b07)
Java HotSpot(TM) 64-Bit Server VM (build 25.0-b62, mixed mode)

(3) JPRT Full builds:

java version "1.9.0-internal"
#   Java(TM) SE Runtime Environment (build
1.9.0-internal-201404091627.amurillo.jdk9-hs-ver-str-co-b00)
#   Java HotSpot(TM) Server VM (build
1.9.0-internal-201404091627.amurillo.jdk9-hs-ver-str-co-b00, mixed mode)


(4) JPRT hotspot only builds:

java version "1.9.0-ea-fastdebug"
#   Java(TM) SE Runtime Environment (build 1.9.0-ea-fastdebug-b06)
#   Java HotSpot(TM) Server VM (build
1.9.0-internal-201404031820.amurillo.jdk9-hs-ver-str-HS-fastdebug, mixed
mode)


in this one the built VM is inserted into a promoted build bundle,
since we do not have the JDK build number info in the hotspot repo,
we can't match the build number in the JDK portion.
With the RFE mentioned above, we can extract the build info from the
repo
and add it to the hotspot portion.

I want to mention, that this may change once the new JDK version change
is implemented
but we don't know when that will be implemented yet, so we need to go
ahead with this to
get rid of the old hotspot express output. And most of these changes
will still have to be done
anyways

BTW, john Coomes and Dan Daugherty provided feedback in some pieces of
this webrev,
but I need full reviews now.

Thanks



--
Alejandro

Reply via email to