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

[Magnus Ihse Bursie]

Hi Daniel,

Thank you for your thorough review!

On 2015-06-09 01:31, Daniel D. Daugherty wrote:
> > 
> http://cr.openjdk.java.net/~ihse/JDK-8085822-JEP-223-initial-patch/webrev.01 
>
>
> General comment: Not all copyright years were updated.
> General comment: It looks like support for the 'patch' value is not 
> completely
>     implemented through all the Makefiles. I didn't audit for this, 
> but it's
>     just my impression.

You are basically correct. The makefiles all propagate the patch value 
where needed, but the actual source code changes in hotspot and jdk 
ignores the patch field as of now. This will be corrected in a later 
add-on patch, submitted by someone from the jdk and/or hotspot team 
rather than the build team.

>
> common/autoconf/generated-configure.sh
>     There are multiple Copyright notices in this file. Why?
Oh dear, you've reviewed the generated file. :-& I'm really impressed by 
your effort! However, the generated-configure.sh file is automatically 
created by the autoconf framework from the rest of the files in 
common/autoconf/*, so we cannot direcly modify it's contents - only 
indirectly. The reason it's checked in, is that otherwise every user 
would need to generate it before being able to run configure. In build 
reviews, we usually either revert changes to generated-configure.sh 
before posting a review to hide it (and re-generate it before pushing), 
or we just ignore it during reviews. I should have done that, or pointed 
out that it was not needed nor possible to review when I cross-posted. 
I'm sorry.

>
>     L4076: # Verify that a given string represent a valid version 
> number, and assing it to
>     L4077: # a variable.
>         Fixed two typos and reformat a bit:
>           # Verify that a given string represents a valid version 
> number, and
>           # assigning it to a variable.
I'll put that fix in the source .m4 file instead. Thanks.
>
>     L20262:     as_fn_error $? "--with--version-string must have a 
> value" "$LINENO" 5
>         The '--with--version...' part doesn't match previous usages where
>         '--with-version...' is used
Yes, you're right. Erik pointed it out as well. It's a typo in the error 
message. It's all over the place due to copied code. I'll fix it in the 
source .m4 file.

(As a side note, I have a half-finished follow-up patch that will reduce 
the amount of code duplication, but it requires some framework changes 
so it'll have to be a separate thing.)

>
>     L20275:       # Unspecified numerical fields is interpreted as 0.
>         Grammar: 'is interpreted' -> 'are interpreted'
>
>     L20286:         as_fn_error $? "Version string contains + but both 
> 'BUILD' and 'OPT' is missing" "$LINENO" 5
>         Grammar: 'is missing' -> 'are missing'
Those darn English plural forms! Why can't you all do as we sensible 
Swedes and get rid of them? ;-)

(I'll fix)

>
>     L20388:        username=`$ECHO "$USER" | $TR -d -c '[a-z][A-Z][0-9]'`
>         This assumes that the "USER" variable is set. Should there
>         be a check for "" and perhaps use "no_user_specified" or
>         something like that? Perhaps the build setup always makes
>         sure that "USER" is set to something. Don't know.
Hm. I think the worst thing that'll happen is that the username part of 
the opt string gets empty. This part is basically copied right off the 
old build, where we have relied on the $USER variable being present for 
all the time with no problems, so I think it's quite safe to assume.
>
> common/autoconf/jdk-options.m4
>     Don't know why the 'elliptic curve crypto implementation' support
>     is relocated as part of this changeset, but ...
It was incorrectly placed not at the top indentation level, but in the 
middle of the function definition where the old versioning code were 
handled. (Which, mostly by chance, happens to work in autoconf, but is 
really bad style).

> make/Javadoc.gmk
>     Did you mean to remove the 'clean' target?
Yep. Cleaning is done by the top-level Main.gmk makefile nowadays, 
that's just some dead code.

>
> hotspot/make/windows/makefiles/compile.make
>     No changes in the frames view.
>     Update: udiff view shows a blank line deleted at the end of the file.

That's probably the result of some change going forth and then back 
again during development. But then again, removing extra blank linkes is 
not a bad thing. (jcheck unfortunately does not enforce any style checks 
for make files :-( so they tend to detoriate over time.)

>
> hotspot/src/share/vm/runtime/java.cpp
>     L661: void JDK_Version::fully_initialize(
>     L662:     uint8_t major, uint8_t minor, uint8_t security, uint8_t 
> update) {
>     L663:   // This is only called when current is less than 1.6 and 
> we've gotten
>
>         Since you're ripping out vestigial version support, I think this
>         function can go away since this is version 9 and newer. Don't 
> really
>         know for sure, but based on that comment...
It probably can, yes. This and other core changes to the actual 
.cpp/.java files will be addressed in a follow-up patch.
>
> hotspot/src/share/vm/runtime/java.hpp
>     No comments.
>
> hotspot/src/share/vm/runtime/vmStructs.cpp
>     L1240: please make the 'int' parameter align like the rest.
Are you okay with defering such a change to a follow-up patch? It's 
likely the entire struct will need changes to be able to handle a 
theoretically arbitrarily long version number.

>
> hotspot/src/share/vm/runtime/vm_version.cpp
>     L84: void Abstract_VM_Version::initialize() {
>     L85:   // FIXME: Initialization can probably be removed now.
>         I agree, but that would entail also removing the
>         _initialized and the uses of it... Follow on bug fix?
Definitely follow on.

> jdk/src/java.base/share/classes/sun/misc/Version.java.template
>     L149:      * Returns the security version of the running JVM if 
> it's 1.6 or newer
>         This JavaDoc update is wrong, but it might not be important
>         if sun.misc.Version class is going away.
I'm not sure if it's going to be replaced by, or just complemented by 
java.util.Version, but in any case it will need a follow-up patch to 
clean up this and other issues.
> langtools/src/java.compiler/share/classes/javax/lang/model/SourceVersion.java 
>
>     old L171:                 case "1.9":
>     new L171:                 case "9":
>         Should this logic support both versions? Will dropping
>         "1.9" here prevent a pre-version changeset JVM from
>         being dropped into a JDK for triage purposes?
>
>         Granted we don't often triage 'javac' with different JVMs, but...
I'll defer that question to Kumar, who wrote that piece of code. My 
guess is that when Hotspot express was dropped, the ability to use a JVM 
from another JDK release bit rotteded away.

/Magnus