Hi, Alan. Thanks for looking.

Is jdk.OracleVersion really intended to be pushed to OpenJDK?

That class should be available to users of the Oracle JDK. It is not applicable to OpenJDK.

Should it be final?

Yes.

There are few imports at the top that don't appear to be used.

Oops. Thought I cleaned those up. Here's the diff for both of these changes to OracleVersion: 28,33d27 < import java.util.regex.Matcher; < import java.util.regex.Pattern; < import java.util.ArrayList; < import java.util.List; < import java.util.Optional; < 86c80 < public class OracleVersion extends Version --- > public final class OracleVersion extends Version

jdk.Version looks good. Has there been any thought/suggestions yet on which module this API will move too?

No. My best guess is jdk.dev for Version but I'm willing to take suggestions. I'd prefer to keep Version and OracleVersion in the same module, but if they need to be split, I'll need to change the access modifier for the Version constructor.

I suspect the docs build will need to be looked at.

Definitely. I was surprised (but pleased) that javadoc ran on the new files, but the actual bundle surprised me. It is not correct for either jdk.Exported or these classes. I was unable to find a more suitable existing javadoc bundle. Is there one? Regards, iris -----Original Message----- From: Alan Bateman Sent: Wednesday, November 25, 2015 4:02 AM To: Iris Clark; core-libs-dev@openjdk.java.net Cc: verona-dev@openjdk.java.net Subject: Re: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion On 25/11/2015 01:54, Iris Clark wrote:

Hi.

Please review the new classes jdk.Version and jdk.OracleVersion. These are simple Java APIs to parse, validate, and compare version numbers.

Is jdk.OracleVersion really intended to be pushed to OpenJDK? Should it be final? There are few imports at the top that don't appear to be used. jdk.Version looks good. Has there been any thought/suggestions yet on which module this API will move too? I suspect the docs build will need to be looked at. The javadoc for jdk.Exported is currently generated in the javac tree javadoc for some reason (maybe because that annotation started out in the langtools repo). -Alan

Hi, Mandy. Thanks for looking at this.

We will need to find a home for this jdk.* API since java.base only exports Java SE API per the design principles in JEP 200 The Modular JDK. It’s fine to file an issue to follow up this.

I've filed this bug and assigned it to myself: 8144062: Determine appropriate module for jdk.Version and jdk.OracleVersion https://bugs.openjdk.java.net/browse/JDK-8144062

You will also need to find a docs page to publish this new API as well (the current way is a page linked from the layer cake and this may be changed).

Yes, I'm aware of these discussions. Since I suspect that they won't be resolved within the next week or so, I've filed this bug: 8144069: Determine correct publication for jdk.Version and jdk.OracleVersion APIs https://bugs.openjdk.java.net/browse/JDK-8144069 Regards, iris -----Original Message----- From: Mandy Chung Sent: Wednesday, November 25, 2015 8:48 AM To: Iris Clark Cc: core-libs-dev@openjdk.java.net; verona-dev@openjdk.java.net Subject: Re: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion

On Nov 24, 2015, at 5:54 PM, Iris Clark <iris.clark@oracle.com> wrote:

Hi.

Please review the new classes jdk.Version and jdk.OracleVersion. These are simple Java APIs to parse, validate, and compare version numbers.

Webrev

http://cr.openjdk.java.net/~iris/verona/8072379/webrev.1/

This looks good to me. Alan raises a good question whether jdk.OracleVersion is intended to be pushed to OpenJDK. We will need to find a home for this jdk.* API since java.base only exports Java SE API per the design principles in JEP 200 The Modular JDK. It’s fine to file an issue to follow up this. W.r.t. the docs build, jdk is listed in top/make/common/NON_CORE_PKGS.gmk but there is no target in make/Javadoc.gmk building it. You will also need to find a docs page to publish this new API as well (the current way is a page linked from the layer cake and this may be changed). Mandy

PS If the concepts the two classes Version and OracleVersion are trying to capture is a "Vendor-Version" then perhaps that can be surfaced more directly in the API. That is, if the basic notion is to interpret a version string in a way appropriate to and specialized for a given vendor of the JDK (a la the java.vendor system property), then perhaps a type like // API sketch public final class VendorVersion { public VendorVersion(String vendor, Version version, Comparator<Version> versionComp>) { ...} @Override public boolean equals(VendorVersion vv) { // Usual instance of checks return Objects.equals(vendor, vv.vendor()) && versionComp.version(), vv.version()); } int compareVersion(VendorVersion vv) { if (!vendor.equals(vv.vendor())) // throw an exception return versionComp(version, vv.version); } // ... } might serve the purposes at hand. HTH, -Joe On 11/25/2015 1:31 PM, joe darcy wrote:

Hello,

On 11/25/2015 8:48 AM, Mandy Chung wrote:

