[NativeAOT] improve build logic, part 1 #9614
Conversation
* Introduce `$(_AndroidNativeAot)` to be used throughout a build to know if it is a NativeAOT build. The logic for detecting a NativeAOT build will likely change for Debug vs Release in the future. * Skip generation of `mono.MonoRuntimeProvider` for NativeAOT and don't emit the value in `AndroidManifest.xml`. * For a project, `Hello.csproj` ensure that a native library, `libHello.so` is created. Use `@(LinkerArg)` and `%(ArchiveFileName)` to ensure that the native library is renamed with a `lib` prefix. * Update the `NativeAOT` MSBuild test to verify these changes.
There was a problem hiding this comment.
Copilot reviewed 4 out of 9 changed files in this pull request and generated no suggestions.
Files not reviewed (5)
- src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/in/Microsoft.Android.Sdk.BundledVersions.in.targets: Language not supported
- src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets: Language not supported
- src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.NativeAOT.targets: Language not supported
- src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.targets: Language not supported
- src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets: Language not supported
| <_AndroidRuntimePackRuntime Condition=" '$(PublishAot)' == 'true' ">NativeAOT</_AndroidRuntimePackRuntime> | ||
| <_AndroidRuntimePackRuntime Condition=" '$(_AndroidRuntimePackRuntime)' == '' ">Mono</_AndroidRuntimePackRuntime> |
There was a problem hiding this comment.
This is now set in Microsoft.Android.Sdk.NativeAOT.targets.
| <!-- Define a $(_AndroidNativeAot) property, as the logic detecting NativeAOT may change in the future --> | ||
| <_AndroidNativeAot Condition=" '$(PublishAot)' == 'true' ">true</_AndroidNativeAot> | ||
| <_AndroidNativeAot Condition=" '$(_AndroidNativeAot)' == '' ">false</_AndroidNativeAot> | ||
| <UseMonoRuntime Condition=" '$(_AndroidNativeAot)' == 'true' and '$(UseMonoRuntime)' == '' ">false</UseMonoRuntime> |
There was a problem hiding this comment.
What happens/what should happen when $(PublishAot)=true and $(UseMonoRuntime)=true? As $(UseMonoRuntime) is a public property, there is potential expectation that users could do this, so what should happen?
Perhaps we should warn?
There was a problem hiding this comment.
Customers usually would not set $(UseMonoRuntime), but they would set $(PublishAot) to opt into NativeAOT.
iOS has the following logic in .NET 9, that is up for discussion:
dotnet builduses Mono, unless you specify-p:_IsPublishing=truedotnet publishuses NativeAOT, both Debug & Release
I don't think we should change anything, until we know what it should do and align with iOS.
I kind of feel like Debug mode should use Mono (regardless of $(PublishAot)) and Release is NativeAOT when $(PublishAot) is true. No need to use a private $(_IsPublishing) property or dotnet publish.
| @@ -94,7 +97,7 @@ | |||
| <RuntimeIdentifier Condition=" '$(RuntimeIdentifiers)' != '' And '$(RuntimeIdentifier)' != '' " /> | |||
| <GenerateApplicationManifest Condition=" '$(GenerateApplicationManifest)' == '' ">true</GenerateApplicationManifest> | |||
| <!-- Default to Mono's AOT in Release mode --> | |||
| <RunAOTCompilation Condition=" '$(RunAOTCompilation)' == '' and '$(AotAssemblies)' == '' and '$(Configuration)' == 'Release' and '$(PublishAot)' != 'true' ">true</RunAOTCompilation> | |||
| <RunAOTCompilation Condition=" '$(RunAOTCompilation)' == '' and '$(AotAssemblies)' == '' and '$(Configuration)' == 'Release' and '$(_AndroidNativeAot)' != 'true' ">true</RunAOTCompilation> | |||
There was a problem hiding this comment.
I realize that this is pre-existing logic, but we really shouldn't be using '$(Configuration)' == 'Release' (or == 'Debug', for that matter). Looking at the structure of Microsoft.Android.Sdk.DefaultProperties.targets, I'm not sure why we have thisCondition setup this way either, given that the file already has a block:
<PropertyGroup Condition=" '$(Configuration)' == 'Release' ">
…
</PropertyGroup>Do we need to check $(Configuration) at all here?
There was a problem hiding this comment.
we really shouldn't be using '$(Configuration)' == 'Release' (or == 'Debug', for that matter)
This is the same way the .NET SDK specifies $(Optimize), for example, which is problematic due to:
For SDK-style projects, we basically have to declare defaults based on $(Configuration).
the file already has a block:
We could move this expression up to be inside <PropertyGroup Condition=" '$(Configuration)' == 'Release' ">.
| <Target Name="_AndroidFixNativeLibraryFileName" AfterTargets="ComputeFilesToPublish"> | ||
| <ItemGroup> | ||
| <!-- Fix paths to contain lib-prefix --> | ||
| <ResolvedFileToPublish Update="@(ResolvedFileToPublish)" ArchiveFileName="lib%(FileName)%(Extension)" Condition=" '%(Filename)%(Extension)' == '$(TargetName)$(NativeBinaryExt)' " /> |
There was a problem hiding this comment.
This means that if $(TargetName) starts with lib, there may be too many lib prefixes.
I don't know if this is an actual problem.
* main: (25 commits) [CI] Break "Linux Tests" into 2 parallel jobs. (#9642) Fix `WorkloadDependencies.proj` build. (#9648) [CI] Set "WearOS Tests" parallelization to 2 agents. (#9639) [CI] Break "Package Tests" into 2 parallel jobs. (#9638) Bump to DevDiv/android-platform-support@3b4e16f1 (#9632) [NativeAOT] improve build logic, part 2 (#9631) Bump to dotnet/java-interop@2c06b3c2 (#9633) [NativeAOT] improve build logic, part 1 (#9614) [build] Generate `WorkloadDependencies.json` (#9613) [monodroid] remove `monodroid_get_log_categories()` (#9625) [monodroid] remove `_monodroid_get_identity_hash_code` (#9622) Bump to dotnet/java-interop@f800ea52 (#9607) [XABT] Break BuildApk into individual tasks for each content type. (#9612) [Mono.Android] Bind Android API-Baklava DP1 (#9594) [Xamarin.Android.Build.Tasks] Extract `BuildArchive` from `BuildApk` (#9556) [NativeAOT] MSBuild-related logic to get projects to build (#9583) [build] remove remnants of `OpenTK-1.0.dll` (#9610) [build] remove `Xamarin.Android.CSharp.targets` (#9609) [build] runtime "flavors" part 2 (#9598) Bump com.android.tools.build:manifest-merger to 31.7.3 (#9600) ...
* dev/grendel/use-libc++: (25 commits) [CI] Break "Linux Tests" into 2 parallel jobs. (#9642) Fix `WorkloadDependencies.proj` build. (#9648) [CI] Set "WearOS Tests" parallelization to 2 agents. (#9639) [CI] Break "Package Tests" into 2 parallel jobs. (#9638) Bump to DevDiv/android-platform-support@3b4e16f1 (#9632) [NativeAOT] improve build logic, part 2 (#9631) Bump to dotnet/java-interop@2c06b3c2 (#9633) [NativeAOT] improve build logic, part 1 (#9614) [build] Generate `WorkloadDependencies.json` (#9613) [monodroid] remove `monodroid_get_log_categories()` (#9625) [monodroid] remove `_monodroid_get_identity_hash_code` (#9622) Bump to dotnet/java-interop@f800ea52 (#9607) [XABT] Break BuildApk into individual tasks for each content type. (#9612) [Mono.Android] Bind Android API-Baklava DP1 (#9594) [Xamarin.Android.Build.Tasks] Extract `BuildArchive` from `BuildApk` (#9556) [NativeAOT] MSBuild-related logic to get projects to build (#9583) [build] remove remnants of `OpenTK-1.0.dll` (#9610) [build] remove `Xamarin.Android.CSharp.targets` (#9609) [build] runtime "flavors" part 2 (#9598) Bump com.android.tools.build:manifest-merger to 31.7.3 (#9600) ...
Introduce
$(_AndroidNativeAot)to be used throughout a build to know if it is a NativeAOT build. The logic for detecting a NativeAOT build will likely change for Debug vs Release in the future.Skip generation of
mono.MonoRuntimeProviderfor NativeAOT and don't emit the value inAndroidManifest.xml.For a project,
Hello.csprojensure that a native library,libHello.sois created. Use@(LinkerArg)and%(ArchiveFileName)to ensure that the native library is renamed with alibprefix.Update the
NativeAOTMSBuild test to verify these changes.