From 099fd95f5459d9df78af0ad28f46e3016dd7ca1d Mon Sep 17 00:00:00 2001 From: Dean Ellis Date: Wed, 18 Jan 2023 18:57:53 +0000 Subject: [PATCH] Add *Task.ProjectSpecificTaskObjectKey() for RegisterTaskObject() use (#202) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: https://github.com/dotnet/maui/issues/11605 Context: https://github.com/dotnet/maui/pull/11387#issuecomment-1318738792 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 [``][1]: var marshalMethodsState = BuildEngine4.GetRegisteredTaskObjectAssemblyLocal (".:!MarshalMethods!:.", RegisteredTaskObjectLifetime.Build); Further consider a `.sln` with two "App" projects, as the "App" project build hits ``. 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 ``. 4. `App2.csproj` is later built as part of the process, and *also* calls ``. In particular note the key within ``: `".:!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 ( 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 --- .../AndroidAsyncTask.cs | 2 + .../AndroidTask.cs | 10 ++ .../AndroidToolTask.cs | 10 ++ .../MSBuildExtensions.cs | 22 +++- .../AndroidToolTaskTests.cs | 100 +++++++++++++++++ .../Utilites/MockBuildEngine.cs | 102 ++++++++++++++++++ 6 files changed, 245 insertions(+), 1 deletion(-) create mode 100644 tests/Microsoft.Android.Build.BaseTasks-Tests/AndroidToolTaskTests.cs create mode 100644 tests/Microsoft.Android.Build.BaseTasks-Tests/Utilites/MockBuildEngine.cs diff --git a/src/Microsoft.Android.Build.BaseTasks/AndroidAsyncTask.cs b/src/Microsoft.Android.Build.BaseTasks/AndroidAsyncTask.cs index f3859e5..a04d86f 100644 --- a/src/Microsoft.Android.Build.BaseTasks/AndroidAsyncTask.cs +++ b/src/Microsoft.Android.Build.BaseTasks/AndroidAsyncTask.cs @@ -46,5 +46,7 @@ public virtual bool RunTask () /// * RunTaskAsync is already on a background thread /// public virtual System.Threading.Tasks.Task RunTaskAsync () => System.Threading.Tasks.Task.CompletedTask; + + protected object ProjectSpecificTaskObjectKey (object key) => (key, WorkingDirectory); } } diff --git a/src/Microsoft.Android.Build.BaseTasks/AndroidTask.cs b/src/Microsoft.Android.Build.BaseTasks/AndroidTask.cs index 4b511c3..9acb7b8 100644 --- a/src/Microsoft.Android.Build.BaseTasks/AndroidTask.cs +++ b/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 @@ -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 { @@ -22,5 +30,7 @@ public override bool Execute () } public abstract bool RunTask (); + + protected object ProjectSpecificTaskObjectKey (object key) => (key, WorkingDirectory); } } diff --git a/src/Microsoft.Android.Build.BaseTasks/AndroidToolTask.cs b/src/Microsoft.Android.Build.BaseTasks/AndroidToolTask.cs index 38093a6..cc01e8f 100644 --- a/src/Microsoft.Android.Build.BaseTasks/AndroidToolTask.cs +++ b/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 @@ -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 { @@ -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); } } diff --git a/src/Microsoft.Android.Build.BaseTasks/MSBuildExtensions.cs b/src/Microsoft.Android.Build.BaseTasks/MSBuildExtensions.cs index d50b3d9..b467e83 100644 --- a/src/Microsoft.Android.Build.BaseTasks/MSBuildExtensions.cs +++ b/src/Microsoft.Android.Build.BaseTasks/MSBuildExtensions.cs @@ -251,18 +251,30 @@ public static void SetDestinationSubPath (this ITaskItem assembly) /// /// 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. /// public static void RegisterTaskObjectAssemblyLocal (this IBuildEngine4 engine, object key, object value, RegisteredTaskObjectLifetime lifetime, bool allowEarlyCollection = false) => engine.RegisterTaskObject ((AssemblyLocation, key), value, lifetime, allowEarlyCollection); /// /// 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. /// public static object GetRegisteredTaskObjectAssemblyLocal (this IBuildEngine4 engine, object key, RegisteredTaskObjectLifetime lifetime) => engine.GetRegisteredTaskObject ((AssemblyLocation, key), lifetime); /// /// 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. /// public static T GetRegisteredTaskObjectAssemblyLocal (this IBuildEngine4 engine, object key, RegisteredTaskObjectLifetime lifetime) where T : class => @@ -271,12 +283,20 @@ public static T GetRegisteredTaskObjectAssemblyLocal (this IBuildEngine4 engi /// /// 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. /// public static object UnregisterTaskObjectAssemblyLocal (this IBuildEngine4 engine, object key, RegisteredTaskObjectLifetime lifetime) => engine.UnregisterTaskObject ((AssemblyLocation, key), lifetime); /// - /// 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. /// public static T UnregisterTaskObjectAssemblyLocal (this IBuildEngine4 engine, object key, RegisteredTaskObjectLifetime lifetime) where T : class => diff --git a/tests/Microsoft.Android.Build.BaseTasks-Tests/AndroidToolTaskTests.cs b/tests/Microsoft.Android.Build.BaseTasks-Tests/AndroidToolTaskTests.cs new file mode 100644 index 0000000..8b35ce5 --- /dev/null +++ b/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 (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); + } + } +} diff --git a/tests/Microsoft.Android.Build.BaseTasks-Tests/Utilites/MockBuildEngine.cs b/tests/Microsoft.Android.Build.BaseTasks-Tests/Utilites/MockBuildEngine.cs new file mode 100644 index 0000000..38155fc --- /dev/null +++ b/tests/Microsoft.Android.Build.BaseTasks-Tests/Utilites/MockBuildEngine.cs @@ -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 errors = null, IList warnings = null, IList messages = null, IList customEvents = null) + { + this.Output = output; + this.Errors = errors; + this.Warnings = warnings; + this.Messages = messages; + this.CustomEvents = customEvents; + } + + private TextWriter Output { get; } + + private IList Errors { get; } + + private IList Warnings { get; } + + private IList Messages { get; } + + private IList 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 Tasks = new Dictionary (); + + 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 [] 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; + } +}