...

...

On Nov 24, 2015, at 5:54 PM, Iris Clark <iris.clark@oracle.com> wrote:

Hi.

Please review the new classes jdk.Version and jdk.OracleVersion. These are simple Java APIs to parse, validate, and compare version numbers.

Webrev

http://cr.openjdk.java.net/~irisa/verona/8072379/webrev.1/ This looks good to me. Alan raises a good question whether jdk.OracleVersion is intended to be pushed to OpenJDK.

I share the concerns that have been raised about the naming and placement of a class named "OracleVersion".

Is the intention that downstream JDK distributions, such as IcedTea, whether based on OpenJDK or otherwise, would provide their own specialization of the jdk.Version class?

If so, should there be some kind of provider interface to find system's the version interpreter?

For example, what class, if any would be expected to provide detailed analysis of the version string emitted by, say, a OpenJDK build used as the reference implementation of Java SE 9?

The equals / hashCode behavior of Version and OracleVersion is bit surprising and I think somewhat underspecified given the possibility of defining subclasses.

The (overridable) equals method in Version says "Two Versions are equal if and only if they represent the same version string." If that is interpreted to mean the Versions are equal if their toString output is equal, the toString spec says "Returns a string representation of this version." and the (overridable) implementation in Version looks at versions, pre, build, and optional.

Since OracleVersion does not override equals, a plain Version object and an OracleVersion object can compare as equal. Additionally, a VersionWithDifferentToStringOutput("1.2.3") object which overrides toString would not be semantically equivalent to a Version("1.2.3") object. Worse, there could be a non-communicability behavior in the equals method since

(new Version("1.2.3")).equals(new VersionWithDifferentToStringOutput("1.2.3"))

could return "true" while

(new VersionWithDifferentToStringOutput("1.2.3")).equals(new Version("1.2.3"))

could easily and legitimately return "false" by the specification.

How is this API supposed to behave if the component version strings have a numerical value greater than Integer.MAX_VALUE?

Was using Longs to record numerical versions rather than Integers considered?

Thanks,

-Joe

Hi, Joe. I've filed the following JDK 9 RFE to provide this functionality: 8145794: Provide Verona interpretation of historical JDK version strings https://bugs.openjdk.java.net/browse/JDK-8145794 Thanks, iris -----Original Message----- From: joe darcy Sent: Sunday, November 29, 2015 4:18 PM To: Mandy Chung; Iris Clark Cc: verona-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion PPS Perhaps this is already planned as future work, but it might be a kindness to those analyzing JDK version strings if there was a class that did a "best effort" at understanding Sun and Oracle JDK version strings pre-Verona to those post-Verona. In other words, a version class where new JdkVersion("1.8.0_25") would have major() == 8 and minor() == 25, but have the specified meaning for strings for Verona strings where the first group was numerically 9 or larger. Cheers, -Joe

Hi, Joe. Thanks for the review comments.

...

...

http://cr.openjdk.java.net/~irisa/verona/8072379/webrev.1/

Is the intention that downstream JDK distributions, such as IcedTea, whether based on OpenJDK or otherwise, would provide their own specialization of the jdk.Version class?

No. Downstream users may provide their own specialization, but there is no requirement that they must do so. jdk.OracleVersion has been removed. The functionality it provided, access to the fourth element of the version number, is trivially accessed by existing methods in jdk.Version. I've filed the following bug to address the more general use case: 8145793: Provide vendor-specific interpretation of a JDK version string https://bugs.openjdk.java.net/browse/JDK-8145793

The equals / hashCode behavior of Version and OracleVersion is bit surprising and I think somewhat underspecified given the possibility of defining subclasses.

Given the above regarding downstream use, I've make jdk.Version final and the constructor is now private. Oracle or any other JDK distribution wishing to impose additional semantics to the version string interpretation will need to do so via encapsulation rather than inheritance.

How is this API supposed to behave if the component version strings have a numerical value greater than Integer.MAX_VALUE?

From the specification of the only instance method, Version.parse():

* @throws NumberFormatException * If an element of the version number or the build number cannot * be represented as an {@link Integer}

Was using Longs to record numerical versions rather than Integers considered?

Yes. In an informal poll conducted during implementation, it was thought that 2^32 bits would be more than sufficient to represent the components of typical version numbers. An updated webrev is forthcoming. Thanks, iris PS: I believe that we are both out the week of 21 December, so I don't expect to hear back from you until the New Year.

On 11/01/2016 21:44, Iris Clark wrote:

Hi, Joe, Roger, Alan, Magnus, and Mandy.

At the end of December (shortly before the Christmas/Winter break and my vacation), I provided responses to your messages and an updated webrev:

http://cr.openjdk.java.net/~iris/verona/8072379/webrev.2/

