[Xamarin.Android.Build.Tasks] cache java -version output (#1938)

jonathanpeppers · jonpryor · commit 4a5dd0195108 · 2018-07-15T16:53:40.000-04:00
Context: https://github.com/Microsoft/msbuild/blob/f147a76a60bf9e516800f65f9614c914df0a4f15/src/Framework/IBuildEngine4.cs#L34-L84 Context: https://github.com/xamarin/Xamarin.Forms/blob/42c07d1ae5aa56eb574b7d169499f1a9af7ec080/Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj The new `<ValidateJavaVersion/>` MSBuild task (93ddf96) gets invoked for *every* project on *every* build, even if nothing changed. Unfortunately, this task also takes around 250ms to run, since it shells out to `java -version` and `javac -version`. We can use MSBuild's `GetRegisteredTaskObject()` and `RegisterTaskObject()` to cache the output of running these command line tools. If we key with the full path to `java` or `javac`, the cache should be properly invalidated if a different `$(JavaSdkDirectory)` value is passed in. I added a new set of `ValidateJavaVersionTests` to directly test this caching functionality, plus a couple smoke tests. In the case of `Xamarin.Forms.ControlGallery.Android`: - `<ValidateJavaVersion/>` runs 7 times - A build (with no changes) went from 12.820s to 11.185s, saving ~1.635s of build time - I also tested the changes in VS Windows, and was able to see logs indicating the value is cached. Improvements to `MockBuildEngine`: - General code formatting / refactoring - Added a `Messages` list that can be asserted against - Refactored the `Task` APIs so that they are fully functional Other changes: - Since `BaseTest` now has a `SetUp` class, we can't have a method named `SetUp` or we get a warning. I used a `Setup` method (with lowercase u) instead, and modified `KeyToolTests` to fix this warning.
diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/ValidateJavaVersion.cs b/src/Xamarin.Android.Build.Tasks/Tasks/ValidateJavaVersion.cs
@@ -73,40 +73,59 @@ bool ValidateJava (string javaExe, Regex versionRegex, string targetFrameworkVer
 
 			MinimumRequiredJdkVersion = required.ToString ();
 
-			var sb = new StringBuilder ();
-
-			var javaTool = Path.Combine (JavaSdkPath, "bin", javaExe);
 			try {
-				MonoAndroidHelper.RunProcess (javaTool, "-version", (s, e) => {
-					if (!string.IsNullOrEmpty (e.Data))
-						sb.AppendLine (e.Data);
-				}, (s, e) => {
-					if (!string.IsNullOrEmpty (e.Data))
-						sb.AppendLine (e.Data);
-				});
+				var versionNumber = GetVersionFromTool (javaExe, versionRegex);
+				if (versionNumber != null) {
+					Log.LogMessage (MessageImportance.Normal, $"Found Java SDK version {versionNumber}.");
+					if (versionNumber < requiredJavaForFrameworkVersion) {
+						Log.LogCodedError ("XA0031", $"Java SDK {requiredJavaForFrameworkVersion} or above is required when targeting FrameworkVersion {targetFrameworkVersion}.");
+					}
+					if (versionNumber < requiredJavaForBuildTools) {
+						Log.LogCodedError ("XA0032", $"Java SDK {requiredJavaForBuildTools} or above is required when using build-tools {buildToolsVersion}.");
+					}
+					if (versionNumber > Version.Parse (LatestSupportedJavaVersion)) {
+						Log.LogCodedError ("XA0030", $"Building with JDK Version `{versionNumber}` is not supported. Please install JDK version `{LatestSupportedJavaVersion}`. See https://aka.ms/xamarin/jdk9-errors");
+					}
+				}
 			} catch (Exception ex) {
 				Log.LogWarningFromException (ex);
 				Log.LogCodedWarning ("XA0034", $"Failed to get the Java SDK version. Please ensure you have Java {required} or above installed.");
 				return false;
 			}
+
+			return !Log.HasLoggedErrors;
+		}
+
+		Version GetVersionFromTool (string javaExe, Regex versionRegex)
+		{
+			var javaTool = Path.Combine (JavaSdkPath, "bin", javaExe);
+			var key = new Tuple<string, string> (nameof (ValidateJavaVersion), javaTool);
+			var cached = BuildEngine4.GetRegisteredTaskObject (key, RegisteredTaskObjectLifetime.AppDomain) as Version;
+			if (cached != null) {
+				Log.LogDebugMessage ($"Using cached value for `{javaTool} -version`: {cached}");
+				JdkVersion = cached.ToString ();
+				return cached;
+			}
+
+			var sb = new StringBuilder ();
+			MonoAndroidHelper.RunProcess (javaTool, "-version", (s, e) => {
+				if (!string.IsNullOrEmpty (e.Data))
+					sb.AppendLine (e.Data);
+			}, (s, e) => {
+				if (!string.IsNullOrEmpty (e.Data))
+					sb.AppendLine (e.Data);
+			});
 			var versionInfo = sb.ToString ();
 			var versionNumberMatch = versionRegex.Match (versionInfo);
 			Version versionNumber;
 			if (versionNumberMatch.Success && Version.TryParse (versionNumberMatch.Groups ["version"]?.Value, out versionNumber)) {
+				BuildEngine4.RegisterTaskObject (key, versionNumber, RegisteredTaskObjectLifetime.AppDomain, allowEarlyCollection: false);
 				JdkVersion = versionNumberMatch.Groups ["version"].Value;
-				Log.LogMessage (MessageImportance.Normal, $"Found Java SDK version {versionNumber}.");
-				if (versionNumber < requiredJavaForFrameworkVersion) {
-					Log.LogCodedError ("XA0031", $"Java SDK {requiredJavaForFrameworkVersion} or above is required when targeting FrameworkVersion {targetFrameworkVersion}.");
-				}
-				if (versionNumber < requiredJavaForBuildTools) {
-					Log.LogCodedError ("XA0032", $"Java SDK {requiredJavaForBuildTools} or above is required when using build-tools {buildToolsVersion}.");
-				}
-				if (versionNumber > Version.Parse (LatestSupportedJavaVersion)) {
-					Log.LogCodedError ("XA0030", $"Building with JDK Version `{versionNumber}` is not supported. Please install JDK version `{LatestSupportedJavaVersion}`. See https://aka.ms/xamarin/jdk9-errors");
-				}
-			} else
+				return versionNumber;
+			} else {
 				Log.LogCodedWarning ("XA0033", $"Failed to get the Java SDK version as it does not appear to contain a valid version number. `{javaExe} -version` returned: ```{versionInfo}```");
-			return !Log.HasLoggedErrors;
+				return null;
+			}
 		}
 
 		Version GetJavaVersionForFramework (string targetFrameworkVersion)
diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/KeyToolTests.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/KeyToolTests.cs
@@ -19,7 +19,7 @@ public class KeyToolTests : BaseTest
 		string keyToolPath;
 
 		[SetUp]
-		public void SetUp()
+		public void Setup()
 		{
 			engine = new MockBuildEngine (TestContext.Out, errors = new List<BuildErrorEventArgs> (), warnings = new List<BuildWarningEventArgs> ());
 			temp = Path.GetTempFileName ();
diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/ValidateJavaVersionTests.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/ValidateJavaVersionTests.cs
@@ -0,0 +1,121 @@
+﻿using Microsoft.Build.Framework;
+using NUnit.Framework;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using Xamarin.Android.Tasks;
+
+namespace Xamarin.Android.Build.Tests
+{
+	[TestFixture]
+	public class ValidateJavaVersionTests : BaseTest
+	{
+		string path;
+		List<BuildErrorEventArgs> errors;
+		List<BuildMessageEventArgs> messages;
+		MockBuildEngine engine;
+
+		[SetUp]
+		public void Setup ()
+		{
+			path = Path.Combine ("temp", TestName);
+			engine = new MockBuildEngine (TestContext.Out,
+				errors: errors = new List<BuildErrorEventArgs> (),
+				messages: messages = new List<BuildMessageEventArgs> ());
+
+			//Setup statics on MonoAndroidHelper
+			var referencePath = CreateFauxReferencesDirectory (Path.Combine (path, "references"), new [] {
+				new ApiInfo { Id = "27", Level = 27, Name = "Oreo", FrameworkVersion = "v8.1",  Stable = true },
+			});
+			MonoAndroidHelper.RefreshSupportedVersions (new [] {
+				Path.Combine (referencePath, "MonoAndroid"),
+			});
+		}
+
+		[TearDown]
+		public void TearDown ()
+		{
+			Directory.Delete (Path.Combine (Root, path), recursive: true);
+		}
+
+		[Test]
+		public void TargetFramework8_1_Requires_1_8_0 ()
+		{
+			var javaPath = CreateFauxJavaSdkDirectory (Path.Combine (path, "jdk"), "1.7.0", out string javaExe, out string javacExe);
+			var validateJavaVersion = new ValidateJavaVersion {
+				BuildEngine = engine,
+				TargetFrameworkVersion = "v8.1",
+				JavaSdkPath = javaPath,
+				JavaToolExe = javaExe,
+				JavacToolExe = javacExe,
+				LatestSupportedJavaVersion = "1.8.0",
+				MinimumSupportedJavaVersion = "1.7.0",
+			};
+			Assert.False (validateJavaVersion.Execute (), "Execute should *not* succeed!");
+			Assert.IsTrue (errors.Any (e => e.Message == $"Java SDK 1.8 or above is required when targeting FrameworkVersion {validateJavaVersion.TargetFrameworkVersion}."), "Should get error about TargetFrameworkVersion=v8.1");
+		}
+
+		[Test]
+		public void BuildTools27_Requires_1_8_0 ()
+		{
+			var javaPath = CreateFauxJavaSdkDirectory (Path.Combine (path, "jdk"), "1.7.0", out string javaExe, out string javacExe);
+			var validateJavaVersion = new ValidateJavaVersion {
+				BuildEngine = engine,
+				AndroidSdkBuildToolsVersion = "27.0.0",
+				JavaSdkPath = javaPath,
+				JavaToolExe = javaExe,
+				JavacToolExe = javacExe,
+				LatestSupportedJavaVersion = "1.8.0",
+				MinimumSupportedJavaVersion = "1.7.0",
+			};
+			Assert.False (validateJavaVersion.Execute (), "Execute should *not* succeed!");
+			Assert.IsTrue (errors.Any (e => e.Message == $"Java SDK 1.8 or above is required when using build-tools {validateJavaVersion.AndroidSdkBuildToolsVersion}."), "Should get error about build-tools=27.0.0");
+		}
+
+		[Test]
+		public void CacheWorks ()
+		{
+			var javaPath = CreateFauxJavaSdkDirectory (Path.Combine (path, "jdk"), "1.8.0", out string javaExe, out string javacExe);
+			var validateJavaVersion = new ValidateJavaVersion {
+				BuildEngine = engine,
+				JavaSdkPath = javaPath,
+				JavaToolExe = javaExe,
+				JavacToolExe = javacExe,
+				LatestSupportedJavaVersion = "1.8.0",
+				MinimumSupportedJavaVersion = "1.7.0",
+			};
+			Assert.IsTrue (validateJavaVersion.Execute (), "first Execute should succeed!");
+
+			messages.Clear ();
+
+			Assert.IsTrue (validateJavaVersion.Execute (), "second Execute should succeed!");
+			var javaFullPath = Path.Combine (javaPath, "bin", javaExe);
+			var javacFullPath = Path.Combine (javaPath, "bin", javacExe);
+			Assert.IsTrue (messages.Any (m => m.Message == $"Using cached value for `{javaFullPath} -version`: 1.8.0"), "`java -version` should be cached!");
+			Assert.IsTrue (messages.Any (m => m.Message == $"Using cached value for `{javacFullPath} -version`: 1.8.0"), "`javac -version` should be cached!");
+		}
+
+		[Test]
+		public void CacheInvalidates ()
+		{
+			var javaPath = CreateFauxJavaSdkDirectory (Path.Combine (path, "jdk-1"), "1.8.0", out string javaExe, out string javacExe);
+			var validateJavaVersion = new ValidateJavaVersion {
+				BuildEngine = engine,
+				JavaSdkPath = javaPath,
+				JavaToolExe = javaExe,
+				JavacToolExe = javacExe,
+				LatestSupportedJavaVersion = "1.8.0",
+				MinimumSupportedJavaVersion = "1.7.0",
+			};
+			Assert.IsTrue (validateJavaVersion.Execute (), "first Execute should succeed!");
+
+			messages.Clear ();
+
+			javaPath = CreateFauxJavaSdkDirectory (Path.Combine (path, "jdk-2"), "1.8.0", out javaExe, out javacExe);
+			validateJavaVersion.JavaSdkPath = javaPath;
+
+			Assert.IsTrue (validateJavaVersion.Execute (), "second Execute should succeed!");
+			Assert.IsFalse (messages.Any (m => m.Message.StartsWith ("Using cached value for")), "`java -version` should *not* be cached!");
+		}
+	}
+}
diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/MockBuildEngine.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/MockBuildEngine.cs
@@ -2,19 +2,18 @@
 using System.Collections;
 using System.Collections.Generic;
 using System.IO;
-using System.Linq;
-using System.Reflection;
-using System.Text;
-using System.Threading.Tasks;
 using Microsoft.Build.Framework;
 
-namespace Xamarin.Android.Build.Tests {
-	public class MockBuildEngine : IBuildEngine, IBuildEngine2, IBuildEngine3, IBuildEngine4 {
-		public MockBuildEngine (TextWriter output, IList<BuildErrorEventArgs> errors = null, IList<BuildWarningEventArgs> warnings = null)
+namespace Xamarin.Android.Build.Tests
+{
+	public class MockBuildEngine : IBuildEngine, IBuildEngine2, IBuildEngine3, IBuildEngine4
+	{
+		public MockBuildEngine (TextWriter output, IList<BuildErrorEventArgs> errors = null, IList<BuildWarningEventArgs> warnings = null, IList<BuildMessageEventArgs> messages = null)
 		{
-			this.Output = output;
-			this.Errors = errors;
-			this.Warnings = warnings;
+			Output = output;
+			Errors = errors;
+			Warnings = warnings;
+			Messages = messages;
 		}
 
 		private TextWriter Output { get; }
@@ -23,6 +22,8 @@ public MockBuildEngine (TextWriter output, IList<BuildErrorEventArgs> errors = n
 
 		private IList<BuildWarningEventArgs> Warnings { get; }
 
+		private IList<BuildMessageEventArgs> Messages { get; }
+
 		int IBuildEngine.ColumnNumberOfTaskNode => -1;
 
 		bool IBuildEngine.ContinueOnError => false;
@@ -37,24 +38,26 @@ public MockBuildEngine (TextWriter output, IList<BuildErrorEventArgs> errors = n
 
 		void IBuildEngine.LogCustomEvent (CustomBuildEventArgs e)
 		{
-			this.Output.WriteLine ($"Custom: {e.Message}");
+			Output.WriteLine ($"Custom: {e.Message}");
 		}
 
 		void IBuildEngine.LogErrorEvent (BuildErrorEventArgs e)
 		{
-			this.Output.WriteLine ($"Error: {e.Message}");
+			Output.WriteLine ($"Error: {e.Message}");
 			if (Errors != null)
 				Errors.Add (e);
 		}
 
 		void IBuildEngine.LogMessageEvent (BuildMessageEventArgs e)
 		{
-			this.Output.WriteLine ($"Message: {e.Message}");
+			Output.WriteLine ($"Message: {e.Message}");
+			if (Messages != null)
+				Messages.Add (e);
 		}
 
 		void IBuildEngine.LogWarningEvent (BuildWarningEventArgs e)
 		{
-			this.Output.WriteLine ($"Warning: {e.Message}");
+			Output.WriteLine ($"Warning: {e.Message}");
 			if (Warnings != null)
 				Warnings.Add (e);
 		}
@@ -63,19 +66,21 @@ void IBuildEngine.LogWarningEvent (BuildWarningEventArgs e)
 
 		void IBuildEngine4.RegisterTaskObject (object key, object obj, RegisteredTaskObjectLifetime lifetime, bool allowEarlyCollection)
 		{
-			Tasks.Add (key, obj);
+			Tasks [key] = obj;
 		}
 
 		object IBuildEngine4.GetRegisteredTaskObject (object key, RegisteredTaskObjectLifetime lifetime)
 		{
-			return null;
+			Tasks.TryGetValue (key, out object value);
+			return value;
 		}
 
 		object IBuildEngine4.UnregisterTaskObject (object key, RegisteredTaskObjectLifetime lifetime)
 		{
-			var obj = Tasks [key];
-			Tasks.Remove (key);
-			return obj;
+			if (Tasks.TryGetValue (key, out object value)) {
+				Tasks.Remove (key);
+			}
+			return value;
 		}
 
 		BuildEngineResult IBuildEngine3.BuildProjectFilesInParallel (string [] projectFileNames, string [] targetNames, IDictionary [] globalProperties, IList<string> [] removeGlobalProperties, string [] toolsVersion, bool returnTargetOutputs)
diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj
@@ -82,6 +82,7 @@
     <Compile Include="ManagedResourceParserTests.cs" />
     <Compile Include="Tasks\KeyToolTests.cs" />
     <Compile Include="Aapt2Tests.cs" />
+    <Compile Include="Tasks\ValidateJavaVersionTests.cs" />
     <Compile Include="ZipArchiveExTests.cs" />
   </ItemGroup>
   <ItemGroup>

Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@ public class KeyToolTests : BaseTest`
`19`	`19`	`string keyToolPath;`
`20`	`20`
`21`	`21`	`[SetUp]`
`22`		`- public void SetUp()`
	`22`	`+ public void Setup()`
`23`	`23`	`{`
`24`	`24`	`engine = new MockBuildEngine (TestContext.Out, errors = new List<BuildErrorEventArgs> (), warnings = new List<BuildWarningEventArgs> ());`
`25`	`25`	`temp = Path.GetTempFileName ();`
Original file line number	Diff line number	Diff line change
`@@ -2,19 +2,18 @@`
`2`	`2`	`using System.Collections;`
`3`	`3`	`using System.Collections.Generic;`
`4`	`4`	`using System.IO;`
`5`		`-using System.Linq;`
`6`		`-using System.Reflection;`
`7`		`-using System.Text;`
`8`		`-using System.Threading.Tasks;`
`9`	`5`	`using Microsoft.Build.Framework;`
`10`	`6`
`11`		`-namespace Xamarin.Android.Build.Tests {`
`12`		`- public class MockBuildEngine : IBuildEngine, IBuildEngine2, IBuildEngine3, IBuildEngine4 {`
`13`		`- public MockBuildEngine (TextWriter output, IList<BuildErrorEventArgs> errors = null, IList<BuildWarningEventArgs> warnings = null)`
	`7`	`+namespace Xamarin.Android.Build.Tests`
	`8`	`+{`
	`9`	`+ public class MockBuildEngine : IBuildEngine, IBuildEngine2, IBuildEngine3, IBuildEngine4`
	`10`	`+ {`
	`11`	`+ public MockBuildEngine (TextWriter output, IList<BuildErrorEventArgs> errors = null, IList<BuildWarningEventArgs> warnings = null, IList<BuildMessageEventArgs> messages = null)`
`14`	`12`	`{`
`15`		`- this.Output = output;`
`16`		`- this.Errors = errors;`
`17`		`- this.Warnings = warnings;`
	`13`	`+ Output = output;`
	`14`	`+ Errors = errors;`
	`15`	`+ Warnings = warnings;`
	`16`	`+ Messages = messages;`
`18`	`17`	`}`
`19`	`18`
`20`	`19`	`private TextWriter Output { get; }`
`@@ -23,6 +22,8 @@ public MockBuildEngine (TextWriter output, IList<BuildErrorEventArgs> errors = n`
`23`	`22`
`24`	`23`	`private IList<BuildWarningEventArgs> Warnings { get; }`
`25`	`24`
	`25`	`+ private IList<BuildMessageEventArgs> Messages { get; }`
	`26`	`+`
`26`	`27`	`int IBuildEngine.ColumnNumberOfTaskNode => -1;`
`27`	`28`
`28`	`29`	`bool IBuildEngine.ContinueOnError => false;`
`@@ -37,24 +38,26 @@ public MockBuildEngine (TextWriter output, IList<BuildErrorEventArgs> errors = n`
`37`	`38`
`38`	`39`	`void IBuildEngine.LogCustomEvent (CustomBuildEventArgs e)`
`39`	`40`	`{`
`40`		`- this.Output.WriteLine ($"Custom: {e.Message}");`
	`41`	`+ Output.WriteLine ($"Custom: {e.Message}");`
`41`	`42`	`}`
`42`	`43`
`43`	`44`	`void IBuildEngine.LogErrorEvent (BuildErrorEventArgs e)`
`44`	`45`	`{`
`45`		`- this.Output.WriteLine ($"Error: {e.Message}");`
	`46`	`+ Output.WriteLine ($"Error: {e.Message}");`
`46`	`47`	`if (Errors != null)`
`47`	`48`	`Errors.Add (e);`
`48`	`49`	`}`
`49`	`50`
`50`	`51`	`void IBuildEngine.LogMessageEvent (BuildMessageEventArgs e)`
`51`	`52`	`{`
`52`		`- this.Output.WriteLine ($"Message: {e.Message}");`
	`53`	`+ Output.WriteLine ($"Message: {e.Message}");`
	`54`	`+ if (Messages != null)`
	`55`	`+ Messages.Add (e);`
`53`	`56`	`}`
`54`	`57`
`55`	`58`	`void IBuildEngine.LogWarningEvent (BuildWarningEventArgs e)`
`56`	`59`	`{`
`57`		`- this.Output.WriteLine ($"Warning: {e.Message}");`
	`60`	`+ Output.WriteLine ($"Warning: {e.Message}");`
`58`	`61`	`if (Warnings != null)`
`59`	`62`	`Warnings.Add (e);`
`60`	`63`	`}`
`@@ -63,19 +66,21 @@ void IBuildEngine.LogWarningEvent (BuildWarningEventArgs e)`
`63`	`66`
`64`	`67`	`void IBuildEngine4.RegisterTaskObject (object key, object obj, RegisteredTaskObjectLifetime lifetime, bool allowEarlyCollection)`
`65`	`68`	`{`
`66`		`- Tasks.Add (key, obj);`
	`69`	`+ Tasks [key] = obj;`
`67`	`70`	`}`
`68`	`71`
`69`	`72`	`object IBuildEngine4.GetRegisteredTaskObject (object key, RegisteredTaskObjectLifetime lifetime)`
`70`	`73`	`{`
`71`		`- return null;`
	`74`	`+ Tasks.TryGetValue (key, out object value);`
	`75`	`+ return value;`
`72`	`76`	`}`
`73`	`77`
`74`	`78`	`object IBuildEngine4.UnregisterTaskObject (object key, RegisteredTaskObjectLifetime lifetime)`
`75`	`79`	`{`
`76`		`- var obj = Tasks [key];`
`77`		`- Tasks.Remove (key);`
`78`		`- return obj;`
	`80`	`+ if (Tasks.TryGetValue (key, out object value)) {`
	`81`	`+ Tasks.Remove (key);`
	`82`	`+ }`
	`83`	`+ return value;`
`79`	`84`	`}`
`80`	`85`
`81`	`86`	`BuildEngineResult IBuildEngine3.BuildProjectFilesInParallel (string [] projectFileNames, string [] targetNames, IDictionary [] globalProperties, IList<string> [] removeGlobalProperties, string [] toolsVersion, bool returnTargetOutputs)`