Take BUILD_NUMBER into consideration for Version sorting (#45)

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.

Author: jonpryor

src/Xamarin.Android.Tools.AndroidSdk/AndroidSdkInfo.cs

@@ -151,7 +151,7 @@ internal static void DefaultConsoleLogger (TraceLevel level, string message)
 				Console.Error.WriteLine (message);
 				break;
 			default:
-				Console.WriteLine (message);
+				Console.WriteLine ($"[{level}] {message}");
 				break;
 			}
 		}

src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs

@@ -51,7 +51,7 @@ public JdkInfo (string homePath)
 			JavaPath            = ProcessUtils.FindExecutablesInDirectory (binPath, "java").FirstOrDefault ();
 			JavacPath           = ProcessUtils.FindExecutablesInDirectory (binPath, "javac").FirstOrDefault ();
 			JdkJvmPath = OS.IsMac
-				? FindLibrariesInDirectory (HomePath, "jli").FirstOrDefault ()
+				? FindLibrariesInDirectory (Path.Combine (HomePath, "jre"), "jli").FirstOrDefault ()
 				: FindLibrariesInDirectory (Path.Combine (HomePath, "jre"), "jvm").FirstOrDefault ();
 
 			ValidateFile ("jar",    JarPath);
@@ -107,6 +107,8 @@ public bool GetJavaSettingsPropertyValue (string key, out string value)
 
 		static IEnumerable<string> FindLibrariesInDirectory (string dir, string libraryName)
 		{
+			if (!Directory.Exists (dir))
+				return Enumerable.Empty<string> ();
 			var library = string.Format (OS.NativeLibraryFormat, libraryName);
 			return Directory.EnumerateFiles (dir, library, SearchOption.AllDirectories);
 		}
@@ -117,29 +119,44 @@ void ValidateFile (string name, string path)
 				throw new ArgumentException ($"Could not find required file `{name}` within `{HomePath}`; is this a valid JDK?", "homePath");
 		}
 
