[TrimmableTypeMap] Fix trimmable manifest generation parity#11752
Conversation
Match legacy manifest behavior for enum values, plural intent-filter data attributes, type-level application properties, runtime provider directBootAware, and extractNativeLibs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Ports manifest-generation parity fixes into the trimmable typemap manifest generator so generated AndroidManifest.xml matches legacy ManifestDocument behavior.
Changes:
- Extend enum/property mapping parity (e.g., activity
ScreenOrientation, default enum values likeLaunchMode=0, and other enum/value conversions). - Add support for plural
IntentFilterdata list properties (e.g.,DataSchemes,DataHosts,DataPaths, etc.) to emit multiple<data>elements. - Align application-level attributes (
directBootAware,networkSecurityConfig,extractNativeLibs) and relax a build-test assertion to be less brittle.
Show a summary per file
| File | Description |
|---|---|
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/ManifestGeneratorTests.cs | Adds/extends tests for new parity behaviors (screenOrientation, plural intent-filter data lists, app/provider directBootAware, extractNativeLibs, networkSecurityConfig). |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/ManifestTest.cs | Relaxes error-count assertion to tolerate varying numbers of errors. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/AssemblyIndex.cs | Ensures IntentFilter named args that are string[] decode as string lists for downstream manifest emission. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/PropertyMapper.cs | Adds new mapped properties and threads targetSdkVersion into enum conversion to match legacy behavior. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ManifestGenerator.cs | Uses targetSdkVersion for parity behaviors; propagates directBootAware and conditions extractNativeLibs. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ComponentElementBuilder.cs | Emits additional <data> elements for plural IntentFilter data-list properties. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/AndroidEnumConverter.cs | Adjusts enum-to-string conversion to include legacy defaults/edge values and targetSdk-specific screenOrientation mapping. |
Copilot's findings
- Files reviewed: 7/7 changed files
- Comments generated: 1
Scan the application element and child components for directBootAware=true before creating runtime providers. Emit android:directBootAware only when the scan is true, matching legacy ManifestDocument behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Emit trimmable manifest data elements for DataPathSuffix/DataPathAdvancedPattern and their plural forms, matching legacy manifest generation. Strengthen tests to verify exact data attribute names and values instead of only counting elements. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 Code Review: ⚠️ Needs Changes
Thanks for porting the manifest-generator parity fix from #11617 — the bulk of this is solid, well-targeted work. I found one parity divergence worth fixing plus two minor suggestions (all posted inline).
Findings
⚠️ 1 warning — Plural<data>element ordering inComponentElementBuilder.AddIntentFilterDataElementsdiverges from the legacyIntentFilterAttribute.GetDataorder (and from this PR's own singular method), undercutting the byte-for-byte parity goal. The newActivity_IntentFilterPluralDataPropertiestest currently locks in the divergent order.- 💡 2 suggestions —
GetTargetSdkVersionValuedoesn't resolve codename SDK ids the way legacyManifestDocumentdoes (and silently returns0instead of failing loudly); and aBug12935assertion was relaxed from an exact error count.
Verified correct (nice work)
- ✅ The
sensorPortaitspelling fortargetSdkVersion < 16is intentional legacy parity (bugzilla #12935), not a typo. - ✅ The
ScreenOrientationint→string remap now matches the realAndroid.Content.PM.ScreenOrientationvalues (SensorPortrait = 7, etc.) — the previous mapping was wrong, so this is a genuine fix. - ✅
WindowSoftInputModenow always emitsstate|adjust(including the*Unspecifieddefaults), matchingManifestDocumentElement. - ✅
extractNativeLibs(>= 23) andDirectBootAware(app attribute + direct child elements) matchManifestDocumentexactly; computingDirectBootAwareonce and threading it through the providers is a clean improvement. - ✅ Singular
AddIntentFilterDataElementnow correctly emits one<data>element per attribute.
CI
At review time the Azure DevOps build (38 checks) is still in progress with no failures; mergeable_state is blocked pending those checks. Worth confirming a green run after the ordering fix.
The only real blocker is the plural <data> ordering. Once that (and its test) is aligned with the legacy order, this should be good to go.
Generated by Android PR Reviewer for issue #11752 · 1.8K AIC · ⌖ 48.6 AIC · ⊞ 40K
Comment /review to run again
The plural DataSchemes/DataHosts/etc. <data> elements were emitted in a different order than the legacy IntentFilterAttribute.GetData path, breaking the byte-for-byte manifest parity this change targets. Drive both the singular and plural emitters from a single ordered (singular, plural, attribute) table so they share the legacy order (host, mimeType, path, pathPattern, pathPrefix, port, scheme, pathSuffix, pathAdvancedPattern) and cannot drift apart. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GetTargetSdkVersionValue silently returned 0 for an unparseable targetSdkVersion, which would emit a wrong manifest (flipping screenOrientation to the pre-API-16 spelling and skipping the extractNativeLibs gate). Throw instead, matching legacy ManifestDocument which fails loudly on an unrecognized targetSdkVersion. In practice the MSBuild TargetSdkVersion fallback is always an already-resolved integer, so this only removes a latent landmine. Document why codename resolution (legacy's VersionResolver.GetApiLevelFromId) is intentionally not replicated here: it isn't referenceable from this netstandard2.0 project. Also document that the relaxed 'Error(s)' assertion in Bug12935 is intentional, since the CoreCLR/NativeAOT runtimes may emit a different number of errors; the specific aapt2 codes are still pinned. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 Android PR Reviewer — re-review
No blocking issues found. (CI still running — not a formal LGTM until the pipeline is green.)
This is a re-review after the previous /review. I read the full changed files and verified the generated output against the legacy ManifestDocument / ManifestDocumentElement / IntentFilterAttribute.GetData implementations before reading the PR description.
Previously-raised items — all resolved ✅
- directBootAware propagation (
526e6e8):DirectBootAware(app)now scans the<application>element and its direct children, and the provider only ever emitsdirectBootAware="true"— matchesManifestDocument.DirectBootAware()/CreateMonoRuntimeProviderexactly, including attribute order. ✔ - Plural
<data>ordering (4465b56): driving both the singular and plural emitters from one ordered(singular, plural, attribute)table restores legacy order (host, mimeType, path, pathPattern, pathPrefix, port, scheme, pathSuffix, pathAdvancedPattern), singular block before plural — matchesIntentFilterAttribute.GetData. Good call collapsing the two methods to prevent drift. ✔ - Unresolvable
targetSdkVersion(3a23cbb): now throwsInvalidOperationExceptioninstead of silently using0and emitting a wrong manifest, matching legacy. The netstandard2.0 codename caveat is documented and, in practice, unobservable. ✔ - Bug12935 assertion: relaxed-with-comment is reasonable; the specific aapt2 codes (
APT2259,APT2067) are still asserted, so a real regression is still caught. ✔
Parity spot-checks (all match legacy)
ScreenOrientation2–14 remap + thesensorPortait(sic)< 16quirk — matchesManifestDocumentElement.cs:308. The oldsensor=3 mapping was wrong; the new table is correct.- Zero/default enum values (
launchMode=standard,documentLaunchMode=none,uiOptions=none,stateUnspecified|adjustUnspecified) — match. extractNativeLibsgated ontargetSdk >= 23 && (force || attr == null)— matchesManifestDocument.cs:451.TryGetStringArray→List<string>flows correctly into theIReadOnlyList<string>plural cast;ScreenOrientationToStringbinds to the new 2-argPropertyMappingctor while 1-arg converters keep the wrapper lambda — overload resolution is sound.
New suggestion (inline, non-blocking)
- 💡 Testing — the
targetSdkVersion < 16 → "sensorPortait"branch (the whole point of the newtargetSdkVersionplumbing) has no unit coverage. See the inline comment onManifestGeneratorTests.cs.
CI
Azure DevOps builds (dotnet-android Linux/Mac/Windows) are in progress; license/cla passed; mergeable_state is blocked (pending CI + required review). Nothing here should block merge once the pipeline goes green.
Solid, well-scoped parity work with good regression tests. 👍
Generated by Android PR Reviewer for issue #11752 · 1.4K AIC · ⌖ 47.4 AIC · ⊞ 37.9K
Comment /review to run again
When a manifest template contains an empty <uses-sdk />, fill android:minSdkVersion from the configured SupportedOSPlatformVersion. This matches the normal manifest path and avoids manifest merger treating the app minSdk as 1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
#11798) ## Problem When R8 code shrinking is enabled, R8 removes any Java type that nothing keeps. .NET for Android keeps the Java callable wrappers (JCWs) of bound managed types by generating `-keep` rules from the **acw-map** (managed peer ↔ Java type). That works for generated JCWs, but **user-authored `AndroidJavaSource` `.java` files marked `Bind` != `true` have no managed peer**, so they never appear in the acw-map — and therefore get no keep rule. With shrinking on, R8 silently deletes them, and the app fails at runtime when it references those classes (often as a `NoClassDefFoundError` that only reproduces in Release/shrunk builds). ## Fix Pass user `AndroidJavaSource` (`Bind` != `True`) to the `R8` task and emit `-keep class <package>.<Type> { *; }` for each, so user Java survives shrinking: * **`D8.targets`** — collect `@(AndroidJavaSource)` items where `%(Bind) != 'True'` into `_R8KeepJavaSource` and pass them to `R8` via the new `JavaSourceFiles` property. * **`R8.cs`** — append keep rules in the same block that emits acw-map keeps. `GetUserJavaTypes()` derives the fully-qualified name as `<package>.<FileNameWithoutExtension>` (Java requires the public top-level type name to match the file name); `ReadJavaPackage()` parses the `package` declaration, skipping comments and stopping at the first `import`/type declaration. Names are de-duplicated. * **`R8Tests.cs`** — unit tests covering `ReadJavaPackage`: package present, trailing space before `;`, comment headers, no package, and `package` after `import`/type (ignored). ## Why split out This is being carved out of the trimmable typemap mega-PR #11617. Although the bug surfaced while bringing up the NativeAOT trimmable path, **it is not trimmable-specific** — the acw-map keep logic and R8 shrinking apply equally to legacy/MonoVM and CoreCLR, so user `AndroidJavaSource` was at risk of being shrunk there too. Landing it independently de-risks #11617 and ships a general correctness fix sooner. ## Context / related PRs - #11617 — parent mega-PR (reflection-free trimmable typemap) this is sliced from. - #11643 — removes the managed NativeAOT typemap in favor of the trimmable one (the path that exposed this gap). - Sibling slices already split out off `main`: #11794 (AndroidApplicationJavaClass/multidex), #11796 (manifest generation parity). Other merged groundwork: #11749, #11751, #11752, #11753, #11764, #11769. ## Verification 24/24 R8 unit tests pass (18 existing + 6 new). `BuildAfterMultiDexIsNotRequired` verified locally on NativeAOT and CoreCLR — user Java is retained after shrinking. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
) ## Summary Follow-up to the trimmable typemap manifest-generation work being split out of #11617 (and to the XA4243 manifest fix in #11785). This brings the **trimmable typemap** `ManifestGenerator` to parity with the legacy `ManifestDocument` for NativeAOT, completing the activity attribute coverage and the library/placeholder merge behavior that #11752 didn't cover. ## Changes * **`PropertyMapper`** — add the remaining activity manifest attributes: `banner`, `colorMode`, `enableVrMode`, `lockTaskMode`, `logo`, `maxAspectRatio`, `maxRecents`, `allowEmbedded`, `autoRemoveFromRecents`, `relinquishTaskIdentity`, `resumeWhilePausing`, `showForAllUsers`/`showOnLockScreen`/`showWhenLocked`, `singleUser`, `visibleToInstantApps`, `recreateOnConfigChanges`, `rotationAnimation`. Adds a `Number` mapping kind with invariant-culture formatting for `maxAspectRatio`/`maxRecents`. * **`ComponentElementBuilder`** — emit `<layout>` child properties from `LayoutProperties`. * **`ComponentInfo`** — carry `LayoutProperties`. * **`AndroidEnumConverter`** — `RotationAnimation` and `persistableMode` conversions. * **`ManifestGenerator`** — merge library (`.aar`) manifests for the legacy merger, resolve/normalize `$(AndroidManifestPlaceholders)` (incl. package-name resolution), warn XA1010 on invalid placeholders, sort component attributes alphabetically, dedup `tools:`-namespace nodes, and emit `uses-sdk` correctly when no min is specified. ## Context Part of slicing the #11617 mega-PR into reviewable units. `ManifestGenerator` and friends already exist on `main` (via #10991, #11037, #11143, #11752), so this applies cleanly on its own. Companion slices: #11794 (AndroidApplicationJavaClass/multidex). ## Tests Extends `ManifestGeneratorTests` to cover all activity attributes, layout properties, placeholder resolution/warnings, library-manifest merge, and dedup. **575 generator unit tests pass locally.**
Summary
Ports the focused manifest generator parity fix from #11617 without bringing over the broader trimmable typemap/runtime work from that branch.
The trimmable typemap manifest generator is intended to produce the same manifest semantics as the legacy
ManifestDocumentpath. This PR fixes several small parity gaps that showed up while comparing generated manifests:launchMode="standard",documentLaunchMode="none",uiOptions="none", andwindowSoftInputModeunspecified valuesScreenOrientation.SensorPortraitusing the legacy target-SDK-dependent spelling behavior[IntentFilter]data properties (DataSchemes,DataHosts,DataPaths,DataPathPatterns,DataPathPrefixes,DataMimeTypes,DataPorts,DataPathSuffixes, andDataPathAdvancedPatterns) by emitting one<data>element per value, in the same order legacyIntentFilterAttribute.GetDataemits them. The singular and plural emitters are driven from a single ordered table so their ordering cannot drift apart.NetworkSecurityConfigandDirectBootAware.android:directBootAware="true"when the application element or any direct child component isdirectBootAware="true"(and never emit an explicit"false"), matching legacy startup-provider behavior.android:extractNativeLibs="true"for target SDK >= 23 when the application element does not already specify a value, while still honoring explicit template values.android:minSdkVersionfrom$(SupportedOSPlatformVersion)when a template already contains<uses-sdk />without a minSdk, matching legacyManifestDocumentbehavior and avoiding manifest merger failures where the app is treated as minSdk 1.targetSdkVersioncannot be resolved to an integer, matching legacyManifestDocument.Validation
dotnet test tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests.csproj --no-restore -v:minimaldotnet test src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj --no-restore --filter Name~Bug12935 -v:minimalgit diff --check