Skip to content
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.

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

Add support for Project Specific RegisterTaskObject. #199

Merged
merged 4 commits into from Jan 11, 2023

Conversation

dellis1972
Copy link
Contributor

@dellis1972 dellis1972 commented Jan 5, 2023

Context dotnet/maui#11605

If a solution has more than one Android "App" project there is the potential that objects registered using the RegisterTaskObject will be reused between the projects.

In most cases this is NOT the desired outcome. There are a few instances where it is safe to share the registered objects between projects. But for most of the time it is specific to that project that is being built. Historically we have probably got away with this because "most" users only have one project.....

So lets update the MSBuildExtensions extension methods to include an additional flags parameter. This will control if the registered object will be "project" scope or "solution" scope. Yes those are terms I just made up :D.

We do this by including the engine.ProjectFileOfTaskNode as part of the key.

Initially the thought was that everything would be "solution" scope if the LifeTime was set to AppDomain. However there are instances if "solution" scope usage for non AppDomain entires. A good example is the aapt2 daemon which has a LifeTime of Build but can be "solution" scope. This is why we add a new parameter to control this.

The default for this new parameter is RegisterTaskObjectKeyFlags.IncludeProjectFile, this is to reduce the number of changes. We assume most items will be "project" scope.

Context dotnet/maui#11605

If a solution has more than one Android "App" project
there is the potential that objects registered using the
`RegisterTaskObject` will be reused between the projects.

In most cases this is NOT the desired outcome. There are
a few instances where it is safe to share the registered
objects between projects. But for most of the time it is
specific to that project that is being built. Historically
we have probably got away with this because "most" users
only have one project.....

So lets update the MSBuildExtensions extension methods to
include an additional parameter. This will control if the
registered object will be "project" scope or "solution" scope.
Yes those are terms I just made up :D.

We do this by including the `engine.ProjectFileOfTaskNode` as
part of the key.

Initially the thought was that everything would be "solution" scope
if the `LiftTime` was set to `AppDomain`. However there are instances
if "solution" scope usage for non "AppDomain" entires. A good example
is the `aapt2` daemon which has a "LiftTime" of "Build" but can be
"solution" scope. This is why we add a new parameter to control this.

The default for this new parameter is `true`, this is to reduce the number
of changes. We assume most items will be "project" scope.
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

Otherwise the code changes make sense here, I think.

src/Microsoft.Android.Build.BaseTasks/MSBuildExtensions.cs Outdated Show resolved Hide resolved
@jonpryor
Copy link
Member

jonpryor commented Jan 10, 2023

@dellis1972: i'm trying to expand upon the commit message -- draft message incoming -- but I'm suddenly less certain that we understand the problem, because the <GenerateCompressedAssembliesNativeSourceFiles/> task already uses the project file as part of its key!

https://github.com/xamarin/xamarin-android/blob/c92ae5eb9fdcb3a2fd7c20f5b42dddf8b3ea781a/src/Xamarin.Android.Build.Tasks/Tasks/GenerateCompressedAssembliesNativeSourceFiles.cs#L70-L72

string key = CompressedAssemblyInfo.GetKey (ProjectFullPath);
Log.LogDebugMessage ($"Storing compression assemblies info with key '{key}'");
BuildEngine4.RegisterTaskObjectAssemblyLocal (key, assemblies, RegisteredTaskObjectLifetime.Build);

CompressedAssemblyInfo.GetKey() is:

public static string GetKey (string projectFullPath)
{
	if (String.IsNullOrEmpty (projectFullPath))
		throw new ArgumentException ("must be a non-empty string", nameof (projectFullPath));

	return $"{CompressedAssembliesInfoKey}:{projectFullPath}";
}

As we believe that the (a?) MAUI build issue is related to <GenerateCompressedAssembliesNativeSourceFiles/>, and <GenerateCompressedAssembliesNativeSourceFiles/> is already using $(MSBuildProjectFullPath) as part of the key provided to IBuildEngine4.RegisterTaskObject(), will this PR actually help? Or is <GenerateCompressedAssembliesNativeSourceFiles/> find and it's some other task which is problematic? Or are we missing something?


Update: @jonathanpeppers pointed me to the right task; the problem is <GeneratePackageManagerJava/>, not <GenerateCompressedAssembliesNativeSourceFiles/>. I've updated the draft commit message accordingly.

@jonpryor
Copy link
Member

jonpryor commented Jan 10, 2023

