[TrimmableTypeMap] Manifest generator fixes#11143
Open
simonrozsival wants to merge 6 commits intomainfrom
Open
[TrimmableTypeMap] Manifest generator fixes#11143simonrozsival wants to merge 6 commits intomainfrom
simonrozsival wants to merge 6 commits intomainfrom
Conversation
…ge name The legacy ManifestDocument automatically sets targetPackage to the app's PackageName when [Instrumentation] doesn't specify it. Without this, the generated manifest has <instrumentation> without android:targetPackage, causing INSTALL_PARSE_FAILED_MANIFEST_MALFORMED on the device. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> (cherry picked from commit 70deb30)
…on mode switch
The CoreCLRTrimmable CI test was crashing with:
ClassNotFoundException: Didn't find class "android.apptests.App"
Root cause: when switching from the default (LLVM IR) typemap build to the
trimmable path in the same intermediate directory (sequential CI test runs),
_GenerateTrimmableTypeMap was incorrectly skipped because its output DLL
existed from a prior run. The stale LLVM IR manifest (which uses compat JNI
names like "android.apptests.App") remained while the JCW was generated with
the CRC-based name ("crc64.../App"), causing a name mismatch at runtime.
Fixes:
1. Add a sentinel file (.trimmable) written when _GenerateTrimmableTypeMap
runs. A new target _CleanStaleNonTrimmableState deletes the stale
typemap DLL when the sentinel is missing, forcing regeneration.
2. Pass extraBuildArgs to the Clean step in apk-instrumentation.yaml so
the Clean imports the same targets as the build and properly cleans
trimmable-specific files.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
(cherry picked from commit f719fb1)
…est generation The _GenerateTrimmableTypeMap target runs AfterTargets="CoreCompile" which executes during per-RID inner builds where IntermediateOutputPath includes the RID subdirectory (e.g. obj/.../android-arm64/). The generated manifest was written there, but _ManifestMerger and _GenerateJavaStubs (packaging phase) read from the outer IntermediateOutputPath (obj/.../net11.0-android/). This caused the stale LLVM IR manifest (with compat JNI names like "android.apptests.App") to be used instead of the trimmable manifest (with CRC-based JCW names), resulting in ClassNotFoundException at runtime. Fix: prefer _OuterIntermediateOutputPath (set by the outer build for inner per-RID builds) so typemap outputs land where the packaging phase expects. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> (cherry picked from commit a5a6dd8)
…RC names Manifest templates may hardcode compat JNI names (e.g., android.apptests.App) but the trimmable JCW generator uses CRC-based names (e.g., crc64.../App). The compat name rewrite must happen before collecting existingTypes so the duplicate check works correctly and we don't end up with both versions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> (cherry picked from commit 1f22129)
simonrozsival
added
copilot
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes several issues in the trimmable typemap manifest/tooling pipeline so manifests remain consistent with trimmable (CRC-based) JCW names and behave correctly across incremental/CI mode switches.
Changes:
- Default
[Instrumentation]targetPackageto the application package name when omitted. - Prevent stale non-trimmable state from skipping
_GenerateTrimmableTypeMapby adding a.trimmablesentinel and cleanup target; ensure CICleanruns with the same extra args. - Rewrite compat JNI names in manifest templates to CRC names and adjust typemap outputs to be written to the outer intermediate directory.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/ManifestGeneratorTests.cs | Adds regression tests for instrumentation defaulting and compat→CRC name rewriting. |
| src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.TypeMap.Trimmable.targets | Fixes intermediate output location and adds sentinel/cleanup to avoid stale typemap/manifest outputs across mode switches. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ManifestGenerator.cs | Rewrites compat names in templates to CRC names before duplicate detection and emission. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ComponentElementBuilder.cs | Defaults instrumentation android:targetPackage when not explicitly provided. |
| build-tools/automation/yaml-templates/apk-instrumentation.yaml | Passes extraBuildArgs through to -t:Clean so Clean imports the same targets/conditions as build. |
This was referenced Apr 17, 2026
- Resolve relative `android:name` forms (".Type", bare "Type") before the
compat→CRC rewrite in ManifestGenerator. The previous implementation only
matched fully-qualified dot-names and would silently leave relative/
unqualified names pointing at the compat Java name, also breaking the
duplicate-detection check (it would emit a second CRC-named component next
to the original relative entry). Extract TrimmableTypeMapGenerator's
existing ResolveManifestClassName into a shared ManifestNameResolver helper.
- Drop the `_CleanStaleNonTrimmableState` target and the `.trimmable`
sentinel file in Microsoft.Android.Sdk.TypeMap.Trimmable.targets. The
sentinel-based cleanup was narrow (only deleted the typemap DLL), asymmetric
(only covered llvm-ir → trimmable), and a one-off pattern. Replace with the
idiomatic fix: add `_AndroidTypeMapImplementation` to `_PropertyCacheItems`
in Xamarin.Android.Common.targets. This hooks typemap-mode switches into the
existing `_CleanIntermediateIfNeeded` → `_CleanMonoAndroidIntermediateDir`
mechanism used by ~30 other build-shape properties (AOT mode, link mode,
package format, etc.), so all stale intermediate artifacts (typemap DLL,
JCWs, manifest, acw-map) are cleaned on every switch in both directions,
and pre-existing `obj/` directories from a prior release are handled too.
- Add ManifestGeneratorTests cases for the ".Type" relative form, the bare
"Type" unqualified form, and a regression guarding that duplicate-detection
still works when a template references a peer by its relative name.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
/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.
🤖 AI Review Summary
Verdict: ✅ LGTM
Found 0 errors, 0 warnings, 2 suggestions.
- 💡 Correctness:
RewriteCompatNamesiterates all manifest descendants — consider scoping to component element types to avoid theoretically rewriting non-classandroid:namevalues (ManifestGenerator.cs:166) - 💡 Performance: Pre-allocate
compatToCrcdictionary (ManifestGenerator.cs:146)
👍 Positives:
- Clean refactoring of
ManifestNameResolverinto its own file, properly reused in bothTrimmableTypeMapGeneratorandManifestGenerator - Excellent test coverage: all three Android name forms (fully-qualified, relative dot, bare), the deduplication regression case, and the instrumentation targetPackage default
- The
_PropertyCacheItemsaddition is a neat, minimal solution that hooks into existing_CleanIntermediateIfNeededmachinery - The YAML change correctly passes
extraBuildArgsto Clean so it uses the same build properties - Code follows Mono style, nullable conventions, and project patterns throughout
All public CI checks pass (Linux, Windows, macOS builds + CLA).
Review generated by android-reviewer from review guidelines.
Generated by Android PR Reviewer for issue #11143 · ● 3.8M
| void RewriteCompatNames (XElement manifest, IReadOnlyList<JavaPeerInfo> allPeers) | ||
| { | ||
| // Build mapping: fully-qualified compat Java name → CRC Java name | ||
| var compatToCrc = new Dictionary<string, string> (StringComparer.Ordinal); |
There was a problem hiding this comment.
🤖 💡 Performance — Minor: the size is known upfront, so you could pre-allocate with allPeers.Count:
var compatToCrc = new Dictionary<string, string> (allPeers.Count, StringComparer.Ordinal);Rule: Pre-allocate collections when size is known (Postmortem #57)
Comment on lines
+166
to
+175
| foreach (var element in manifest.DescendantsAndSelf ()) { | ||
| var nameAttr = element.Attribute (AttName); | ||
| if (nameAttr is null) { | ||
| continue; | ||
| } | ||
| var resolved = ManifestNameResolver.Resolve (nameAttr.Value, PackageName); | ||
| if (compatToCrc.TryGetValue (resolved, out var crcName)) { | ||
| nameAttr.Value = crcName; | ||
| } | ||
| } |
There was a problem hiding this comment.
🤖 💡 Correctness — DescendantsAndSelf() iterates all manifest elements, so this rewrites android:name on <meta-data>, <action>, <category>, <permission>, <uses-permission>, <uses-feature>, etc. — not just component elements where android:name denotes a Java class.
While a collision is unlikely in practice (the compatToCrc dictionary only contains actual peer class names), scoping to component element types would be more defensive:
static readonly HashSet<string> ComponentElements = new (StringComparer.Ordinal) {
"application", "activity", "service", "receiver", "provider", "instrumentation",
};
// ...
foreach (var element in manifest.DescendantsAndSelf ()) {
if (!ComponentElements.Contains (element.Name.LocalName)) {
continue;
}
// ...
}This matches the scope that RootManifestReferencedTypes uses in TrimmableTypeMapGenerator.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Part 3 of the split of #11091.
Manifest / targets fixes
Default
instrumentation/@targetPackageto the app package name(
ComponentElementBuilder,ManifestGenerator) — when an[Instrumentation]attribute omits
TargetPackage, fall back to the application package ratherthan emitting an empty attribute.
Use the outer
IntermediateOutputPathfor typemap/manifest generation(
Microsoft.Android.Sdk.TypeMap.Trimmable.targets) — write generatedartifacts to the outer
obj/dir instead of the inner-build per-RID dir sothey are discoverable from the outer build.
Rewrite compat JNI names in the manifest template to CRC names
(
ManifestGenerator,ManifestGeneratorTests) — when the trimmable typemapuses CRC64-derived class names, the manifest template must reference the
same names, not the compat forms, or Android can't resolve the
<application>/<activity>/<instrumentation>targets.Handles all three forms Android accepts for
android:name:com.example.app.MainActivity).MainActivity)MainActivity)Relative and unqualified names are resolved against the manifest package
(via the shared
ManifestNameResolverhelper, factored out ofTrimmableTypeMapGenerator.ResolveManifestClassName) before thecompat → CRC lookup. This also makes the downstream duplicate-detection
check work correctly on templates that use relative names.
Cleaning stale state on typemap-mode switch
Earlier iterations of this PR included a narrow
_CleanStaleNonTrimmableStatetarget plus a
.trimmablesentinel file to handle the sequential-CI case ofllvm-ir→trimmablereusing the sameobj/. That approach was dropped infavor of the more general fix from #11098: adding
_AndroidTypeMapImplementationto
_PropertyCacheItemsinXamarin.Android.Common.targets. This hookstypemap-mode switches into the existing
_CleanIntermediateIfNeeded→_CleanMonoAndroidIntermediateDirmachinery that already coversAotAssemblies,AndroidLinkMode,AndroidPackageFormat, and ~30 other build-shape knobs, so:llvm-ir→trimmable).android/src/, mergedmanifest,
acw-map.txt), not just the typemap DLL.obj/from a prior release arehandled the same way mode changes are handled today.
The single-line
_PropertyCacheItemsaddition is included here as an interimin case #11098 isn't merged first; if #11098 lands first, the line cleanly
deduplicates.
The yaml hunk threading
${{ parameters.extraBuildArgs }}into-t:Cleaniskept — Clean must import the same targets as the build so the cleanup runs
with the correct
_AndroidTypeMapImplementationvalue.Scope
6 files, +174/-37. All 358
Microsoft.Android.Sdk.TrimmableTypeMap.Testspasslocally.