SI-9750 scala.util.Properties.isJavaAtLeast works with JDK9 #5276
Conversation
The utility method compares javaSpecVersion, which has the form "1.8" previously and "9" going forward. The method accepts "1.n" for n < 9. More correctly, the string argument should be a single number. Supports JEP-223.
| def versionOf(s: String): Int = s.indexOf('.') match { | ||
| case 1 if s.charAt(0) == '1' => | ||
| val v = versionOf(s.substring(2)) | ||
| if (v < 9) v else -1 |
There was a problem hiding this comment.
Probably didn't intend to accept 1.1.8.
Leaves the error string as is, but adds test to show how it looks. Java calls it a version number. `Not a version: 1.9`. Don't strip `1.` prefix recursively. (That was Snytt's fault.)
A good opportunity to simplify the API. Versions are strings, but a spec version is just a number.
| val i = x.indexOf('.') | ||
| if (i < 0) throw new NumberFormatException("Not a version: " + x) | ||
| (x.substring(0, i), x.substring(i+1, x.length)) | ||
| def versionOf(s: String): Int = s.indexOf('.') match { |
There was a problem hiding this comment.
JEP223 specify that the system property java.specification.version is a $VNUM and $VNUMs satisfy the regular expression: ^[1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*$. This code doesn't look ready to pick the major version from all strings satisfying that regular expression.
There was a problem hiding this comment.
@varming You're right, it does say $VNUM. I was fooled by the table of examples where the spec is $MAJOR? Is the spec $MAJOR in practice? I think it can't hurt to accept leading "1." and then ignore suffix of ".anything". The doc should say the arg must be the $MAJOR, if that is the desired behavior. Certainly previous usage has been just to distinguish major versions of the platform.
There was a problem hiding this comment.
Actually, re-reading what they intend by $MINOR, which includes revision of API, it's possible this API should support $MAJOR.$MINOR. I know I care if we're on java 8 or 9, but will I care if it is 9.1 or 9.2? assuming the spec string is 9.1. Alternatively, maybe supply isJavaMajorSpec for platforms that don't want to use regex for this test; otherwise just use regex like normal people.
There was a problem hiding this comment.
Dear @som-snytt,
I don't know if there will be a time where $minor, $security, and later parts of the $VNUM will be important, but I would expect the code to work with all $VNUMs. In most settings you probably want to support $major, $minor, and $security. Having special functions to check that the major is at least greater than some integer seems like a good idea.
Right now the code handles backwards compatibility with <= JDK8, but it cannot handle JDK10. It would be nice if it handled backwards compatibility separately from general $VNUMs and let backwards compatibility take precedence. It doesn't look like you need full regular expressions to parse a $VNUM, simply take all characters up to the next dot (or the end of the string if there is no more dots) and parse them as an integer, and repeat this until you can decide the version question.
Don't assume spec is just major, but allow arbitrary version number for both spec value and user value to check. Only the first three dot-separated fields are considered, after skipping optional leading value "1" in legacy format. Minimal validity checks of user arg are applied. Leading three fields, if present, must be number values, but subsequent fields are ignored. Note that a version number is not a version string, which optionally includes pre and build info, `9-ea+109`.
|
By coincidence, there's a performance regression on start-up in JDK-9 that has to do with loading regex classes, fixed by checking if the version is just a major number. The call for review. After the nuclear apocalypse, the only things left will be cockroaches and programs that do |
|
Looks like we can actually remove the only use of this method from our own code. Would you mind doing that too? We know statically that |
Formatting suppressed exceptions required reflection for platform compatibility. No longer, since Java 8 is assumed. Minor tidying.
|
Sure thing, it gave me something to do while babysitting my second Windows 10 upgrade this week, with a few hours to spare. I only wish that guy would comment his code. Frankly, I never want to touch his code ever again. I don't see how anyone puts up with it. Technically, |
|
shouldn't this target 2.11.x? |
|
@SethTisue Too risky! |
|
LGTM. I think we can get target Java 9 support to the 2.12.1+, and then backport to 2.11.x once some problems are shaken out. |
The dotty implementation of Properties.isJavaAtLeast is broken for Java 9 (Fixed in Scalac at scala/scala#5276, but maybe we just shouldn't have a copy of Properties.scala in Dotty). The test is useless now anyway since we only compile with 2.12 which requires Java 8, so this code is never going to run on an older version of the JVM.
The dotty implementation of Properties.isJavaAtLeast is broken for Java 9 (Fixed in Scalac at scala/scala#5276, but maybe we just shouldn't have a copy of Properties.scala in Dotty). The test is useless now anyway since we only compile with 2.12 which requires Java 8, so this code is never going to run on an older version of the JVM.
The dotty implementation of Properties.isJavaAtLeast is broken for Java 9 (Fixed in Scalac at scala/scala#5276, but maybe we just shouldn't have a copy of Properties.scala in Dotty). The test is useless now anyway since we only compile with 2.12 which requires Java 8, so this code is never going to run on an older version of the JVM.
The dotty implementation of Properties.isJavaAtLeast is broken for Java 9 (Fixed in Scalac at scala/scala#5276, but maybe we just shouldn't have a copy of Properties.scala in Dotty). The test is useless now anyway since we only compile with 2.12 which requires Java 8, so this code is never going to run on an older version of the JVM.
6 participants
The utility method compares javaSpecVersion, which has the
form "1.8" previously and "9" going forward.
The method accepts "1.n" for n < 9. More correctly, the string
argument should be a single number.
Supports JEP-223.
Supersedes #5270
Review by @golem131
This supposes golem was annoyed by our preciousness.