[msbuild] Port the ALToolUpload and ALToolValidate tasks to subclass …

…XamarinTask.

This has a few advantages:

* We simplify and unify more of our code.
* We have more control over the error reporting / logging behavior.

Additionally:

* Use 'xcrun' to invoke 'altool' (partial fix for #3931).
* Allow for overriding the path to the command-line tool in question.
* Add support for cancellation.
* Fix nullability.

docs/build-apps/build-properties.md

@@ -11,6 +11,12 @@ MSBuild properties control the behavior of the
 They're specified within the project file, for example **MyApp.csproj**, within
 an MSBuild PropertyGroup.
 
+## AltoolPath
+
+The full path to the `altool` tool.
+
+The default behavior is to use `xcrun altool`.
+
 ## AppIcon
 
 The `AppIcon` item group can be used to specify an app icon for the app.

msbuild/Xamarin.MacDev.Tasks/Tasks/ALToolUpload.cs

@@ -1,24 +1,7 @@
-using Microsoft.Build.Framework;
-using Xamarin.Messaging.Build.Client;
+#nullable enable
 
 namespace Xamarin.MacDev.Tasks {
-	public class ALToolUpload : ALToolTaskBase, ICancelableTask {
+	public class ALToolUpload : ALToolTaskBase {
 		protected override string ALToolAction => "--upload-app";
-
-		public override bool Execute ()
-		{
-			if (ShouldExecuteRemotely ())
-				return new TaskRunner (SessionId, BuildEngine4).RunAsync (this).Result;
-
-			return base.Execute ();
-		}
-
-		public override void Cancel ()
-		{
-			if (ShouldExecuteRemotely ())
-				BuildConnection.CancelAsync (BuildEngine4).Wait ();
-
-			base.Cancel ();
-		}
 	}
 }

msbuild/Xamarin.MacDev.Tasks/Tasks/ALToolValidate.cs

@@ -1,24 +1,7 @@
-using Microsoft.Build.Framework;
-using Xamarin.Messaging.Build.Client;
+#nullable enable
 
 namespace Xamarin.MacDev.Tasks {
-	public class ALToolValidate : ALToolTaskBase, ICancelableTask {
+	public class ALToolValidate : ALToolTaskBase {
 		protected override string ALToolAction => "--validate-app";
-
-		public override bool Execute ()
-		{
-			if (ShouldExecuteRemotely ())
-				return new TaskRunner (SessionId, BuildEngine4).RunAsync (this).Result;
-
-			return base.Execute ();
-		}
-
-		public override void Cancel ()
-		{
-			if (ShouldExecuteRemotely ())
-				BuildConnection.CancelAsync (BuildEngine4).Wait ();
-
-			base.Cancel ();
-		}
 	}
 }

msbuild/Xamarin.MacDev.Tasks/Tasks/AlTool.cs

@@ -1,92 +1,80 @@
 using System;
+using System.Collections.Generic;
 using System.IO;
+using System.Threading;
+using System.Threading.Tasks;
 
 using Microsoft.Build.Utilities;
 using Microsoft.Build.Framework;
 using System.Text;
 
 using Xamarin.Utils;
 using Xamarin.Localization.MSBuild;
-
-// Disable until we get around to enable + fix any issues.
-#nullable disable
+using Xamarin.Messaging.Build.Client;
 
 namespace Xamarin.MacDev.Tasks {
-	public abstract class ALToolTaskBase : XamarinToolTask {
-		string sdkDevPath;
-		StringBuilder toolOutput;
+	public abstract class ALToolTaskBase : XamarinTask, ICancelableTask {
+		CancellationTokenSource? cancellationTokenSource;
 
-		[Required]
-		public string Username { get; set; }
+		public string AltoolPath { get; set; } = string.Empty;
 
 		[Required]
-		public string Password { get; set; }
+		public string Username { get; set; } = string.Empty;
 
 		[Required]
-		public string FilePath { get; set; }
-
-		protected override string ToolName {
-			get { return "altool"; }
-		}
+		public string Password { get; set; } = string.Empty;
 
 		[Required]
-		public string SdkDevPath {
-			get { return sdkDevPath; }
-			set {
-				sdkDevPath = value;
+		public string FilePath { get; set; } = string.Empty;
+
+		static string GetExecutable (List<string> arguments, string toolName, string toolPathOverride)
+		{
+			if (string.IsNullOrEmpty (toolPathOverride)) {
+				arguments.Insert (0, toolName);
+				return "xcrun";
 			}
+			return toolPathOverride;
 		}
 
-		string DevicePlatformBinDir {
-			get { return Path.Combine (SdkDevPath, "usr", "bin"); }
-		}
+		[Required]
+		public string SdkDevPath { get; set; } = string.Empty;
 
 		protected abstract string ALToolAction { get; }
 
 		public override bool Execute ()
 		{
-			toolOutput = new StringBuilder ();
+			if (ShouldExecuteRemotely ())
+				return new TaskRunner (SessionId, BuildEngine4).RunAsync (this).Result;
 
-			base.Execute ();
+			var args = GenerateCommandLineCommands ();
+			var executable = GetExecutable (args, "altool", AltoolPath);
 
-			LogErrorsFromOutput (toolOutput.ToString ());
+			if (Log.HasLoggedErrors)
+				return false;
 
-			return !HasLoggedErrors;
+			cancellationTokenSource = new CancellationTokenSource ();
+			var rv = ExecuteAsync (Log, executable, args, sdkDevPath: SdkDevPath, cancellationToken: cancellationTokenSource.Token).Result;
+			LogErrorsFromOutput (rv.StandardOutput?.ToString ());
+			return !Log.HasLoggedErrors;
 		}
 
-		protected override string GenerateFullPathToTool ()
+		protected virtual List<string> GenerateCommandLineCommands ()
 		{
-			if (!string.IsNullOrEmpty (ToolPath))
-				return Path.Combine (ToolPath, ToolExe);
-
-			var path = Path.Combine (DevicePlatformBinDir, ToolExe);
-
-			return File.Exists (path) ? path : ToolExe;
-		}
-
-		protected override string GenerateCommandLineCommands ()
-		{
-			var args = new CommandLineArgumentBuilder ();
+			var args = new List<string> ();
 
 			args.Add (ALToolAction);
 			args.Add ("--file");
-			args.AddQuoted (FilePath);
+			args.Add (FilePath);
 			args.Add ("--type");
-			args.AddQuoted (GetFileTypeValue ());
+			args.Add (GetFileTypeValue ());
 			args.Add ("--username");
-			args.AddQuoted (Username);
+			args.Add (Username);
 			args.Add ("--password");
-			args.AddQuoted (Password);
+			args.Add (Password);
 			args.Add ("--output-format");
 			args.Add ("xml");
 
-			return args.ToString ();
-		}
-
-		protected override void LogEventsFromTextOutput (string singleLine, MessageImportance messageImportance)
-		{
-			toolOutput.Append (singleLine);
-			Log.LogMessage (messageImportance, "{0}", singleLine);
+			return args;
 		}
 
 		string GetFileTypeValue ()
@@ -99,11 +87,16 @@ string GetFileTypeValue ()
 			}
 		}
 
-		void LogErrorsFromOutput (string output)
+		void LogErrorsFromOutput (string? output)
 		{
 			try {
+#if NET
 				if (string.IsNullOrEmpty (output))
 					return;
+#else
+				if (output is null || string.IsNullOrEmpty (output))
+					return;
+#endif
 
 				var plist = PObject.FromString (output) as PDictionary;
 				var errors = PObject.Create (PObjectType.Array) as PArray;
@@ -113,13 +106,22 @@ void LogErrorsFromOutput (string output)
 					foreach (var error in errors) {
 						var dict = error as PDictionary;
 						if (dict?.TryGetValue ("message", out message) == true) {
-							Log.LogError (ToolName, null, null, null, 0, 0, 0, 0, "{0}", message.Value);
+							Log.LogError ("altool", null, null, null, 0, 0, 0, 0, "{0}", message.Value);
 						}
 					}
 				}
 			} catch (Exception ex) {
 				Log.LogWarning (MSBStrings.W0095, ex.Message, output);
 			}
 		}
+
+		public void Cancel ()
+		{
+			if (ShouldExecuteRemotely ()) {
+				BuildConnection.CancelAsync (BuildEngine4).Wait ();
+			} else {
+				cancellationTokenSource?.Cancel ();
+			}
+		}
 	}
 }

msbuild/Xamarin.Shared/Xamarin.iOS.Common.targets

@@ -94,6 +94,7 @@ Copyright (C) 2013-2016 Xamarin. All rights reserved.
 	<Target Name="ValidateAppStoreBundle" DependsOnTargets="_DetectSdkLocations;_ComputeTargetFrameworkMoniker">
 		<ALToolValidate
 			SessionId="$(BuildSessionId)"
+			AltoolPath="$(AltoolPath)"
 			Username="$(Username)"
 			Password="$(Password)"
 			FilePath="$(FilePath)"
@@ -105,6 +106,7 @@ Copyright (C) 2013-2016 Xamarin. All rights reserved.
 	<Target Name="UploadAppStoreBundle" DependsOnTargets="_DetectSdkLocations;_ComputeTargetFrameworkMoniker">
 		<ALToolUpload
 			SessionId="$(BuildSessionId)"
+			AltoolPath="$(AltoolPath)"
 			Username="$(Username)"
 			Password="$(Password)"
 			FilePath="$(FilePath)"