[Xamarin.Android.Build.Tasks] Rework Aapt log processing. (#1153)

Fixes: #1134

`aapt` is pretty inconsistent with how it reports errors and
warnings; for example, warnings are written to stderr.

This is a bit of a conundrum:

  * Messages *may* contain `error` or `warning`,
    but they might not (no level specified; 2135856).
  * Messages written to stderr may be warnings, *not* errors.
  * The `aapt` return value may be non-zero when warnings are
    written but output files are still generated.

In particular, consider Issue #1134: `aapt` writes to stderr:

	max res 10, skipping values-sw720dp-land-v13 "max res 10, skipping values-sw720dp-land-v13".

This was previously being reported as an error, "simply" because it
was written to stderr, but the above is *not* an error.

Rework `aapt` output processing to hopefully handle errors and
warnings in a more consistent way. Instead of processing the output
in realtime we will instead buffer it and process the output once
`aapt` has completed. This will allow us to check to see if the
required output file (`R.java`) was created.

If `R.java` *is* created, messages written to stderr without an
explicit level will be treated as warnings.

If `R.java` is *not* created, messages written to stderr without an
explicit level will be treated as *errors*.

src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs

@@ -93,14 +93,25 @@ public class Aapt : AsyncTask
 		Dictionary<string,string> resource_name_case_map = new Dictionary<string,string> ();
 		AssemblyIdentityMap assemblyMap = new AssemblyIdentityMap ();
 
+		struct OutputLine {
+			public string Line;
+			public bool StdError;
+
+			public OutputLine (string line, bool stdError)
+			{
+				Line = line;
+				StdError = stdError;
+			}
+		}
+
 		bool ManifestIsUpToDate (string manifestFile)
 		{
 			return !String.IsNullOrEmpty (AndroidComponentResgenFlagFile) &&
 				File.Exists (AndroidComponentResgenFlagFile) && File.Exists (manifestFile) &&
 				File.GetLastWriteTime (AndroidComponentResgenFlagFile) > File.GetLastWriteTime (manifestFile);
 		}
 
-		bool RunAapt (string commandLine)
+		bool RunAapt (string commandLine, IList<OutputLine> output)
 		{
 			var stdout_completed = new ManualResetEvent (false);
 			var stderr_completed = new ManualResetEvent (false);
@@ -117,13 +128,13 @@ bool RunAapt (string commandLine)
 			using (var proc = new Process ()) {
 				proc.OutputDataReceived += (sender, e) => {
 					if (e.Data != null)
-						LogMessage (e.Data, MessageImportance.Normal);
+						output.Add (new OutputLine (e.Data, stdError: false));
 					else
 						stdout_completed.Set ();
 				};
 				proc.ErrorDataReceived += (sender, e) => {
 					if (e.Data != null)
-						LogEventsFromTextOutput (e.Data, MessageImportance.Normal);
+						output.Add (new OutputLine (e.Data, stdError: true));
 					else
 						stderr_completed.Set ();
 				};
@@ -149,7 +160,16 @@ bool RunAapt (string commandLine)
 
 		bool ExecuteForAbi (string cmd, string currentResourceOutputFile)
 		{
-			var ret = RunAapt (cmd);
+			var output = new List<OutputLine> ();
+			var ret = RunAapt (cmd, output);
+			var success = File.Exists (Path.Combine (JavaDesignerOutputDirectory, "R.java"));
+			foreach (var line in output) {
+				if (line.StdError) {
+					LogEventsFromTextOutput (line.Line, MessageImportance.Normal, success);
+				} else {
+					LogMessage (line.Line, MessageImportance.Normal);
+				}
+			}
 			if (ret && !string.IsNullOrEmpty (currentResourceOutputFile)) {
 				var tmpfile = currentResourceOutputFile + ".bk";
 				MonoAndroidHelper.CopyIfZipChanged (tmpfile, currentResourceOutputFile);
@@ -357,7 +377,7 @@ protected string GenerateFullPathToTool ()
 			return Path.Combine (ToolPath, string.IsNullOrEmpty (ToolExe) ? ToolName : ToolExe);
 		}
 
-		protected void LogEventsFromTextOutput (string singleLine, MessageImportance messageImportance)
+		protected void LogEventsFromTextOutput (string singleLine, MessageImportance messageImportance, bool apptResult)
 		{
 			if (string.IsNullOrEmpty (singleLine)) 
 				return;
@@ -394,7 +414,11 @@ protected void LogEventsFromTextOutput (string singleLine, MessageImportance mes
 				}
 			}
 
-			LogError ("APT0000", string.Format("{0} \"{1}\".", singleLine.Trim(), singleLine.Substring(singleLine.LastIndexOfAny(new char[] { '\\', '/' }) + 1)), ToolName);
+			if (!apptResult) {
+				LogError ("APT0000", string.Format ("{0} \"{1}\".", singleLine.Trim (), singleLine.Substring (singleLine.LastIndexOfAny (new char [] { '\\', '/' }) + 1)), ToolName);
+			} else {
+				LogWarning (singleLine);
+			}
 		}
 	}
 }

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidRegExTests.cs

@@ -74,6 +74,14 @@ public IEnumerator GetEnumerator ()
 					/*expectedLevel*/	"",
 					/*expectedMessage*/	"max res 10, skipping values-sw600dp-land"
 				};
+				yield return new object [] {
+ 					/*message*/		"max res 10, skipping values-sw720dp-land-v13",
+ 					/*expectedToMatch*/	true,
+ 					/*expectedFile*/	"",
+ 					/*expectedLine*/	"",
+ 					/*expectedLevel*/	"",
+ 					/*expectedMessage*/	"max res 10, skipping values-sw720dp-land-v13"
+				};
 				yield return new object [] {
 					/*message*/		"Error: unable to generate entry for resource data",
 					/*expectedToMatch*/	true,

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidUpdateResourcesTest.cs

@@ -183,6 +183,23 @@ public void ReportAaptErrorsInOriginalFileName ()
 			}
 		}
 
+		[Test]
+		public void ReportAaptWarningsForBlankLevel ()
+		{
+			//This test should get the warning `Invalid file name: must contain only [a-z0-9_.]`
+			//    However, <Aapt /> still fails due to aapt failing, Resource.designer.cs is not generated
+			var proj = new XamarinAndroidApplicationProject ();
+			proj.AndroidResources.Add (new AndroidItem.AndroidResource ("Resources\\drawable\\Image (1).png") {
+				BinaryContent = () => XamarinAndroidCommonProject.icon_binary_mdpi
+			});
+			using (var b = CreateApkBuilder ("temp/ReportAaptWarningsForBlankLevel")) {
+				b.ThrowOnBuildFailure = false;
+				Assert.IsFalse (b.Build (proj), "Build should have failed.");
+				StringAssertEx.Contains ("APT0000", b.LastBuildOutput, "An error message with a blank \"level\", should be reported as an error!");
+				Assert.IsTrue (b.Clean (proj), "Clean should have succeeded.");
+			}
+		}
+
 		[Test]
 		public void RepetiviteBuildUpdateSingleResource ()
 		{