Repository navigation
[Xamarin.Android.Build.Tasks] Preserve deterministic AAR extraction - #13506
simonrozsival wants to merge 6 commits into
Conversation
…n Unzip _ExtractAar now stages the .jar and annotations.zip entries with <Unzip/>, then uses the retained CopyIfChanged task to rewrite only files whose content changed, and deletes files that are no longer in any archive. Incremental behavior matches the removed task. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The final copy step omits descendant-link validation, allowing archive files to be written through symlinks or junctions outside the intended extraction directories.
5 open issues
What changed in this PR
Replaces the custom AAR extraction task with MSBuild’s built-in Unzip, reducing custom task maintenance in the Android binding build pipeline.
Changes:
- Adds staged extraction for bound and reference JARs and annotation archives.
- Uses timestamp refresh, content-aware copying, and stale-file deletion to preserve incremental behavior.
- Removes
ExtractJarsFromAar, its task registration, and its dedicated tests.
| File | Description |
|---|---|
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/ExtractJarsFromAarTests.cs |
Removes the custom extractor’s tests. |
src/Xamarin.Android.Build.Tasks/Tasks/ExtractJarsFromAar.cs |
Removes the custom extraction implementation. |
src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.AndroidLibraries.targets |
Replaces the task invocation with staged extraction, copying, and cleanup. |
Review effort: Balanced
Address review feedback on the Unzip-based _ExtractAar: * Stage with Unzip SkipUnchangedFiles="false" so later archives win basename collisions regardless of timestamp/size. * Add SyncExtractedAarFiles (multithreadable) to sync staged files into the final directories: refuse to write through symlinks/junctions, normalize backslash entry paths, compare stale files with platform-aware case sensitivity, and only copy changed content. * Add unit tests and target-level regression tests for fixed-timestamp same-size updates, unchanged timestamps, basename collisions, and stale-output cleanup after removing the last .aar. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Extract AAR contents into a staging directory that is removed before, after, and on failure. On Unix, repair entries with '\' separators in MSBuild (rejecting escaping or too deeply nested entries with XA4332), refuse to write through symbolic links or junctions (XA4333), delete stale outputs using platform-aware case sensitivity, and copy staged files with CopyIfChanged. 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 content-based synchronization correctly avoids trusting ZIP timestamps, and the new tests cover equal-size content changes, unchanged timestamps, stale-output cleanup, ordinary basename collisions, and linked destinations. The new task also uses the multithreadable base and engine-supplied path environment.
CI: All 40 GitHub check runs report success, including the aggregate dotnet-android check for Azure build 1677346. Direct Azure build/timeline verification could not complete because the Azure CLI returned initialization/authentication errors; no independent Azure verdict is claimed.
Reviewed all six changed files, their surrounding source and consumers, and the upstream Unzip implementation. No local build or tests were run for this read-only review.
Generated by Android PR Reviewer for #13506 · copilot · gpt61 · 41.7 AIC · ⌖ 7.75 AIC · ⊞ 19.5K
Comment /review to run again
Extract each library into its own staging directory, repair '\' separators per archive, then merge staging directories in archive order so later archives win for both JARs and annotations regardless of separator spelling. Within one archive, a '/' entry wins over a '\' entry naming the same file; multiple '\' entries that normalize to the same path fail with XA4332 because entry order is not observable from MSBuild. Escape and depth guards are preserved. Add two-archive tests for both separator orders and output types, and update XA4332 documentation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Restore the extraction task to resolve normalized entry collisions in archive order and reject output links before traversing them. Retain incremental content comparison and stale-file cleanup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Order extraction explicitly by archive, separator priority, and entry index so case aliases cannot reverse collision precedence. Stop registering dynamic task outputs as FileWrites, which causes IncrementalClean to delete extraction outputs when the task skips. Add target regressions for both case-alias entry orders, consecutive no-change full builds, and explicit clean. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
✅ LGTM — code review
Reviewed all eight changed files in full, the extraction helper and target consumers, and the changes against the PR's stated behavior. No merge-blocking correctness issues found. Findings: 0 errors, 0 warnings, 1 non-blocking suggestion, posted inline for project-directory isolation coverage of the multithreadable task.
The ZIP-aware selection preserves explicit archive precedence rather than relying on directory enumeration. Validating both library categories before deleting stale outputs is a useful safeguard, and the regression coverage exercises content-based updates, unchanged timestamps, separator/case collisions, depth rejection, and no-change builds. The new diagnostics have matching resources and documentation.
CI: All 40 head-commit GitHub checks report success, including the aggregate dotnet-android check for Azure build 1679332. The direct Azure build query was blocked by the CLI's authentication requirement, so I could not independently inspect its build result/timeline. No local builds or tests were run for this review.
Generated by Android PR Reviewer for #13506 · copilot · gpt61 · 57.3 AIC · ⌖ 7.9 AIC · ⊞ 19.5K
Comment /review to run again
| /// </summary> | ||
| public class ExtractJarsFromAar : AndroidTask | ||
| /// <summary>Extracts JARs and annotations from Android library archives for bindings.</summary> | ||
| [MSBuildMultiThreadableTask] |
There was a problem hiding this comment.
🤖 💡 Testing — Consider adding a focused project-directory isolation test for this multithreadable migration. The existing ExtractJarsFromAarTests use absolute paths and the fallback environment, so they would still pass if a future extraction or cleanup path accidentally used the process working directory. Instantiate two tasks with distinct TaskEnvironment.CreateWithProjectDirectoryAndEnvironment (...) contexts, relative library/output paths, and same-named AARs containing different bytes; run them concurrently and verify extraction and stale-file cleanup stay within each project's outputs. This would complement the target-level ordering and incremental tests without requiring another full SDK build per case.
{Rule: Test project-relative paths and multithreaded task isolation}
| public class ExtractJarsFromAar : MultithreadedAndroidTask | ||
| { | ||
| const int MaxEntryDepth = 8; | ||
| static readonly StringComparer PathComparer = Path.DirectorySeparatorChar == '\\' ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal; |
There was a problem hiding this comment.
I don't think this is right -- macOS can have both case sensitive and insensitive drives.
Could we just do a solution that works for both and not detect this?
|
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: #11212
Context: #13505
Context: #13506 (comment)
_ExtractAarextracts JARs and annotations used by binding generation. ReplacingExtractJarsFromAarwith MSBuild'sUnziploses archive-entry ordering when differently spelled paths normalize to the same destination. On Unix,libs/helper.jarandlibs\helper.jarinitially extract to separate files, so subsequent directory enumeration cannot reliably determine which entry was last. Recursive enumeration can also follow output directory links.Retain the ZIP-aware
ExtractJarsFromAartask and make it multithreadable. It processes archives in order, so a later archive wins for JARs and annotations. Within an archive,/entries win over colliding\entries; otherwise the later entry wins. Ordered writes preserve these rules on case-insensitive filesystems without merging distinct paths on case-sensitive filesystems.The task rejects escaping or excessively deep entry paths with XA4332 and output symlinks or junctions with XA4333. It removes stale files and compares content before writing. The existing clean target owns the extraction directories; their files are not dynamically added to
FileWrites, which would letIncrementalCleandelete them when_ExtractAaris skipped.Testing
GenerateDocumentationFile=true NoWarn=1591 make leeroy— passed with generated-documentation warnings and no errors; built the local Debug SDK../dotnet-local.sh build src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj --no-restore -nologo -v:q -m -mt -nr:false -warnaserror:MSB4286,MSB4287,MSB4288— passed../dotnet-local.sh test bin/TestDebug/net11.0/Xamarin.Android.Build.Tests.dll --filter 'Name~ExtractAar|FullyQualifiedName~ExtractJarsFromAarTests' -- NUnit.NumberOfTestWorkers=2— passed (24/24)../dotnet-local.sh test bin/TestDebug/net11.0/Xamarin.Android.Build.Tests.dll --filter 'FullyQualifiedName~BindingBuildTest' -- NUnit.NumberOfTestWorkers=2— passed (87/87)../dotnet-local.sh test bin/TestDebug/net11.0/Xamarin.Android.Build.Tests.dll --filter 'Name~ExtractAarPreservesCaseAliasOrdering'— passed (4/4) withBUILD_STAGINGDIRECTORYon a case-sensitive APFS volume; the case-insensitive volume was also covered.Windows-specific junction behavior was not verified locally.