Repository navigation
[Xamarin.Android.Build.Tasks] Move GetAndroidDefineConstants to Microsoft.Android.Build.Tasks - #13477
simonrozsival wants to merge 1 commit into
Conversation
…soft.Android.Build.Tasks Context: #13460 `<GetAndroidDefineConstants/>` turns `$(MonoAndroidVersion)` into a `__XAMARIN_ANDROID_*__` define by replacing every non-alphanumeric character via `Regex.Replace (ProductVersion, "[^A-Za-z0-9]", "_")`. Xamarin.Android.Build.Tasks targets netstandard2.0, so that pattern is parsed and interpreted at runtime on every build. Following #13460, move the task into Microsoft.Android.Build.Tasks (net12.0) and the `Microsoft.Android.Tasks` namespace, so the fixed pattern can use a source-generated `[GeneratedRegex]` partial property. The pattern text is unchanged and uses no `RegexOptions`, so matching semantics are identical. The task keeps its existing `MultithreadedAndroidTask` base. The task is registered through the same Full/Core `<UsingTask/>` pair as the other Microsoft.Android.Build.Tasks tasks; the unqualified `<GetAndroidDefineConstants/>` invocation in `Xamarin.Android.Common.targets` is unchanged. The old `Xamarin.Android.Tasks.GetAndroidDefineConstants` type is removed. `MonoAndroidHelper.TryParseApiLevel()` moves from `MonoAndroidHelper.cs` to `MonoAndroidHelper.Basic.cs`, which is already compile-linked into Microsoft.Android.Build.Tasks. As that file is nullable-enabled, the method is now annotated `(string? apiLevel, [NotNullWhen (true)] out Version? version)`; `Aapt2Link` uses `out var` to match. Behavior is unchanged. The existing tests move to Microsoft.Android.Build.Tasks.Tests, plus a test which loads the registrations from `Xamarin.Android.Common.targets` and runs both `<GetAndroidDefineConstants/>` and `<Microsoft.Android.Tasks.GetAndroidDefineConstants/>` through `dotnet msbuild`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The task migration has no active review issues blocking approval.
No open issues 🎉
What changed in this PR
Moves GetAndroidDefineConstants into the modern .NET task assembly, continuing the migration in #13460 and enabling a source-generated regex without changing the matching pattern.
Changes:
- Migrates the task to
Microsoft.Android.Tasksand updates its Full/Core MSBuild registrations. - Relocates API-level parsing into the shared helper, adding nullable annotations and adjusting callers.
- Moves existing tests and adds registration tests for both short and fully qualified task names.
| File | Description |
|---|---|
src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets |
Registers the migrated task using the Full/Core hosting pair. |
src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs |
Removes the relocated API-level parser. |
src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.Basic.cs |
Adds the shared parser with nullable annotations. |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/GetAndroidDefineConstantsTests.cs |
Removes tests from the legacy test project. |
src/Xamarin.Android.Build.Tasks/Tasks/Aapt2Link.cs |
Uses inferred output types for the updated parser. |
src/Microsoft.Android.Build.Tasks/Tests/Microsoft.Android.Build.Tasks.Tests/Tasks/GetAndroidDefineConstantsTests.cs |
Adds migrated tests and MSBuild task-name registration coverage. |
src/Microsoft.Android.Build.Tasks/Tasks/GetAndroidDefineConstants.cs |
Migrates the task and replaces runtime regex parsing with a generated regex. |
Review effort: Balanced
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
Code assessment: no blocking findings. 0 errors, 0 warnings, 1 non-blocking inline suggestion.
The generated regex preserves the original ASCII character-class replacement semantics, the API-level parser move preserves behavior while adding nullable flow annotations, and the task retains its multithreadable declaration and base class. The registrations follow the existing Full/Core pattern, and the moved tests retain the previous constant-generation coverage while adding short-name and qualified-name task resolution coverage. The reported timing measurements appropriately distinguish whole-task execution from regex-only performance.
CI: All 41 GitHub check runs report success, including the aggregate dotnet-android check for Azure build 1674337. Independent Azure build/timeline verification was blocked by CLI initialization/authentication errors, so I cannot independently confirm the Azure result or its stage details. No local tests were run in this review; the active SDK is .NET 10.0.403, while the affected task and test projects target net12.0.
Generated by Android PR Reviewer for #13477 · copilot · gpt61 · 45.9 AIC · ⌖ 4.1 AIC · ⊞ 19.4K
Comment /review to run again
| .Save (project); | ||
|
|
||
| string dotnet = Environment.GetEnvironmentVariable ("DOTNET_HOST_PATH") ?? "dotnet"; | ||
| var result = NativeAotBootstrapTestTools.Run (dotnet, "msbuild", project, "-t:Build", "-v:minimal", "-nr:false"); |
There was a problem hiding this comment.
🤖 💡 Testing — Consider adding a Windows-only variant that runs this generated project with full-framework MSBuild.exe. Both current cases invoke dotnet msbuild, so MSBuildRuntimeType is always Core; copying both registrations and asserting their count does not exercise the Full registration or the new out-of-process task-host boundary. Running the same output assertions through that host would protect the Visual Studio path when migrating this task out of the netstandard assembly.
{Rule: Test host-specific execution paths}
Context: #13460
<GetAndroidDefineConstants/>turns$(MonoAndroidVersion)into a__XAMARIN_ANDROID_*__define by replacing every non-alphanumeric character viaRegex.Replace (ProductVersion, "[^A-Za-z0-9]", "_"). Xamarin.Android.Build.Tasks targets netstandard2.0, so that pattern is parsed and interpreted at runtime on every build.Following #13460, move the task into Microsoft.Android.Build.Tasks (net12.0) and the
Microsoft.Android.Tasksnamespace, so the fixed pattern can use a source-generated[GeneratedRegex]partial property. The pattern text is unchanged and uses noRegexOptions, so matching semantics are identical (no culture-sensitiveIgnoreCaseconcerns). The task keeps its existingMultithreadedAndroidTaskbase.The task is registered through the same Full/Core
<UsingTask/>pair as the other Microsoft.Android.Build.Tasks tasks; the unqualified<GetAndroidDefineConstants/>invocation inXamarin.Android.Common.targetsis unchanged. The oldXamarin.Android.Tasks.GetAndroidDefineConstantstype is removed.MonoAndroidHelper.TryParseApiLevel()moves fromMonoAndroidHelper.cstoMonoAndroidHelper.Basic.cs, which is already compile-linked into Microsoft.Android.Build.Tasks. As that file is nullable-enabled, the method is now annotated(string? apiLevel, [NotNullWhen (true)] out Version? version);Aapt2Linkusesout varto match. Behavior is unchanged.The existing tests move to Microsoft.Android.Build.Tasks.Tests, plus a test that loads the registrations from
Xamarin.Android.Common.targetsand runs both<GetAndroidDefineConstants/>and<Microsoft.Android.Tasks.GetAndroidDefineConstants/>throughdotnet msbuild.Known limitation (shared with every net12.0 Microsoft.Android.Build.Tasks task): it fails with MSB4061 under .NET 11 MSBuild.
Alternative considered: MSBuild XML
Rewriting the task as a plain MSBuild target would avoid the task, its
<UsingTask/>pair, and the MSB4061 limitation. A prototype using$([System.Version]::Parse())and$([System.Text.RegularExpressions.Regex]::Replace())produced identical@(AndroidDefineConstants)for_AndroidApiLevelvalues 21, 36, 36.1, 37, 37.0.1, 37.2, 38.2, and 40.3. However, MSBuild has no range loop, so1..$(major)and the quarterly__ANDROID_{api}_{minor}__constants needed a "tens digit × ones digit" item cross-product capped at API-99, with property round-trips to split transform output. The target took ~11 ms vs ~3 ms for the task (median of 15 freshdotnet msbuildruns,-clp:PerformanceSummary). The C# task is kept because it is much easier to read and test.Validation
dotnet build src/Microsoft.Android.Build.Tasks/Tests/Microsoft.Android.Build.Tasks.Tests/Microsoft.Android.Build.Tasks.Tests.csproj(builds MABT too): succeeded, 0 errors.dotnet test bin/TestDebug/net12.0/Microsoft.Android.Build.Tasks.Tests.dll --filter "FullyQualifiedName~GetAndroidDefineConstantsTests": Passed 7, Failed 0../dotnet-local.sh build src/Xamarin.Android.Build.Tasks/Xamarin.Android.Build.Tasks.csproj -p:BuildProjectReferences=false: succeeded, 0 errors../dotnet-local.sh build src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj -p:DisableApiCompatibilityCheck=True: succeeded (compile check only; ApiCompat skipped because its package was missing from the isolated NuGet cache used locally).Xamarin.Android.Build.Tests. Free disk space on the shared build host was too low formake leeroy. The newdotnet msbuildtest covers theXamarin.Android.Common.targetsregistration.Measurements
Fresh-process harness: load the task assembly,
Execute()withAndroidApiLevel=37.1andProductVersion=37.0.99-ci.main+sha.9f23d997e4. Medians over 30 processes, two interleaved rounds:Execute()Execute()Regex.Replace)[GeneratedRegex])That is about 1.5 ms (~20%) less on first use. The numbers measure the whole task, including assembly differences, not just the regex.