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

Tue Jun 9 13:52:20 UTC 2015

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