Repository navigation
[Xamarin.Android.Build.Tasks] Wire opt-in R8 runtime remapping - #12692
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It makes cross-cutting changes across MSBuild targets, build tasks, native runtime lookup code, and public API surface that require careful human validation beyond automated review.
Review tier: Lite
Findings: 3
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/GenerateJniRemappingNativeCodeTests.cs — ❌ error: Avoid the null-forgiving operator (!) in tests as well; it hides real nullability issues… |
|
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/R8Tests.cs — |
|
src/Xamarin.Android.Build.Tasks/Utilities/JniRemapping/R8Mapping.cs — |
What changed in this PR
Adds an experimental opt-in for R8 obfuscation in .NET for Android by generating and consuming runtime JNI remapping tables (rather than rewriting managed assemblies), enabling obfuscated DEX outputs while preserving managed JNI name expectations.
Changes:
- Introduces a two-pass R8 pipeline (seed mapping pre-trim + final R8
-applymapping) and generates JNI remapping XML/native tables for CoreCLR and NativeAOT. - Extends runtime remapping to cover reverse type lookups, rewritten method descriptors, and field remapping; shares native lookup code between CoreCLR and NativeAOT.
- Adds/updates tests and documentation for new public properties and XA4327/8/9 diagnostics.
| File | Description |
|---|---|
| tests/MSBuildDeviceIntegration/Tests/R8RuntimeRemappingTests.cs | Device test validating obfuscated members/types and incremental/missing-output recovery. |
| src/Xamarin.Android.Build.Tasks/Xamarin.Android.D8.targets | Wires new R8 inputs/outputs and enables mapping input/output + obfuscation flag. |
| src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets | Adds opt-in properties, validation (XA4329), incremental inputs, and AAPT rules tracking changes. |
| src/Xamarin.Android.Build.Tasks/Utilities/JniRemapping/R8Mapping.cs | Extends mapping parsing/projection for class/method/field data used by remapping generation. |
| src/Xamarin.Android.Build.Tasks/Utilities/JniRemapping/NativeAotJniRetention.cs | NativeAOT ELF-based literal retention to conservatively select required remap entries. |
| src/Xamarin.Android.Build.Tasks/Utilities/JniRemapping/JniDescriptorText.cs | Converts Java source-form types to JNI tokens + builds method descriptors. |
| src/Xamarin.Android.Build.Tasks/Utilities/JniRemapping/JniAssemblyRewriter.cs | Adds scan-only entrypoint for linked-assembly analysis (no rewriting). |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/R8Tests.cs | Adds unit coverage for keep-option and config generation behavior. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/GenerateTrimmableTypeMapTests.cs | Verifies NativeAOT proguard generation respects allowobfuscation. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/GenerateJniRemappingNativeCodeTests.cs | New tests for native remap table emission, ordering, and legacy compatibility. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/InvalidConfigTests.cs | Tests defaults + invalid configuration errors for new MSBuild properties. |
| src/Xamarin.Android.Build.Tasks/Tasks/R8.cs | Adds seed mapping mode, applymapping support, and conditional dontobfuscate removal. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateR8JniRemapping.cs | Generates JNI remapping XML from R8 mapping + existing remaps; supports NativeAOT retention path. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateR8JniManifestProguardConfiguration.cs | Generates manifest keep rules to stabilize seed mapping applicability. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateProguardConfiguration.cs | Emits allowobfuscation on keep rules when runtime remapping is enabled. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateNativeAotProguardConfiguration.cs | Emits allowobfuscation for NativeAOT-generated keep rules when enabled. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateJniRemappingNativeCode.cs | Extends generated tables to include reverse types + fields; exposes info for tests. |
| src/Xamarin.Android.Build.Tasks/Resources/proguard_xamarin.cfg | Adds/adjusts keep rules needed for stable seed/final graphs and bootstrap types. |
| src/Xamarin.Android.Build.Tasks/Resources/proguard_trimmable_nativeaot.cfg | Aligns NativeAOT baseline keep rules with remapping needs and seed/final stability. |
| src/Xamarin.Android.Build.Tasks/Properties/Resources.resx | Adds XA4327/8/9 localized strings for errors/warnings/validation. |
| src/Xamarin.Android.Build.Tasks/Properties/Resources.Designer.cs | Updates generated resource accessors for XA4327/8/9. |
| src/Xamarin.Android.Build.Tasks/MSBuild/Xamarin/Android/Xamarin.Android.Aapt2.targets | Moves AAPT proguard rule tracking to incremental parent target. |
| src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.TypeMap.Trimmable.targets | Imports new R8 JNI remapping targets last to override pre-trim outputs as needed. |
| src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.TypeMap.Trimmable.NativeAOT.targets | Includes new properties in incremental stamps; passes obfuscation state into proguard generation. |
| src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.TypeMap.Trimmable.CoreCLR.targets | Reworks linked-assembly proguard inputs; adds remapping-assembly prep + incremental inputs. |
| src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.TypeMap.LlvmIr.targets | Adds remapping enable flag into proguard generation and incremental inputs. |
| src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.R8JniRemapping.targets | New MSBuild pipeline for seed mapping, remap XML, and NativeAOT late-linked table build. |
| src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.NativeAOT.targets | Links remapping object into NativeAOT shared library and updates incremental inputs. |
| src/native/nativeaot/include/runtime-base/internal-pinvokes.hh | Adds internal pinvokes for reverse-type and field lookup. |
| src/native/nativeaot/host/jni-remapping-tables-stub.cc | Provides weak empty table symbols for apps without remapping objects. |
| src/native/nativeaot/host/internal-pinvoke-stubs.cc | Removes now-shared remapping pinvoke stubs from NativeAOT host stubs. |
| src/native/nativeaot/host/host.cc | Plumbs jniRemappingInUse via shared JniRemapping::is_in_use(). |
| src/native/nativeaot/host/CMakeLists.txt | Adds shared remapping sources and stub table compilation to NativeAOT host build. |
| src/native/native.targets | Includes shared remapping sources/headers in NativeAOT flavor build inputs. |
| src/native/mono/xamarin-app-stub/xamarin-app.hh | Updates stub ABI structs to include target_signature + field remapping structures. |
| src/native/mono/xamarin-app-stub/application_dso_stub.cc | Updates stub table initializers for new method signature field. |
| src/native/mono/runtime-base/internal-pinvokes.hh | Adds internal pinvoke declarations for reverse-type and field lookup. |
| src/native/mono/pinvoke-override/pinvoke-tables.include | Extends pinvoke table entries/count for the new remapping exports. |
| src/native/mono/pinvoke-override/generate-pinvoke-tables.cc | Adds new internal pinvoke names to generator input list. |
| src/native/mono/monodroid/internal-pinvokes.cc | Adds MonoVM-safe placeholder exports for new remapping entrypoints. |
| src/native/clr/xamarin-app-stub/application_dso_stub.cc | Extends CLR stub tables to include reverse types + fields + signature pinning. |
| src/native/clr/runtime-base/jni-remapping.cc | Implements binary-search remapping lookups (types, reverse types, methods, fields) + is_in_use(). |
| src/native/clr/pinvoke-override/precompiled.cc | Maps new internal pinvoke entrypoints to implementations. |
| src/native/clr/include/xamarin-app.hh | Declares remapping table symbols and adds field + reverse type structures. |
| src/native/clr/include/runtime-base/jni-remapping.hh | Declares shared lookup surface including reverse type and field lookup. |
| src/native/clr/include/runtime-base/internal-pinvokes.hh | Declares new remapping pinvokes for CoreCLR runtime. |
| src/native/clr/host/internal-pinvokes-shared.cc | Centralizes shared remapping pinvoke implementations for CoreCLR/NativeAOT. |
| src/native/clr/host/internal-pinvokes-clr.cc | Removes remapping implementations now provided by shared file. |
| src/native/clr/host/host.cc | Uses JniRemapping::is_in_use() for init flag and includes remapping header. |
| src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMapValueManager.cs | Ensures FindClass uses replacement type name when remapping is enabled. |
| src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMapTypeManager.cs | Adds reverse-type handling for Java-to-managed lookups; uses replacement type for signatures. |
| src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs | Uses reverse type for proxy lookup and replacement type for FindClass checks. |
| src/Mono.Android/Microsoft.Android.Runtime/JniRemappingLookup.cs | Adds reverse type + field lookup plumbing and supports target-method-signature. |
| src/Mono.Android/Android.Runtime/RuntimeNativeMethods.cs | Adds LibraryImport declarations for reverse type + field lookup pinvokes. |
| src/Mono.Android/Android.Runtime/AndroidRuntime.cs | Exposes GetOriginalTypeCore via reverse-type lookup. |
| external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniPeerMembersTests.cs | Adds tests validating remapped field names and pinned target signatures. |
| external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JavaVMFixture.cs | Extends test runtime type manager with field replacement support. |
| external/Java.Interop/src/Java.Interop/PublicAPI.Unshipped.txt | Records new public API surface additions for replacement fields + original type. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JniType.cs | Adds TryGet{Static,Instance}Field helpers to support remapped field probing. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.ReflectionJniTypeManager.cs | Adds null default implementation for field replacement in reflection manager. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniTypeManager.cs | Adds ReplacementFieldInfo + original type + replacement field APIs. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniStaticMethods.cs | Uses replacement lookup keyed by original type name (compat + remapping). |
| external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniStaticFields.cs | Adds remapped static field probing and fallback to original lookup. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniInstanceMethods.cs | Tracks original vs effective JNI type names; remaps ctor/method lookup accordingly. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniInstanceFields.cs | Adds remapped instance field probing and fallback behavior. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.cs | Tracks original type name and performs replacement lookups across base types/types. |
| Documentation/docs-mobile/TOC.yml | Adds XA4327/8/9 docs to TOC. |
| Documentation/docs-mobile/messages/xa4329.md | Documents invalid/unsupported configuration errors and resolutions. |
| Documentation/docs-mobile/messages/xa4328.md | Documents remapping incompleteness warnings (conflicts/signature conversion). |
| Documentation/docs-mobile/messages/xa4327.md | Documents remapping generation failures and troubleshooting steps. |
| Documentation/docs-mobile/messages/index.md | Adds XA4327/8/9 to messages index. |
| Documentation/docs-mobile/building-apps/build-properties.md | Documents AndroidEnableR8Obfuscation + AndroidR8ObfuscationMode properties. |
Files not reviewed (1)
- src/Xamarin.Android.Build.Tasks/Properties/Resources.Designer.cs: Generated file
Suppressed comments (1)
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/GenerateJniRemappingNativeCodeTests.cs:112
- ❌ error: This uses the null-forgiving operator (
info!) afterAssert.IsNotNull, which the compiler can’t reason about. Prefer?? throwso nullability is enforced without suppressions.
Context: #12692 Follow the existing Intune contract: member lookup keys contain the replaced owner type and the original managed member name and descriptor. Generate R8 member entries and collision keys in that same form rather than teaching every consumer to try both original and replaced owners. Remove JniPeerOriginalTypeName, the duplicate constructor identity, and the now-unused Java.Interop reverse-type hook. Keep reverse lookup for peer activation and derive desugared companion names in the Android remapping helper where the original name is actually needed. Cover residual-owner keys, descriptors, MAM conflicts and duplicates, and real renamed-class construction and member access on both runtimes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
❌ Needs Changes — 1 error found.
The remapping pipeline is well covered and the runtime/native lookup design is coherent, but field-entry conflict detection currently omits the JNI descriptor from the key. Under R8 class merging this can discard a valid field remap and cause runtime lookup failure. CI has no observed failures, with the latest build checks still in progress.
Generated by Android PR Reviewer for #12692 · gpt56 · 770.9 AIC · ⌖ 5.5 AIC · ⊞ 26.3K
Comment /review to run again
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Needs Changes
Found 2 errors in JNI field remapping: inherited remaps can override hidden derived fields, and the newly enabled field fixtures run on MonoVM even though its field-remapping entry point is a stub. The overall cross-runtime design and focused coverage are substantial, but these correctness gaps should be addressed before merge.
CI has not run for the latest commit: the dotnet-android check is currently action_required (awaiting pipeline authorization); CLA passes.
Generated by Android PR Reviewer for #12692 · gpt56 · 1.1K AIC · ⌖ 18.7 AIC · ⊞ 25.7K
Comment /review to run again
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
❌ Needs changes
Found 4 errors in the new runtime-remapping paths:
- Member lookup can select a renamed base member before an unchanged hiding member on the current type.
- Java-to-managed lookup can resolve a residual-name/original-name collision to the wrong managed type.
- MonoVM wildcard and parameter-only remaps can shadow more-specific descriptors because of generated sort order.
- Generated LLVM member-array symbols can collide or become invalid for Unicode JNI names.
The overall single-pass R8 pipeline is thoughtfully integrated across CoreCLR and NativeAOT, with substantial build, unit, and device coverage. All 44 current CI checks completed successfully, but the cases above require adversarial regression coverage before merge.
Generated by Android PR Reviewer for #12692 · copilot · gpt56 · 1.8K AIC · ⌖ 11.5 AIC · ⊞ 21K
Comment /review to run again
|
/review |
|
✅ Android PR Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
|
There was a problem hiding this comment.
The runtime-remapping implementation has broad coverage across Java.Interop, CoreCLR, NativeAOT, MSBuild incrementality, and device/build tests. One MSBuild ordering issue remains: the post-R8 NativeAOT link can run after _CompileToDalvik fails and may consume stale output or obscure the primary failure.
CI is still in progress: the completed checks are passing, with no failures currently reported.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
azcliprod.blob.core.windows.net
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
Generated by Android PR Reviewer for #12692 · copilot · gpt56 · 738.1 AIC · ⌖ 18.2 AIC · ⊞ 21K
Comment /review to run again
## Motivation The existing Intune/MAM JNI method-remapping path stores target type and method names as stable NUL-terminated UTF-8 strings in generated native data. The old path materialized those names as managed UTF-16 strings and then encoded them back to UTF-8 for `FindClass` and `GetMethodID`. That round trip is unnecessary and becomes more important as remapping is reused by larger consumers such as R8. ## Approach - Add pointer-backed target type, method-name, and method-signature values to `ReplacementMethodInfo`. - Keep pointer and string representations independent so reading a compatibility string property does not implicitly decode native memory. - Add `JniType` lookup paths for pointer/pointer and mixed pointer/span member names and signatures. - Let `JniPeerMembers` retain a replacement type pointer and use it directly for `FindClass` and later member-remapping lookups. - Keep native pointers in `JniMethodInfo` Debug metadata and decode them only if `Name`, `Signature`, or `ToString()` is explicitly requested. - Retain the existing string/span paths for custom `JniTypeManager` implementations. - Document UTF-8 encoding, NUL termination, ownership, and lifetime requirements for every pointer API. The successful generated-remapping path therefore passes the pregenerated UTF-8 type, method name, and optional signature directly to JNI without copying them or converting them to a managed string. This PR is method-only and does not add R8, field remapping, reverse-type mapping, inherited-member fallback, or NativeAOT remapping support. ## Relationship to other PRs - This PR is based directly on `main` and provides the Java.Interop representation and JNI lookup primitives. - #12796 supplies the generated pointers, performs managed table search, and removes the native remapping P/Invokes. - #12692 can build on this stack to add the R8-specific field, reverse-type, inherited-member, and NativeAOT pieces. ## Validation - Java.Interop Debug build. - Java.Interop `JniPeerMembersTests`: 12 passed, 1 skipped. - The JVM fixture exercises stable unmanaged UTF-8 type/name/signature storage, signature fallback, instance/static lookup, and instance-to-static remapping.
Integrate the typemap selector removal while retaining runtime-remapping validation and OS-native generated ProGuard path construction. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Integrate the typemap selector removal while retaining runtime-remapping validation and OS-native generated ProGuard path construction. Compute the clean directory before evaluating Exists so the condition remains valid MSBuild syntax. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Resolve expected and actual configuration paths with System.IO.Path before comparing them so equivalent mixed Windows separators do not fail the target test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@dalexsoto review |
dalexsoto
left a comment
There was a problem hiding this comment.
Completed the full current 32-file correctness, integration/completeness, candidate-ledger and final additional-blocker passes. The resolved lookup/codegen concerns, accepted post-R8 target ordering and explicit-primary-RID coverage do not leave additional independent blockers.
Preserve live field remaps for JavaInterop1 bindings. The class-only TypeMap retention change removes the fallback that retained field identities, while ScanFieldLikeMember still recognizes only RegisterAttribute. The supported AndroidCodegenTarget=JavaInterop1 emitter deliberately omits that attribute on generated field properties; its accessors instead use constant JNI identifiers such as value.I.
A concrete supported case is a JavaInterop1 binding for public example.PurePeer.value, consumed by a trimmed Release CoreCLR application with runtime-remapping and an empty custom ProGuard file, as used by the existing device fixture. The binding option is distinct from _AndroidJcwCodegenTarget, which stays at its supported default. The active primary rule retains PurePeer and its fields with allowobfuscation; no name-preserving rule matches this field in that configuration. This is not a blanket claim about every default AGP policy.
If R8 renames the field, the head scanner records the class but no field key, so GenerateR8JniRemapping drops the replacement. Java.Interop then probes the original value:I and cannot resolve the renamed field. The base scanner/task contract retained the field key through RecordAllMappings; no complete base application run is claimed. These are live generated constants, not the accepted arbitrary-computed-JNI-name limitation.
Please provide and consume field-specific retention metadata for this emitter, or preserve these field names until supported, and add generated JavaInterop1 instance/static-field coverage. This need not restore all unused member mappings. Evidence is immutable producer/consumer source tracing and a source-derived model; I did not execute the binding compiler, R8 or a JNI/device reproduction, and a is only an illustrative permitted residual name.
JavaInterop1 binding field accessors use constant JNI field identifiers but do not carry RegisterAttribute metadata. Preserve field names for runtime-remapping builds using this emitter while leaving class and method renaming enabled. Cover generated instance and static field access in the runtime fixture and retain the existing manually registered field-remapping assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Addressed Alex’s JavaInterop1 field-remapping blocker in
The existing CoreCLR/NativeAOT device fixture now selects JavaInterop1 and covers both generated instance/static fields while retaining the manually registered hidden-field remapping assertions. Validation:
A private API 35 arm64 runtime attempt was blocked before app build because this worktree’s local workload registration is incomplete after the unavailable pinned-pack bootstrap failure ( |
dalexsoto
left a comment
There was a problem hiding this comment.
Re-reviewed the complete 32-file change at f4fee64. Preserving field names is an acceptable conservative fix; I am not requesting maximal field obfuscation or reintroducing the rejected MSBuildLastTaskResult guard. Four concrete blockers remain:
-
The original mixed-emitter field-retention case is still uncovered.
R8.cs:167-168gates preservation on the consuming application'sAndroidCodegenTarget, forwarded byXamarin.Android.D8.targets:86. The original supported case was a JavaInterop1 binding library consumed by a default-XA, trimmed CoreCLR application, not an application globally switched to JavaInterop1. That application still emits no field-name-preserving rule. With the reachable empty custom R8 configuration, the generated library field can be obfuscated while the metadata-only scanner omits its F key and runtime lookup falls back to its original name/signature. This is the same previous blocker, not a new objection to preservation. Cover the actual consumed binding emitters, or preserve fields conservatively for runtime-remapping; if the decision stays conditional, include its effective inputs in the production incremental cache. -
The new JavaInterop1 fixture currently generates uncompilable C#. The emitter switch at
R8RuntimeRemappingTests.cs:80activates a preexisting reference-importer defect. In current-source build 1627672, bothObfuscatedMembersRun(CoreCLR)andObfuscatedMembersRun(NativeAOT)failCoreCompilein generatedExample.RuntimePeer.cs, with six errors each:CS1056,CS1519,CS1002, andCS1525involving backticks and the followingrefexpression. Neither reaches installation or runtime assertions.The producer path is
JavaArray<T>/JavaObjectArray<T>declaringJniTypeSignature("java/lang/Object", ArrayRank=1), whileexternal/Java.Interop/tools/generator/Java.Interop.Tools.Generator.Importers/CecilApiImporter.cs:118-143,253-265ignores the rank.SymbolTable.cs:153-181,271-325then permits those array wrappers to shadow scalarjava.lang.Object; their CLR generic-arity names pass through the base andCreateHiddenwriters. The source-derivedJavaObjectArrayshape matches all reported backtick/ref columns, but I have not retrieved the generated C# or inspected the published reference DLL metadata. Correct the ranked-versus-scalar registration, preserving the proper Object peer, rather than merely stripping the backtick. The importer code itself is unchanged from base/788; this revision's fixture selection exposes it. -
The handwritten binding must also be adapted to the JavaInterop1 base API. Independently of those syntax errors,
HiddenPeerBinding'sbase(handle, transfer)call at line 106 still expects the XAIntPtr, JniHandleOwnershipconstructor. The selected generator'sSourceWriters/JavaLangObjectConstructor.cs:23-32emitsref JniObjectReference, JniObjectReferenceOptionsinstead. After restoring canonicalJava.Interop.JavaObjectselection, the oldThresholdClass/ThresholdTypeoverrides are also absent from that base API. Adapt or bridge the handwritten activation path and overrides while preserving constructor rooting. A successful integration-test-project build compiles the embedded fixture strings, not this generated application. This is source-proven and currently masked by item 2; the CI logs do not show observedCS1503/CS1620constructor diagnostics. -
The hidden-field mapping assertion contradicts the active keep policy.
R8RuntimeRemappingTests.cs:178-180requireshiddenValue's target name to differ from its source name. The new-keepclassmembernames class * { <fields>; }rule inR8.cs:287-288also preserves manually registered fields, includinghiddenValue. Owner-only remapping can legitimately remain while the field name stays unchanged, as supported byGenerateR8JniRemapping.cs:254-273. Update the fixture to check that valid remapping/lookup contract rather than demand field-name obfuscation that its own policy forbids. Fixing either compilation issue does not fix this assertion.
The review used immutable source tracing and source-derived models, plus the published current-head CI build logs; no local SDK/R8/JNI/device execution is claimed. The actual CI failure is generated-code syntax, not the author's separate local NETSDK1147 workload/bootstrap limitation, and it does not establish an observed runtime field or assertion failure.
JavaInterop1 binding libraries can be consumed by applications using a different code generator, so the consuming application property cannot identify every constant JNI field lookup. Preserve field names conservatively for all runtime-remapping builds. Keep class and method renaming enabled, and validate field remapping through renamed owners with stable field names. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Addressed all four blockers from the
Validation:
The prior current-head CI generated-code failure path has been removed rather than patched around. |
Fields whose names and signatures are preserved do not need replace-field entries. The renamed owner is handled by the type mapping and the runtime assertions exercise the generated field accessors. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A field whose name and signature are unchanged does not require a replace-field entry even when its owner is renamed. The replace-type entry handles owner translation and the device execution verifies the actual lookup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@dalexsoto review |
|
@simonrozsival I haven't submitted a new GitHub review for this context: the head changed from |
|
@dalexsoto review |
|
@dalexsoto stamp |
1 similar comment
|
@dalexsoto stamp |
|
/review |
|
✅ Android PR Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
|
There was a problem hiding this comment.
The runtime-remapping orchestration is comprehensive and the production changes are well covered across CoreCLR and NativeAOT. I found one XA4329 documentation mismatch and two integration-test gaps where the assertions can pass without proving the stated ordering or table-update behavior. All 42 reported GitHub/Azure checks are green.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
azcliprod.blob.core.windows.net
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
Generated by Android PR Reviewer for #12692 · copilot · gpt56 · 561.9 AIC · ⌖ 11.3 AIC · ⊞ 26.4K
Comment /review to run again
Document the actual PublishTrimmed validation for XA4329, assert runtime-specific trim and native-link nodes exist before checking R8 ordering, and prove a changed rule removes the int-overload remapping entry while leaving the other overload mapped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
|
There was a problem hiding this comment.
✅ No additional findings on the current head.
I reviewed the complete 32-file change and the latest fixes at 925d390. The XA4329 documentation now matches the actual validation, the ordering test proves the runtime-specific trim and native-link nodes exist, and the changed-rule test verifies that the late-linked remapping table content changes. The previously raised JNI lookup, field-retention, code-generation, orchestration, and incrementality concerns are addressed or resolved in the current implementation.
CI is still in progress: CLA and both Android Tools lanes pass; the Linux, macOS, Windows, and aggregate Azure DevOps checks remain running/queued, so this review does not claim a green CI result.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
azcliprod.blob.core.windows.net
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
Generated by Android PR Reviewer for #12692 · copilot · gpt56 · 170.5 AIC · ⌖ 11.2 AIC · ⊞ 26.4K
Comment /review to run again
…mbly (#12844) ## Summary This PR is stacked on and depends on #12692. That parent is built on the focused lower chain #12846, #12847, and #12848. This final layer is only a task-assembly boundary change for the runtime-R8 remapping producer. It moves `GenerateR8JniRemapping` and the .NET-only helpers it directly requires into `Microsoft.Android.Build.Tasks`: - `JniRemappingAssemblyScanner` - `R8Mapping` - `JniDescriptorText` - `NativeAotJniRetention` The generic runtime table producer remains in `Xamarin.Android.Build.Tasks`: `GenerateJniRemappingNativeCode` and `JniRemappingNativeCodeGenerator` are unchanged and continue to serve both MAM and R8 remapping. The relocation also: - loads only `GenerateR8JniRemapping` from the modern assembly using the existing Full/Core `UsingTask` pattern - tracks both modern and legacy task assemblies in mixed incremental targets - links only the metadata helpers required by the read-only scanner - packages `ELFSharp.dll` beside the modern task assembly under `tools/net` - keeps net10 `MSBuildDeviceIntegration` compatible by linking `R8Mapping.cs` instead of referencing the net11 task project - preserves a test-only aliased legacy task reference to validate generated XML through `MergeRemapXml` and `GenerateJniRemappingNativeCode` This PR does not restore or relocate the removed managed assembly rewriter, rewrite planners/rebuilders, rewrite-only tests, or `XA4325`/`XA4326` behavior. ## Validation - `Microsoft.Android.Build.Tasks` build - `Xamarin.Android.Build.Tasks` build - 86 focused modern R8 remapping tests - 41 trimmable typemap integration tests - `MSBuildDeviceIntegration` project build - verified packaged `tools/net/ELFSharp.dll` - final read-only review found no blocking issues
Context: #12535
This is the product policy and build-orchestration layer for opt-in R8 runtime JNI remapping. It is stacked on:
A final dependent PR, #12844, moves the .NET-only R8 mapping task into
Microsoft.Android.Build.Tasks.This PR does not rewrite managed assemblies. Managed bindings retain their original JNI names; the lower layers generate native tables that translate lookups to the names and descriptors emitted by R8.
Opt-in
For a trimmed CoreCLR or NativeAOT application:
runtime-remappingis the sole opt-in; there is no separate enable property. It does not apply to library projects. The existingprivate-membersanddisabledbehavior remains unchanged. Unknown mode values report XA1050, while incompatible runtime-remapping configurations report XA4329.Build orchestration
Coverage in this layer
Host tests cover mode defaults, configuration validation, keep-rule policy, NativeAOT ProGuard configuration, packaging metadata, and incremental behavior. Build/device integration tests cover renamed types and members, constructors, overloads, inherited lookups, peer activation, single-pass ordering, multi-RID builds, missing-output recovery, and opt-out for CoreCLR and NativeAOT.
The generic lookup semantics and mapping/table-generation tests live in #12847 and #12848 respectively.
Experimental limitations
NativeAOT literal matching is conservative and can retain extra entries. Arbitrarily computed JNI names may require explicit remaps or keep rules. Conservative public/nested-class, interface, bootstrap, and native-callback keeps limit obfuscation. Existing Intune/R8 conflict handling is not full remapping-chain composition, and ambiguous reverse mappings for merged classes are omitted. This remains an experimental opt-in, not a production-readiness claim.
Fixes: #12535