[Xamarin.Android.Build.Tasks] Remove dead InstantRun leftovers#11656
Open
jonathanpeppers wants to merge 2 commits into
Open
[Xamarin.Android.Build.Tasks] Remove dead InstantRun leftovers#11656jonathanpeppers wants to merge 2 commits into
jonathanpeppers wants to merge 2 commits into
Conversation
Context: dotnet#9292 Context: dotnet#9297 When "Enhanced Fast Deployment" / Instant Run was removed in dotnet#9292 (MSBuild plumbing) and dotnet#9297 (native runtime), the corresponding Java support and the manifest-side hook were left behind. This commit finishes the cleanup that dotnet#9292 said was for "a future PR": * Delete `src-ThirdParty/bazel/java/mono/android/{debug,release}/MultiDexLoader.java` and `incrementaldeployment/IncrementalClassLoader.java`. These were introduced in dotnet#609 to spice fast-deployed `.dex` files into the app classloader at startup. With the native side gone, the release variant's `getDexList()` always returned an empty list (so `IncrementalClassLoader.inject()` was never called) and the debug variant scanned an `__override__/dexes/` directory that nothing in the build pipeline ever populates. * Delete `src/java-runtime/java/mono/android/incrementaldeployment/Placeholder.java`, the corresponding "tiny main dex" Bazel-style placeholder. * Drop the `<src-ThirdParty/bazel/...>` glob and the now-stale `RemoveItems` entries from `src/java-runtime/java-runtime.{csproj,targets}`. * Drop `ManifestDocument.MultiDex` and the `mono.android.MultiDexLoader` ContentProvider injection in `MergeManifest()`. Nothing else referenced the class name, and no test asserted the provider's presence. * Drop the corresponding `MultiDex` parameter from `GenerateMainAndroidManifest` and its callsite in `Microsoft.Android.Sdk.TypeMap.LlvmIr.targets`. * Remove the `bazelbuild/bazel` entry from `THIRD-PARTY-NOTICES.TXT` and the `update-tpn` skill, since the only vendored Bazel sources have been deleted. The `$(AndroidEnableMultiDex)` feature itself is unaffected. It still works via `android.support.multidex.MultiDexApplication` and D8/R8 producing `classes.dex` + `classes2.dex`; existing tests (`BuildMultiDexApplication`, `BuildAfterMultiDexIsNotRequired`, `MultiDexAndCodeShrinker`, etc.) cover that path and don't depend on the removed types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Removes the remaining unreachable “Enhanced Fast Deployment” / Instant Run Java + manifest plumbing from the build tasks and Java runtime packaging, following the prior removal of the MSBuild and native runtime support. This simplifies the codebase and eliminates stale third-party Bazel remnants.
Changes:
- Removed the
mono.android.MultiDexLoaderContentProvider injection (and the associatedMultiDextask/manifest plumbing) which was only tied to dead Instant Run behavior. - Deleted the Bazel-derived Java sources (
MultiDexLoader,IncrementalClassLoader) and the matching runtime placeholder type, and stopped includingsrc-ThirdParty/bazelsources injava-runtime. - Updated third-party notices and the
update-tpnskill documentation to reflect the removal of vendored Bazel sources.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| THIRD-PARTY-NOTICES.TXT | Removes the Bazel entry and its license block since Bazel sources are no longer vendored. |
| src/Xamarin.Android.Build.Tasks/Utilities/ManifestDocument.cs | Drops MultiDex property and removes provider injection of mono.android.MultiDexLoader. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateMainAndroidManifest.cs | Removes the MultiDex MSBuild task parameter and stops passing it into manifest merging. |
| src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.TypeMap.LlvmIr.targets | Stops passing MultiDex="$(AndroidEnableMultiDex)" to GenerateMainAndroidManifest. |
| src/java-runtime/java/mono/android/incrementaldeployment/Placeholder.java | Deletes unused Instant Run placeholder Java type. |
| src/java-runtime/java-runtime.targets | Removes now-stale RemoveItems entries that referenced deleted Bazel Java sources. |
| src/java-runtime/java-runtime.csproj | Stops glob-including src-ThirdParty/bazel/java/**/*.java into the Java runtime build inputs. |
| src-ThirdParty/bazel/java/mono/android/release/MultiDexLoader.java | Deletes unused Bazel-derived MultiDexLoader implementation. |
| src-ThirdParty/bazel/java/mono/android/incrementaldeployment/IncrementalClassLoader.java | Deletes unused Bazel-derived incremental classloader. |
| src-ThirdParty/bazel/java/mono/android/debug/MultiDexLoader.java | Deletes unused Bazel-derived debug MultiDexLoader implementation. |
| .github/skills/update-tpn/SKILL.md | Removes the Bazel row from the “vendored source” table since src-ThirdParty/bazel is gone. |
The BuildReleaseArm64 apkdiff regression tests fail because removing the mono.android.incrementaldeployment Java sources shrinks classes.dex by more than the 5% threshold: - BuildReleaseArm64(False,MonoVM) classes.dex 22384 -> 19036 - BuildReleaseArm64(False,NativeAOT) classes.dex 25400 -> 22016 - BuildReleaseArm64(True,CoreCLR) classes2.dex 156448 -> 108080 Refresh the reference .apkdesc files using the descriptions saved by apkdiff during the failing CI runs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jonathanpeppers
added
the
ready-to-review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
"Enhanced Fast Deployment" / Instant Run support was removed in #9292 (MSBuild plumbing) and #9297 (native runtime side). #9292''s description left a TODO: "In a future PR, we could consider making runtime changes to remove
instant_run_enabledas well." This is that future PR — it deletes the corresponding Java types and theAndroidManifest.xmlhook that instantiated them.What was actually still doing anything?
Nothing useful. After #9297 deleted the native side, the Bazel-derived Java types in
src-ThirdParty/bazel/java/mono/android/...became unreachable runtime no-ops:release/MultiDexLoader.java''sgetDexList()always returned an emptyList<String>, soIncrementalClassLoader.inject()was never called.debug/MultiDexLoader.javascanned$filesDir/.__override__/dexes/for hot-pushed.dexfiles. Nothing in the build pipeline ever populates that directory anymore —FastDeploy.csand the native runtime use.__override__/for managed.dlls and.__override__/$abi/for.sos, never.dex.incrementaldeployment/IncrementalClassLoader.javawas only reachable through the deadinject()call above.src/java-runtime/java/mono/android/incrementaldeployment/Placeholder.javawas the matching "tiny main dex" Bazel-style placeholder.The only consumer of the Java side was
ManifestDocument.MergeManifest(), which injected amono.android.MultiDexLoaderContentProviderwhen$(AndroidEnableMultiDex)was true. No test asserted the provider''s presence, and itsattachInfo()body was effectively a no-op.What this PR does
src-ThirdParty/bazel/java/...files andPlaceholder.java.bazelbuild/bazelentry fromTHIRD-PARTY-NOTICES.TXTand theupdate-tpnskill, since no vendored Bazel sources remain.<src-ThirdParty\bazel\...>glob and now-staleRemoveItemsentries fromsrc/java-runtime/java-runtime.{csproj,targets}.ManifestDocument.MultiDex, themono.android.MultiDexLoaderprovider injection, and the correspondingMultiDextask parameter onGenerateMainAndroidManifest(plus its callsite inMicrosoft.Android.Sdk.TypeMap.LlvmIr.targets).What is unaffected
$(AndroidEnableMultiDex)itself. The real multidex feature still works viaandroid.support.multidex.MultiDexApplicationand D8/R8 producingclasses.dex+classes2.dex. Existing testsBuildMultiDexApplication,BuildAfterMultiDexIsNotRequired,CustomApplicationClassAndMultiDex, andMultiDexAndCodeShrinkercover that path and don''t reference any of the removed types.$(_InstantRunEnabled)checks #9292 and Remove native runtime support for instant run #9297.BuildTest2MultiDex tests cover the supported path; no new tests are needed for code that nothing reaches.