-		static  Regex   VersionExtractor  = new Regex (@"(?<version>[\d]+(\.\d+)+)(_(?<patch>\d+))?", RegexOptions.Compiled);
+		static  Regex   NonDigitMatcher     = new Regex (@"[^\d]", RegexOptions.Compiled | RegexOptions.CultureInvariant);
 
 		Version GetJavaVersion ()
 		{
 			string version = null;
-			if (!ReleaseProperties.TryGetValue ("JAVA_VERSION", out version)) {
-				if (GetJavaSettingsPropertyValue ("java.version", out string vs))
-					version = vs;
+			if (ReleaseProperties.TryGetValue ("JAVA_VERSION", out version) && !string.IsNullOrEmpty (version)) {
+				version = GetParsableVersion (version);
+				if (ReleaseProperties.TryGetValue ("BUILD_NUMBER", out var build) && !string.IsNullOrEmpty (build))
+					version += "." + build;
+			}
+			else if (GetJavaSettingsPropertyValue ("java.version", out version) && !string.IsNullOrEmpty (version)) {
+				version = GetParsableVersion (version);
+			}
+			if (string.IsNullOrEmpty (version))
+				throw new NotSupportedException ("Could not determine Java version.");
+			var normalizedVersion   = NonDigitMatcher.Replace (version, ".");
+			var versionParts        = normalizedVersion.Split (new[]{"."}, StringSplitOptions.RemoveEmptyEntries);
+
+			try {
+				if (versionParts.Length < 2)
+					return null;
+				if (versionParts.Length == 2)
+					return new Version (major: int.Parse (versionParts [0]), minor: int.Parse (versionParts [1]));
+				if (versionParts.Length == 3)
+					return new Version (major: int.Parse (versionParts [0]), minor: int.Parse (versionParts [1]), build: int.Parse (versionParts [2]));
+				// We just ignore elements 4+
+				return new Version (major: int.Parse (versionParts [0]), minor: int.Parse (versionParts [1]), build: int.Parse (versionParts [2]), revision: int.Parse (versionParts [3]));
 			}
-			if (version == null)
-				throw new NotSupportedException ("Could not determine Java version");
-			var m = VersionExtractor.Match (version);
-			if (!m.Success)
+			catch (Exception) {
 				return null;
-			version   = m.Groups ["version"].Value;
-			var patch = m.Groups ["patch"].Value;
-			if (!string.IsNullOrEmpty (patch))
-				version += "." + patch;
+			}
+		}
+
+		static string GetParsableVersion (string version)
+		{
 			if (!version.Contains ("."))
 				version += ".0";
-			if (Version.TryParse (version, out Version v))
-				return v;
-			return null;
+			return version;
 		}
 
 		ReadOnlyDictionary<string, string> GetReleaseProperties ()

src/Xamarin.Android.Tools.AndroidSdk/Tests/AndroidSdkInfoTests.cs

@@ -75,7 +75,7 @@ public void Constructor_Paths ()
 		public void Constructor_SetValuesFromPath ()
 		{
 			CreateSdks (out string root, out string jdk, out string ndk, out string sdk);
-			JdkInfoTests.CreateFauxJdk (jdk, "1.8.0");
+			JdkInfoTests.CreateFauxJdk (jdk, releaseVersion: "1.8.0", releaseBuildNumber: "42", javaVersion: "100.100.100_100");
 
 			Action<TraceLevel, string> logger = (level, message) => {
 				Console.WriteLine ($"[{level}] {message}");
@@ -267,8 +267,8 @@ public void DetectAndSetPreferredJavaSdkPathToLatest ()
 					return;
 				}
 				Assert.Throws<NotSupportedException>(() => AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest (logger));
-				var newJdkPath  = Path.Combine (PreferredJdksOverridePath, "microsoft_dist_openjdk_1.8.999");
-				JdkInfoTests.CreateFauxJdk (newJdkPath, "1.8.999");
+				var newJdkPath  = Path.Combine (PreferredJdksOverridePath, "microsoft_dist_openjdk_1.8.999.9");
+				JdkInfoTests.CreateFauxJdk (newJdkPath, releaseVersion: "1.8.999", releaseBuildNumber: "9", javaVersion: "1.8.999-9");
 
 				if (File.Exists (UnixConfigPath))
 					File.Move (UnixConfigPath, backupConfig);

src/Xamarin.Android.Tools.AndroidSdk/Tests/JdkInfoTests.cs

@@ -35,25 +35,25 @@ public void CreateFauxJdk ()
 			var dir = Path.GetTempFileName();
 			File.Delete (dir);
 
-			CreateFauxJdk (dir, "1.2.3.4");
+			CreateFauxJdk (dir, releaseVersion: "1.2.3", releaseBuildNumber: "42", javaVersion: "100.100.100-100");
 
 			FauxJdkDir = dir;
 		}
 
-		internal static void CreateFauxJdk (string dir, string version)
+		internal static void CreateFauxJdk (string dir, string releaseVersion, string releaseBuildNumber, string javaVersion)
 		{
 			Directory.CreateDirectory (dir);
 
 			using (var release = new StreamWriter (Path.Combine (dir, "release"))) {
-				release.WriteLine ($"JAVA_VERSION=\"{version}\"");
-				release.WriteLine ($"BUILD_NUMBER=42");
+				release.WriteLine ($"JAVA_VERSION=\"{releaseVersion}\"");
+				release.WriteLine ($"BUILD_NUMBER={releaseBuildNumber}");
 				release.WriteLine ($"JUST_A_KEY");
 			}
 
 			var bin = Path.Combine (dir, "bin");
 			var inc = Path.Combine (dir, "include");
-			var jli = Path.Combine (dir, "jli");
 			var jre = Path.Combine (dir, "jre");
+			var jli = Path.Combine (jre, "lib", "jli");
 
 			Directory.CreateDirectory (bin);
 			Directory.CreateDirectory (inc);
@@ -65,15 +65,15 @@ internal static void CreateFauxJdk (string dir, string version)
 				$"echo Property settings:{Environment.NewLine}" +
 				$"echo \"    java.home = {dir}\"{Environment.NewLine}" +
 				$"echo \"    java.vendor = Xamarin.Android Unit Tests\"{Environment.NewLine}" +
-				$"echo \"    java.version = 100.100.100\"{Environment.NewLine}" +
+				$"echo \"    java.version = {javaVersion}\"{Environment.NewLine}" +
 				$"echo \"    xamarin.multi-line = line the first\"{Environment.NewLine}" +
 				$"echo \"        line the second\"{Environment.NewLine}" +
 				$"echo \"        .\"{Environment.NewLine}";
 
 			CreateShellScript (Path.Combine (bin, "jar"), "");
 			CreateShellScript (Path.Combine (bin, "java"), java);
 			CreateShellScript (Path.Combine (bin, "javac"), "");
-			CreateShellScript (Path.Combine (dir, "jli", "libjli.dylib"), "");
+			CreateShellScript (Path.Combine (jli, "libjli.dylib"), "");
 			CreateShellScript (Path.Combine (jre, "libjvm.so"), "");
 			CreateShellScript (Path.Combine (jre, "jvm.dll"), "");
 		}
@@ -127,9 +127,9 @@ public void PathPropertyValidation ()
 		public void VersionPrefersRelease ()
 		{
 			var jdk     = new JdkInfo (FauxJdkDir);
-			// Note: `release` has JAVA_VERSION=1.2.3.4, while `java` prints java.version=100.100.100.
-			// We prefer the value within `releas`.
-			Assert.AreEqual (jdk.Version, new Version ("1.2.3.4"));
+			// Note: `release` has JAVA_VERSION=1.2.3 + BUILD_NUMBER=42, while `java` prints java.version=100.100.100.
+			// We prefer the value constructed from `release`.
+			Assert.AreEqual (jdk.Version, new Version ("1.2.3.42"));
 		}
 
 		[Test]
@@ -138,7 +138,7 @@ public void ReleaseProperties ()
 			var jdk     = new JdkInfo (FauxJdkDir);
 
 			Assert.AreEqual (3,         jdk.ReleaseProperties.Count);
-			Assert.AreEqual ("1.2.3.4", jdk.ReleaseProperties ["JAVA_VERSION"]);
+			Assert.AreEqual ("1.2.3",   jdk.ReleaseProperties ["JAVA_VERSION"]);
 			Assert.AreEqual ("42",      jdk.ReleaseProperties ["BUILD_NUMBER"]);
 			Assert.AreEqual ("",        jdk.ReleaseProperties ["JUST_A_KEY"]);
 		}
