Add support for Project Specific RegisterTaskObject. #199
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
jonathanpeppers
approved these changes
Jan 5, 2023
jonpryor
reviewed
Jan 9, 2023
jonathanpeppers
approved these changes
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 string key = CompressedAssemblyInfo.GetKey (ProjectFullPath);
Log.LogDebugMessage ($"Storing compression assemblies info with key '{key}'");
BuildEngine4.RegisterTaskObjectAssemblyLocal (key, assemblies, RegisteredTaskObjectLifetime.Build);
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 Update: @jonathanpeppers pointed me to the right task; the problem is |
|
@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 |
|
Context: dotnet/maui#11387 (comment) |
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 * dotnet/android-tools@47f95ab: Fix CS0121 ambiguity errors. (dotnet/android-tools#200) * dotnet/android-tools@76c076f: Add support for Project Specific RegisterTaskObject. (dotnet/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. dotnet/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
Changes: dotnet/android-tools@29f11f2...47f95ab * dotnet/android-tools@47f95ab: Fix CS0121 ambiguity errors. (dotnet/android-tools#200) * dotnet/android-tools@76c076f: Add support for Project Specific RegisterTaskObject. (dotnet/android-tools#199) * dotnet/android-tools@9f02d77: Add reference to System.Security.Cryptography.Xml (dotnet/android-tools#198) * dotnet/android-tools@fa3711b: [build] Update NuGet package versions (dotnet/android-tools#196) * dotnet/android-tools@59cac90: Enable CodeQL (dotnet/android-tools#197) * dotnet/android-tools@9f56dec: Move from `netcoreapp3.1` to `net6.0` (dotnet/android-tools#195) * dotnet/android-tools@0be567a: Use Environment.SpecialFolder.UserProfile, not SpecialFolder.Personal (dotnet/android-tools#194)
jonpryor
added a commit
to jonpryor/java.interop
that referenced
this pull request
Jan 18, 2023
Changes: dotnet/android-tools@29f11f2...099fd95 * dotnet/android-tools@099fd95: Add *Task.ProjectSpecificTaskObjectKey() for RegisterTaskObject() use (dotnet/android-tools#202) * dotnet/android-tools@ac9ea09: Revert IBuildEngine.ProjectFileOfTaskNode use. (dotnet/android-tools#201) * dotnet/android-tools@47f95ab: Fix CS0121 ambiguity errors. (dotnet/android-tools#200) * dotnet/android-tools@76c076f: Add support for Project Specific RegisterTaskObject. (dotnet/android-tools#199) * dotnet/android-tools@9f02d77: Add reference to System.Security.Cryptography.Xml (dotnet/android-tools#198) * dotnet/android-tools@fa3711b: [build] Update NuGet package versions (dotnet/android-tools#196) * dotnet/android-tools@59cac90: Enable CodeQL (dotnet/android-tools#197) * dotnet/android-tools@9f56dec: Move from `netcoreapp3.1` to `net6.0` (dotnet/android-tools#195) * dotnet/android-tools@0be567a: Use Environment.SpecialFolder.UserProfile, not SpecialFolder.Personal (dotnet/android-tools#194)
jonpryor
pushed a commit
to dotnet/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: dotnet/android-tools@9f02d77...099fd95 * dotnet/android-tools@099fd95: Add *Task.ProjectSpecificTaskObjectKey() for RegisterTaskObject() use (dotnet/android-tools#202) * dotnet/android-tools@ac9ea09: Revert IBuildEngine.ProjectFileOfTaskNode use. (dotnet/android-tools#201) * dotnet/android-tools@47f95ab: Fix CS0121 ambiguity errors. (dotnet/android-tools#200) * dotnet/android-tools@76c076f: Add support for Project Specific RegisterTaskObject. (dotnet/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. dotnet/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 dotnet/java-interop
that referenced
this pull request
Jan 25, 2023
Changes: dotnet/android-tools@29f11f2...099fd95 * dotnet/android-tools@099fd95: Add *Task.ProjectSpecificTaskObjectKey() for RegisterTaskObject() use (dotnet/android-tools#202) * dotnet/android-tools@ac9ea09: Revert IBuildEngine.ProjectFileOfTaskNode use. (dotnet/android-tools#201) * dotnet/android-tools@47f95ab: Fix CS0121 ambiguity errors. (dotnet/android-tools#200) * dotnet/android-tools@76c076f: Add support for Project Specific RegisterTaskObject. (dotnet/android-tools#199) * dotnet/android-tools@9f02d77: Add reference to System.Security.Cryptography.Xml (dotnet/android-tools#198) * dotnet/android-tools@fa3711b: [build] Update NuGet package versions (dotnet/android-tools#196) * dotnet/android-tools@59cac90: Enable CodeQL (dotnet/android-tools#197) * dotnet/android-tools@9f56dec: Move from `netcoreapp3.1` to `net6.0` (dotnet/android-tools#195) * dotnet/android-tools@0be567a: Use Environment.SpecialFolder.UserProfile, not SpecialFolder.Personal (dotnet/android-tools#194)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context dotnet/maui#11605
If a solution has more than one Android "App" project there is the potential that objects registered using the
RegisterTaskObjectwill 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.ProjectFileOfTaskNodeas part of the key.Initially the thought was that everything would be "solution" scope if the
LifeTimewas set toAppDomain. However there are instances if "solution" scope usage for nonAppDomainentires. A good example is theaapt2daemon which has aLifeTimeofBuildbut 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.