Skip to content

Move AddKeepAlives trimmer step to post-trimming task#10952

Merged
jonathanpeppers merged 12 commits intomainfrom
dev/sbomer/addkeepalives-inner
Mar 23, 2026
Merged

Move AddKeepAlives trimmer step to post-trimming task#10952
jonathanpeppers merged 12 commits intomainfrom
dev/sbomer/addkeepalives-inner

Conversation

@sbomer
Copy link
Member

@sbomer sbomer commented Mar 16, 2026

Migrate AddKeepAlivesStep out of the ILLink custom step pipeline into a standalone MSBuild task that runs AfterTargets="ILLink", following the same pattern established by StripEmbeddedLibraries in #10894.

Core IL-rewriting logic is extracted into AddKeepAlivesHelper, shared by both the new task (trimmed builds) and the existing pipeline step (non-trimmed builds via LinkAssembliesNoShrink).

Migrate AddKeepAlivesStep out of the ILLink custom step pipeline into a
standalone MSBuild task that runs AfterTargets="ILLink", following the
same pattern established by StripEmbeddedLibraries in #10894.

Core IL-rewriting logic is extracted into AddKeepAlivesHelper, shared by
both the new task (trimmed builds) and the existing pipeline step
(non-trimmed builds via LinkAssembliesNoShrink).
@sbomer sbomer self-assigned this Mar 18, 2026
sbomer added 3 commits March 18, 2026 16:19
Replace DefaultAssemblyResolver with DirectoryAssemblyResolver (ReadWrite=true)
to avoid file handle conflicts on Windows. The directory resolver caches all
assemblies (both explicit and dependency-resolved), preventing duplicate file
opens that caused IOException when the same assembly was opened as both a
dependency and a primary target.
Replace standalone AddKeepAlives and StripEmbeddedLibraries MSBuild tasks
with a single PostTrimmingPipeline task that opens assemblies once (via
DirectoryAssemblyResolver with ReadWrite) and runs both modifications in
a single pass. Extract StripEmbeddedLibrariesStep as an
IAssemblyModifierPipelineStep for reuse.
…Step pattern

Use List<IAssemblyModifierPipelineStep> with StripEmbeddedLibrariesStep and
PostTrimmingAddKeepAlivesStep instead of calling helpers directly. Move the
IsFrameworkAssembly check into StripEmbeddedLibrariesStep.ProcessAssembly and
remove all outer-loop filtering so each step handles its own guards internally.
@sbomer sbomer marked this pull request as ready for review March 20, 2026 16:21
Copilot AI review requested due to automatic review settings March 20, 2026 16:21
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Moves the AddKeepAlives IL rewriting from an ILLink custom step into a post-ILLink MSBuild task, aligning with the earlier “post-trimming task” pattern and allowing the same KeepAlive injection to run for trimmed builds after ILLink.

Changes:

  • Introduces a new PostTrimmingPipeline MSBuild task that runs post-trimming assembly modifications (resource stripping + optional KeepAlive injection) in one pass.
  • Extracts KeepAlive IL-rewrite logic into AddKeepAlivesHelper and reuses it from both the existing non-trim pipeline and the new post-trim pipeline.
  • Removes the old StripEmbeddedLibraries task and drops AddKeepAlivesStep from the ILLink project/custom step registration.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/Xamarin.Android.Build.Tasks/Xamarin.Android.Build.Tasks.csproj Adds compilation of new linker helper/steps required by the new post-trim pipeline.
src/Xamarin.Android.Build.Tasks/Tasks/StripEmbeddedLibraries.cs Removes the old standalone StripEmbeddedLibraries task implementation.
src/Xamarin.Android.Build.Tasks/Tasks/PostTrimmingPipeline.cs Adds new post-ILLink task that opens assemblies read/write and applies post-trim modification steps.
src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.TypeMap.LlvmIr.targets Switches from StripEmbeddedLibraries to PostTrimmingPipeline and removes ILLink custom-step registration for AddKeepAlives.
src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/StripEmbeddedLibrariesStep.cs Adds pipeline step that strips embedded Android resource payloads from assemblies.
src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/PostTrimmingAddKeepAlivesStep.cs Adds post-trimming pipeline step wrapper to call the shared KeepAlive helper.
src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/AddKeepAlivesStep.cs Refactors to delegate logic to AddKeepAlivesHelper for the non-trim pipeline path.
src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/AddKeepAlivesHelper.cs Adds shared IL-rewrite helper used by both non-trim and post-trim paths.
src/Microsoft.Android.Sdk.ILLink/Microsoft.Android.Sdk.ILLink.csproj Stops compiling/linking AddKeepAlivesStep into the ILLink project.

@sbomer
Copy link
Member Author

sbomer commented Mar 20, 2026

@copilot open a new pull request to apply changes based on the comments in this thread

Copy link
Contributor

Copilot AI commented Mar 20, 2026

@sbomer I've opened a new pull request, #10981, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 5 commits March 20, 2026 17:05
…ated csproj comment

Co-authored-by: sbomer <787361+sbomer@users.noreply.github.com>
…esolution attempts

Co-authored-by: sbomer <787361+sbomer@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/android/sessions/399b2f01-0576-42f4-a1d4-d0f0d31f5eb7
Fix nullable Func delegate, whitespace, and csproj comment in PostTrimmingPipeline
…beddedLibrariesStep.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@sbomer sbomer changed the title Move AddKeepAlives trimmer step to standalone MSBuild task Move AddKeepAlives trimmer step to post-trimming task Mar 23, 2026
@sbomer sbomer requested a review from jonathanpeppers March 23, 2026 18:47
@sbomer
Copy link
Member Author

sbomer commented Mar 23, 2026

@jonathanpeppers this now moves the step to a separate post-trimming pipeline. #10891 might let us merge this with the existing assembly modification pipeline, but for now it is separate. PTAL!

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI Review Summary

Verdict: ✅ LGTM

Found 0 issues.

Clean refactoring that extracts AddKeepAlives IL-rewriting logic from the ILLink custom step pipeline into a shared AddKeepAlivesHelper class, then runs it as a post-trimming MSBuild task via PostTrimmingPipeline. This follows the same pattern established by StripEmbeddedLibraries in #10894.

What I verified:

  • PostTrimmingPipeline extends AndroidTask, has TaskPrefix = "PTP", returns !Log.HasLoggedErrors
  • [Required] property Assemblies has proper default = []
  • Non-required AddKeepAlives and Deterministic are value types (bool), no nullable issue ✅
  • Core IL-rewriting logic in AddKeepAlivesHelper is a pure extraction — no behavioral changes, same guards (HasTypeReference, IsDotNetAndroidAssembly) ✅
  • StripEmbeddedLibrariesStep correctly moved from the deleted StripEmbeddedLibraries task with logic preserved ✅
  • PostTrimmingAddKeepAlivesStep correctly documents that it skips the IsAndroidUserAssembly pre-filter (matching original ILLink behavior) ✅
  • Corlib resolution is properly memoized in the lambda closure ✅
  • Target renamed from _StripEmbeddedLibraries to _PostTrimmingPipeline with AfterTargets="ILLink" — same ordering ✅
  • AddKeepAlivesStep in the ILLink _TrimmerCustomSteps list is correctly removed ✅
  • All CI checks pass ✅

👍 Nice consolidation — running both steps in a single pass over assemblies with one assembly open/write cycle is a good efficiency improvement.


Review generated by android-reviewer from review guidelines.

@jonathanpeppers jonathanpeppers merged commit 6b94e21 into main Mar 23, 2026
6 checks passed
@jonathanpeppers jonathanpeppers deleted the dev/sbomer/addkeepalives-inner branch March 23, 2026 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants