[TrimmableTypeMap] Extract TrimmableTypeMapGenerator from MSBuild task#11034
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the trimmable typemap generation pipeline by extracting orchestration logic from the GenerateTrimmableTypeMap MSBuild task into a standalone TrimmableTypeMapGenerator class, aiming to make the logic more testable and the task a thin adapter.
Changes:
- Introduces
TrimmableTypeMapGenerator+TrimmableTypeMapResultto host the scan → typemap → JCW → acw-map orchestration outside the MSBuild task. - Updates
GenerateTrimmableTypeMapto adapt MSBuild inputs/outputs to the new generator and adjusts tests accordingly (null → empty outputs, removedget_TargetTypeassertions). - Adds resource infrastructure and MSBuild package references to the TrimmableTypeMap project.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapModelBuilderTests.cs | Removes get_TargetType wrapper assertion. |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapAssemblyGeneratorTests.cs | Removes get_TargetType wrapper assertion. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/GenerateTrimmableTypeMapTests.cs | Updates expectations for empty outputs (null → empty). |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateTrimmableTypeMap.cs | Converts task into an adapter calling TrimmableTypeMapGenerator. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/TrimmableTypeMapTypes.cs | Adds TrimmableTypeMapResult record. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/TrimmableTypeMapGenerator.cs | Adds the extracted orchestration logic. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Properties/Resources.resx | Adds XA4212/13/17 resource strings. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Properties/Resources.Designer.cs | Adds strongly-typed resource accessors (auto-generated). |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/NullableExtensions.cs | Adds NRT-annotated string null/empty helpers for netstandard2.0. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Microsoft.Android.Sdk.TrimmableTypeMap.csproj | Adds MSBuild package refs and resource codegen configuration. |
| eng/Versions.props | Adds MSBuildPackageReferenceVersion. |
Files not reviewed (1)
- src/Microsoft.Android.Sdk.TrimmableTypeMap/Properties/Resources.Designer.cs: Language not supported
src/Microsoft.Android.Sdk.TrimmableTypeMap/TrimmableTypeMapGenerator.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Android.Sdk.TrimmableTypeMap/Properties/Resources.resx
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Build.Tasks/Tasks/GenerateTrimmableTypeMap.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Build.Tasks/Tasks/GenerateTrimmableTypeMap.cs
Outdated
Show resolved
Hide resolved
90b195d to
21facfc
Compare
24dd92f to
5a931c3
Compare
simonrozsival
left a comment
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict:
Found 4 issues (0 errors, 2 warnings, 2 suggestions).
Overall this is a well-executed refactor. The generator is now a clean pure transformation and the IO boundary is clearly in the MSBuild task. The PEReader-based API, metadata-derived assembly names, and in-memory result types are all good design choices.
⚠️ Resource management:MemoryStreaminstances inGeneratedAssemblyare never disposed (TrimmableTypeMapGenerator.cs)⚠️ Performance: Incremental build optimization removed — every run now regenerates all assemblies (GenerateTrimmableTypeMap.cs)- 💡 Formatting: Indentation error on doc-comment line (
JcwJavaSourceGenerator.cs:63) - 💡 Code organization:
GenerateContentmissing XML doc-comment (JcwJavaSourceGenerator.cs:44)
👍 Good:
- Clean IO-free generator with pure transformation semantics
- Assembly name derived from PE metadata, not file stem — correct for cross-assembly resolution
- Result types use
record— good use of value equality Files.CopyIfStreamChangedused consistently for incremental-safe writes
Review generated by android-reviewer from review guidelines.
src/Xamarin.Android.Build.Tasks/Tasks/GenerateTrimmableTypeMap.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/JcwJavaSourceGenerator.cs
Show resolved
Hide resolved
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/JcwJavaSourceGenerator.cs
Show resolved
Hide resolved
e3c2f79 to
2e2461f
Compare
simonrozsival
left a comment
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict:
Found 2 issues (0 errors, 1 warning, 1 suggestion):
⚠️ Testing: Several old task tests were removed but not replaced — incremental build tests, TFV parsing, no-peers-found (GenerateTrimmableTypeMapTests.cs)- 💡 CI: Linux build is failing (build 1358253) — Mac/Windows still pending. Worth investigating before merge.
👍 Positive callouts
- Clean IO separation — generator is fully in-memory, MSBuild task owns all filesystem operations
- No
null!anywhere PEReader-based API is a smart design choice — avoids stream ownership issuesFiles.CopyIfStreamChangedin the task prevents incremental build breakage- New unit tests exercise the generator directly with xUnit, no MSBuild ceremony
Review generated by android-reviewer from review guidelines.
tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TrimmableTypeMapGeneratorTests.cs
Show resolved
Hide resolved
Extract the core generation pipeline (scan → typemaps → JCW → acw-map) from GenerateTrimmableTypeMap into a standalone TrimmableTypeMapGenerator class that takes Action<string> for logging, keeping the TrimmableTypeMap project free of Microsoft.Build.* dependencies. The MSBuild task becomes a thin adapter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The per-assembly acw-map files are consumed by existing targets. Keep GeneratePerAssemblyAcwMaps in the task (it uses MSBuild types like MemoryStreamPool and Files) rather than moving the merged acw-map.txt logic to the generator, which is PR 4 work. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Restore GetJavaInteropAssemblyPaths (MonoAndroidHelper.IsMonoAndroidAssembly) filtering to match main's behavior — removing the filter is a build pipeline change that belongs in a later PR. - Add ArgumentNullException checks to TrimmableTypeMapGenerator.Execute(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…d.Tests Move incremental build, parsing, and empty-input tests to TrimmableTypeMapGeneratorTests (xUnit, uses TestFixtures.dll). Keep MSBuild-specific tests (error handling, Mono.Android integration) in GenerateTrimmableTypeMapTests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The scanner needs all assemblies — not just those with HasMonoAndroidReference. Framework JCW pre-compilation is tracked by #10792; until then we generate JCWs for everything. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This is MSBuild input parsing, not core generator logic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Generator accepts (name, PEReader) pairs, returns in-memory content - MSBuild task owns all filesystem IO - Tests assert on in-memory content, no temp dirs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
All IO is now exclusively in the MSBuild task. The generator library only exposes stream/content-based APIs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Restore Execute_SecondRun_OutputsAreUpToDate at task level (verifies Files.CopyIfStreamChanged does not rewrite unchanged outputs) - Restore Execute_ParsesTargetFrameworkVersion at task level - Add Execute_AssemblyWithNoPeers_ReturnsEmpty at generator level (uses test assembly itself which has no [Register] types) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The type CecilTypeDefinitionCache does not exist. The correct class name is TypeDefinitionCache from Java.Interop.Tools.Cecil namespace. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add CecilAssemblyDefinition alias for Mono.Cecil.AssemblyDefinition to resolve ambiguity with System.Reflection.Metadata. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
38848b5 to
ba00b01
Compare
jonathanpeppers
left a comment
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict:
Found 3 issues (1 warning, 2 suggestions) and 3 positive callouts.
⚠️ Dead code:perAssemblyItemslist inWriteAssembliesToDiskis populated but never read (GenerateTrimmableTypeMap.cs:89,112)⚠️ Dead code / Incomplete API:frameworkAssemblyNamesis always an emptyHashSet— JCW peer filtering logic is a no-op (GenerateTrimmableTypeMap.cs:41,TrimmableTypeMapGenerator.cs:34-36)- 💡 Resource management: Test code creates
PEReaderinstances withoutusingor try/finally — ifScan()throws, underlyingFileStreams leak (ScannerRunner.cs:84-95,TypeDataBuilder.cs:139-153,FixtureTestBase.cs:27-33)
👍 Positive callouts:
- Clean IO-free generator design with
PEReaderinputs andMemoryStream/string outputs — easy to unit test. - Proper try/finally disposal of PEReaders and generated MemoryStreams in the MSBuild task (
GenerateTrimmableTypeMap.cs:50-73). - Good use of
Files.CopyIfStreamChangedto preserve downstream incrementality.
Review generated by android-reviewer from review guidelines.
src/Xamarin.Android.Build.Tasks/Tasks/GenerateTrimmableTypeMap.cs
Outdated
Show resolved
Hide resolved
tests/Microsoft.Android.Sdk.TrimmableTypeMap.IntegrationTests/ScannerRunner.cs
Outdated
Show resolved
Hide resolved
- Remove unused perAssemblyItems list (dead code) - Add TODO(#10792) comment for empty frameworkAssemblyNames - Wrap PEReader creation in try/finally in ScannerRunner.RunNew() Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous PR (#11034) established the pattern that TTMG has no IO operations. The build pipeline PR regressed this by adding acw-map writing, manifest template loading, and manifest file saving into the generator. Move all IO back to GenerateTrimmableTypeMap (MSBuild task): - Load manifest template XDocument from file path - Write merged manifest to disk via XDocument.Save() - Write merged acw-map.txt via Files.CopyIfStreamChanged() Refactor ManifestGenerator.Generate to return (XDocument, IList<string>) instead of writing to disk. Add GeneratedManifest record to carry the in-memory result. Update ManifestGeneratorTests to work entirely in-memory (no temp dirs, no IDisposable). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…on, feature switches (#11036) * PR 4: Build pipeline changes for trimmable typemap - Update TrimmableTypeMapGenerator.Execute() with manifest generation, assembly manifest scanning, acw-map writing, and new optional parameters - Add ManifestConfig record to TrimmableTypeMapTypes.cs - Update TrimmableTypeMapResult with AdditionalProviderSources - Update GenerateTrimmableTypeMap MSBuild task with manifest/config properties - Create GenerateEmptyTypemapStub task for LLVM IR native typemap stubs - Create ApplicationRegistration.Trimmable.java (empty registerApplications) - Create Trimmable.CoreCLR.xml preserve list for JNIEnvInit.Initialize - Rewrite Trimmable.targets with full build pipeline (separate generation and _GenerateJavaStubs targets, native stub generation, manifest handling) - Rewrite Trimmable.CoreCLR.targets with ILLink integration and per-ABI assembly store support - Add TrimmableTypeMap=false feature switch to MonoVM and NativeAOT targets - Handle trimmed JNIEnvInit tokens in GenerateNativeApplicationConfigSources Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review: remove #nullable enable, use IsNullOrEmpty extension - Remove redundant #nullable enable from 6 Generator files and task file - Convert string.IsNullOrEmpty to IsNullOrEmpty extension in task Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix _PrepareNativeAssemblySources Outputs typo (typemaps → typemap) The target declared Outputs as typemaps.{abi}.ll but GenerateEmptyTypemapStub writes typemap.{abi}.ll. The mismatch caused MSBuild to always rerun the target. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix CS1503: load XDocument from manifest path, guard GetDirectoryName null - Load manifestTemplatePath into XDocument before passing to ManifestGenerator.Generate() which expects XDocument? (not string) - Guard Path.GetDirectoryName() result with IsNullOrEmpty check before passing to Directory.CreateDirectory() Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Move all IO from TrimmableTypeMapGenerator to MSBuild task The previous PR (#11034) established the pattern that TTMG has no IO operations. The build pipeline PR regressed this by adding acw-map writing, manifest template loading, and manifest file saving into the generator. Move all IO back to GenerateTrimmableTypeMap (MSBuild task): - Load manifest template XDocument from file path - Write merged manifest to disk via XDocument.Save() - Write merged acw-map.txt via Files.CopyIfStreamChanged() Refactor ManifestGenerator.Generate to return (XDocument, IList<string>) instead of writing to disk. Add GeneratedManifest record to carry the in-memory result. Update ManifestGeneratorTests to work entirely in-memory (no temp dirs, no IDisposable). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Generate ApplicationRegistration.java with registerNatives for App/Instrumentation types Application and Instrumentation types skip the JCW static initializer block (CannotRegisterInStaticCtor) because libmonodroid.so isn't loaded when these classes are first instantiated by the Android framework. The legacy path deferred their registration to the generated ApplicationRegistration.registerApplications() method. The trimmable path incorrectly used an empty static file, meaning these types would never get their native methods registered → UnsatisfiedLinkError. Fix: dynamically generate ApplicationRegistration.java in the GenerateTrimmableTypeMap MSBuild task with mono.android.Runtime.registerNatives(MyApp.class); for each Application/Instrumentation type. This triggers the trimmable TrimmableTypeMap.OnRegisterNatives handler which dispatches to the UCO-generated IAndroidCallableWrapper.RegisterNatives. Call graph: MonoPackageManager.LoadApplication() → Runtime.initInternal() → ApplicationRegistration.registerApplications() → Runtime.registerNatives(MyApp.class) [generated] → OnRegisterNatives (JNI native) [TrimmableTypeMap.cs] → IAndroidCallableWrapper.RegisterNatives() [UCO IL] → Application.onCreate() → n_onCreate() [now registered] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address trimmable typemap review feedback Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix CS0246/CS1503: add missing 'using System.Xml.Linq' to GenerateTrimmableTypeMap.cs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Restore pre-existing feature switches in NativeAOT.targets; fix null! in TrimmableTypeMapTypes - Restore IsMonoRuntime=false and IsCoreClrRuntime=false RuntimeHostConfigurationOption items accidentally removed from NativeAOT.targets - Change ApplicationRegistrationTypes parameter from null! to nullable (null?) to comply with the no null-forgiving operator rule Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review: fix manifest save, remove dead PerAssemblyAcwMaps, style - Use Files.CopyIfStringChanged instead of XDocument.Save() to preserve manifest file timestamp when content is unchanged (fixes incremental builds) - Remove dead PerAssemblyAcwMapFiles [Output] property, GeneratePerAssemblyAcwMaps() method, and AcwMapDirectory [Required] property — the only consumer (_MergeAcwMaps) was removed in this PR; nothing reads the per-assembly acw-map files anymore - Add 'using System.Text' and use short StringBuilder form Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix: use MemoryStream to preserve XML declaration in manifest output XDocument.ToString() drops the <?xml version="1.0" encoding="utf-8"?> declaration. Use XDocument.Save(Stream) + Files.CopyIfStreamChanged to preserve both the XML declaration and incremental-build safety. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Update BuildReleaseArm64SimpleDotNet.MonoVM.apkdesc from CI Updated sizes from macOS > Tests > MSBuild 3 build artifacts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix indentation in GenerateTrimmableTypeMap.cs Address code review feedback: properly indent the entire file using tabs per the repo's .editorconfig convention (1 tab for class members, 2 tabs for method body, etc.). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
Summary
Extract the core generation pipeline (scan → typemaps → JCW → acw-map) from the
GenerateTrimmableTypeMapMSBuild task into a standaloneTrimmableTypeMapGeneratorclass. The generator is IO-free — it acceptsPEReaderinstances and returns in-memory content. The MSBuild task owns all filesystem IO.Motivation
The generator logic was tightly coupled to MSBuild types (
TaskLoggingHelper,ITaskItem) and performed filesystem IO directly (creating directories, checking timestamps, writing files). This made it hard to test without full MSBuild ceremony and temp directories. Extracting it with an IO-free API enables fast, focused unit tests using xUnit +TestFixtures.dllthat assert on in-memory content.Changes
New files
TrimmableTypeMapGenerator.cs— orchestrates scan → typemaps → JCW pipeline as a pure transformation:(name, PEReader)[]in →(GeneratedAssembly, GeneratedJavaSource)[]out. No filesystem IO.TrimmableTypeMapTypes.cs—TrimmableTypeMapResult,GeneratedAssembly(Name, MemoryStream),GeneratedJavaSource(RelativePath, Content)recordsNullableExtensions.cs— NRT-annotatedIsNullOrEmpty/IsNullOrWhiteSpacefor netstandard2.0TrimmableTypeMapGeneratorTests.cs— 6 xUnit tests covering the generator directly (empty input, no-peers output, null validation, PE validity, Java source structure). Tests are IO-free.Modified files
GenerateTrimmableTypeMap.cs— rewritten as IO adapter: createsPEReaderinstances from files, calls the generator, writes all outputs to disk usingFiles.CopyIfStreamChanged. Owns directory creation, assembly writing, Java source writing, acw-map generation, andMemoryStreamdisposal in afinallyblock.GenerateTrimmableTypeMapTests.cs— 5 task-level NUnit tests: empty list, invalid TFV, valid TFV parsing (v11.0/v10.0/11.0), Mono.Android integration, incremental up-to-date checkAssemblyIndex.cs— addedCreate(PEReader, string)overload, removedFilePathproperty, no longer ownsPEReaderdisposalJavaPeerScanner.cs— addedScan((string, PEReader)[])overload with sharedScanCore()JcwJavaSourceGenerator.cs— addedGenerateContent()returning(relativePath, content)pairs without filesystem writes, madeGenerate(type, TextWriter)publicPEAssemblyBuilder,RootTypeMapAssemblyGenerator,TypeMapAssemblyEmitter,TypeMapAssemblyGenerator— removed file-path writing overloads, stream-based output onlyDesign decisions
Action<string>instead ofTaskLoggingHelper— keeps TrimmableTypeMap project free of Microsoft.Build packagesPEReaderpairs (not file paths or raw streams) —PEReaderalready owns PE parsing; MSBuild task creates them from files, tests from in-memory bytesMemoryStreamoutputs are disposed in afinallyblock in the MSBuild taskFiles.CopyIfStreamChangedprevents unnecessary file writes (preserving downstream incrementality)