[XABT] Refactor <LinkAssembliesNoShrink>/<AssemblyModifierPipeline> tasks to use an actual pipeline.

Author: jpobst

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/AddKeepAlivesStep.cs

@@ -11,6 +11,9 @@
 namespace MonoDroid.Tuner
 {
 	public class AddKeepAlivesStep : BaseStep
+#if !ILLINK
+		, IAssemblyModifierPipelineStep
+#endif  // !ILLINK
 	{
 
 		protected override void ProcessAssembly (AssemblyDefinition assembly)
@@ -25,6 +28,17 @@ protected override void ProcessAssembly (AssemblyDefinition assembly)
 			}
 		}
 
+#if !ILLINK
+		public bool ProcessAssembly (AssemblyDefinition assembly, StepContext context)
+		{
+			// Only run this step on user Android assemblies
+			if (context.IsFrameworkAssembly || !context.IsAndroidAssembly)
+				return false;
+
+			return AddKeepAlives (assembly);
+		}
+#endif  // !ILLINK
+
 		internal bool AddKeepAlives (AssemblyDefinition assembly)
 		{
 			if (!assembly.MainModule.HasTypeReference ("Java.Lang.Object"))

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FindJavaObjectsStep.cs

@@ -17,7 +17,7 @@ namespace MonoDroid.Tuner;
 /// <summary>
 /// Scans an assembly for JLOs that need JCWs generated and writes them to an XML file.
 /// </summary>
-public class FindJavaObjectsStep : BaseStep
+public class FindJavaObjectsStep : BaseStep, IAssemblyModifierPipelineStep
 {
 	public string ApplicationJavaClass { get; set; } = "";
 
@@ -29,8 +29,26 @@ public class FindJavaObjectsStep : BaseStep
 
 	public FindJavaObjectsStep (TaskLoggingHelper log) => Log = log;
 
-	public bool ProcessAssembly (AssemblyDefinition assembly, string destinationJLOXml)
+	public bool ProcessAssembly (AssemblyDefinition assembly, StepContext context)
 	{
+		var destinationJLOXml = JavaObjectsXmlFile.GetJavaObjectsXmlFilePath (context.Destination.ItemSpec);
+		var scanned = ScanAssembly (assembly, context, destinationJLOXml);
+
+		if (!scanned) {
+			// We didn't scan for Java objects, so write an empty .xml file for later steps
+			JavaObjectsXmlFile.WriteEmptyFile (destinationJLOXml, Log);
+			return false;
+		}
+
+		// This step does not change the assembly
+		return false;
+	}
+
+	public bool ScanAssembly (AssemblyDefinition assembly, StepContext context, string destinationJLOXml)
+	{
+		if (!ShouldScan (context))
+			return false;
+
 		var action = Annotations.HasAction (assembly) ? Annotations.GetAction (assembly) : AssemblyAction.Skip;
 
 		if (action == AssemblyAction.Delete)
@@ -56,6 +74,25 @@ public bool ProcessAssembly (AssemblyDefinition assembly, string destinationJLOX
 		return true;
 	}
 
+	bool ShouldScan (StepContext context)
+	{
+		if (!context.IsAndroidAssembly)
+			return false;
+
+		// When marshal methods or non-JavaPeerStyle.XAJavaInterop1 are in use we do not want to skip non-user assemblies (such as Mono.Android) - we need to generate JCWs for them during
+		// application build, unlike in Debug configuration or when marshal methods are disabled, in which case we use JCWs generated during Xamarin.Android
+		// build and stored in a jar file.
+		var useMarshalMethods = !context.IsDebug && context.EnableMarshalMethods;
+		var shouldSkipNonUserAssemblies = !useMarshalMethods && context.CodeGenerationTarget == JavaPeerStyle.XAJavaInterop1;
+
+		if (shouldSkipNonUserAssemblies && !context.IsUserAssembly) {
+			Log.LogDebugMessage ($"Skipping assembly '{context.Source.ItemSpec}' because it is not a user assembly and we don't need JLOs from non-user assemblies");
+			return false;
+		}
+
+		return true;
+	}
+
 	List<TypeDefinition> ScanForJavaTypes (AssemblyDefinition assembly)
 	{
 		var types = new List<TypeDefinition> ();

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs

@@ -21,6 +21,9 @@ namespace MonoDroid.Tuner
 	/// NOTE: this step is subclassed so it can be called directly from Xamarin.Android.Build.Tasks
 	/// </summary>
 	public class FixAbstractMethodsStep : BaseMarkHandler
+#if !ILLINK
+		, IAssemblyModifierPipelineStep
+#endif  // !ILLINK
 	{
 		public override void Initialize (LinkContext context, MarkContext markContext)
 		{
@@ -79,6 +82,17 @@ internal bool FixAbstractMethods (AssemblyDefinition assembly)
 			return changed;
 		}
 
+#if !ILLINK
+		public bool ProcessAssembly (AssemblyDefinition assembly, StepContext context)
+		{
+			// Only run this step on non-main user Android assemblies
+			if (context.IsMainAssembly || context.IsFrameworkAssembly || !context.IsAndroidAssembly)
+				return false;
+
+			return FixAbstractMethods (assembly);
+		}
+#endif  // !ILLINK
+
 		readonly HashSet<string> warnedAssemblies = new (StringComparer.Ordinal);
 
 		internal void CheckAppDomainUsage (AssemblyDefinition assembly, Action<string> warn)

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixLegacyResourceDesignerStep.cs

@@ -21,6 +21,9 @@
 namespace MonoDroid.Tuner
 {
 	public class FixLegacyResourceDesignerStep : LinkDesignerBase
+#if !ILLINK
+		, Xamarin.Android.Tasks.IAssemblyModifierPipelineStep
+#endif  // !ILLINK
 	{
 		internal const string DesignerAssemblyName = "_Microsoft.Android.Resource.Designer";
 		internal const string DesignerAssemblyNamespace = "_Microsoft.Android.Resource.Designer";
@@ -67,6 +70,17 @@ protected override void LoadDesigner ()
 			}
 		}
 
+#if !ILLINK
+		public bool ProcessAssembly (AssemblyDefinition assembly, Xamarin.Android.Tasks.StepContext context)
+		{
+			// Only run this step on non-main user Android assemblies
+			if (context.IsMainAssembly || context.IsFrameworkAssembly || !context.IsAndroidAssembly)
+				return false;
+
+			return ProcessAssemblyDesigner (assembly);
+		}
+#endif  // !ILLINK
+
 		internal override bool ProcessAssemblyDesigner (AssemblyDefinition assembly)
 		{
 			if (!FindResourceDesigner (assembly, mainApplication: false, out TypeDefinition designer, out CustomAttribute designerAttribute)) {

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

@@ -20,16 +20,8 @@ namespace Xamarin.Android.Tasks;
 /// are run *after* the linker has run. Additionally, this task is run by
 /// LinkAssembliesNoShrink to modify assemblies when ILLink is not used.
 /// </summary>
-public class AssemblyModifierPipeline : AndroidTask
+public partial class AssemblyModifierPipeline : AndroidTask
 {
-	// Names of assemblies which don't have Mono.Android.dll references, or are framework assemblies, but which must
-	// be scanned for Java types.
-	static readonly HashSet<string> SpecialAssemblies = new HashSet<string> (StringComparer.OrdinalIgnoreCase) {
-			"Java.Interop.dll",
-			"Mono.Android.dll",
-			"Mono.Android.Runtime.dll",
-		};
-
 	public override string TaskPrefix => "AMP";
 
 	public string ApplicationJavaClass { get; set; } = "";
@@ -66,6 +58,12 @@ public class AssemblyModifierPipeline : AndroidTask
 	[Required]
 	public ITaskItem [] SourceFiles { get; set; } = [];
 
+	/// <summary>
+	/// $(TargetName) would be "AndroidApp1" with no extension
+	/// </summary>
+	[Required]
+	public string TargetName { get; set; } = "";
+
 	protected JavaPeerStyle codeGenerationTarget;
 
 	public override bool RunTask ()
@@ -86,14 +84,14 @@ public override bool RunTask ()
 
 		Dictionary<AndroidTargetArch, Dictionary<string, ITaskItem>> perArchAssemblies = MonoAndroidHelper.GetPerArchAssemblies (ResolvedAssemblies, Array.Empty<string> (), validate: false);
 
-		RunState? runState = null;
+		AssemblyPipeline? pipeline = null;
 		var currentArch = AndroidTargetArch.None;
 
 		for (int i = 0; i < SourceFiles.Length; i++) {
 			ITaskItem source = SourceFiles [i];
-			AndroidTargetArch sourceArch = GetValidArchitecture (source);
+			AndroidTargetArch sourceArch = MonoAndroidHelper.GetRequiredValidArchitecture (source);
 			ITaskItem destination = DestinationFiles [i];
-			AndroidTargetArch destinationArch = GetValidArchitecture (destination);
+			AndroidTargetArch destinationArch = MonoAndroidHelper.GetRequiredValidArchitecture (destination);
 
 			if (sourceArch != destinationArch) {
 				throw new InvalidOperationException ($"Internal error: assembly '{sourceArch}' targets architecture '{sourceArch}', while destination assembly '{destination}' targets '{destinationArch}' instead");
@@ -102,147 +100,85 @@ public override bool RunTask ()
 			// Each architecture must have a different set of context classes, or otherwise only the first instance of the assembly may be rewritten.
 			if (currentArch != sourceArch) {
 				currentArch = sourceArch;
-				runState?.Dispose ();
+				pipeline?.Dispose ();
 
 				var resolver = new DirectoryAssemblyResolver (this.CreateTaskLogger (), loadDebugSymbols: ReadSymbols, loadReaderParameters: readerParameters);
-				runState = new RunState (resolver);
 
 				// Add SearchDirectories for the current architecture's ResolvedAssemblies
 				foreach (var kvp in perArchAssemblies [sourceArch]) {
 					ITaskItem assembly = kvp.Value;
 					var path = Path.GetFullPath (Path.GetDirectoryName (assembly.ItemSpec));
-					if (!runState.resolver.SearchDirectories.Contains (path)) {
-						runState.resolver.SearchDirectories.Add (path);
+					if (!resolver.SearchDirectories.Contains (path)) {
+						resolver.SearchDirectories.Add (path);
 					}
 				}
 
 				// Set up the FixAbstractMethodsStep and AddKeepAlivesStep
-				var context = new MSBuildLinkContext (runState.resolver, Log);
+				var context = new MSBuildLinkContext (resolver, Log);
+				pipeline = new AssemblyPipeline (resolver);
 
-				CreateRunState (runState, context);
+				BuildPipeline (pipeline, context);
 			}
 
 			Directory.CreateDirectory (Path.GetDirectoryName (destination.ItemSpec));
 
-			RunPipeline (source, destination, runState!, writerParameters);
+			RunPipeline (pipeline!, source, destination, writerParameters);
 		}
 
-		runState?.Dispose ();
+		pipeline?.Dispose ();
 
 		return !Log.HasLoggedErrors;
 	}
 
-	protected virtual void CreateRunState (RunState runState, MSBuildLinkContext context)
+	protected virtual void BuildPipeline (AssemblyPipeline pipeline, MSBuildLinkContext context)
 	{
+		// FindJavaObjectsStep
 		var findJavaObjectsStep = new FindJavaObjectsStep (Log) {
 			ApplicationJavaClass = ApplicationJavaClass,
 			ErrorOnCustomJavaObject = ErrorOnCustomJavaObject,
 			UseMarshalMethods = EnableMarshalMethods,
 		};
 
 		findJavaObjectsStep.Initialize (context);
-
-		runState.findJavaObjectsStep = findJavaObjectsStep;
+		pipeline.Steps.Add (findJavaObjectsStep);
 	}
 
-	protected virtual void RunPipeline (ITaskItem source, ITaskItem destination, RunState runState, WriterParameters writerParameters)
+	void RunPipeline (AssemblyPipeline pipeline, ITaskItem source, ITaskItem destination, WriterParameters writerParameters)
 	{
-		var destinationJLOXml = JavaObjectsXmlFile.GetJavaObjectsXmlFilePath (destination.ItemSpec);
-
-		if (!TryScanForJavaObjects (source, destination, runState, writerParameters)) {
-			// Even if we didn't scan for Java objects, we still write an empty .xml file for later steps
-			JavaObjectsXmlFile.WriteEmptyFile (destinationJLOXml, Log);
-		}
-	}
-
-	bool TryScanForJavaObjects (ITaskItem source, ITaskItem destination, RunState runState, WriterParameters writerParameters)
-	{
-		if (!ShouldScanAssembly (source))
-			return false;
-
-		var destinationJLOXml = JavaObjectsXmlFile.GetJavaObjectsXmlFilePath (destination.ItemSpec);
-		var assemblyDefinition = runState.resolver!.GetAssembly (source.ItemSpec);
-
-		var scanned = runState.findJavaObjectsStep!.ProcessAssembly (assemblyDefinition, destinationJLOXml);
-
-		return scanned;
-	}
-
-	bool ShouldScanAssembly (ITaskItem source)
-	{
-		// Skip this assembly if it is not an Android assembly
-		if (!IsAndroidAssembly (source)) {
-			Log.LogDebugMessage ($"Skipping assembly '{source.ItemSpec}' because it is not an Android assembly");
-			return false;
-		}
-
-		// When marshal methods or non-JavaPeerStyle.XAJavaInterop1 are in use we do not want to skip non-user assemblies (such as Mono.Android) - we need to generate JCWs for them during
-		// application build, unlike in Debug configuration or when marshal methods are disabled, in which case we use JCWs generated during Xamarin.Android
-		// build and stored in a jar file.
-		var useMarshalMethods = !Debug && EnableMarshalMethods;
-		var shouldSkipNonUserAssemblies = !useMarshalMethods && codeGenerationTarget == JavaPeerStyle.XAJavaInterop1;
-
-		if (shouldSkipNonUserAssemblies && !ResolvedUserAssemblies.Any (a => a.ItemSpec == source.ItemSpec)) {
-			Log.LogDebugMessage ($"Skipping assembly '{source.ItemSpec}' because it is not a user assembly and we don't need JLOs from non-user assemblies");
-			return false;
-		}
-
-		return true;
-	}
-
-	bool IsAndroidAssembly (ITaskItem source)
-	{
-		string name = Path.GetFileName (source.ItemSpec);
-
-		if (SpecialAssemblies.Contains (name))
-			return true;
+		var assembly = pipeline.Resolver.GetAssembly (source.ItemSpec);
+
+		var context = new StepContext (source, destination) {
+			CodeGenerationTarget = codeGenerationTarget,
+			EnableMarshalMethods = EnableMarshalMethods,
+			IsAndroidAssembly = MonoAndroidHelper.IsAndroidAssembly (source),
+			IsDebug = Debug,
+			IsFrameworkAssembly = MonoAndroidHelper.IsFrameworkAssembly (source),
+			IsMainAssembly = Path.GetFileNameWithoutExtension (source.ItemSpec) == TargetName,
+			IsUserAssembly = ResolvedUserAssemblies.Any (a => a.ItemSpec == source.ItemSpec),
+		};
 
-		return MonoAndroidHelper.IsMonoAndroidAssembly (source);
-	}
+		var changed = pipeline.Run (assembly, context);
 
-	AndroidTargetArch GetValidArchitecture (ITaskItem item)
-	{
-		AndroidTargetArch ret = MonoAndroidHelper.GetTargetArch (item);
-		if (ret == AndroidTargetArch.None) {
-			throw new InvalidOperationException ($"Internal error: assembly '{item}' doesn't target any architecture.");
+		if (changed) {
+			Log.LogDebugMessage ($"Saving modified assembly: {destination.ItemSpec}");
+			Directory.CreateDirectory (Path.GetDirectoryName (destination.ItemSpec));
+			writerParameters.WriteSymbols = assembly.MainModule.HasSymbols;
+			assembly.Write (destination.ItemSpec, writerParameters);
+		} else {
+			// If we didn't write a modified file, copy the original to the destination
+			CopyIfChanged (source, destination);
 		}
-
-		return ret;
 	}
 
-	protected sealed class RunState : IDisposable
+	void CopyIfChanged (ITaskItem source, ITaskItem destination)
 	{
-		public DirectoryAssemblyResolver resolver;
-		public FixAbstractMethodsStep? fixAbstractMethodsStep = null;
-		public AddKeepAlivesStep? addKeepAliveStep = null;
-		public FixLegacyResourceDesignerStep? fixLegacyResourceDesignerStep = null;
-		public FindJavaObjectsStep? findJavaObjectsStep = null;
-		bool disposed_value;
-
-		public RunState (DirectoryAssemblyResolver resolver)
-		{
-			this.resolver = resolver;
-		}
-
-		private void Dispose (bool disposing)
-		{
-			if (!disposed_value) {
-				if (disposing) {
-					resolver?.Dispose ();
-					fixAbstractMethodsStep = null;
-					fixLegacyResourceDesignerStep = null;
-					addKeepAliveStep = null;
-					findJavaObjectsStep = null;
-				}
-				disposed_value = true;
-			}
-		}
+		if (MonoAndroidHelper.CopyAssemblyAndSymbols (source.ItemSpec, destination.ItemSpec)) {
+			Log.LogDebugMessage ($"Copied: {destination.ItemSpec}");
+		} else {
+			Log.LogDebugMessage ($"Skipped unchanged file: {destination.ItemSpec}");
 
-		public void Dispose ()
-		{
-			// Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method
-			Dispose (disposing: true);
-			GC.SuppressFinalize (this);
+			// NOTE: We still need to update the timestamp on this file, or this target would run again
+			File.SetLastWriteTimeUtc (destination.ItemSpec, DateTime.UtcNow);
 		}
 	}
 }