Skip to content

Commit

Permalink
Add *Task.ProjectSpecificTaskObjectKey() for RegisterTaskObject() use (
Browse files Browse the repository at this point in the history
…#202)

Context: dotnet/maui#11605
Context: dotnet/maui#11387 (comment)
Context: http://github.com/xamarin/xamarin-android/commit/8bc7a3e84f95e70fe12790ac31ecd97957771cb2

In dotnet/maui#11605, when `$(AndroidEnableMarshalMethods)`=True
(xamarin/xamarin-android@8bc7a3e8), the build may fail if the `.sln`
contains more than one Android "App" project.  We tracked this down
to "undesired sharing" between project builds; the `obj` provided to
[`IBuildEngine4.RegisterTaskObject()`][0] can be visible across
project builds.  Consider [`<GeneratePackageManagerJava/>`][1]:

	var marshalMethodsState = BuildEngine4.GetRegisteredTaskObjectAssemblyLocal<MarshalMethodsState> (".:!MarshalMethods!:.", RegisteredTaskObjectLifetime.Build);

Further consider a `.sln` with two "App" projects, as the "App"
project build hits `<GeneratePackageManagerJava/>`.
The lifetime of `.Build` is *not* tied to the the `Build` target of a
given `.csproj`; rather, it's for the *build process*.  This can
result in:

 1. `dotnet build Project.sln` is run; `Project.sln` references
    `App1.csproj` and `App2.csproj`.
 2. `App1.csproj` is built.
 3. `App1.csproj` calls `<GeneratePackageManagerJava/>`.
 4. `App2.csproj` is later built as part of the process, and *also*
    calls `<GeneratePackageManagerJava/>`.

In particular note the key within `<GeneratePackageManagerJava/>`:
`".:!MarshalMethods!:."`.  This value is identical in all projects,
and means that that when `App2.csproj` is built, it will be using the
same key as was used with `App1.csproj`, and thus could be
inadvertently using data intended for `App1.csproj`!

This would result build errors:

	…\Xamarin.Android.Common.targets(1717,3): error XAGPM7009: System.InvalidOperationException: Unable to translate assembly name 'Xamarin.AndroidX.Activity' to its index
	…\Xamarin.Android.Common.targets(1717,3): error XAGPM7009:    at Xamarin.Android.Tasks.MarshalMethodsNativeAssemblyGenerator.WriteNativeMethods(LlvmIrGenerator generator, Dictionary`2 asmNameToIndex, LlvmIrVariableReference get_function_pointer_ref)
	…\Xamarin.Android.Common.targets(1717,3): error XAGPM7009:    at Xamarin.Android.Tasks.MarshalMethodsNativeAssemblyGenerator.Write(LlvmIrGenerator generator)
	…\Xamarin.Android.Common.targets(1717,3): error XAGPM7009:    at Xamarin.Android.Tasks.LLVMIR.LlvmIrComposer.Write(AndroidTargetArch arch, StreamWriter output, String fileName)
	…\Xamarin.Android.Common.targets(1717,3): error XAGPM7009:    at Xamarin.Android.Tasks.GeneratePackageManagerJava.AddEnvironment()
	…\Xamarin.Android.Common.targets(1717,3): error XAGPM7009:    at Xamarin.Android.Tasks.GeneratePackageManagerJava.RunTask()

The sharing of `RegisterTaskObject()` data across project builds is
rarely desirable.  There are a few instances where it is safe to share
the registered objects between projects, e.g. `java -version` version
information (keyed on `java` path).  However, most of the time it is
specific to the project that is being built.  Historically we have
probably got away with this because "most" users only have one project.

Update `AndroidTask` and `AndroidToolTask` to capture the current
directory in a `WorkingDirectory` property like [`AsyncTask`][2] does.
Introduce new `ProjectSpecificTaskObjectKey()` instance methods into
`AndroidTask`, `AndroidAsyncTask`, and `AndroidToolTask` which can be
used to generate a key which includes `WorkingDirectory`.  This allows:

	var marshalMethodsState = BuildEngine4.GetRegisteredTaskObjectAssemblyLocal<MarshalMethodsState> (
	        ProjectSpecificTaskObjectKey (".:!MarshalMethods!:."),
	        RegisteredTaskObjectLifetime.Build
	);

When `ProjectSpecificTaskObjectKey()` is used the `WorkingDirectory`
is included in the key with `RegisterTaskObject()`.  This helps ensure
that builds in different `.csproj` files will result in different keys.

[0]: https://learn.microsoft.com/en-us/dotnet/api/microsoft.build.framework.ibuildengine4.registertaskobject?view=msbuild-17-netcore
[1]: https://github.com/xamarin/xamarin-android/blob/c92ae5eb9fdcb3a2fd7c20f5b42dddf8b3ea781a/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs#L407
[2]: https://github.com/xamarin/Xamarin.Build.AsyncTask/blob/8b5fc6c4d13a3dfd1d17a2007e2143b6da3447d7/Xamarin.Build.AsyncTask/AsyncTask.cs#L59
  • Loading branch information
dellis1972 committed Jan 18, 2023
1 parent ac9ea09 commit 099fd95
Show file tree
Hide file tree
Showing 6 changed files with 245 additions and 1 deletion.
2 changes: 2 additions & 0 deletions src/Microsoft.Android.Build.BaseTasks/AndroidAsyncTask.cs
Expand Up @@ -46,5 +46,7 @@ public virtual bool RunTask ()
/// * RunTaskAsync is already on a background thread
/// </summary>
public virtual System.Threading.Tasks.Task RunTaskAsync () => System.Threading.Tasks.Task.CompletedTask;

protected object ProjectSpecificTaskObjectKey (object key) => (key, WorkingDirectory);
}
}
10 changes: 10 additions & 0 deletions src/Microsoft.Android.Build.BaseTasks/AndroidTask.cs
@@ -1,6 +1,7 @@
// https://github.com/xamarin/xamarin-android/blob/9fca138604c53989e1cff7fc0c2e939583b4da28/src/Xamarin.Android.Build.Tasks/Tasks/AndroidTask.cs#L10

using System;
using System.IO;
using Microsoft.Build.Utilities;

namespace Microsoft.Android.Build.Tasks
Expand All @@ -11,6 +12,13 @@ public abstract class AndroidTask : Task
{
public abstract string TaskPrefix { get; }

protected string WorkingDirectory { get; private set; }

public AndroidTask ()
{
WorkingDirectory = Directory.GetCurrentDirectory();
}

public override bool Execute ()
{
try {
Expand All @@ -22,5 +30,7 @@ public override bool Execute ()
}

public abstract bool RunTask ();

protected object ProjectSpecificTaskObjectKey (object key) => (key, WorkingDirectory);
}
}
10 changes: 10 additions & 0 deletions src/Microsoft.Android.Build.BaseTasks/AndroidToolTask.cs
@@ -1,6 +1,7 @@
// https://github.com/xamarin/xamarin-android/blob/9fca138604c53989e1cff7fc0c2e939583b4da28/src/Xamarin.Android.Build.Tasks/Tasks/AndroidTask.cs#L75

using System;
using System.IO;
using Microsoft.Build.Utilities;

namespace Microsoft.Android.Build.Tasks
Expand All @@ -9,6 +10,13 @@ public abstract class AndroidToolTask : ToolTask
{
public abstract string TaskPrefix { get; }

protected string WorkingDirectory { get; private set; }

public AndroidToolTask ()
{
WorkingDirectory = Directory.GetCurrentDirectory();
}

public override bool Execute ()
{
try {
Expand All @@ -22,5 +30,7 @@ public override bool Execute ()
// Most ToolTask's do not override Execute and
// just expect the base to be called
public virtual bool RunTask () => base.Execute ();

protected object ProjectSpecificTaskObjectKey (object key) => (key, WorkingDirectory);
}
}
22 changes: 21 additions & 1 deletion src/Microsoft.Android.Build.BaseTasks/MSBuildExtensions.cs
Expand Up @@ -251,18 +251,30 @@ public static void SetDestinationSubPath (this ITaskItem assembly)

/// <summary>
/// IBuildEngine4.RegisterTaskObject, but adds the current assembly path into the key
/// The `key` should be unique to a project unless it is a global item.
/// Ideally the key should be the full path of a file in the project directory structure.
/// Or you can use the `ProjectSpecificTaskObjectKey` method of the `AndroidTask` to generate
/// a project specific key if needed.
/// </summary>
public static void RegisterTaskObjectAssemblyLocal (this IBuildEngine4 engine, object key, object value, RegisteredTaskObjectLifetime lifetime, bool allowEarlyCollection = false) =>
engine.RegisterTaskObject ((AssemblyLocation, key), value, lifetime, allowEarlyCollection);

/// <summary>
/// IBuildEngine4.GetRegisteredTaskObject, but adds the current assembly path into the key
/// The `key` should be unique to a project unless it is a global item.
/// Ideally the key should be the full path of a file in the project directory structure.
/// Or you can use the `ProjectSpecificTaskObjectKey` method of the `AndroidTask` to generate
/// a project specific key if needed.
/// </summary>
public static object GetRegisteredTaskObjectAssemblyLocal (this IBuildEngine4 engine, object key, RegisteredTaskObjectLifetime lifetime) =>
engine.GetRegisteredTaskObject ((AssemblyLocation, key), lifetime);

/// <summary>
/// Generic version of IBuildEngine4.GetRegisteredTaskObject, but adds the current assembly path into the key
/// The `key` should be unique to a project unless it is a global item.
/// Ideally the key should be the full path of a file in the project directory structure.
/// Or you can use the `ProjectSpecificTaskObjectKey` method of the `AndroidTask` to generate
/// a project specific key if needed.
/// </summary>
public static T GetRegisteredTaskObjectAssemblyLocal<T> (this IBuildEngine4 engine, object key, RegisteredTaskObjectLifetime lifetime)
where T : class =>
Expand All @@ -271,12 +283,20 @@ public static T GetRegisteredTaskObjectAssemblyLocal<T> (this IBuildEngine4 engi

/// <summary>
/// IBuildEngine4.UnregisterTaskObject, but adds the current assembly path into the key
/// The `key` should be unique to a project unless it is a global item.
/// Ideally the key should be the full path of a file in the project directory structure.
/// Or you can use the `ProjectSpecificTaskObjectKey` method of the `AndroidTask` to generate
/// a project specific key if needed.
/// </summary>
public static object UnregisterTaskObjectAssemblyLocal (this IBuildEngine4 engine, object key, RegisteredTaskObjectLifetime lifetime) =>
engine.UnregisterTaskObject ((AssemblyLocation, key), lifetime);

/// <summary>
/// Generic version of IBuildEngine4.UnregisterTaskObject, but adds the current assembly path into the key
/// Generic version of IBuildEngine4.UnregisterTaskObject, but adds the current assembly path into the key.
/// The `key` should be unique to a project unless it is a global item.
/// Ideally the key should be the full path of a file in the project directory structure.
/// Or you can use the `ProjectSpecificTaskObjectKey` method of the `AndroidTask` to generate
/// a project specific key if needed.
/// </summary>
public static T UnregisterTaskObjectAssemblyLocal<T> (this IBuildEngine4 engine, object key, RegisteredTaskObjectLifetime lifetime)
where T : class =>
Expand Down
100 changes: 100 additions & 0 deletions tests/Microsoft.Android.Build.BaseTasks-Tests/AndroidToolTaskTests.cs
@@ -0,0 +1,100 @@
using System.IO;
using System.Collections.Generic;
using System.Threading.Tasks;
using Microsoft.Android.Build.BaseTasks.Tests.Utilities;
using Microsoft.Android.Build.Tasks;
using NUnit.Framework;
using Microsoft.Build.Framework;
using Xamarin.Build;

namespace Microsoft.Android.Build.BaseTasks.Tests
{
[TestFixture]
public class AndroidToolTaskTests
{
public class MyAndroidTask : AndroidTask {
public override string TaskPrefix {get;} = "MAT";
public string Key { get; set; }
public string Value { get; set; }
public bool ProjectSpecific { get; set; } = false;
public override bool RunTask ()
{
var key = ProjectSpecific ? ProjectSpecificTaskObjectKey (Key) : (Key, (object)string.Empty);
BuildEngine4.RegisterTaskObjectAssemblyLocal (key, Value, RegisteredTaskObjectLifetime.Build);
return true;
}
}

public class MyOtherAndroidTask : AndroidTask {
public override string TaskPrefix {get;} = "MOAT";
public string Key { get; set; }
public bool ProjectSpecific { get; set; } = false;

[Output]
public string Value { get; set; }
public override bool RunTask ()
{
var key = ProjectSpecific ? ProjectSpecificTaskObjectKey (Key) : (Key, (object)string.Empty);
Value = BuildEngine4.GetRegisteredTaskObjectAssemblyLocal<string> (key, RegisteredTaskObjectLifetime.Build);
return true;
}
}

[Test]
[TestCase (true, true, true)]
[TestCase (false, false, true)]
[TestCase (true, false, false)]
[TestCase (false, true, false)]
public void TestRegisterTaskObjectCanRetrieveCorrectItem (bool projectSpecificA, bool projectSpecificB, bool expectedResult)
{
var engine = new MockBuildEngine (TestContext.Out) {
};
var task = new MyAndroidTask () {
BuildEngine = engine,
Key = "Foo",
Value = "Foo",
ProjectSpecific = projectSpecificA,
};
task.Execute ();
var task2 = new MyOtherAndroidTask () {
BuildEngine = engine,
Key = "Foo",
ProjectSpecific = projectSpecificB,
};
task2.Execute ();
Assert.AreEqual (expectedResult, string.Compare (task2.Value, task.Value, ignoreCase: true) == 0);
}

[Test]
[TestCase (true, true, false)]
[TestCase (false, false, true)]
[TestCase (true, false, false)]
[TestCase (false, true, false)]
public void TestRegisterTaskObjectFailsWhenDirectoryChanges (bool projectSpecificA, bool projectSpecificB, bool expectedResult)
{
var engine = new MockBuildEngine (TestContext.Out) {
};
MyAndroidTask task;
var currentDir = Directory.GetCurrentDirectory ();
Directory.SetCurrentDirectory (Path.Combine (currentDir, ".."));
try {
task = new MyAndroidTask () {
BuildEngine = engine,
Key = "Foo",
Value = "Foo",
ProjectSpecific = projectSpecificA,
};
} finally {
Directory.SetCurrentDirectory (currentDir);
}
task.Execute ();
var task2 = new MyOtherAndroidTask () {
BuildEngine = engine,
Key = "Foo",
ProjectSpecific = projectSpecificB,
};
task2.Execute ();
Assert.AreEqual (expectedResult, string.Compare (task2.Value, task.Value, ignoreCase: true) == 0);
}
}
}
@@ -0,0 +1,102 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
using Microsoft.Build.Framework;

namespace Microsoft.Android.Build.BaseTasks.Tests.Utilities {
public class MockBuildEngine : IBuildEngine, IBuildEngine2, IBuildEngine3, IBuildEngine4 {
public MockBuildEngine (TextWriter output, IList<BuildErrorEventArgs> errors = null, IList<BuildWarningEventArgs> warnings = null, IList<BuildMessageEventArgs> messages = null, IList<CustomBuildEventArgs> customEvents = null)
{
this.Output = output;
this.Errors = errors;
this.Warnings = warnings;
this.Messages = messages;
this.CustomEvents = customEvents;
}

private TextWriter Output { get; }

private IList<BuildErrorEventArgs> Errors { get; }

private IList<BuildWarningEventArgs> Warnings { get; }

private IList<BuildMessageEventArgs> Messages { get; }

private IList<CustomBuildEventArgs> CustomEvents { get; }

int IBuildEngine.ColumnNumberOfTaskNode => -1;

bool IBuildEngine.ContinueOnError => false;

int IBuildEngine.LineNumberOfTaskNode => -1;

string IBuildEngine.ProjectFileOfTaskNode => "this.xml";

bool IBuildEngine2.IsRunningMultipleNodes => false;

bool IBuildEngine.BuildProjectFile (string projectFileName, string [] targetNames, IDictionary globalProperties, IDictionary targetOutputs) => true;

void IBuildEngine.LogCustomEvent (CustomBuildEventArgs e)
{
this.Output.WriteLine ($"Custom: {e.Message}");
if (CustomEvents != null)
CustomEvents.Add (e);
}

void IBuildEngine.LogErrorEvent (BuildErrorEventArgs e)
{
this.Output.WriteLine ($"Error: {e.Message}");
if (Errors != null)
Errors.Add (e);
}

void IBuildEngine.LogMessageEvent (BuildMessageEventArgs e)
{
this.Output.WriteLine ($"Message: {e.Message}");
if (Messages != null)
Messages.Add (e);
}

void IBuildEngine.LogWarningEvent (BuildWarningEventArgs e)
{
this.Output.WriteLine ($"Warning: {e.Message}");
if (Warnings != null)
Warnings.Add (e);
}

private Dictionary<object, object> Tasks = new Dictionary<object, object> ();

void IBuildEngine4.RegisterTaskObject (object key, object obj, RegisteredTaskObjectLifetime lifetime, bool allowEarlyCollection)
{
Tasks.Add (key, obj);
}

object IBuildEngine4.GetRegisteredTaskObject (object key, RegisteredTaskObjectLifetime lifetime)
{
object obj = null;
Tasks.TryGetValue (key, out obj);
return obj;
}

object IBuildEngine4.UnregisterTaskObject (object key, RegisteredTaskObjectLifetime lifetime)
{
var obj = Tasks [key];
Tasks.Remove (key);
return obj;
}

BuildEngineResult IBuildEngine3.BuildProjectFilesInParallel (string [] projectFileNames, string [] targetNames, IDictionary [] globalProperties, IList<string> [] removeGlobalProperties, string [] toolsVersion, bool returnTargetOutputs)
{
throw new NotImplementedException ();
}

void IBuildEngine3.Yield () { }

void IBuildEngine3.Reacquire () { }

bool IBuildEngine2.BuildProjectFile (string projectFileName, string [] targetNames, IDictionary globalProperties, IDictionary targetOutputs, string toolsVersion) => true;

bool IBuildEngine2.BuildProjectFilesInParallel (string [] projectFileNames, string [] targetNames, IDictionary [] globalProperties, IDictionary [] targetOutputsPerProject, string [] toolsVersion, bool useResultsCache, bool unloadProjectsOnCompletion) => true;
}
}

0 comments on commit 099fd95

Please sign in to comment.