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

[Erik Joelsson]

Looks good.

/Erik

On 2015-06-09 15:52, 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/ 
>
>
> And here's a (much simpler) delta webrev which shows just these changes:
> http://cr.openjdk.java.net/~ihse/JDK-8085822-JEP-223-initial-patch-delta-01/webrev.01/ 
>
>
> /Magnus
>
> On 2015-06-09 15:20, Daniel D. Daugherty wrote:
>> On 6/9/15 7:12 AM, Magnus Ihse Bursie wrote:
>>> Hi Daniel,
>>>
>>> Thank you for your thorough review!
>>
>> This was my (failing) attempt at a "fast pass" 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! 
>>
>> Ahhh... now that you say it... it sounds familiar... I may have
>> made this same mistake before. I'll try to remember next time.
>>
>>
>>> 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?
>>
>> Yes.
>>
>>
>>> 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
>>
>> Dan
>