Skip to content

Conversation

@jonpryor
Copy link
Contributor

@jonpryor jonpryor commented Aug 9, 2018

Fixes: http://work.devdiv.io/661401

The version numbers which the Microsoft OpenJDK packages provide was
not consistent with what JdkInfo expected: the release file would
have a JAVA_VERSION value which was a three-tuple, and didn't
include the BUILD_NUMBER value. For example:

JAVA_VERSION="1.8.0"
BUILD_NUMBER=7

Unfortunately, JdkInfo.GetJavaVersion() was only taking
JAVA_VERSION into consideration, so when multiple different OpenJDK
packages were processed which all had the same JAVA_VERSION value
but different BUILD_NUMBER values,
JdkInfo.GetMacOSMicrosoftJdks() returned the "wrong" one first.
(In actuality, the one it returned first was not knowable, and was
probably whatever Directory.EnumerateDirectories() returned first.)

Simple enough, we just need to take BUILD_NUMBER into consideration
as part of constructing JdkInfo.Version, right?

Not so fast. Turns Out™ that the version value held within
JAVA_VERSION or the java.version property -- which need not be
identical! -- can also contain -, not just _. A "Java Version" is
*really" (at least) 4 optional parts:

JAVA_VERSION : VERSION ('_' UPDATE)? ('-' MILESTONE)? ('-' SUFFIX)?

Which means Java version values such as 1.8.0_1-9-microsoft-b00 are
possible, from various different locations, e.g. the version value
vs. the build value in java -version output:

$ bin/java -version
openjdk version "1.8.0-9"
OpenJDK Runtime Environment (build 1.8.0-9-microsoft-b00)
OpenJDK 64-Bit Server VM (build 25.71-b00, mixed mode)

Instead of trying to top-down parse a version number, go for a
"bottom-up" parsing:

  1. Replace all non-digit characters with .
  2. Split the value on (1) on ., creating an array, and remove all
    empty values.
  3. Separately parse the values in (2) as part of System.Version
    construction.

This allows at least a sane-ish, plausible, Version construction.
1.8.0_161-b12 will be parsed as new Version(1, 8, 0, 161) (as
we'll just grab up to the first four values), and we'll gracefully
ignore any other non-digit characters in the string.

This allows us to better construct a Version value for Microsoft
OpenJDK packages, allowing us in turn to sort the packages, which
allows JdkInfo>GetMacOSMicrosoftJdks() to return the highest version
first, as is desired.

}

static Regex VersionExtractor = new Regex (@"(?<version>[\d]+(\.\d+)+)(_(?<patch>\d+))?", RegexOptions.Compiled);
static Regex NonDigitMatcher = new Regex (@"[^\d]");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this still use RegexOptions.Compiled, or is it better to not do that for shorter Regex strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. For years Mono didn't support them, so I didn't use them...

