[TrimmableTypeMap] Package ReadyToRun trimmable typemap assemblies#11473
[TrimmableTypeMap] Package ReadyToRun trimmable typemap assemblies#11473simonrozsival wants to merge 1 commit into
Conversation
Add generated trimmable typemap assemblies to ResolvedFileToPublish so the .NET SDK ReadyToRun pipeline sees them, then replace linked typemap assemblies with the R2R outputs before Android packaging. Extend the CoreCLR trimmable typemap packaging test to assert the packaged entry assembly is ReadyToRun and that typemap payloads are unique. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
added
copilot
simonrozsival
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates the CoreCLR trimmable TypeMap packaging flow so the generated TypeMap assemblies are routed through the standard publish pipeline, allowing crossgen2/ReadyToRun to process them, and adds a test to verify the packaged TypeMap entry assembly is R2R and not duplicated.
Changes:
- Add trimmable TypeMap assemblies to
ResolvedFileToPublishwithPostprocessAssembly=truesoCreateReadyToRunImagesprocesses them. - After R2R, swap publish items to use the R2R outputs rather than the linked IL inputs.
- Extend the CoreCLR trimmable typemap packaging test to assert R2R headers and ensure TypeMap payloads are packaged only once.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.TypeMap.Trimmable.CoreCLR.targets | Routes typemap assemblies through ResolvedFileToPublish for R2R and replaces linked inputs with R2R outputs. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/TrimmableTypeMapBuildTests.cs | Adds an end-to-end packaging test validating the TypeMap entry assembly is R2R and not duplicated. |
| <Target Name="_UseReadyToRunTrimmableTypeMapAssembliesForPublish" | ||
| Condition=" '$(RuntimeIdentifier)' != '' and '$(PublishReadyToRun)' == 'true' " | ||
| AfterTargets="CreateReadyToRunImages"> | ||
| <ItemGroup> | ||
| <_LinkedTypeMapDlls Remove="@(_LinkedTypeMapDlls)" /> | ||
| <_TypeMapDlls Remove="@(_TypeMapDlls)" /> | ||
| <_TrimmableTypeMapAbi Remove="@(_TrimmableTypeMapAbi)" /> | ||
| <_TrimmableTypeMapLinkedAssemblies Include="$(IntermediateOutputPath)linked\_*.TypeMap.dll;$(IntermediateOutputPath)linked\_Microsoft.Android.TypeMap*.dll" /> | ||
| <_TrimmableTypeMapReadyToRunAssemblies Include="$(IntermediateOutputPath)R2R\_*.TypeMap.dll;$(IntermediateOutputPath)R2R\_Microsoft.Android.TypeMap*.dll" /> | ||
| <ResolvedFileToPublish Remove="@(_TrimmableTypeMapLinkedAssemblies)" /> | ||
| <ResolvedFileToPublish Remove="@(_TrimmableTypeMapReadyToRunAssemblies)" /> | ||
| <ResolvedFileToPublish Include="@(_TrimmableTypeMapReadyToRunAssemblies)"> | ||
| <RelativePath>%(Filename)%(Extension)</RelativePath> | ||
| <CopyToPublishDirectory>PreserveNewest</CopyToPublishDirectory> | ||
| <RuntimeIdentifier>$(RuntimeIdentifier)</RuntimeIdentifier> | ||
| </ResolvedFileToPublish> |
|
/review |
|
✅ Android PR Reviewer completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ LGTM — Clean simplification
Summary: This PR replaces the manual per-ABI assembly store injection (_AddTrimmableTypeMapAssembliesToStore) with a much simpler approach: flowing TypeMap assemblies through the standard ResolvedFileToPublish pipeline so the SDK's existing R2R target processes them. The post-R2R target then swaps linked IL copies for R2R images. The new test thoroughly verifies both the intermediate R2R output and the final APK contents.
Positives:
- Significant complexity reduction (~56 lines removed, ~29 added) — the old ABI-batching logic was error-prone
- Good use of the existing SDK pipeline (
PostprocessAssembly=true) rather than reimplementing R2R integration - Test is well-structured: verifies PE headers for R2R, checks for duplicates in the APK, and confirms the packaged assembly is actually the R2R image
Minor suggestions (2):
- Duplicated glob patterns across the two targets could drift — consider a shared property
- Nullable flow in test could be cleaner
CI is green across all checks.
Generated by Android PR Reviewer for issue #11473 · ● 7.7M
|
|
||
| Assert.IsTrue (helper.Exists ("assemblies/arm64-v8a/_Microsoft.Android.TypeMaps.dll"), "_Microsoft.Android.TypeMaps.dll should exist in the APK."); | ||
| using (var packagedTypeMap = helper.ReadEntry ("assemblies/arm64-v8a/_Microsoft.Android.TypeMaps.dll", AndroidTargetArch.Arm64)) { | ||
| Assert.IsNotNull (packagedTypeMap, "_Microsoft.Android.TypeMaps.dll should be readable from the APK."); |
There was a problem hiding this comment.
🤖 💡 Testing — After Assert.IsNotNull, the nullable flow analysis still considers packagedTypeMap potentially null. Per repo conventions, avoid relying on assert-then-use patterns for null safety. Consider extracting to a local:
var stream = helper.ReadEntry ("assemblies/arm64-v8a/_Microsoft.Android.TypeMaps.dll", AndroidTargetArch.Arm64);
Assert.IsNotNull (stream, "...");
using var packagedReader = new System.Reflection.PortableExecutable.PEReader (stream);This makes the non-null guarantee explicit and avoids a nested using block.
Rule: Test assertions must be specific
| <_LinkedTypeMapDlls Remove="@(_LinkedTypeMapDlls)" /> | ||
| <_TypeMapDlls Remove="@(_TypeMapDlls)" /> | ||
| <_TrimmableTypeMapAbi Remove="@(_TrimmableTypeMapAbi)" /> | ||
| <_TrimmableTypeMapLinkedAssemblies Include="$(IntermediateOutputPath)linked\_*.TypeMap.dll;$(IntermediateOutputPath)linked\_Microsoft.Android.TypeMap*.dll" /> |
There was a problem hiding this comment.
🤖 💡 MSBuild targets — The _TrimmableTypeMapLinkedAssemblies glob pattern ($(IntermediateOutputPath)linked\_*.TypeMap.dll;...) is duplicated verbatim in both _AddTrimmableTypeMapToResolvedFileToPublish (line 36) and here. Since MSBuild item groups are target-scoped, this duplication is functionally necessary, but could you extract the glob strings into a property (e.g. _TrimmableTypeMapLinkedGlob) to keep them in sync? If one changes without the other, the Remove won't match and you'd get duplicate entries in ResolvedFileToPublish.
Rule: Don't duplicate item group transforms
Summary
Validation
Notes
References #10792