I didn't hear from anybody, so I'd like to optimistically assume that you were satisfied. Is that correct?

For you convenience, here's a reference to the December and November threads:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-December/037062.ht... http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036904.ht...

I'd like to wrap up this work for the initial implementation of jdk.Version soon.

I think this looks good but we'll to decide which module to put this in. It can't be java.base (see design principles in JEP 200). If it's going into java.base temporarily then the top-level modules.xml will need to be updated to export the "jdk" package. -Alan

Dear Iris,

The JavaDoc and JEP are both slightly wrong in different ways. The implementation is correct. Note that it enforces additional constraints beyond just the regular expression.

It would be great if those additional constraints are also documented outside the code (JavaDoc or JEP). Also, please update the JavaDoc at line 48, 113, optionally line 130 of Version.java, plus the comment at line 160 of Version.java to match the JEP, since you confirm "9-pre-opt" is valid. Best regards, Hong Dai Thanh. -----Original Message----- From: Iris Clark [mailto:iris.clark@oracle.com] Sent: Wednesday, 3 February, 2016 6:16 AM To: Thanh Hong Dai <hdthanh@tma.com.vn>; Alan Bateman <alan.bateman@oracle.com>; core-libs-dev@openjdk.java.net Subject: RE: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion Hi. Thanks again for looking at this change.

The JEP & implementation allows -$OPT to be specified without +, but the Javadoc one does not allow that. For example, "9-pre-opt" is allowed by JEP, but disallowed by Javadoc.

The JavaDoc and JEP are both slightly wrong in different ways. The implementation is correct. Note that it enforces additional constraints beyond just the regular expression. I've added a few more regression tests to verify that the behavior is as expected. This is the updated webrev and javadoc: http://cr.openjdk.java.net/~iris/verona/8072379/webrev.4/ http://cr.openjdk.java.net/~iris/verona/8072379/doc.4/jdk/Version.html (unchanged from doc.3)

Understood, but I didn't see any part of the JEP or the Javadoc explaining that + is needed to make the parser recognize the text followed as options instead of pre-release identifier. It would be great if that is added.

I've created a new bug to cover updating both the javadoc and JEP for this case. https://bugs.openjdk.java.net/browse/JDK-8148877 JDK-8148877: (spec) Specify when an empty '+' is required in a version string This is the key sentence in the description: '+' is required for empty build when OPT is present and PRE is not. "9+-opt" and "9-pre-opt" are legal. "9+" is not. Regards, iris -----Original Message----- From: Thanh Hong Dai [mailto:hdthanh@tma.com.vn] Sent: Monday, February 01, 2016 11:48 PM To: Iris Clark; Alan Bateman; core-libs-dev@openjdk.java.net Subject: RE: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion Dear Iris, On closer look, there seems to be some conflicting definition of version string. In JEP: http://openjdk.java.net/jeps/223 $VNUM(-$PRE)?(\+$BUILD)?(-$OPT)? In Javadoc: http://cr.openjdk.java.net/~iris/verona/8072379/doc.3/jdk/Version.html $VNUM(-$PRE)?(\+($BUILD)?(-$OPT)?)? In implementation: The regex follows JEP's definition. The JEP & implementation allows -$OPT to be specified without +, but the Javadoc one does not allow that. For example, "9-pre-opt" is allowed by JEP, but disallowed by Javadoc.

I need to capture the plus to distinguish between cases where an empty build is allowed (e.g. "9+-foo") and when it is not ("9+"). See code in Version.java, line 226-230 and in Basic.java, line 98, 107-109. (Note that we use the empty "+" to distinguish "9-foo" from "9+-foo".)

Understood, but I didn't see any part of the JEP or the Javadoc explaining that + is needed to make the parser recognize the text followed as options instead of pre-release identifier. It would be great if that is added. Best regards, Hong Dai Thanh.

On 13/01/2016 21:54, Iris Clark wrote:

: This diff has been applied to modules.xml: This looks okay.

:

When this came up earlier, I filed this bug to track finding a more appropriate module for jdk.Version:

8144062: Determine appropriate module for jdk.Version https://bugs.openjdk.java.net/browse/JDK-8144062

Thanks, I've changed this to be a P2 as we will need to decide on this soon. -Alan.

Hi, Mandy. Thanks for looking at this webrev again.

273 current = parse(System.getProperty("java.version"));

System.getProperty will do a permission check and it needs to be called within a doPrivileged block.

Nice catch! I've added the permission check and the associated @throws.

154 * @see <a href="http://openjdk.java.net/jeps/223">JEP 223: New Version-String Scheme</a>

Does the javadoc have the essential information from this JEP? Wonder if this @see is necessary.