Certainly wouldn't hurt to use them.

}
else if (GetJavaSettingsPropertyValue ("java.version", out version) && !string.IsNullOrEmpty (version)) {
version = GetParsableVersion (version);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do the !string.IsNullOrEmpty check when constructing ReleaseProperties?

Then wouldn't have to do this check so much here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes: http://work.devdiv.io/661401

The version numbers which the Microsoft OpenJDK packages provide was
not consistent with what `JdkInfo` expected: the `release` file would
have a `JAVA_VERSION` value which was a three-tuple, and didn't
include the `BUILD_NUMBER` value.  For example:

	JAVA_VERSION="1.8.0"
	BUILD_NUMBER=7

Unfortunately, `JdkInfo.GetJavaVersion()` was only taking
`JAVA_VERSION` into consideration, so when multiple different OpenJDK
packages were processed which all had the *same* `JAVA_VERSION` value
but *different* `BUILD_NUMBER` values,
`JdkInfo.GetMacOSMicrosoftJdks()` returned the "wrong" one first.
(In actuality, the one it returned first was not knowable, and was
probably whatever `Directory.EnumerateDirectories()` returned first.)

Simple enough, we just need to take `BUILD_NUMBER` into consideration
as part of constructing `JdkInfo.Version`, right?

Not so fast.  Turns Out™ that the version value held within
`JAVA_VERSION` or the `java.version` property -- which need not be
identical! -- can also contain `-`, not just `_`.  A "Java Version" is
*really" (at least) 4 optional parts:

	JAVA_VERSION : VERSION ('_' UPDATE)? ('-' MILESTONE)? ('-' SUFFIX)?

Which means Java version values such as `1.8.0_1-9-microsoft-b00` are
possible, from various different locations, e.g. the `version` value
vs. the `build` value in `java -version` output:

	$ bin/java -version
	openjdk version "1.8.0-9"
	OpenJDK Runtime Environment (build 1.8.0-9-microsoft-b00)
	OpenJDK 64-Bit Server VM (build 25.71-b00, mixed mode)

Instead of trying to top-down parse a version number, go for a
"bottom-up" parsing:

 1. Replace all *non-digit* characters with `.`
 2. Split the value on (1) on `.`, creating an array, and remove all
    empty values.
 3. Separately parse the values in (2) as part of `System.Version`
    construction.

This allows at least a sane-ish, plausible, `Version` construction.
`1.8.0_161-b12` will be parsed as `new Version(1, 8, 0, 161)` (as
we'll just grab up to the first four values), and we'll gracefully
ignore any other non-digit characters in the string.

This allows us to better construct a `Version` value for Microsoft
OpenJDK packages, allowing us in turn to *sort* the packages, which
allows `JdkInfo>GetMacOSMicrosoftJdks()` to return the highest version
*first*, as is desired.
@jonpryor jonpryor force-pushed the jonp-use-build-number branch from 4771a4d to a4b61e0 Compare August 9, 2018 21:04
@jonpryor jonpryor merged commit 3ef860b into dotnet:master Aug 10, 2018
jonpryor added a commit that referenced this pull request Aug 10, 2018
Fixes: http://work.devdiv.io/661401

The version numbers which the Microsoft OpenJDK packages provide was
not consistent with what `JdkInfo` expected: the `release` file would
have a `JAVA_VERSION` value which was a three-tuple, and didn't
include the `BUILD_NUMBER` value.  For example:

	JAVA_VERSION="1.8.0"
	BUILD_NUMBER=7

Unfortunately, `JdkInfo.GetJavaVersion()` was only taking
`JAVA_VERSION` into consideration, so when multiple different OpenJDK
packages were processed which all had the *same* `JAVA_VERSION` value
but *different* `BUILD_NUMBER` values,
`JdkInfo.GetMacOSMicrosoftJdks()` returned the "wrong" one first.
(In actuality, the one it returned first was not knowable, and was
probably whatever `Directory.EnumerateDirectories()` returned first.)

Simple enough, we just need to take `BUILD_NUMBER` into consideration
as part of constructing `JdkInfo.Version`, right?

Not so fast.  Turns Out™ that the version value held within
`JAVA_VERSION` or the `java.version` property -- which need not be
identical! -- can also contain `-`, not just `_`.  A "Java Version" is
*really" (at least) 4 optional parts:

	JAVA_VERSION : VERSION ('_' UPDATE)? ('-' MILESTONE)? ('-' SUFFIX)?

Which means Java version values such as `1.8.0_1-9-microsoft-b00` are
possible, from various different locations, e.g. the `version` value
vs. the `build` value in `java -version` output:

	$ bin/java -version
	openjdk version "1.8.0-9"
	OpenJDK Runtime Environment (build 1.8.0-9-microsoft-b00)
	OpenJDK 64-Bit Server VM (build 25.71-b00, mixed mode)

Instead of trying to top-down parse a version number, go for a
"bottom-up" parsing:

 1. Replace all *non-digit* characters with `.`
 2. Split the value on (1) on `.`, creating an array, and remove all
    empty values.
 3. Separately parse the values in (2) as part of `System.Version`
    construction.

This allows at least a sane-ish, plausible, `Version` construction.
`1.8.0_161-b12` will be parsed as `new Version(1, 8, 0, 161)` (as
we'll just grab up to the first four values), and we'll gracefully
ignore any other non-digit characters in the string.

This allows us to better construct a `Version` value for Microsoft
OpenJDK packages, allowing us in turn to *sort* the packages, which
allows `JdkInfo>GetMacOSMicrosoftJdks()` to return the highest version
*first*, as is desired.
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Aug 16, 2018
grendello pushed a commit to dotnet/android that referenced this pull request Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants