Repository navigation
[Microsoft.Android.Build.Tasks] Move the AAPT2 tasks and use the .NET 11 Process APIs - #13515
simonrozsival wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
An uncancellable flush during stdin writer disposal can block daemon recovery and queued jobs, and the cancellation regressions do not cover Windows.
2 open issues
What changed in this PR
This PR extends the process-lifetime work in #12988 to .NET for Android’s persistent AAPT2 daemons, addressing hangs and incomplete resource-processing jobs tracked in #12981.
Changes:
- Replaces blocking workers with channel-based processing, cancellable I/O, and continuous stdout draining.
- Adds bounded output-file and shutdown waits, failure completion, cancellation recovery, and stopped-cache replacement.
- Adds daemon-lifecycle regressions and explicit initialization checks for task access.
| File | Description |
|---|---|
src/Xamarin.Android.Build.Tasks/Utilities/Aapt2Daemon.cs |
Implements asynchronous job processing, bounded cleanup, and daemon recovery. |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/Aapt2Tests.cs |
Adds protocol, cancellation, failure, disposal, and shutdown regressions. |
src/Xamarin.Android.Build.Tasks/Tasks/Aapt2.cs |
Replaces null-forgiving daemon access with a checked accessor. |
Review effort: Balanced
Context: #13515 The executing .NET 12.0.0-alpha.1.26556.115 runtime leaves accessed synchronous process readers caller-owned. A process handle alone therefore cannot prove that a daemon's redirected pipe handles have closed. Exercise the actual AAPT2 daemon through 100 start/stop cycles in a no-GC region, count real file descriptors, and ensure cleanup does not depend on reader finalization. Warm the process path before measuring and allow a small fixed margin for test-host bookkeeping, rather than growth proportional to daemon launches. The existing daemon already disposes both readers after its producer tasks join; no production fix is needed. On the pinned macOS runtime, the isolated measurement stays at 113 descriptors. Removing just the two reader ownership scopes as a temporary control makes the test fail with 315 descriptors; restoring them returns the measurement to 113. The complete Aapt2Tests suite passes all 34 cases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
7b7fced to
8815d04
Compare
Context: #12981 `Aapt2Compile`, `Aapt2Link`, and `Aapt2LinkAssetPack` shared a single `Aapt2Daemon` pool through `IBuildEngine4.RegisterTaskObject()`. The pool lived for the whole build (or, with the private `$(_Aapt2DaemonKeepInDomain)` property, for the whole AppDomain), and MSBuild disposed it at some unspecified point on an unspecified thread. This hidden shared state also prevents moving the AAPT2 tasks to `Microsoft.Android.Build.Tasks`: under full MSBuild those tasks run in a per-task `TaskHostFactory` process, so a registered object cannot outlive a single task anyway. Make each task own its daemon pool for the duration of `Execute()`, and have `Aapt2Daemon.Dispose()` join its worker threads so every `aapt2 daemon` process has received `quit` before the task returns. The batching of many compile jobs into a few `aapt2` processes within a task is unchanged; only cross-task reuse is lost. Starting `aapt2 daemon` locally takes ~15–25 ms. Remove `Aapt2Daemon.GetInstance()`, `$(_Aapt2DaemonKeepInDomain)`, and the `DaemonKeepInDomain` task parameter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…id.Build.Tasks Context: #12981 Context: #13476 Context: #13525 `Aapt2Daemon` keeps a persistent `aapt2 daemon` process per worker and never reads its stdout. Fixing that with the .NET 11 `Process` APIs (`File.OpenNullHandle()`, `ProcessStartInfo.StandardOutputHandle`, and friends) is not possible while the code lives in Xamarin.Android.Build.Tasks, which targets netstandard2.0. Microsoft.Android.Build.Tasks targets net12.0. Move `<Aapt2Compile/>`, `<Aapt2Link/>`, `<Aapt2LinkAssetPack/>`, their `Aapt2` base class, `Aapt2Daemon`, and `OutputLine` into Microsoft.Android.Build.Tasks and the `Microsoft.Android.Tasks` namespace, as #13476 did for `<CheckForInvalidResourceFileNames/>`: * Register the tasks with the existing Full/Core `<UsingTask/>` pair. .NET MSBuild loads them in-proc and full MSBuild uses `TaskHostFactory`. The unqualified task invocations in the `.targets` files are unchanged. * The previous commit scoped each `Aapt2Daemon` to a single task, so nothing depends on sharing a daemon across task-host processes. * Move the helpers that both assemblies need (`ItemGroupToCaseMap()`, `FixUpAndroidResourcePath()`, `TryParseApiLevel()`, `GetSupportedVersions()`, `LogInternalExceptions`, and `Aapt2.IsInvalidFilename()`, now `MonoAndroidHelper.IsInvalidFilename()`) from `MonoAndroidHelper.cs` and `MonoAndroidHelper.Nullable.cs` into a new shared `MonoAndroidHelper.Tasks.cs`. `<CollectNonEmptyDirectories/>` and `<AndroidComputeResPaths/>` still use `IsInvalidFilename()`. * `<ManifestMerger/>` still uses `ManifestDocument` and `AndroidResource`, so link them into Microsoft.Android.Build.Tasks instead of moving them. #13525 removed `ManifestDocument`'s Mono.Cecil dependencies, which is what makes this possible. `ManifestDocument` uses `XABuildConfig`, so Microsoft.Android.Build.Tasks now references `AndroidBuildConfig.csproj`, and the installer ships `tools/net/AndroidBuildConfig.dll`. * Microsoft.Android.Build.Tasks treats CA1305 and nullable warnings as errors. Pass `CultureInfo.InvariantCulture` for numbers, paths, and other machine-readable strings, and `CultureInfo.CurrentCulture` for localized `Resources` messages. Handle the `string?`/`XElement?` values that net12.0 annotates without using `!`. Tests: * Move `Aapt2Tests` into Microsoft.Android.Build.Tasks.Tests. A small `AndroidSdkTestHelper` finds `aapt2` and `android.jar` the same way `NativeToolTestHelper` finds the NDK: `$TEST_ANDROID_SDK_PATH`, `$ANDROID_SDK_ROOT`, `~/android-toolchain/sdk`, then the build-time `$(AndroidSdkDirectory)`. * `CollectNonEmptyDirectoriesTest` covers a Xamarin.Android.Build.Tasks task, so it stays behind in a new `CollectNonEmptyDirectoriesTests.cs`. * `ManagedResourceParserTests.CompareAapt2AndManagedParserOutput` compares Xamarin.Android.Build.Tasks' `<GenerateResourceDesigner/>` with aapt2's `R.txt`, and the net11.0 test assembly cannot reference net12.0 Microsoft.Android.Build.Tasks. It now runs `aapt2 compile`/`aapt2 link` directly with the arguments the tasks pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…aapt2 daemon Context: #12981 Context: https://devblogs.microsoft.com/dotnet/process-api-improvements-in-dotnet-11/ `Aapt2Daemon` redirected the daemon's stdout but never read it. aapt2 reports results on stderr, so stdout normally stays quiet, but anything it does write fills a pipe that nobody drains until aapt2 blocks. Now that the daemon lives in Microsoft.Android.Build.Tasks (net12.0), start it with the .NET 11 `Process` APIs: * Pass `File.OpenNullHandle()` as `ProcessStartInfo.StandardOutputHandle` instead of redirecting stdout. Close our copy of the handle once the child has started. * Set `ProcessStartInfo.StandardInputEncoding` to UTF-8 without a BOM directly. This replaces the reflection lookup of `StandardInputEncoding` (unavailable in netstandard2.0) and the fallback that temporarily changed the process-wide `Console.InputEncoding` under a lock. * Set `ProcessStartInfo.InheritedHandles = []`. A task can start several daemons concurrently; this keeps each `aapt2` from inheriting the other daemons' stdin/stderr pipes and holding them open. * Set `ProcessStartInfo.KillOnParentExit` on Windows and Linux, where it is supported, so a crashed MSBuild node or task host does not leave `aapt2 daemon` running. * Dispose the `Process`, and kill the daemon if it has not exited 5 seconds after `quit`. `Process.Dispose()` alone does not end the process. The stdin/stderr `Done`/`Error` protocol is unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
8815d04 to
1d71269
Compare
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 1 error, 0 warnings, 0 suggestions. The new synchronous worker join can prevent a cancelled build from returning because cancellation does not unblock active AAPT2 pipe I/O; details and the requested regression coverage are inline.
Reviewed all 22 changed files against the stack base 2b13ca60. The task registrations, shared helper migration, installer dependency additions, and test relocation are consistent with the move to the modern task assembly. Redirecting unused stdout to a null handle and removing process-wide console encoding mutation address the original problems directly.
CI: All 40 GitHub checks report success, including the aggregate Azure dotnet-android check for build 1679394. Independent Azure build/timeline verification was unavailable because the CLI requests failed; full Azure status is therefore not independently confirmed. Local tests were not rerun for this review.
Generated by Android PR Reviewer for #13515 · copilot · gpt61 · 57.8 AIC · ⌖ 4.77 AIC · ⊞ 20.2K
Comment /review to run again
| Stop (); | ||
| tcs.Cancel (); | ||
| foreach (var thread in daemons) { | ||
| thread.Join (); |
There was a problem hiding this comment.
🤖 ❌ Resource management — This unbounded join makes build cancellation wait for the active AAPT2 command to finish, and hang indefinitely if that command stalls. AsyncTask.WaitForCompletion() returns on cancellation, so the new using in Aapt2.Execute() immediately enters this disposal path. However, tcs.Cancel() cannot interrupt the worker's synchronous StandardError.ReadLine() or stdin writes/flushes. The five-second shutdown timeout is only reached after those operations return, and KillOnParentExit does not help while the MSBuild node/task host remains alive. Please terminate the owned process and unblock its pipe I/O on cancellation before joining, with a regression test that cancels while the worker is blocked and asserts bounded completion of Execute().
{Rule: Honor cancellation and avoid blocking process I/O during cleanup.}


Context: #12981
Context: https://devblogs.microsoft.com/dotnet/process-api-improvements-in-dotnet-11/
Context: #13476
Context: #13525
Aapt2Daemonkeeps a persistentaapt2 daemonprocess per worker, redirects its stdout, and never reads it. The .NET 11ProcessAPIs fix this directly, but Xamarin.Android.Build.Tasks targets netstandard2.0. This PR moves the AAPT2 tasks to Microsoft.Android.Build.Tasks (net12.0) and then adopts the new APIs. It's three commits:Scope the aapt2 daemon to each task
<Aapt2Compile/>,<Aapt2Link/>, and<Aapt2LinkAssetPack/>shared one daemon pool throughIBuildEngine4.RegisterTaskObject(). It lived for the whole build (or AppDomain, with$(_Aapt2DaemonKeepInDomain)) and MSBuild disposed it at some unspecified point. Under full MSBuild, Microsoft.Android.Build.Tasks tasks run in a per-taskTaskHostFactoryprocess, so that cache can't survive a single task anyway. Each task now owns its pool for the duration ofExecute(), andDispose()joins the worker threads. Batching within a task is unchanged; startingaapt2 daemontakes ~15–25 ms locally.Move the AAPT2 tasks to Microsoft.Android.Build.Tasks
As #13476 did for
<CheckForInvalidResourceFileNames/>: the tasks,Aapt2Daemon, andOutputLinemove to theMicrosoft.Android.Tasksnamespace, registered with the Full/Core<UsingTask/>pair.MonoAndroidHelper.Tasks.cs.ManifestDocumentandAndroidResourceare linked rather than moved, because<ManifestMerger/>still uses them; [Xamarin.Android.Build.Tasks] Remove obsolete manifest code #13525 removedManifestDocument's Cecil dependencies.ManifestDocumentusesXABuildConfig, so Microsoft.Android.Build.Tasks referencesAndroidBuildConfigand the installer shipstools/net/AndroidBuildConfig.dll.!.Aapt2Testsmoves to Microsoft.Android.Build.Tasks.Tests with a smallAndroidSdkTestHelper(same lookup order asNativeToolTestHelper).CollectNonEmptyDirectoriesTeststays behind.CompareAapt2AndManagedParserOutputrunsaapt2 compile/linkdirectly, because the net11.0 test assembly cannot reference net12.0 Microsoft.Android.Build.Tasks.Use the .NET 11 Process APIs for the aapt2 daemon
File.OpenNullHandle()viaStandardOutputHandle, instead of an undrained pipe.StandardInputEncodingis set directly to UTF-8 without a BOM, replacing the reflection lookup and the process-wideConsole.InputEncodingswap under a lock.InheritedHandles = []keeps concurrently started daemons from inheriting each other's pipes.KillOnParentExitis set on Windows and Linux, so a crashed node or task host doesn't leaveaapt2 daemonbehind.Processis disposed, and the daemon is killed if it hasn't exited 5 s afterquit.The stdin/stderr
Done/Errorprotocol is unchanged.Validation
Validated tree = published head
1d71269f(stack base2b13ca60), macOS arm64, local Debug SDK frommake leeroy. Commands run from the repository root:All four: 0 errors.
19 passed, 0 failed (includes the
ÜmläütUTF-8 input cases).13 passed, 2 skipped (NativeAOT), 0 failed.
GenerateDesignerFileFromRtxt(True,CoreCLR)builds a library through the moved tasks.Succeeded and produced a signed APK; the log shows
Aapt2Compile/Aapt2Linkloaded fromtools/net/Microsoft.Android.Build.Tasks.dll. Noaapt2 daemonprocesses remained afterwards.Provenance limit: the local SDK packs (Mono.Android, runtime packs, workload) came from
make leeroyrun earlier on the same three patches overf15fa663(main with #13525); after rebasing onto2b13ca60, only the projects above were rebuilt. That SDK'sMicrosoft.Android.RefincludesMono.Android.xmlexported from a locally initializedexternal/android-api-docscheckout ate4380b2c(thef15fa663pin;1d71269fpins97d1641d).Mono.Android.xmlis API documentation only and is not an input to the AAPT2 tasks or tests.Not run: Windows (
KillOnParentExitvia job objects, full-MSBuildTaskHostFactorypath) and device tests.