[TrimmableTypeMap] Keep user AndroidJavaSource Java under R8 shrinking#11790
Closed
simonrozsival wants to merge 1 commit into
Closed
[TrimmableTypeMap] Keep user AndroidJavaSource Java under R8 shrinking#11790simonrozsival wants to merge 1 commit into
simonrozsival wants to merge 1 commit into
Conversation
The trimmable NativeAOT path enables R8 with shrinking (AndroidLinkTool=r8 ->
_R8EnableShrinking=True). When the application ProGuard config is generated from
the acw-map (the default, UseTrimmableNativeAotProguardConfiguration=false), the
R8 task only emits -keep rules for managed-mapped Java types. User-authored
AndroidJavaSource (Bind != true) has no managed peer and is therefore absent from
the acw-map, so R8 shrank it away. This made BuildAfterMultiDexIsNotRequired fail
on NativeAOT: the huge ManyMethods.java classes were removed, so multidex was no
longer required and classes2.dex was never produced.
Pass the user AndroidJavaSource (.java with Bind != true) to the R8 task and emit
'-keep class <package>.<Type> { *; }' for each, so user Java survives shrinking.
The type name is '<package>.<FileNameWithoutExtension>' (Java requires the public
top-level type name to match the file name).
Verified locally: BuildAfterMultiDexIsNotRequired(NativeAOT) and (CoreCLR) pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Ensures user-authored AndroidJavaSource Java files (with Bind != True) are not removed by R8 shrinking by explicitly generating -keep rules for them during the R8 configuration generation step.
Changes:
- Collect
@(AndroidJavaSource)items withBind != Trueand pass them into theR8MSBuild task. - Add
JavaSourceFilesinput toR8and generate-keep class <package>.<TypeName> { *; }rules by deriving type names from.javasource. - Implement package parsing from Java source to construct fully-qualified class names for keep rules.
Show a summary per file
| File | Description |
|---|---|
| src/Xamarin.Android.Build.Tasks/Xamarin.Android.D8.targets | Collects user AndroidJavaSource items and passes them to the R8 task. |
| src/Xamarin.Android.Build.Tasks/Tasks/R8.cs | Adds Java-source-driven keep rule generation to prevent R8 shrinking from removing user Java classes. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 3
Comment on lines
+84
to
+103
| static string? ReadJavaPackage (string path) | ||
| { | ||
| foreach (var raw in File.ReadLines (path)) { | ||
| var line = raw.Trim (); | ||
| if (line.Length == 0 || line.StartsWith ("//", StringComparison.Ordinal) || line.StartsWith ("*", StringComparison.Ordinal) || line.StartsWith ("/*", StringComparison.Ordinal)) { | ||
| continue; | ||
| } | ||
| if (line.StartsWith ("package ", StringComparison.Ordinal)) { | ||
| var end = line.IndexOf (';'); | ||
| if (end > "package ".Length) { | ||
| return line.Substring ("package ".Length, end - "package ".Length).Trim (); | ||
| } | ||
| } | ||
| // The package declaration, if present, must precede any type declaration. | ||
| if (line.StartsWith ("import ", StringComparison.Ordinal) || line.Contains ("class ") || line.Contains ("interface ") || line.Contains ("enum ")) { | ||
| break; | ||
| } | ||
| } | ||
| return null; | ||
| } |
Comment on lines
+162
to
+166
| // User-authored AndroidJavaSource (Bind != true) has no managed peer and is absent | ||
| // from the acw-map, so keep it explicitly; otherwise shrinking removes it. | ||
| foreach (var java in GetUserJavaTypes ()) { | ||
| appcfg.WriteLine ($"-keep class {java} {{ *; }}"); | ||
| } |
Comment on lines
+68
to
+72
| foreach (var item in JavaSourceFiles) { | ||
| var path = item.ItemSpec; | ||
| if (path.IsNullOrEmpty () || !File.Exists (path)) { | ||
| continue; | ||
| } |
simonrozsival
added
copilot
jonathanpeppers
left a comment
jonathanpeppers
left a comment
Member
There was a problem hiding this comment.
Were there two of the same PR?
Member
Author
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.
Summary
User-authored
AndroidJavaSource(.javafiles withBind != True) are plain Java the developer adds to their project. They have no managed (.NET) peer, so they are not present in the acw-map (the map of Java types that have a generated Android Callable Wrapper).When R8 generates the application ProGuard configuration from the acw-map (the
else if (!AcwMapFile.IsNullOrEmpty ())branch inR8.cs— the default for R8 release builds), it only emits-keeprules for the acw-map's managed-mapped Java types. Because userAndroidJavaSourceisn't in that map, no keep rule is emitted for it and R8 shrinks it away.This is most visible on the trimmable NativeAOT path, which enables R8 with shrinking by default (
AndroidLinkTool=r8→_R8EnableShrinking=True). It madeBuildAfterMultiDexIsNotRequired(NativeAOT)fail: the test's hugeManyMethods.javauser-Java classes were removed, so the app no longer exceeded the per-dex method limit, multidex was no longer required, and the expectedclasses2.dexwas never produced.Fixes #11774.
Fix
Pass the user
AndroidJavaSource(.javawithBind != True) to theR8task and emit an explicit keep rule for each so it survives shrinking:The type name is
<package>.<FileNameWithoutExtension>— Java requires the public top-level type name to match the file name. The package is read from the file'spackage …;declaration (skipping comments, and bailing once a type/import is reached, since the package declaration must precede them).R8.cs: newJavaSourceFilesinput +GetUserJavaTypes()/ReadJavaPackage()helpers; emits the keep rules alongside the existing acw-map keep rules.Xamarin.Android.D8.targets: collect@(AndroidJavaSource)withBind != Trueinto@(_R8KeepJavaSource)and pass it to the task.Scope / risk
The keep rules are emitted in the acw-map-generated config branch, which is the default for R8 release builds across runtimes — not NativeAOT-only. The effect is purely additive (more
-keeprules), so it can only prevent removal of user-authored Java; it never removes anything that was previously kept. User-authored Java is generally intended to be present (e.g. referenced via JNI/reflection from native), so keeping it is the conservative, correct behavior.Testing
Verified locally:
BuildAfterMultiDexIsNotRequired(NativeAOT)and(CoreCLR)both pass. The change is also exercised by the full test matrix in #11617, from which it is sliced for focused review.