Repository navigation
[NativeAOT] Replace GetNativeAotRuntimeProviders with XmlPeek - #13484
simonrozsival wants to merge 3 commits into
Conversation
Read NativeAOT runtime provider names from the merged manifest with the built-in XmlPeek task (ProhibitDtd=true, android namespace) and an XPath that requires the exact prefix plus a non-empty ASCII-decimal suffix. Malformed, DTD-bearing, or missing manifests now fail with MSB3733. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context: #12971 The merged manifest controls the provider sources and their incremental inputs. Keep the XPath fixed and verify namespace and name filtering, document order, duplicate removal, invalid manifests, and DTD rejection. Also verify that adding or removing a provider regenerates the expected sources without leaving stale files. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context: 364627b Give missing, malformed, internal-DTD and external-DTD manifests separate tests instead of dispatching from string test cases. Extract the manifest/project XML writes from the build assertions, and use try/finally to remove the test files even when setup or an assertion fails. Validation: the NativeAotBootstrapTargetsTests suite passes all 10 tests, including the four manifest failures and incremental provider generation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The XML-parsing host change remains unvalidated on Windows Full MSBuild, while complete NativeAOT application validation is blocked by the documented local SDK build failure.
No open issues 🎉
What changed in this PR
Simplifies NativeAOT bootstrap provider discovery by replacing the custom managed task with built-in MSBuild XML processing. This preserves recovery of provider sources when Java stub generation is skipped.
Changes:
- Uses namespace-aware
XmlPeekfiltering and item transforms while retaining bootstrap fingerprinting and incremental build tracking. - Removes
GetNativeAotRuntimeProvidersand its task registrations. - Expands tests for provider filtering, ordering, addition/removal, and invalid manifests; manifest failures now use
MSB3733.
| File | Description |
|---|---|
src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.NativeAOT.targets |
Replaces the custom task with DTD-prohibiting XmlPeek and provider-name transforms. |
src/Microsoft.Android.Build.Tasks/Tests/Microsoft.Android.Build.Tasks.Tests/NativeAotBootstrapTargetsTests.cs |
Extends filtering, incremental generation, and manifest-error coverage. |
src/Microsoft.Android.Build.Tasks/Tasks/GetNativeAotRuntimeProviders.cs |
Deletes the superseded manifest-reading task. |
Review effort: Balanced
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
simonrozsival
added
the
ready-to-review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code assessment: no blocking findings. 0 ❌ errors · 0
Reviewed all three changed files, the removed parser, the bootstrap source generator and its consumers. The XPath retains namespace-aware attribute matching, the case-sensitive prefix and nonempty ASCII-digit suffix restriction. Deduplication preserves the selected provider order, and the existing fingerprint, output recovery and generated-file tracking remain wired through the same targets. The namespace and incremental add/remove assertions are useful coverage; the diagnostic change to MSB3733 is explicitly documented.
CI: All 41 GitHub-reported checks are completed successfully, including the aggregate dotnet-android check for Azure build 1675189. Direct Azure build-result/timeline verification was blocked because az devops invoke requires authentication in this environment, so I cannot independently certify the full Azure CI status. This review was based on source inspection; I did not rerun the tests locally.
Generated by Android PR Reviewer for #13484 · copilot · gpt61 · 25.4 AIC · ⌖ 5.13 AIC · ⊞ 20.2K
Comment /review to run again
| CollectionAssert.AreEqual ( | ||
| new [] { "TargetName=App", "Provider=NativeAotRuntimeProvider_1", "Provider=NativeAotRuntimeProvider_9" }, | ||
| File.ReadAllLines (Path.Combine (path, "nativeaot-bootstrap.inputs"))); | ||
| FileAssert.Exists (Path.Combine (sourceDirectory, "NativeAotRuntimeProvider_9.java"), "An added provider must be generated on an incremental build."); |
There was a problem hiding this comment.
🤖 💡 Testing — Could this incremental test also remove all remaining numbered providers and run _FindJavaStubFiles twice? The current removal step always leaves one match, so it does not exercise XmlPeek returning no items through the new item transform. Assert that the fingerprint contains only TargetName=App, all numbered provider sources are deleted, JavaInteropRuntime.java remains, and the second build preserves its timestamp. That covers the ordinary single-process case and the transition back to it without adding another fixture.
{Rule: Test empty collections and incremental edge cases}
Context: #12971
NativeAOT bootstrap generation must recover additional process-provider names from the manifest even when Java stub generation is skipped. This discovery does not need a custom managed task: replace
<GetNativeAotRuntimeProviders/>with built-in<XmlPeek/>and item transforms, removing 44 net product-code lines.The XPath preserves namespace-aware attribute lookup, the exact case-sensitive prefix, a nonempty ASCII-decimal suffix, document order and distinct provider names.
ProhibitDtd="true"rejects DTDs before external resolution; no schema validation is enabled. Keep the existing bootstrap fingerprint, incremental inputs/outputs andFileWritestracking. Tests cover invalid names, namespace aliases/mismatches, duplicate ordering, missing-output recovery, provider addition/removal and invalid manifests. Manifest-error tests have separate names, a shared XML writer andtry/finallycleanup.Intentional diagnostic change: missing, malformed or DTD-bearing manifests still fail the build, but now report
MSB3733rather thanXAGNARP7000/XAGNARP7028, with built-in error text and no exception stack.Validation
All heavy commands used the shared
heavy-slot.sh --toolchainwrapper, private SDK/toolchain, HOME, temporary directories and NuGet caches, with a disk-space guard. Focused tests used SDK12.0.100-alpha.1.26551.101; full-SDK preparation used the repository-pinned12.0.100-alpha.1.26556.115.dotnet test src/Microsoft.Android.Build.Tasks/Tests/Microsoft.Android.Build.Tasks.Tests/Microsoft.Android.Build.Tasks.Tests.csproj --no-restore --filter 'FullyQualifiedName~NativeAotBootstrapTargetsTests' -p:UseSharedCompilation=false -v:minimal— 10 passed after the final cleanup changes, including actual MSBuild incremental generation.python3 "$H/actual-differential.py"($His the session-local XML probe directory) — 18/18 matched against the original parser's outputs and success/failure, allowing the diagnostic change above. Includes ASCII suffix boundaries, namespaces, nesting, duplicates/order, internal/external DTDs/entities, malformed and missing input.make prepare— passed.make leeroy— failed duringConfigureLocalWorkload:MSB4018/FileNotFoundExceptionforMicrosoft.Android/37/Mono.Android.xml. Documentation-generation recovery succeeded for API 37 and 37.1; the API 37.2 attempt ended withMSB4166child-node failures. Full-SDK/application validation remains blocked.Three task-only measurements of 300 calls each gave 524–561 ms for the original task and 409–610 ms for
<XmlPeek/>. The ranges overlap and are noisy; this does not establish a speedup or measure whole-build performance.Windows Full MSBuild, Linux, complete NativeAOT application builds and on-device execution were not validated. Host security compatibility is based on explicit DTD prohibition and the null resolver defaults of supported .NET/.NET Framework runtimes; only macOS Core MSBuild was exercised.