I thought it might be nice to include the JEP reference as it contains additional data about versioning beyond the JavaDoc for this method; however, it's not critical to the understanding or use of this class. For some reason I'd thought that there was precedence in including JEP numbers as @see references, but I find no evidence of that. I've removed the @see. These are the diffs to address both of your comments: $ diff Version.java_save Version.java 28a29,30

import java.security.AccessController; import java.security.PrivilegedAction; 154,155d155 < * @see <a href="http://openjdk.java.net/jeps/223">JEP 223: New Version-String Scheme</a> < * 268a269,274 * @throws SecurityException * If a security manager exists and its {@link * SecurityManager#checkPropertyAccess(String) * checkPropertyAccess} method does not allow access to the * system property "java.version" * 272,273c278,285 < if (current == null) < current = parse(System.getProperty("java.version"));

if (current == null) { current = parse(AccessController.doPrivileged( new PrivilegedAction<>() { public String run() { return System.getProperty("java.version"); } })); }

Regards, iris -----Original Message----- From: Mandy Chung Sent: Tuesday, January 12, 2016 2:26 PM To: Iris Clark Cc: Mandy Chung; Joe Darcy; Magnus Ihse Bursie; Roger Riggs; Alan Bateman; core-libs-dev; verona-dev@openjdk.java.net Subject: Re: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion

On Jan 11, 2016, at 1:44 PM, Iris Clark <iris.clark@oracle.com> wrote:

Hi, Joe, Roger, Alan, Magnus, and Mandy.

At the end of December (shortly before the Christmas/Winter break and my vacation), I provided responses to your messages and an updated webrev:

http://cr.openjdk.java.net/~iris/verona/8072379/webrev.2/

It’s good to see that jdk.OracleVersion has been removed and use jdk.Version to obtain the fourth element of the version number. This patch looks good in general. Minor comment: 273 current = parse(System.getProperty("java.version")); System.getProperty will do a permission check and it needs to be called within a doPrivileged block. 154 * @see <a href="http://openjdk.java.net/jeps/223">JEP 223: New Version-String Scheme</a> Does the javadoc have the essential information from this JEP? Wonder if this @see is necessary. Alan already comment on the “jdk” package that needs to find the proper module to export it (that’s a future RFE) and modules.xml should be updated. Mandy

Hi Iris, Catching up on old reviews, the regular expression for versions is given as ^[1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*$ Is this equivalent to ^[1-9][0-9]*(\.[0-9])* Trying to put this in words, "The version number starts with a sequence of digits where the leading digit is not "0". Afterward, there are zero or more period separated groups of digits with no restrictions on the digits. Note that this implies The version number ends with a digit group." I suggest changing the class-level javadoc discussing comparisons to defer to the various compare methods. It would be helpful to not which compare methods are consistent with equals and which are not. Thanks, -Joe On 1/11/2016 1:44 PM, Iris Clark wrote:

Hi, Joe, Roger, Alan, Magnus, and Mandy.

At the end of December (shortly before the Christmas/Winter break and my vacation), I provided responses to your messages and an updated webrev:

http://cr.openjdk.java.net/~iris/verona/8072379/webrev.2/

I didn't hear from anybody, so I'd like to optimistically assume that you were satisfied. Is that correct?

For you convenience, here's a reference to the December and November threads:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-December/037062.ht... http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036904.ht...

I'd like to wrap up this work for the initial implementation of jdk.Version soon.

Regards, iris

-----Original Message----- From: Iris Clark Sent: Friday, December 18, 2015 1:55 PM To: Joe Darcy; Mandy Chung Cc: core-libs-dev@openjdk.java.net; verona-dev@openjdk.java.net Subject: RE: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion

Hi, Joe.

Thanks for the review comments.

...

...

...

http://cr.openjdk.java.net/~irisa/verona/8072379/webrev.1/

Is the intention that downstream JDK distributions, such as IcedTea, whether based on OpenJDK or otherwise, would provide their own specialization of the jdk.Version class? No. Downstream users may provide their own specialization, but there is no requirement that they must do so.

jdk.OracleVersion has been removed. The functionality it provided, access to the fourth element of the version number, is trivially accessed by existing methods in jdk.Version.

I've filed the following bug to address the more general use case:

8145793: Provide vendor-specific interpretation of a JDK version string https://bugs.openjdk.java.net/browse/JDK-8145793

...

The equals / hashCode behavior of Version and OracleVersion is bit surprising and I think somewhat underspecified given the possibility of defining subclasses. Given the above regarding downstream use, I've make jdk.Version final and the constructor is now private. Oracle or any other JDK distribution wishing to impose additional semantics to the version string interpretation will need to do so via encapsulation rather than inheritance.

...

How is this API supposed to behave if the component version strings have a numerical value greater than Integer.MAX_VALUE? From the specification of the only instance method, Version.parse():

* @throws NumberFormatException * If an element of the version number or the build number cannot * be represented as an {@link Integer}

...

Was using Longs to record numerical versions rather than Integers considered? Yes. In an informal poll conducted during implementation, it was thought that 2^32 bits would be more than sufficient to represent the components of typical version numbers.

An updated webrev is forthcoming.

Thanks, iris

PS: I believe that we are both out the week of 21 December, so I don't expect to hear back from you until the New Year.

Hi Daniel, On 1/13/2016 1:01 AM, Daniel Fuchs wrote:

Hi Joe,

On 1/13/16 2:06 AM, Joseph D. Darcy wrote:

...

Hi Iris,

Catching up on old reviews, the regular expression for versions is given as

^[1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*$

Is this equivalent to

^[1-9][0-9]*(\.[0-9])*

Iris's regexp will disallow trailing .0

9.0.1 will match, but 9.0 and 9.0.0 will not - the version should be plain 9 in those latter cases as documented in the javadoc.

I believe this is important because it ensures that the version string is always in canonical form, which makes it possible to simply compare the 'versions' list (List<Integer>) within the various variations of equals...

So the regexp matching will eliminate non canonical forms without having to perform further checks down the road. Took me a while to figure it, I had to look at the test to pinpoint it. Now that I understand, I find it quite elegant :-)

Ah okay, I missed those points when reading over the regular expression. I suggest some discussion be added to explain and document that those aspects of the design are intentional. Thanks, -Joe

Hi Iris, Do you consider an option to let community reuse JDK versioning style for their own purposes. Probably defining an interface with basic default methods which can be extended by various libraries to provide unified way to gather version information from MANIFEST.MF, ClassLoader's jars and simply to allow developers inventing one wheel lesser.   /Victor --- Original message --- From: "Iris Clark" <iris.clark@oracle.com> Date: 11 January 2016, 23:45:56

Hi, Joe, Roger, Alan, Magnus, and Mandy.

At the end of December (shortly before the Christmas/Winter break and my vacation), I provided responses to your messages and an updated webrev:

http://cr.openjdk.java.net/~iris/verona/8072379/webrev.2/

I didn't hear from anybody, so I'd like to optimistically assume that you were satisfied. Is that correct?

For you convenience, here's a reference to the December and November threads:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-December/037062.ht... http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036904.ht...

I'd like to wrap up this work for the initial implementation of jdk.Version soon.

Regards, iris

Hi, Magnus. Thanks for taking the time to look at this again.

I noted that your java source file had a lot of initial tabs instead of spaces. I'm not sure if we have any code guidelines about this,

jcheck prevents tabs in comments and source. I remove them, one of my editors adds them when I make changes. It's annoying. They won't be in the checked in source. Regards, iris -----Original Message----- From: Magnus Ihse Bursie Sent: Thursday, January 14, 2016 3:38 AM To: Iris Clark; Joe Darcy; Mandy Chung; Roger Riggs; Alan Bateman Cc: core-libs-dev@openjdk.java.net; verona-dev@openjdk.java.net Subject: Re: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion On 2016-01-11 22:44, Iris Clark wrote:

Hi, Joe, Roger, Alan, Magnus, and Mandy.

At the end of December (shortly before the Christmas/Winter break and my vacation), I provided responses to your messages and an updated webrev:

http://cr.openjdk.java.net/~iris/verona/8072379/webrev.2/

I didn't hear from anybody, so I'd like to optimistically assume that you were satisfied. Is that correct?

I'm basically happy. As other reviewers, I think the regexes are a bit tough to parse (regexes always are), and some additional comments on the effect would be appreciated. Thanh Hong Dai had some good suggestions as well on how to improve readability. I noted that your java source file had a lot of initial tabs instead of spaces. I'm not sure if we have any code guidelines about this, but I have not encountered that in the JDK source base before, so I'd recommend you turn them into spaces. /Magnus

For you convenience, here's a reference to the December and November threads:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-December/037062.ht...

http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036 904.html

I'd like to wrap up this work for the initial implementation of jdk.Version soon.

Regards, iris

-----Original Message----- From: Iris Clark Sent: Friday, December 18, 2015 1:55 PM To: Joe Darcy; Mandy Chung Cc: core-libs-dev@openjdk.java.net; verona-dev@openjdk.java.net Subject: RE: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion

Hi, Joe.

Thanks for the review comments.

...

...

...

http://cr.openjdk.java.net/~irisa/verona/8072379/webrev.1/

Is the intention that downstream JDK distributions, such as IcedTea, whether based on OpenJDK or otherwise, would provide their own specialization of the jdk.Version class? No. Downstream users may provide their own specialization, but there is no requirement that they must do so.

jdk.OracleVersion has been removed. The functionality it provided, access to the fourth element of the version number, is trivially accessed by existing methods in jdk.Version.

I've filed the following bug to address the more general use case:

8145793: Provide vendor-specific interpretation of a JDK version string https://bugs.openjdk.java.net/browse/JDK-8145793

...

The equals / hashCode behavior of Version and OracleVersion is bit surprising and I think somewhat underspecified given the possibility of defining subclasses. Given the above regarding downstream use, I've make jdk.Version final and the constructor is now private. Oracle or any other JDK distribution wishing to impose additional semantics to the version string interpretation will need to do so via encapsulation rather than inheritance.

...

How is this API supposed to behave if the component version strings have a numerical value greater than Integer.MAX_VALUE? From the specification of the only instance method, Version.parse():

* @throws NumberFormatException * If an element of the version number or the build number cannot * be represented as an {@link Integer}

...

Was using Longs to record numerical versions rather than Integers considered? Yes. In an informal poll conducted during implementation, it was thought that 2^32 bits would be more than sufficient to represent the components of typical version numbers.

An updated webrev is forthcoming.

Thanks, iris

PS: I believe that we are both out the week of 21 December, so I don't expect to hear back from you until the New Year.

Sure thing. Glad to see this going in. Mandy

On Feb 2, 2016, at 7:17 PM, Iris Clark <iris.clark@oracle.com> wrote:

Hi, Mandy.

Thanks so much for pushing the changeset for the initial implementation of jdk.Version!

Regards, iris

-----Original Message----- From: Iris Clark Sent: Monday, January 11, 2016 1:45 PM To: Iris Clark; Joe Darcy; Mandy Chung; Magnus Ihse Bursie; Roger Riggs; Alan Bateman Cc: core-libs-dev@openjdk.java.net; verona-dev@openjdk.java.net Subject: RE: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion

Hi, Joe, Roger, Alan, Magnus, and Mandy.

At the end of December (shortly before the Christmas/Winter break and my vacation), I provided responses to your messages and an updated webrev:

http://cr.openjdk.java.net/~iris/verona/8072379/webrev.2/

I didn't hear from anybody, so I'd like to optimistically assume that you were satisfied. Is that correct?

For you convenience, here's a reference to the December and November threads:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-December/037062.ht... http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036904.ht...

I'd like to wrap up this work for the initial implementation of jdk.Version soon.

Regards, iris

-----Original Message----- From: Iris Clark Sent: Friday, December 18, 2015 1:55 PM To: Joe Darcy; Mandy Chung Cc: core-libs-dev@openjdk.java.net; verona-dev@openjdk.java.net Subject: RE: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion

Hi, Joe.

Thanks for the review comments.

...

...

...

http://cr.openjdk.java.net/~irisa/verona/8072379/webrev.1/

...

Is the intention that downstream JDK distributions, such as IcedTea, whether based on OpenJDK or otherwise, would provide their own specialization of the jdk.Version class?

No. Downstream users may provide their own specialization, but there is no requirement that they must do so.

jdk.OracleVersion has been removed. The functionality it provided, access to the fourth element of the version number, is trivially accessed by existing methods in jdk.Version.

I've filed the following bug to address the more general use case:

8145793: Provide vendor-specific interpretation of a JDK version string https://bugs.openjdk.java.net/browse/JDK-8145793

...

The equals / hashCode behavior of Version and OracleVersion is bit surprising and I think somewhat underspecified given the possibility of defining subclasses.

Given the above regarding downstream use, I've make jdk.Version final and the constructor is now private. Oracle or any other JDK distribution wishing to impose additional semantics to the version string interpretation will need to do so via encapsulation rather than inheritance.

...

How is this API supposed to behave if the component version strings have a numerical value greater than Integer.MAX_VALUE?

From the specification of the only instance method, Version.parse():

* @throws NumberFormatException * If an element of the version number or the build number cannot * be represented as an {@link Integer}

...

Was using Longs to record numerical versions rather than Integers considered?

Yes. In an informal poll conducted during implementation, it was thought that 2^32 bits would be more than sufficient to represent the components of typical version numbers.

An updated webrev is forthcoming.

Thanks, iris

PS: I believe that we are both out the week of 21 December, so I don't expect to hear back from you until the New Year.

Hi, Alan.

Good to have this in but now we need to decide on where it should live. It's JDK-specific so we'll need it exported by a JDK module rather than java.base.

8144062 was next on my list. Do you have any suggestions for the module? Of the existing modules in the jdk repository, jdk.dev seems to have the most appropriate name. It appears to only contain jimage so I'm not sure about that module's charter. Thanks, iris -----Original Message----- From: Alan Bateman Sent: Wednesday, February 03, 2016 12:53 AM To: Iris Clark; Mandy Chung Cc: verona-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion On 03/02/2016 03:17, Iris Clark wrote:

Hi, Mandy.

Thanks so much for pushing the changeset for the initial implementation of jdk.Version!

Good to have this in but now we need to decide on where it should live. It's JDK-specific so we'll need it exported by a JDK module rather than java.base. -Alan

On 25/02/2016 05:59, Iris Clark wrote:

Hi, Alan, Mandy, and Mark.

After exploring a few module options (jdk.Version, jdk.dev [0]), it looks like the best choice is to move jdk.Version to java.lang.Runtime.Version (a nested class of Runtime). It supports the values returned by the java.{vm.}?version and java.{vm.}?specification.version system properties. By making Version an SE API, it may be exported by the java.base module.

Please review the following changes to move jdk.Version to jdk.lang.Runtime.Version. Note that beyond the package name and class declaration (to static), the only other changes in Version are related to indentation.

This looks good. We should probably change the synopsis on the JIRA issue to make it clearer that it is promoting Version to a standard API. Once this is in then the multi-release JAR file patch can be updated to make use of this. -Alan.

On Feb 24, 2016, at 9:59 PM, Iris Clark <iris.clark@oracle.com> wrote:

Hi, Alan, Mandy, and Mark.

After exploring a few module options (jdk.Version, jdk.dev [0]), it looks like the best choice is to move jdk.Version to java.lang.Runtime.Version (a nested class of Runtime). It supports the values returned by the java.{vm.}?version and java.{vm.}?specification.version system properties. By making Version an SE API, it may be exported by the java.base module.

Please review the following changes to move jdk.Version to jdk.lang.Runtime.Version. Note that beyond the package name and class declaration (to static), the only other changes in Version are related to indentation.

Bug

8144062: Determine appropriate module for jdk.Version https://bugs.openjdk.java.net/browse/JDK-8144062

webrev

http://cr.openjdk.java.net/~iris/verona/8144062/webrev.0/

942 * A representation of the JDK version-string version string for the runtime instead of JDK One suggestion is to move the details about fourth and later elements to @implSpec or @implNote. Have you considered moving the static Version::current method to Runtime::version? Mandy

Hi, Roger. Thanks for the feedback.

...

http://cr.openjdk.java.net/~iris/verona/8072379/webrev.1/

I see the JEP says JDK specific, but does that rule out putting the version API in a Java.* package?

For now, we should avoid including this API in java.*. If for no other reason but that we may need to evolve the API in an update release.

Version.java:

229: The IAE("Terminal...") exception may not be that easily understood; can it say concisely that the build and optional elements are missing

I've changed it to this which is technically more accurate (but verbose): IAE("'+' found with neither build or optional components: ':" + s + "'"). If you have a more succinct suggestion, let me know.

264: current() should cache the value from the parse; it is likely to be called more than once and parsing an version string is a relatively expensive.

Done.

334: the naming of one of the version elements as 'optional' may be confusing because optional is an adjective. Especially when the element is optional and the method named optional returns an Optional. Can the name be a cogent noun. how about 'info' for informational string

The term "optional" to refer to this portion of the string is used in quite a few places, both inside and outside of OpenJDK code. I don't think that it would be helpful to refer to this by two different names. If you feel strongly that the name should be changed, I can file an RFE to investigate the full impact to do so.

396: I'm not sure why BigInteger is needed; other than perhaps because it has a constructor that takes a string.

The components of $VNUM are constrained to be ints, hence if a string of numbers greater than Integer.MAX_VALUE is passed, construction of the Version will fail. $PRE is an arbitrary length String which may consist entirely of digits. As long as the string of digits may be represented as a String, there is no constraint on how many digits there are. The only time when the number of digits is a potential problem is during Version comparison. In the case when both of the Version objects contain $PRE consisting entirely of digits, we need to consider the case where the number of digits would result in a integer larger than Integer.MAX_VALUE.

433: Optional has a .ifPresent(xxx) method that could be used to streamline the code. pre.ifPresent( v -> sb.append("-").append(v));

Done.

OracleVersion.java: Can this be renamed more functionally to reflect that the 4'th component is a patch number. It might be useful to folks other than Oracle.

107: the constructor should be declared private since it is not needed outside the class.

OracleVersion.java is gone. The functionality it provided, access to the fourth element of the version number, is trivially accessed by existing methods in jdk.Version. jdk.Version is now final, with a private constructor. An updated webrev is forthcoming. Regards, iris -----Original Message----- From: Roger Riggs Sent: Wednesday, November 25, 2015 1:23 PM To: core-libs-dev@openjdk.java.net Subject: Re: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion Hi Iris, I see the JEP says JDK specific, but does that rule out putting the version API in a Java.* package? It would support the values found in the java.version, etc properties. Perhaps as an nested class of System or Runtime? Version.java: Line 213: Seems a bit wasteful to reparse the string after the matcher has done its work; but does not use the groups for the version components. 229: The IAE("Terminal...") exception may not be that easily understood; can it say concisely that the build and optional elements are missing 264: current() should cache the value from the parse; it is likely to be called more than once and parsing an version string is a relatively expensive. 334: the naming of one of the version elements as 'optional' may be confusing because optional is an adjective. Especially when the element is optional and the method named optional returns an Optional. Can the name be a cogent noun. how about 'info' for informational string 396: I'm not sure why BigInteger is needed; other than perhaps because it has a constructor that takes a string. 433: Optional has a .ifPresent(xxx) method that could be used to streamline the code. pre.ifPresent( v -> sb.append("-").append(v)); OracleVersion.java: Can this be renamed more functionally to reflect that the 4'th component is a patch number. It might be useful to folks other than Oracle. 107: the constructor should be declared private since it is not needed outside the class. I would have preferred the Tests to be written using TestNG. Thanks, Roger

Another comment below... On 11/27/2015 6:36 AM, Magnus Ihse Bursie wrote:

On 2015-11-25 02:54, Iris Clark wrote:

...

Hi.

Please review the new classes jdk.Version and jdk.OracleVersion. These are simple Java APIs to parse, validate, and compare version numbers.

Bug

8072379: Implement jdk.Version and jdk.OracleVersion https://bugs.openjdk.java.net/browse/JDK-8072379

Webrev

http://cr.openjdk.java.net/~iris/verona/8072379/webrev.1/

Hi Iris,

I thought the end agreement was that the + should always be present even if build was empty, if opt was present but not pre. That is, "9-foo" should unambigiously parse as vnum=9 and pre="foo", while "9-+foo" should umambigously parse as vnum=9 and opt="foo". The javadoc does not seem to reflect this.

I've also had to read and re-read the regexp and parsing logic in the constructor to convince myself that this (most likely) will be correctly handled. Perhaps a few comments around this would be helpful?

The comparison of two version strings which differs only in "pre" does not adhere to the principle of astonishment.

The documentation states: "Pre-release identifiers are compared numerically when they consist only of digits, and lexiographically otherwise. Numeric identifiers are considered to be less than non-numeric identifiers." But consider the following version strings:

9-01 9-01a 9-02 9-003b

That sequence would be ordered like this, which I find highly surprising. 9-01 9-02 9-003b 9-01a

I'm also surprised that equals() does not produce the same result as compareTo(). If opt is to be ignored, surely it should be so in equals() as well?

I'm one of the maintainers of BigDecimal, one of the few Comparable classes in the JDK where compareTo is "inconsistent with equals" (see "Effective Java, 2nd edition", Item 12 - Considering implementing Comparable). It is consistently surprising to users if compareTo is inconsistent with equals ;-) Therefore, if at all possible it is preferable to have compareTo be *consistent* rather than *inconsistent* with equals; although there are cases where the inconsistency is necessary or at least defensible. I suggest providing multiple compareFoo methods for version that did or did not include certain components, some of these could be consistent with equals and others not. HTH, -Joe

Hi, Magnus. Thanks for the review comments.

...

http://cr.openjdk.java.net/~iris/verona/8072379/webrev.1/

I thought the end agreement was that the + should always be present even if build was empty, if opt was present but not pre. That is, "9-foo" should unambigiously parse as vnum=9 and pre="foo", while "9-+foo" should umambigously parse as vnum=9 and opt="foo". The javadoc does not seem to reflect this.

You have correctly summarized the expectation and the implementation. I've made the following update the class javadoc: From: * <blockquote><pre> * $VNUM(-$PRE)?(\+$BUILD)?(-$OPT)? * </pre></blockquote> To: * <blockquote><pre> * $VNUM(-$PRE)?(\+($BUILD)?(-$OPT)?)? * </pre></blockquote>

The comparison of two version strings which differs only in "pre" does not adhere to the principle of astonishment.

The documentation states: "Pre-release identifiers are compared numerically when they consist only of digits, and lexiographically otherwise. Numeric identifiers are considered to be less than non-numeric identifiers." But consider the following version strings:

9-01 9-01a 9-02 9-003b

That sequence would be ordered like this, which I find highly surprising. 9-01 9-02 9-003b 9-01a

The current choice of comparison is motivated by Semantic Versioning. A goal of jdk.Version is to subset this scheme. The sorting may be counter-intuitive and it is possible that in the future we may decide that perfect alignment with semver.org is not critical. For now, we're settling with documenting the behavior.

I'm also surprised that equals() does not produce the same result as compareTo(). If opt is to be ignored, surely it should be so in equals() as well?

Based on your comment and Joe's, I've added comparison methods compareTo()/equals() and compareToIgnoreOpt()/equalsIngnoreOpt() for consistency. I'll be sending an updated webrev shortly. Thanks, iris