Repository navigation
[xabt] Replace CreateAar with MSBuild XML - #13511
simonrozsival wants to merge 10 commits into
Conversation
Stage the .aar layout under $(IntermediateOutputPath)aar/ with <Copy/>, WriteLinesToFile and ReadLinesFromFile, then create the archive with the built-in <ZipDirectory/> task. XA1041 and XA4301 remain localized through <AndroidError/>. ABI resolution for native libraries keeps using AndroidRidAbiHelper through a minimal <GetNativeLibraryAbis/> helper. Hashed libs/*.jar and .net/env/*.env entry names now use [MSBuild]::StableStringHash instead of CRC64; consumers do not depend on these names. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace the <GetNativeLibraryAbis/> helper with item metadata that mirrors AndroidRidAbiHelper.GetNativeLibraryAbi: %(Abi), then %(RuntimeIdentifier), then %(Link), falling back to the library path (ABI-named parent directory with optional interpreter- prefix, else RID-named parent or grandparent). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
… of a regex Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Normalize asset separators, locate the first whole-directory prefix with ordinal case-insensitive comparison, and compute the relative archive path in named metadata steps. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unsanitized asset paths can overwrite files outside staging, and the unguarded ProGuard reader breaks builds for libraries without ProGuard configuration.
4 open issues
What changed in this PR
Replaces the custom CreateAar task with MSBuild-based staging and ZIP creation for Android library archives, reducing custom build-task code while retaining the existing incremental inputs and output.
Changes:
- Moves archive layout, asset-path handling, ABI resolution, and localized diagnostics into MSBuild XML.
- Uses
StableStringHashfor JAR/environment filenames and built-in tasks for ProGuard concatenation. - Removes the obsolete task and asset helper, and drops the CRC64-specific test assertion while retaining entry-count checks.
Reported validation covers diff formatting and XML syntax only; SDK builds and tests were not run because of critically low disk space.
| File | Description |
|---|---|
src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs |
Removes the unused asset-relative-path helper. |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildWithLibraryTests.cs |
Removes the CRC64 filename assertion while retaining archive-content checks. |
src/Xamarin.Android.Build.Tasks/Tasks/CreateAar.cs |
Deletes the custom AAR creation task. |
src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.AndroidLibraries.targets |
Replaces task invocation with metadata processing, file staging, and ZipDirectory. |
Review effort: Balanced
* Reject .aar entries whose archive path escapes the staging directory (XA4332) instead of copying outside of it. * Skip proguard.txt handling when @(ProguardConfiguration) is empty. * De-duplicate native libraries case-sensitively. * Record staged files in @(FileWrites) and remove the staging directory and .aar in _CleanAarCache, since _CreateAar runs after IncrementalClean. * Qualify batched metadata (MSB4096) and split dependent metadata updates (MSB4186). * Report all XA4301/XA4332 diagnostics before failing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reuse ComputeHash with slash-normalized item identities to preserve the existing JAR and environment entry names. Cover all three hashed AAR input types with backslash paths. 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
Findings: 1 ❌ error, 0
The canonicalized staging-path checks, localized XA4332 documentation, guarded ProGuard reads, case-sensitive native-input deduplication, and explicit Clean handling address the earlier review findings. The current ComputeHash implementation still calls Files.HashString, so the description's StableStringHash compatibility note no longer matches the code.
CI: All 40 GitHub checks returned for the head commit report success, including the aggregate dotnet-android check for Azure build 1678798. Direct Azure build-result verification could not be completed because az devops invoke required authentication; no Azure changes or retries were attempted. The review was source-based; no local builds or tests were run.
Generated by Android PR Reviewer for #13511 · copilot · gpt61 · 53.4 AIC · ⌖ 4.63 AIC · ⊞ 20.2K
Comment /review to run again
Filesystem staging can silently merge case-distinct archive entries on Windows and macOS. MSBuild metadata batching can also combine case-only asset names before the staging paths are computed. Keep distinct asset sources in separate metadata batches and reject case-only input/destination aliases with localized XA4333 before copying or zipping. Check both raw and normalized paths, and remove an existing AAR on validation failure. Identically cased duplicate destinations still use the last input. Add a regression with separate source directories and case-distinct Link destinations, starting from an existing valid AAR. Include controls for distinct names and same-case duplicate destinations. Keep the native-library clean test without its former case-sensitive-host exception. Validation: rebuilt product/test assemblies and configured a local Android SDK. The focused DotNetLibraryAar suite passes all 10 cases using TEST_ANDROID_SDK_PATH pointed at a worktree-local platform-directory alias. Full make all encountered an unrelated Gradle distribution download failure in generator tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…13511) Context: #13511 (comment) The first XML prototype staged every `.aar` entry under `$(IntermediateOutputPath)aar/` and zipped that directory with `<ZipDirectory/>`. Using the filesystem as an intermediate archive caused most of the follow-up complexity: case-distinct entries such as `assets/Foo.txt` and `assets/foo.txt` collapsed into one file on case-insensitive hosts (XA4333), archive paths containing `..` escaped the staging directory (XA4332), and the staging directory needed its own `@(FileWrites)` and `_CleanAarCache` handling. Write the archive directly instead. The retained `<BuildArchive/>` task already adds each `@(FilesToAddToArchive)` item under its `%(ArchivePath)`, uses Ordinal entry names, compresses with `Optimal`, and replaces earlier entries with the same name, which keeps the original `CreateAar` "last item wins" behavior. The old `.aar` is deleted first so every build starts from an empty archive, as `CreateAar` did. This removes the staging directory, XA4332, XA4333 and their docs, and the extra cleanup. Only the generated `__res_name_case_map.txt` and `proguard.txt` contents are written to `obj/`; `_CleanAarCache` deletes them. Keep what the prototype review settled: plain-condition ABI resolution with localized XA4301, `<ComputeHash/>` for the existing CRC64 `libs/*.jar` and `.net/env/*.env` names, `<XmlPeek ProhibitDtd="true"/>` for the manifest `package` check, and the `IndexOf`-based asset prefix stripping. Restore `main`'s CRC64 jar-name assertion, and add a regression for case-distinct asset `Link`s from separate source directories. Validation (local Debug SDK, macOS): TEST_ANDROID_SDK_PATH=… ./dotnet-local.sh test bin/TestDebug/net11.0/Xamarin.Android.Build.Tests.dll \ --filter 'BuildWithLibraryTests.DotNetLibraryAar|BuildWithLibraryTests.DotNetBuildLibrary| BuildWithLibraryTests.AndroidLibraryProjectsZipWithOddPaths|BuildAssetsTest.FullPath| BuildTest2.ClassLibraryHasNoWarnings|XASdkTests.DotNetPack| IncrementalBuildTest.CreateAarRunsOnceWithGeneratePackageOnBuild| AndroidUpdateResourcesTest.CheckDontUpdateResourceIfNotNeeded' 23 passed; 10 skipped (NativeAOT variants and `DotNetPack` for the older TFM). A manual library with an ABI-less `libs/libbar.so` fails with XA4301 and leaves no `.aar`. A library with no Android content still skips `_CreateAar` for having no inputs, as on `main`. 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
- "169.254.169.254"See Network Configuration for more information.
|
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 1 error, 0 warnings, 0 suggestions. The inline finding identifies a last-item-wins regression when duplicate archive destinations straddle ZipArchiveEx's internal flush threshold. This can silently package the wrong asset even though the previous AAR is deleted first.
Reusing the retained archive writer avoids the staging filesystem's case-collision problem, and the added case-distinct Link regression is valuable. I reviewed all four changed files, the removed task, and the relevant archive-writing, hashing, asset, and ABI-resolution implementations against commit db727c626af46ed81251de22d79693009d46c5cc.
CI: GitHub reports all 40 checks successful, including the aggregate dotnet-android check for Azure build 1679360. Direct Azure build-status confirmation was unavailable because az devops invoke required login; no Azure-side failure analysis was performed.
Local validation: An isolated MSBuild mapping check could not start: dotnet msbuild failed during first-run initialization with DirectoryNotFoundException for /run/user/1001. The finding is based on the verified source paths; its threshold-crossing scenario was not executed locally. No repository files were changed.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
169.254.169.254
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "169.254.169.254"See Network Configuration for more information.
Generated by Android PR Reviewer for #13511 · copilot · gpt61 · 47.1 AIC · ⌖ 4.43 AIC · ⊞ 20.2K
Comment /review to run again
Context: #13511 (comment) `_CreateAar` relies on `<BuildArchive/>` keeping the last item when multiple `@(FilesToAddToArchive)` share an `%(ArchivePath)`. That held only while the earlier entry was still buffered: once `ZipArchiveEx` flushes (512 writes or 100 MiB), the earlier entry's length becomes readable and `AddFileIfChanged()` skips a later source with the same length and an older timestamp, keeping the first item instead. Track the archive paths written during this `BuildArchive` run and write later duplicates unconditionally. Java archive entries keep their existing first-wins behavior. Add `DuplicateFilesKeepLastItemAfterFlush`, which crosses the 512-write threshold; without the fix it fails with `Expected: "lastt" But was: "first"`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Looking back, this change does not seem beneficial enough to justify the added complexity. I'm closing this to reduce distractions. We can revisit it later. |
Context: #13511 (comment)
<CreateAar/>is a 167-line custom task that maps library inputs to.aarentry names and writes them into a ZIP archive. Mapping items to paths is what MSBuild item metadata is for, and writing a ZIP from%(ArchivePath)items is what the retained<BuildArchive/>task already does for.apkand.aabfiles. One fewer custom task means one fewer thing to host, load, and convert to multithreaded execution.Replace
<CreateAar/>within_CreateAarwith XML that computes%(ArchivePath)for each input, and pass the result to<BuildArchive/>. RemoveCreateAar.cs, its<UsingTask/>, andMonoAndroidHelper.GetRelativePathForAndroidAsset(), which no longer has callers.Why not
<ZipDirectory/>?The first iteration of this PR staged every entry under
$(IntermediateOutputPath)aar/and zipped the directory with<ZipDirectory/>. Using the filesystem as an intermediate archive was the source of nearly all review churn:assets/Foo.txtandassets/foo.txtcollapsed into a single file on case-insensitive filesystems (Windows, typical macOS volumes), silently producing an incomplete.aar.%(Link)values containing..could escape the staging directory.@(FileWrites)and_CleanAarCachehandling, because_CreateAarruns afterIncrementalClean.Working around these required two new error codes (XA4332, XA4333) and considerable XML. Writing the archive directly removes the problems instead:
<BuildArchive/>uses Ordinal entry names, compresses withCompressionLevel.Optimal(asZipArchive.CreateEntryFromFile()did), and replaces an earlier entry with the same name, preservingCreateAar's "last item wins" behavior. The previous.aaris deleted first, so each build starts from an empty archive, as before.Entry mapping
The
.aarlayout is unchanged:assets/…:%(Link)or the item path, removing everything up to and including the first$(MonoAndroidAssetsPrefix)directory (case-insensitive).res/<dir>/<file>, plus.net/__res_name_case_map.txt.libs/<crc64>.jarand.net/env/<crc64>.env:<ComputeHash/>over slash-normalized item paths preserves the existing CRC64 names.jni/<abi>/<file>: ABIs are resolved with plain conditions mirroringAndroidRidAbiHelper.GetNativeLibraryAbi():%(Abi), then%(RuntimeIdentifier), then the parent and grandparent directories of%(Link)and then of the item itself.proguard.txtandAndroidManifest.xml; the manifest is included only when<XmlPeek ProhibitDtd="true"/>finds a/manifest/@packageattribute.XA1041 and XA4301 remain localized via
<AndroidError/>. Every unresolved native library reports XA4301 and no.aaris written, matchingCreateAar, which deleted the archive when errors had been logged.The generated case map and concatenated
proguard.txtare written to$(IntermediateOutputPath)and deleted by_CleanAarCache.Compatibility
proguard.txtis concatenated with<ReadLinesFromFile/>and<WriteLinesToFile/>, which drop blank lines and leading/trailing whitespace. R8 does not treat whitespace as significant in configuration files.Tests
Add
DotNetLibraryAarPreservesCaseDistinctAssets: two sources in separate directories, linked asAssets/Foo.txtandAssets/foo.txt, must produce two entries with their respective contents.Validated against a locally built Debug SDK on macOS.
TEST_ANDROID_SDK_PATHpointed at a worktree-local SDK overlay aliasing the installedandroid-37.0-2platform asandroid-37.0:The skipped cases are the NativeAOT variants and
DotNetPackfor the older TFM.DotNetBuildLibraryasserts the CRC64libs/*.jarname,.net/env/*.env,jni/<abi>/entries, andproguard.txtcontents.Manual checks:
libs/libbar.so, which has no determinable ABI, fails with XA4301 and leaves no.aar._CreateAarbecause it has no inputs, as onmain.make alldid not complete locally:generator-Testsfailed withConnection refusedwhile downloading the Gradle distribution, which is unrelated to this change.