Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 16, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a89dfa4
[XABT] Refactor `<LinkAssembliesNoShrink>`/`<AssemblyModifierPipeline…
jpobst Apr 10, 2025
b670998
Address review feedback.
jpobst Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/AddKeepAlivesStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
namespace MonoDroid.Tuner
{
public class AddKeepAlivesStep : BaseStep
#if !ILLINK
, IAssemblyModifierPipelineStep
#endif // !ILLINK
Comment on lines +14 to +16
Copy link
Member

@jonathanpeppers jonathanpeppers Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{

protected override void ProcessAssembly (AssemblyDefinition assembly)
Expand All @@ -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)
Copy link
Contributor

@jonpryor jonpryor Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jonpryor jonpryor Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jpobst jpobst Apr 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

@jpobst jpobst Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return false;

return AddKeepAlives (assembly);
}
#endif // !ILLINK

internal bool AddKeepAlives (AssemblyDefinition assembly)
{
if (!assembly.MainModule.HasTypeReference ("Java.Lang.Object"))
Expand Down
41 changes: 39 additions & 2 deletions src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FindJavaObjectsStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; } = "";

Expand All @@ -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)
Expand All @@ -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> ();
Expand Down
14 changes: 14 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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)
Expand Down
14 changes: 14 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixLegacyResourceDesignerStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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)) {
Expand Down
162 changes: 49 additions & 113 deletions src/Xamarin.Android.Build.Tasks/Tasks/AssemblyModifierPipeline.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

@jonpryor jonpryor Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

Copy link
Contributor Author

@jpobst jpobst Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jpobst jpobst Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"Java.Interop.dll",
"Mono.Android.dll",
"Mono.Android.Runtime.dll",
};

public override string TaskPrefix => "AMP";

public string ApplicationJavaClass { get; set; } = "";
Expand Down Expand Up @@ -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 ()
Expand All @@ -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");
Expand All @@ -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);
}
}
}
Loading
Loading