[TrimmableTypeMap] Remove post-trim generation pass#11604
[TrimmableTypeMap] Remove post-trim generation pass#11604simonrozsival wants to merge 2 commits into
Conversation
Generate the trimmable typemap once before trimming and reuse the generated Java sources instead of running GenerateTrimmableTypeMap again after ILLink. Keep the linked/R2R typemap assembly packaging path from the previous fix, and remove diagnostics that only supported the deleted post-trim Java copy path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR removes the “post-trim” TrimmableTypeMap Java generation/copy pass (and its associated task switches + error codes/docs), returning to a single pre-trim typemap generation model while keeping the previously-added single-RID CoreCLR packaging fix and its regression coverage.
Changes:
- Removed the CoreCLR post-trim
_GeneratePostTrimTrimmableTypeMapJavaSourcestarget and the second typemap generation pass after ILLink. - Simplified
GenerateTrimmableTypeMap+TrimmableTypeMapGeneratorby removing the “copy from input dir”/“skip assembly generation” modes and associated XA4254/XA4255 resources + docs. - Updated the regression test to focus on verifying the APK packages the linked/R2R typemap assemblies (dropping the post-trim R8/acw-map assertions).
Show a summary per file
| File | Description |
|---|---|
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/TrimmableTypeMapBuildTests.cs | Removes post-trim R8/acw-map assertions from the single-RID trimmable CoreCLR packaging regression test. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateTrimmableTypeMap.cs | Removes post-trim copy/clean switches and always writes typemap assemblies + Java sources in a single mode. |
| src/Xamarin.Android.Build.Tasks/Properties/Resources.resx | Removes XA4254/XA4255 resource strings tied to the deleted post-trim Java copy path. |
| src/Xamarin.Android.Build.Tasks/Properties/Resources.Designer.cs | Removes the generated accessors for XA4254/XA4255. |
| src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.TypeMap.Trimmable.targets | Removes linked-java path plumbing; always uses the pre-trim typemap Java output directory for stub copying. |
| src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.TypeMap.Trimmable.CoreCLR.targets | Deletes the post-trim typemap Java generation target that ran after ILLink. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/TrimmableTypeMapGenerator.cs | Removes the optional “skip typemap assemblies” mode; generator always produces typemap assemblies. |
| Documentation/docs-mobile/messages/xa4254.md | Deletes docs for XA4254, now that the code path is removed. |
| Documentation/docs-mobile/messages/xa4255.md | Deletes docs for XA4255, now that the code path is removed. |
| Documentation/docs-mobile/messages/index.md | Removes XA4254/XA4255 from the error code index. |
Copilot's findings
Files not reviewed (1)
- src/Xamarin.Android.Build.Tasks/Properties/Resources.Designer.cs: Language not supported
- Files reviewed: 9/10 changed files
- Comments generated: 1
| Directory.CreateDirectory (OutputDirectory); | ||
| if (CleanJavaSourceOutputDirectory && Directory.Exists (JavaSourceOutputDirectory)) { | ||
| Directory.Delete (JavaSourceOutputDirectory, recursive: true); | ||
| } | ||
| Directory.CreateDirectory (JavaSourceOutputDirectory); |
Remove generated Java sources that are no longer emitted by the trimmable typemap generator, delete their copied android/src counterparts, and force Java recompilation so stale class outputs do not survive incremental builds. Track a typemap output stamp for Java stub copying so changed generated Java sources are recopied even when typemap assembly bytes are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/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 and well-structured backout
This is a solid PR. The core insight is correct: the trimmable typemap should be generated once before trimming and reused, not regenerated after ILLink.
What works well
- Stamp-based sentinel for
_GenerateJavaStubsinputs is more reliable than the DLL-based approach. SinceWriteAssembliesToDiskusesFiles.CopyIfStreamChanged, the DLL timestamp could stay stale even when Java sources are regenerated. The stamp always reflects the last generation pass. - Cascading stale file cleanup in the MSBuild targets is well-designed: stale
typemap/java/→ staleandroid/src/→ Java compilation invalidation via stamp delete. The->Count()condition avoids unnecessary recompilation. SkipUnchangedFiles="true"on the Copy in_GenerateJavaStubsis a nice incremental build improvement.DeleteStaleJavaSourcescorrectly uses platform-appropriate string comparison (OrdinalIgnoreCaseon Windows,Ordinalon Linux) for the expected file set.- Test coverage is thorough: unit test for
DeleteStaleJavaSources, integration test for the stale copy + recompilation cascade, and a focused test for the stamp-triggered copy regression. - Error code cleanup — XA4254/XA4255 and their docs are properly removed along with the deleted code paths.
Suggestions
See inline comments — two 💡 suggestions about empty directory cleanup and stale .class file handling. Neither blocks merge.
CI Status
🟡 CI is still in progress (dotnet-android build running, Xamarin.Android-PR not yet triggered). No failures observed so far.
Generated by Android PR Reviewer for issue #11604 · ● 50.5M
| deleted.Add (item); | ||
| } | ||
|
|
||
| return deleted.ToArray (); |
There was a problem hiding this comment.
🤖 💡 Code organization — DeleteStaleJavaSources deletes stale .java files but leaves behind empty parent directories (e.g., typemap/java/crc64removed/). These accumulate across rebuilds as types are added/removed. Not harmful since globs and EnumerateFiles ignore them, but you could clean them up after the loop:
foreach (var dir in Directory.EnumerateDirectories (JavaSourceOutputDirectory, "*", SearchOption.AllDirectories)
.OrderByDescending (d => d.Length)) {
if (!Directory.EnumerateFileSystemEntries (dir).Any ()) {
Directory.Delete (dir);
}
}Rule: Cleanup completeness
| <_DeletedCopiedJavaFiles Include="@(_DeletedJavaFiles->'$(IntermediateOutputPath)android/src/%(RelativePath)')" /> | ||
| </ItemGroup> | ||
| <Delete Files="@(_DeletedCopiedJavaFiles)" /> | ||
| <Delete Files="$(_AndroidCompileJavaStampFile)" Condition=" '@(_DeletedCopiedJavaFiles->Count())' != '0' " /> |
There was a problem hiding this comment.
🤖 💡 MSBuild targets — Nice cascading cleanup design: stale generated Java → stale android/src copies → Java compilation invalidation. The ->Count() condition on the stamp delete (line 128) avoids unnecessary recompilation when nothing was cleaned, which is correct.
One observation: if the deleted stale .java files had corresponding stale .class outputs under android/bin/classes/, those would only be cleaned if _CompileJava cleans its output directory before recompilation. If javac doesn't do a clean compilation, stale .class files could persist. Worth verifying that _CompileJava handles this, or adding stale .class deletion alongside the stale copy deletion here.
Rule: Incremental build correctness
|
@copilot pull latest main and implement code review suggestions |
Merged latest main into the branch and verified the code review suggestion about stale Java sources is addressed (the |
Adds `trimmable` typemap coverage to the `DotNetRun` device integration test, and fixes two issues in the trimmable typemap targets that were uncovered while bringing those scenarios up. ## New test cases Added to `InstallAndRunTests.Get_DotNetRun_Data`: - Release CoreCLR + trimmable (`UseMonoRuntime=false`) - Debug CoreCLR + trimmable (`UseMonoRuntime=false`) - Release NativeAOT + trimmable ## Fixes ### `dotnet run --no-build` for trimmable CoreCLR The trimmable `_GenerateJavaStubs` target listed `_CleanIntermediateIfNeeded` in its `DependsOnTargets`. On a full build that is redundant — the target already runs early via `_AndroidBuildDependsOn` in `BuildOrder.targets`. But during an incremental deploy that does **not** rebuild (the `Install` / `DeployToDevice` chain used by `dotnet run` without a build), the early `Build` chain never executes, so this was the only invocation of `_CleanIntermediateIfNeeded` — and it ran *late*, after `_PrepareAssemblies` had already populated `obj/<config>/android/assets/<abi>/*.dll`. It then deleted `obj/<config>/android`, and because `_PrepareAssemblies` had already executed it was skipped on the second pass and did not recreate the assemblies. `_GeneratePackageManagerJava` then failed with `XAGCA7023` (`DirectoryNotFoundException` on `assets/<abi>/Mono.Android.dll`). Removed `_CleanIntermediateIfNeeded` from the trimmable `_GenerateJavaStubs` `DependsOnTargets` so it matches the `llvm-ir` sibling target, which already omits it. Cleaning still happens early for full builds via `BuildOrder.targets`. ### Parallel multi-RID race in post-trim Java source generation CoreCLR trimmable builds with multiple `RuntimeIdentifiers` run per-RID inner builds in parallel. `_GeneratePostTrimTrimmableTypeMapJavaSources` previously ran inside each inner build and wrote to the same outer intermediate `obj/<config>/typemap/linked-java` directory, so inner builds raced cleaning and writing the same shared output. CI observed this as `XAGTT7024` / "Directory not empty" while one inner build deleted the shared `linked-java` directory as another was using it. The trimmed Java sources are RID-independent, so: - Moved `_GeneratePostTrimTrimmableTypeMapJavaSources` to run once in the outer build, after `_ResolveAssemblies` has collected the per-RID `ResolvedFileToPublish` items, and before `_GenerateJavaStubs` / `_CompileJava` / `_CompileToDalvik`. - Filter `@(ResolvedFileToPublish)` to a single representative RID (`_PostTrimTypeMapFirstRuntimeIdentifier`, the first entry in `$(RuntimeIdentifiers)`), so the target only consumes one RID's linked assemblies as input. - Skip the target when running inside an inner per-RID build (`_ComputeFilesToPublishForRuntimeIdentifiers != true`). Output remains in the outer intermediate directory where `_GenerateJavaStubs` expects it. Fixes #11019 Related to #11604 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
This PR partially backs out #11499, but intentionally keeps the part that fixed single-RID CoreCLR packaging.
#11499 mixed two separate changes:
_GeneratePostTrimTrimmableTypeMapJavaSourcestargetGenerateTrimmableTypeMapinvocation after ILLinkJavaSourceInputDirectory,GenerateTypeMapAssemblies=false,CleanJavaSourceOutputDirectory,generateTypeMapAssemblies)typemap/linked-javaJava source copy/filtering pathResolvedFileToPublishtypemap/java/**/*.javaoutputs after the post-trim pass removalandroid/srccopies, and includestypemap/in intermediate cleanThe conceptual fix is: the trimmable typemap should be generated once before trimming and then reused. Running
GenerateTrimmableTypeMapagain after trimming invalidates the cache and defeats the point of the trimmable typemap design.The Java-source cleanup is intentionally scoped to the current whole-world generator model: when
_GenerateTrimmableTypeMapruns, the task has the full current desired Java source set, so files undertypemap/javathat were not emitted by that pass are stale and can be deleted. No-op incremental builds where_GenerateTrimmableTypeMapis skipped do not clean that directory.The future per-assembly incremental split is tracked separately by #11494; that design will need a more explicit Java-output ownership/reconciliation model before this can become partial per assembly.
Are single-RID builds still fixed?
Yes. The single-RID fix from #11499 is still present:
_AddTrimmableTypeMapToResolvedFileToPublishadds linked typemap assemblies to the publish pipeline, and_UseReadyToRunTrimmableTypeMapAssembliesForPublishswaps in R2R typemap assemblies when applicable.The retained regression test builds a trimmable CoreCLR single-RID APK (
RuntimeIdentifier=android-arm64,TrimMode=full,AndroidLinkTool=r8) and verifies the APK packages the linked/R2R typemap assemblies instead of stale pre-link typemap assemblies.Validation
Previous validation:
make prepare./dotnet-local.sh build src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj -c Debug -v:minimal./dotnet-local.sh test bin/TestDebug/net10.0/Xamarin.Android.Build.Tests.dll --filter "Name=ReleaseCoreClrTrimmableTypeMap_SingleRuntimeIdentifier_PackagesLinkedOrReadyToRunTypeMapAssemblies"Latest cleanup update:
git diff --checkExecute_DeletesStaleGeneratedJavaSourcesBuild_WithTrimmableTypeMap_DeletesStaleGeneratedJavaSourcesAndCopies