[TrimmableTypeMap] Use Crc64 package naming by default with LowercaseCrc64 compatibility#11193
[TrimmableTypeMap] Use Crc64 package naming by default with LowercaseCrc64 compatibility#11193Copilot wants to merge 14 commits into
Conversation
|
@copilot I would prefer changing the prefix from |
There was a problem hiding this comment.
Note that there is the property:
<AndroidPackageNamingPolicy>LowercaseCrc64</AndroidPackageNamingPolicy>We will probably want to introduce a new one for XxHash64.
It is a slight breaking change, because people could have a hardcoded package name in a script for launching their main activity, etc.
So, I think what we did in some really old Xamarin.Android release, we changed the default but allowed projects to still use the old one.
So funny thing is that the CSC64 we have in our C++(or C#?) code and CSC64 that is in System.IO.Hashing produce different results. I decided to use the System.IO.Hashing one in the trimmable typemap so that would be a breaking change anyway. I will need to revisit this and try to make the |
|
@copilot let's respect |
Implemented in ec92067: trimmable typemap now respects
... Addressed in ec92067: added trimmable support for |
simonrozsival
left a comment
simonrozsival
left a comment
There was a problem hiding this comment.
@copilot address these comments
I want to move the hashing code from JavaPeerScanner to a separate static ScannerHashingHelper class which can be better tested. I want a test that will just simply validate some known inputs to some known outputs of the legacy crc64 algorithm to prove we're maintaining backwards compatibility.
Implemented in 05cebc7: moved scanner hashing into
Handled in 05cebc7: trimmable typemap continues to respect explicit
|
|
@copilot update the PR by pulling latest main and resolve conflicts |
|
@copilot resolve the merge conflicts in this pull request, also address review comments from Jon |
…ixes Agent-Logs-Url: https://github.com/dotnet/android/sessions/6064358e-ab27-46b0-a022-bc0dfde2443a Co-authored-by: simonrozsival <[email protected]>
…CI fixes # Conflicts: # build-tools/automation/yaml-templates/variables.yaml # src/Microsoft.Android.Sdk.TrimmableTypeMap/TrimmableTypeMapGenerator.cs # src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.TypeMap.Trimmable.targets # src/Xamarin.Android.Build.Tasks/Tasks/GenerateTrimmableTypeMap.cs
There was a problem hiding this comment.
Pull request overview
This PR updates trimmable typemap generation to default to Crc64-based hashed package naming for unregistered types, while preserving legacy LowercaseCrc64 behavior when explicitly requested via AndroidPackageNamingPolicy.
Changes:
- Plumbs
AndroidPackageNamingPolicyinto the trimmable typemap pipeline and introduces a trimmable-specific default (Crc64). - Refactors hashing into a new
ScannerHashingHelperwith both modernSystem.IO.Hashing.Crc64and legacy CRC64 implementations. - Updates and expands test coverage for both the new default and legacy compatibility behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Scanner/ScannerHashingHelperTests.cs | Adds fixed-vector coverage for both legacy and new CRC64 hashing + length-mixing regression test. |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Scanner/JavaPeerScannerTests.cs | Updates expected hashed package naming for the new default and adds a compatibility-difference test. |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/FixtureTestBase.cs | Adds helpers to rescan fixtures using a specified package naming policy. |
| src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.props.in | Introduces a sentinel property to detect when AndroidPackageNamingPolicy is user-specified. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateTrimmableTypeMap.cs | Passes package naming policy through to the trimmable typemap generator. |
| src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.TypeMap.Trimmable.targets | Sets Crc64 as the trimmable default while honoring explicitly-set policy values. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/TrimmableTypeMapGenerator.cs | Threads package naming policy into assembly scanning. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/ScannerHashingHelper.cs | New helper implementing modern CRC64 hashing and legacy compatibility hashing. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/JavaPeerScanner.cs | Switches to policy-aware hashed package naming using ScannerHashingHelper. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Microsoft.Android.Sdk.TrimmableTypeMap.csproj | Enables unsafe blocks and links legacy CRC64 sources for compatibility. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ManifestGenerator.cs | Updates documentation wording to reflect “hashed package names”. |
- Remove unused 'using Java.Interop.Tools.JavaCallableWrappers;' from JavaPeerScanner.cs (CS8019 would break warnings-as-errors). - Wrap PEReader in 'using' inside FixtureTestBase.ScanFixtures (string) so TestFixtures.dll is released even when Scan() throws. - Make JavaPeerScanner.ParsePackageNamingPolicy strict: only null/empty, 'Crc64', and 'LowercaseCrc64' (case-insensitive) are accepted; anything else now throws ArgumentException instead of silently falling back to Crc64. Added xunit Theories covering supported and unsupported values. - Validate AndroidPackageNamingPolicy in Microsoft.Android.Sdk.TypeMap.Trimmable.targets: emit a build error before the trimmable typemap generator runs when the user explicitly set a policy other than Crc64 or LowercaseCrc64 (e.g. 'Lowercase'). Co-authored-by: Copilot <[email protected]>
MSBuild conditions are case-sensitive by default but the C# parser in JavaPeerScanner.ParsePackageNamingPolicy uses StringComparison.OrdinalIgnoreCase. Lowercase the property before comparing so e.g. 'crc64' or 'lowercasecrc64' do not falsely trigger the unsupported-policy build error. Co-authored-by: Copilot <[email protected]>
simonrozsival
left a comment
simonrozsival
left a comment
There was a problem hiding this comment.
@copilot address these comments
…okSupport override Agent-Logs-Url: https://github.com/dotnet/android/sessions/093b5a99-9f87-468c-b7f4-b6c221eb3561 Co-authored-by: simonrozsival <[email protected]>
Agent-Logs-Url: https://github.com/dotnet/android/sessions/093b5a99-9f87-468c-b7f4-b6c221eb3561 Co-authored-by: simonrozsival <[email protected]>
|
/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.
@copilot
⚠️ Needs Changes
Summary: This PR cleanly wires AndroidPackageNamingPolicy into the trimmable typemap path, defaulting to Crc64 (with length mixing) while preserving LowercaseCrc64 backward compatibility. The architecture is sound — hashing logic is well-extracted into ScannerHashingHelper, the MSBuild plumbing correctly detects user-set vs. default policies, and tests cover known-value stability, policy validation, and the zero-length collision edge case.
Issues by severity
| Severity | Count |
|---|---|
| 💡 Suggestion | 3 |
Positive callouts
- Good design: The
_AndroidPackageNamingPolicySetByUsersentinel in.props.incorrectly captures user intent before the default is applied, avoiding the common MSBuild "is this the default or did the user set it?" ambiguity. - Length mixing: XORing the CRC64 hash with the byte length is a simple and effective way to prevent collisions for inputs of different lengths that hash to the same value — the zero-byte regression test validates this nicely.
- Clean separation: Extracting
ScannerHashingHelperas a standaloneinternal staticclass with focused tests is well-structured. Theinternal/InternalsVisibleTopattern follows repo conventions. - Validation: The
_ValidateTrimmableTypeMapPackageNamingPolicytarget gives a clear, actionable error message for unsupported policies, consistent with the existing_ValidateTrimmableTypeMapRuntimepattern.
Notes
- CI checks (
dotnet-android,license/cla) are green. - The 3 inline suggestions are all 💡-level (convention alignment and removing hand-rolled utilities). None block merge.
Generated by Android PR Reviewer for issue #11193 · ● 8.4M
Address review feedback: - ScannerHashingHelper: replace hand-rolled WriteUInt64LittleEndian/ ReadUInt64LittleEndian helpers with System.Buffers.Binary.BinaryPrimitives (available transitively via System.IO.Hashing). - JavaPeerScanner.ParsePackageNamingPolicy: use the project-local NullableExtensions.IsNullOrEmpty() extension method (NRT-aware) instead of the static string.IsNullOrEmpty(). Co-authored-by: Copilot <[email protected]>
simonrozsival
added
the
ready-to-review
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
@copilot
Review Summary
✅ Overall Assessment
Well-structured PR that wires AndroidPackageNamingPolicy into the trimmable typemap path with proper backward compatibility. The implementation cleanly separates concerns:
ScannerHashingHelperisolates hashing logic for focused testingHashedPackageNamingPolicyenum is correctly private toJavaPeerScanner- MSBuild plumbing correctly distinguishes user-set vs default policy via
_AndroidPackageNamingPolicySetByUser - Incremental build correctness is maintained via the existing
AndroidPackageNamingPolicyentry in_PropertyCacheItems - Validation exists at both the MSBuild target level (early, clear error) and C# level (defensive)
Verified
- ✅ Hash output consistency:
ToLegacyCrc64andToCrc64test vectors match scanner expectations - ✅ Legacy compatibility:
LowercaseCrc64path uses the original Jones CRC64 implementation from Java.Interop - ✅ Length mixing prevents zero-byte collisions (with regression test)
- ✅ Byte-level encoding:
GetNamespaceAssemblyUtf8Bytescorrectly handles thens:assemblyNameformat - ✅ Incremental builds:
AndroidPackageNamingPolicyis already in_PropertyCacheItems(line 991 ofXamarin.Android.Common.targets) - ✅
ParsePackageNamingPolicyusesIsNullOrEmpty()extension method per repo convention
CI Status
license/cla: ✅ Passeddotnet-android: ✅ PassedXamarin.Android-PR: Not visible — may not have triggered yet
Issues Found
| Severity | Count |
|---|---|
| 1 | |
| 💡 Suggestion | 3 |
No blocking issues. The warning is about ArrayPool consistency in ToCrc64's heap fallback path — low practical impact since namespace:assembly pairs are almost always under 256 bytes.
Generated by Android PR Reviewer for issue #11193 · ● 6M
| [Fact] | ||
| public void ToCrc64_DifferentLengthsOfZeroBytes_HaveDifferentOutput () | ||
| { | ||
| Assert.NotEqual ( | ||
| ScannerHashingHelper.ToCrc64 ("\0", ""), | ||
| ScannerHashingHelper.ToCrc64 ("\0\0\0\0", "")); | ||
| } |
There was a problem hiding this comment.
🤖 💡 Testing — Good regression test for the length-mixing defense. Consider also adding test vectors for edge cases that exercise the UTF-8 encoding boundary conditions in GetNamespaceAssemblyUtf8Bytes:
- Empty namespace with non-empty assembly name (i.e.,
ToCrc64("", "MyAssembly")) — verifies the":MyAssembly"encoding path - A non-ASCII namespace (e.g., multi-byte UTF-8) — verifies the byte count / byte write consistency
These would increase confidence that the unsafe encoding helper handles boundary inputs correctly.
Rule: Test edge cases
| return bytesWritten; | ||
| } | ||
|
|
||
| static string ToHexString (ReadOnlySpan<byte> hash, bool lowercase) |
There was a problem hiding this comment.
🤖 💡 YAGNI — The lowercase parameter is always passed as true (both call sites). Consider removing it and hardcoding the lowercase behavior to simplify this helper and GetHexValue. If an uppercase variant is needed later, it can be added then.
Rule: YAGNI (Postmortem #58)
| HashedPackageNamingPolicy.LowercaseCrc64 => "crc64" + ScannerHashingHelper.ToLegacyCrc64 (ns, assemblyName), | ||
| HashedPackageNamingPolicy.Crc64 => "crc64" + ScannerHashingHelper.ToCrc64 (ns, assemblyName), |
There was a problem hiding this comment.
Should we put a character to make it easy to tell the difference between these? crc64s (short for System.IO.Hashing?) isn't the best, but that is an example that would work. We probably shouldn't put characters 0-f, as the actual hash has those.
| public sealed class JavaPeerScanner : IDisposable | ||
| { | ||
| enum HashedPackageNamingPolicy { | ||
| Crc64, |
There was a problem hiding this comment.
Should we add the new option to java-interop, too?
I'm wondering if there are cases old code paths could get called, even when opting into the new trimmable typemap.
| } | ||
| } | ||
|
|
||
| internal static string ToCrc64 (string ns, string assemblyName) |
There was a problem hiding this comment.
Could some of this code be deleted and use the dotnet/android-tools API instead?
The new fast CRC64 path (System.IO.Hashing) now emits names with an 'scrc64' prefix so they are visibly distinct from the legacy 'crc64' prefix produced by the LowercaseCrc64 policy. This avoids ambiguity in acw-map.txt / manifests when comparing the two algorithms and makes scanner-vs-generator diffs easier to read. Also drops the unused 'lowercase' parameter from ScannerHashingHelper.ToHexString / GetHexValue (it was always passed as true). Co-authored-by: Copilot <[email protected]>
This updates trimmable type map generation to default to
Crc64-based package naming for unregistered types, while preserving backwards compatibility for projects that explicitly set:Package naming policy support in trimmable typemap
AndroidPackageNamingPolicyinto trimmable typemap generation.Crc64(trimmable default) →crc64...usingSystem.IO.Hashing.Crc64with length mixingLowercaseCrc64→crc64...using the existing legacy/custom CRC64 implementation for compatibility with previous namingXxHash64handling, as it no longer provides value for this path.Mono.Androidnamespace-preserving behavior unchanged.Default policy update
Crc64is now the default only for trimmable typemap generation.AndroidPackageNamingPolicydefault remainsLowercaseCrc64for non-trimmable typemap paths.Hashing implementation refactor
JavaPeerScannerinto a dedicated staticScannerHashingHelperfor easier focused testing.ScannerHashingHelper.ToCrc64()forSystem.IO.Hashing.Crc64.Crc64path to usestackallocwithnew byte[N]fallback for larger buffers (>256 bytes).crc64 ^ byteLength) inToCrc64()to avoid equal outputs for different-length all-zero inputs.Targeted test updates
crc64trimmable defaults.Crc64output,LowercaseCrc64compatibility output.XxHash64-specific scanner expectations.Crc64known-output validation into a[Theory]with multiple input/output vectors.Crc64outputs.Related plumbing updates
TrimmableTypeMapGeneratorand scanner hashing logic.mainconflict changes so trimmable typemap generation keeps both package naming policy support andMaxArrayRankplumbing.