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

[Daniel D. Daugherty]

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