@@ -157,7 +157,7 @@ public void JavaSettingsProperties ()
 			Assert.AreEqual (FauxJdkDir, home);
 
 			Assert.IsTrue (jdk.GetJavaSettingsPropertyValue ("java.version", out var version));
-			Assert.AreEqual ("100.100.100", version);
+			Assert.AreEqual ("100.100.100-100", version);
 
 			Assert.IsTrue (jdk.GetJavaSettingsPropertyValue ("java.vendor", out var vendor));
 			Assert.AreEqual ("Xamarin.Android Unit Tests", vendor);
@@ -170,5 +170,85 @@ public void JavaSettingsProperties ()
 			Assert.AreEqual ("line the second",     lines.ElementAt (1));
 			Assert.AreEqual (".",                   lines.ElementAt (2));
 		}
+
+		[Test]
+		public void ParseOracleReleaseVersion ()
+		{
+			var dir = Path.GetTempFileName();
+			File.Delete (dir);
+
+			try {
+				CreateFauxJdk (dir, releaseVersion: "1.2.3_4", releaseBuildNumber: "", javaVersion: "100.100.100_100");
+				var jdk     = new JdkInfo (dir);
+				Assert.AreEqual (new Version (1, 2, 3, 4),  jdk.Version);
+			}
+			finally {
+				Directory.Delete (dir, recursive: true);
+			}
+		}
+
+		[Test]
+		public void ParseOracleJavaVersion ()
+		{
+			var dir = Path.GetTempFileName();
+			File.Delete (dir);
+
+			try {
+				CreateFauxJdk (dir, releaseVersion: "", releaseBuildNumber: "", javaVersion: "101.102.103_104");
+				var jdk     = new JdkInfo (dir);
+				Assert.AreEqual (new Version (101, 102, 103, 104), jdk.Version);
+			}
+			finally {
+				Directory.Delete (dir, recursive: true);
+			}
+		}
+
+		[Test]
+		public void ParseMicrosoftReleaseVersion ()
+		{
+			var dir = Path.GetTempFileName();
+			File.Delete (dir);
+
+			try {
+				CreateFauxJdk (dir, releaseVersion: "1.2.3", releaseBuildNumber: "4", javaVersion: "100.100.100_100");
+				var jdk     = new JdkInfo (dir);
+				Assert.AreEqual (new Version (1, 2, 3, 4),  jdk.Version);
+			}
+			finally {
+				Directory.Delete (dir, recursive: true);
+			}
+		}
+
+		[Test]
+		public void ParseMicrosoftJavaVersion()
+		{
+			var dir = Path.GetTempFileName();
+			File.Delete (dir);
+
+			try {
+				CreateFauxJdk (dir, releaseVersion: "", releaseBuildNumber: "", javaVersion: "1.2.3-4");
+				var jdk     = new JdkInfo (dir);
+				Assert.AreEqual (new Version (1, 2, 3, 4),  jdk.Version);
+			}
+			finally {
+				Directory.Delete (dir, recursive: true);
+			}
+		}
+
+		[Test]
+		public void Version_ThrowsNotSupportedException ()
+		{
+			var dir = Path.GetTempFileName();
+			File.Delete (dir);
+
+			try {
+				CreateFauxJdk (dir, releaseVersion: "", releaseBuildNumber: "", javaVersion: "");
+				var jdk     = new JdkInfo (dir);
+				Assert.Throws<NotSupportedException> (() => { var _ = jdk.Version; });
+			}
+			finally {
+				Directory.Delete (dir, recursive: true);
+			}
+		}
 	}
 }