@dellis1972: draft commit message for review:

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 [`<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 in the process, and *also*
    calls `<GeneratePackageManagerJava/>`.

In particular note the key within `<GeneratePackageManagerJava/>`:
`".:!MarshalMethods!:."`.  This value is unchanged, 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
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 the `MSBuildExtensions` extension methods to include an
additional `RegisterTaskObjectKeyFlags` parameter:

	[Flags]
	public enum RegisterTaskObjectKeyFlags {
		None = 0,
		IncludeProjectFile = 1 << 0,
	}

Allowing:

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

When `RegisterTaskObjectKeyFlags.IncludeProjectFile` is specified,
then [`IBuildEngine.ProjectFileOfTaskNode`][2] is used as part of
the key with `RegisterTaskObject()`.  This helps ensure that builds
in different `.csproj` files will result in different keys.

The previous `MSBuildExtensions.GetRegisteredTaskObjectAssemblyLocal()`
and related overloads have been updated so that
`RegisterTaskObjectKeyFlags.IncludeProjectFile` is used by default.

[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://learn.microsoft.com/en-us/dotnet/api/microsoft.build.framework.ibuildengine.projectfileoftasknode?view=msbuild-17-netcore

@jonpryor
Copy link
Member

Context: dotnet/maui#11387 (comment)

D:\a\_work\1\s\bin\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.0-preview.1.50\tools\Xamarin.Android.Common.targets(1717,3): error XAGPM7009: System.InvalidOperationException: Unable to translate assembly name 'Xamarin.GooglePlayServices.Base' to its index [D:\a\_work\1\s\src\BlazorWebView\tests\MauiDeviceTests\MauiBlazorWebView.DeviceTests.csproj::TargetFramework=net8.0-android] 
D:\a\_work\1\s\bin\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.0-preview.1.50\tools\Xamarin.Android.Common.targets(1717,3): error XAGPM7009: at Xamarin.Android.Tasks.MarshalMethodsNativeAssemblyGenerator.WriteNativeMethods(LlvmIrGenerator generator, Dictionary`2 asmNameToIndex, LlvmIrVariableReference get_function_pointer_ref) [D:\a\_work\1\s\src\BlazorWebView\tests\MauiDeviceTests\MauiBlazorWebView.DeviceTests.csproj::TargetFramework=net8.0-android] 
D:\a\_work\1\s\bin\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.0-preview.1.50\tools\Xamarin.Android.Common.targets(1717,3): error XAGPM7009: at Xamarin.Android.Tasks.MarshalMethodsNativeAssemblyGenerator.Write(LlvmIrGenerator generator) [D:\a\_work\1\s\src\BlazorWebView\tests\MauiDeviceTests\MauiBlazorWebView.DeviceTests.csproj::TargetFramework=net8.0-android] 
D:\a\_work\1\s\bin\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.0-preview.1.50\tools\Xamarin.Android.Common.targets(1717,3): error XAGPM7009: at Xamarin.Android.Tasks.LLVMIR.LlvmIrComposer.Write(AndroidTargetArch arch, StreamWriter output, String fileName) [D:\a\_work\1\s\src\BlazorWebView\tests\MauiDeviceTests\MauiBlazorWebView.DeviceTests.csproj::TargetFramework=net8.0-android] 
D:\a\_work\1\s\bin\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.0-preview.1.50\tools\Xamarin.Android.Common.targets(1717,3): error XAGPM7009: at Xamarin.Android.Tasks.GeneratePackageManagerJava.AddEnvironment() [D:\a\_work\1\s\src\BlazorWebView\tests\MauiDeviceTests\MauiBlazorWebView.DeviceTests.csproj::TargetFramework=net8.0-android] 
D:\a\_work\1\s\bin\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.0-preview.1.50\tools\Xamarin.Android.Common.targets(1717,3): error XAGPM7009: at Xamarin.Android.Tasks.GeneratePackageManagerJava.RunTask() [D:\a\_work\1\s\src\BlazorWebView\tests\MauiDeviceTests\MauiBlazorWebView.DeviceTests.csproj::TargetFramework=net8.0-android] 
D:\a\_work\1\s\bin\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.0-preview.1.50\tools\Xamarin.Android.Common.targets(1717,3): error XAGPM7009: at Microsoft.Android.Build.Tasks.AndroidTask.Execute() in /Users/runner/work/1/s/xamarin-android/external/xamarin-android-tools/src/Microsoft.Android.Build.BaseTasks/AndroidTask.cs:line 22 [D:\a\_work\1\s\src\BlazorWebView\tests\MauiDeviceTests\MauiBlazorWebView.DeviceTests.csproj::TargetFramework=net8.0-android] 
D:\a\_work\1\s\bin\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.0-preview.1.50\tools\Xamarin.Android.Common.targets(1717,3): error XAGPM7009: System.InvalidOperationException: Unable to translate assembly name 'Xamarin.AndroidX.Activity' to its index [D:\a\_work\1\s\src\Graphics\samples\GraphicsTester.Android\GraphicsTester.Android.csproj] 
D:\a\_work\1\s\bin\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.0-preview.1.50\tools\Xamarin.Android.Common.targets(1717,3): error XAGPM7009: at Xamarin.Android.Tasks.MarshalMethodsNativeAssemblyGenerator.WriteNativeMethods(LlvmIrGenerator generator, Dictionary`2 asmNameToIndex, LlvmIrVariableReference get_function_pointer_ref) [D:\a\_work\1\s\src\Graphics\samples\GraphicsTester.Android\GraphicsTester.Android.csproj] 
D:\a\_work\1\s\bin\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.0-preview.1.50\tools\Xamarin.Android.Common.targets(1717,3): error XAGPM7009: at Xamarin.Android.Tasks.MarshalMethodsNativeAssemblyGenerator.Write(LlvmIrGenerator generator) [D:\a\_work\1\s\src\Graphics\samples\GraphicsTester.Android\GraphicsTester.Android.csproj] 
D:\a\_work\1\s\bin\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.0-preview.1.50\tools\Xamarin.Android.Common.targets(1717,3): error XAGPM7009: at Xamarin.Android.Tasks.LLVMIR.LlvmIrComposer.Write(AndroidTargetArch arch, StreamWriter output, String fileName) [D:\a\_work\1\s\src\Graphics\samples\GraphicsTester.Android\GraphicsTester.Android.csproj] 
D:\a\_work\1\s\bin\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.0-preview.1.50\tools\Xamarin.Android.Common.targets(1717,3): error XAGPM7009: at Xamarin.Android.Tasks.GeneratePackageManagerJava.AddEnvironment() [D:\a\_work\1\s\src\Graphics\samples\GraphicsTester.Android\GraphicsTester.Android.csproj] 
D:\a\_work\1\s\bin\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.0-preview.1.50\tools\Xamarin.Android.Common.targets(1717,3): error XAGPM7009: at Xamarin.Android.Tasks.GeneratePackageManagerJava.RunTask() [D:\a\_work\1\s\src\Graphics\samples\GraphicsTester.Android\GraphicsTester.Android.csproj] 
D:\a\_work\1\s\bin\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.0-preview.1.50\tools\Xamarin.Android.Common.targets(1717,3): error XAGPM7009: at Microsoft.Android.Build.Tasks.AndroidTask.Execute() in /Users/runner/work/1/s/xamarin-android/external/xamarin-android-tools/src/Microsoft.Android.Build.BaseTasks/AndroidTask.cs:line 22 [D:\a\_work\1\s\src\Graphics\samples\GraphicsTester.Android\GraphicsTester.Android.csproj] 
 290 Warning(s) 

@jonpryor jonpryor merged commit 76c076f into xamarin:main Jan 11, 2023
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Jan 11, 2023
Context: dotnet/maui#11605
Context: 8bc7a3e

Changes: http://github.com/xamarin/xamarin-android-tools/compare/9f02d77692bca8c6585941de03750d5eaaca5c5a...47f95ab99f6201d956eecfa1c7b2fd5fa7e43946

  * xamarin/xamarin-android-tools@47f95ab: Fix CS0121 ambiguity errors. (xamarin/xamarin-android-tools#200)
  * xamarin/xamarin-android-tools@76c076f: Add support for Project Specific RegisterTaskObject. (xamarin/xamarin-android-tools#199)

In dotnet/maui#11605, when `$(AndroidEnableMarshalMethods)`=True
(8bc7a3e), 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 in the process, and *also*
    calls `<GeneratePackageManagerJava/>`.

In particular note the key within `<GeneratePackageManagerJava/>`:
`".:!MarshalMethods!:."`.  This value is unchanged, 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 could 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
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.

xamarin/xamarin-android-tools@76c076f updated the `MSBuildExtensions`
extension methods so that [`IBuildEngine.ProjectFileOfTaskNode`][2]
would be part of the `RegisterTaskObject()` key.

Review use of the `.RegisterTaskObjectAssemblyLocal()`,
`.GetRegisteredTaskObjectAssemblyLocal()`, and
`.UnregisterTaskObjectAssemblyLocal()` extension methods to ensure
that `IBuildEngine.ProjectFileOfTaskNode` is used or excluded,
as appropriate.  This should fix the XAGPM7009 build errors.

TODO: add unit test to trigger this build scenario.

[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://learn.microsoft.com/en-us/dotnet/api/microsoft.build.framework.ibuildengine.projectfileoftasknode?view=msbuild-17-netcore
jonpryor added a commit to jonpryor/java.interop that referenced this pull request Jan 12, 2023
jonpryor added a commit to jonpryor/java.interop that referenced this pull request Jan 18, 2023
Changes: xamarin/xamarin-android-tools@29f11f2...099fd95

  * xamarin/xamarin-android-tools@099fd95: Add *Task.ProjectSpecificTaskObjectKey() for RegisterTaskObject() use (xamarin/xamarin-android-tools#202)
  * xamarin/xamarin-android-tools@ac9ea09: Revert IBuildEngine.ProjectFileOfTaskNode use. (xamarin/xamarin-android-tools#201)
  * xamarin/xamarin-android-tools@47f95ab: Fix CS0121 ambiguity errors. (xamarin/xamarin-android-tools#200)
  * xamarin/xamarin-android-tools@76c076f: Add support for Project Specific RegisterTaskObject. (xamarin/xamarin-android-tools#199)
  * xamarin/xamarin-android-tools@9f02d77: Add reference to System.Security.Cryptography.Xml (xamarin/xamarin-android-tools#198)
  * xamarin/xamarin-android-tools@fa3711b: [build] Update NuGet package versions (xamarin/xamarin-android-tools#196)
  * xamarin/xamarin-android-tools@59cac90: Enable CodeQL (xamarin/xamarin-android-tools#197)
  * xamarin/xamarin-android-tools@9f56dec: Move from `netcoreapp3.1` to `net6.0` (xamarin/xamarin-android-tools#195)
  * xamarin/xamarin-android-tools@0be567a: Use Environment.SpecialFolder.UserProfile, not SpecialFolder.Personal (xamarin/xamarin-android-tools#194)
jonpryor pushed a commit to xamarin/xamarin-android that referenced this pull request Jan 19, 2023
Context: dotnet/maui#11605
Context: dotnet/maui#11387 (comment)
Context: http://github.com/xamarin/xamarin-android/commit/8bc7a3e84f95e70fe12790ac31ecd97957771cb2

Changes: xamarin/xamarin-android-tools@9f02d77...099fd95

  * xamarin/xamarin-android-tools@099fd95: Add *Task.ProjectSpecificTaskObjectKey() for RegisterTaskObject() use (xamarin/xamarin-android-tools#202)
  * xamarin/xamarin-android-tools@ac9ea09: Revert IBuildEngine.ProjectFileOfTaskNode use. (xamarin/xamarin-android-tools#201)
  * xamarin/xamarin-android-tools@47f95ab: Fix CS0121 ambiguity errors. (xamarin/xamarin-android-tools#200)
  * xamarin/xamarin-android-tools@76c076f: Add support for Project Specific RegisterTaskObject. (xamarin/xamarin-android-tools#199)

In dotnet/maui#11605, when `$(AndroidEnableMarshalMethods)`=True
(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/>`, e.g. the new
`IncrementalBuildTest.BuildSolutionWithMultipleProjectsInParallel()`
unit test.  The lifetime of `RegisteredTaskObjectLifetime.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.

xamarin/xamarin-android-tools@099fd95 added the methods
`AndroidTask.ProjectSpecificTaskObjectKey(object)`, 
`AndroidAsyncTask.ProjectSpecificTaskObjectKey(object)`, and
`AndroidToolTask.ProjectSpecificTaskObjectKey(object)` which returns
an `object` for use as the key to `RegisteredTaskObject()`, and the
value is a tuple which includes the input `object` *and* the
`Directory.GetCurrentDirectory()` value present when the task was
created.  (Background note: when MSBuild builds a project, it calls
`Directory.SetCurrentDirectory()` to the directory of the current
project, which is why relative file paths to work in `.csproj` files.)

Review use of the `MSBuildExtensions.RegisterTaskObjectAssemblyLocal()`
extension method and audit the key value.  If the registered value is
project specific, update the callsite to use
`ProjectSpecificTaskObjectKey(object)`, ensuring that project-specific
data is not accidentally reused in a different project.

[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
jonpryor added a commit to xamarin/java.interop that referenced this pull request Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants