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

jpobst · 2025-04-10T19:12:14Z

In order to continue moving forward with moving tasks from the <GenerateJavaStubs> task to our "additional linker steps", we need a more flexible assembly scanning/modification pipeline.

Specifically, we will need to run "RewriteMarshalMethods" in the <AssemblyModifierPipeline> task so that it happens for both linked and non-linked builds. However, we want to run it:

After the FindJavaObjectsStep step in the <AssemblyModifierPipeline> pipeline.
In the same loop as the other assembly modification steps in the <LinkAssembliesNoShrink> task (which is before <AssemblyModifierPipeline> steps run) so that we do not need to write the assemblies twice.

Although we could achieve these goals with enough hacks, instead refactor the steps into a proper, flexible "pipeline".

This pipeline provides the features we need:

Steps from both tasks can be run in any order
Steps from both tasks can modify assemblies, and only 1 write call will happen

Additionally, encapsulate the "should this step run on this assembly" logic to each step, so that the pipeline doesn't need to know about each step's requirements.

…>` tasks to use an actual pipeline.

jonpryor · 2025-04-11T18:46:02Z

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

+		public bool ProcessAssembly (AssemblyDefinition assembly, StepContext context)
+		{
+			// Only run this step on user Android assemblies
+			if (context.IsFrameworkAssembly || !context.IsAndroidAssembly)


This line confuses me: context.isFrameworkAssembly appears to be MonoAndroidHelper.IsFrameworkAssembly():

android/src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.Linker.cs

Lines 21 to 22 in 9ad492a

public static bool IsFrameworkAssembly (string assembly) =>

KnownAssemblyNames.Contains (Path.GetFileNameWithoutExtension (assembly));

android/src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs

Lines 381 to 398 in 9ad492a

public static bool IsFrameworkAssembly (ITaskItem assembly)

{

// Known assembly names: Mono.Android, Java.Interop, etc.

if (IsFrameworkAssembly (assembly.ItemSpec))

return true;

// Known %(FrameworkReferenceName)

var frameworkReferenceName = assembly.GetMetadata ("FrameworkReferenceName") ?? "";

if (frameworkReferenceName == "Microsoft.Android") {

return true; // Microsoft.Android assemblies

}

if (frameworkReferenceName.StartsWith ("Microsoft.NETCore.", StringComparison.OrdinalIgnoreCase)) {

return true; // BCL assemblies

}

// Known %(NuGetPackageId) runtime pack names

return IsFromAKnownRuntimePack (assembly);

}

which looks like it should be true for Mono.Android.dll, and IsAndroidAssembly is set from MonoAndroidHelper.IsAndroidAssembly(), which would also be true for Mono.Andorid.dll.

So I read this as true || !true? And just get confused.

I think our naming needs an improvement. Relatedly: what is "Framework" supposed to mean in "IsFrameworkAssembly"? As per MonoAndroidHelper.Linker.cs, it means "assembly name is one of Mono.Android, Mono.Android.Export, or Java.Interop", but in general "framework" could also mean the entire .NET BCL. I think "Framework" may be causing me more confusion than enlightenment.

Maybe we could add a StepContext.ShouldScanAssemblyForJCWs property, and let AssemblyModifierPipeline.cs sort it out?

I think our naming needs an improvement. Relatedly: what is "Framework" supposed to mean in "IsFrameworkAssembly"? As per MonoAndroidHelper.Linker.cs, it means "assembly name is one of Mono.Android, Mono.Android.Export, or Java.Interop", but in general "framework" could also mean the entire .NET BCL. I think "Framework" may be causing me more confusion than enlightenment.

Agreed that this is confusing (this is what led to the temporary "WasScanned" tracking just so I could verify I translated the logic correctly). Here, there are two dimensions that influence whether this step runs:

IsFrameworkAssembly/IsUserAssembly

Framework: "dotnet" ships it, ex: System.Collections.dll, Mono.Android.dll

User: an assembly the user added, ex: MyApp.dll, Xamarin.AndroidX.Core.dll

IsAndroidAssembly/!IsAndroidAssembly

In this context, this is essentially: can it contain JLO objects or not

So I read this as true || !true? And just get confused.

Correct, the key is the word "user" in the comment. The AddKeepAlives step is only run on user assemblies that can contain JLO objects. We assume that everything we ship "in box" these days has keep alives built in, so we don't need to check things like Mono.Android.

Maybe we could add a StepContext.ShouldScanAssemblyForJCWs property, and let AssemblyModifierPipeline.cs sort it out?

The issue is that different steps have different criteria for whether they need to run.

JavaCallableWrappers: only scans user assemblies for JLOs in Debug mode, scans user and framework assemblies in Release mode

Typemaps: scans both user and framework assemblies for JLOs in both Debug/Release modes

Given this, I am happy with where the logic is, but maybe we can address naming so that it's less confusing. If we have better naming than Framework/User or Android we can certainly go with that. Another way to make it clearer might be to add some compound properties in StepContext like:

public class StepContext { public bool IsAndroidFrameworkAssembly => IsAndroidAssembly && IsFrameworkAssembly; public bool IsAndroidUserAssembly => IsAndroidAssembly && IsUserAssembly; }

Then our check becomes the much clearer:

// Only run this step on user Android assemblies if (!context.IsAndroidUserAssembly) return false;

The decision was to use StepContext.IsAndroidUserAssembly for better clarity here. PR updated.

jonpryor · 2025-04-11T18:51:32Z

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

 {
-	// 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) {


How many "copies" of this list do we have?!

See also MonoAndroidHelper.Linker.cs which has MonoAndroidHelper.KnownAssemblyNames, which includes:

Mono.Android

Mono.Android.Export

Java.Interop

The ones that "don't match" are Mono.Android.Export and Mono.Android.Runtime, which is probably more an oversight than intentional…

Why have/use SpecialAssembles instead of KnownAssemblyNames, in MonoAndroidHelper.Linker.cs?

The unsatisfying answer is I simply moved the existing logic.😉

The ones that "don't match" are Mono.Android.Export and Mono.Android.Runtime, which is probably more an oversight than intentional…

Maybe? We would likely need to audit all users of both lists to see what their intentions are.

Unified the lists and CI isn't complaining, so I guess that's good enough! 🤷‍♂️

src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs

jonathanpeppers

I just had the one comment if we could get rid of some of the #if ILLINK. But seems OK to me as-is.

jonathanpeppers · 2025-04-16T20:44:36Z

src/Xamarin.Android.Build.Tasks/Utilities/AssemblyPipeline.cs

+public interface IAssemblyModifierPipelineStep
+{
+	bool ProcessAssembly (AssemblyDefinition assembly, StepContext context);
+}


Could this go in its own file, so then src\Microsoft.Android.Sdk.ILLink\Microsoft.Android.Sdk.ILLink.csproj could use it?

The biggest issue is that it would also require StepContext which would then require things like ITaskItem which I don't think exist in ILLink.

We would likely need to add more if #ILLINK to add dummy implementations of the missing types.

Ok, if ITaskItem is required, then we shouldn't do it.

jonathanpeppers · 2025-04-16T20:45:14Z

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

+#if !ILLINK
+		, IAssemblyModifierPipelineStep
+#endif  // !ILLINK


If src\Microsoft.Android.Sdk.ILLink\Microsoft.Android.Sdk.ILLink.csproj had IAssemblyModifierPipelineStep.cs we could drop the #if?

jonathanpeppers · 2025-04-16T20:45:30Z

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

+#if !ILLINK
+		public bool ProcessAssembly (AssemblyDefinition assembly, StepContext context)
+		{
+			// Only run this step on user Android assemblies
+			if (!context.IsAndroidUserAssembly)
+				return false;
+
+			return AddKeepAlives (assembly);
+		}
+#endif  // !ILLINK


Same here, maybe we could lose the #if?

jpobst force-pushed the dev/jpobst/new-pipeline branch from c1585c9 to f0627ef Compare April 10, 2025 20:49

[XABT] Refactor <LinkAssembliesNoShrink>/`<AssemblyModifierPipeline…

a89dfa4

…>` tasks to use an actual pipeline.

jpobst force-pushed the dev/jpobst/new-pipeline branch from f0627ef to a89dfa4 Compare April 10, 2025 22:29

jpobst marked this pull request as ready for review April 11, 2025 00:31

jpobst requested review from jonpryor, dellis1972 and jonathanpeppers as code owners April 11, 2025 00:31

jonpryor reviewed Apr 11, 2025

View reviewed changes

src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs Outdated Show resolved Hide resolved

Address review feedback.

b670998

jonathanpeppers approved these changes Apr 16, 2025

View reviewed changes

jpobst merged commit b54ec05 into main Apr 16, 2025
57 of 59 checks passed

jpobst deleted the dev/jpobst/new-pipeline branch April 16, 2025 21:10

jpobst mentioned this pull request Apr 17, 2025

[XABT] Move JLO scanning needed for typemap generation to a linker step. #10015

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jpobst commented Apr 10, 2025 •

edited

Loading

jonpryor Apr 11, 2025

jonpryor Apr 11, 2025

jpobst Apr 11, 2025 •

edited

Loading

jpobst Apr 14, 2025

jonpryor Apr 11, 2025

jpobst Apr 11, 2025

jpobst Apr 14, 2025

jonathanpeppers left a comment

jonathanpeppers Apr 16, 2025

jpobst Apr 16, 2025

jonathanpeppers Apr 16, 2025

jonathanpeppers Apr 16, 2025

jonathanpeppers Apr 16, 2025

	public static bool IsFrameworkAssembly (string assembly) =>
	KnownAssemblyNames.Contains (Path.GetFileNameWithoutExtension (assembly));

	public static bool IsFrameworkAssembly (ITaskItem assembly)
	{
	// Known assembly names: Mono.Android, Java.Interop, etc.
	if (IsFrameworkAssembly (assembly.ItemSpec))
	return true;

	// Known %(FrameworkReferenceName)
	var frameworkReferenceName = assembly.GetMetadata ("FrameworkReferenceName") ?? "";
	if (frameworkReferenceName == "Microsoft.Android") {
	return true; // Microsoft.Android assemblies
	}
	if (frameworkReferenceName.StartsWith ("Microsoft.NETCore.", StringComparison.OrdinalIgnoreCase)) {
	return true; // BCL assemblies
	}

	// Known %(NuGetPackageId) runtime pack names
	return IsFromAKnownRuntimePack (assembly);
	}

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

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

Conversation

jpobst commented Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpobst Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanpeppers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

jpobst commented Apr 10, 2025 •

edited

Loading

jpobst Apr 11, 2025 •

edited

Loading