[flaky-ci] Atomically replace Android SDK packages - #12767
Open
simonrozsival wants to merge 1 commit into
Open
simonrozsival wants to merge 1 commit into
simonrozsival wants to merge 1 commit into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
added
the
flaky-ci
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved cleanup and staged-directory validation bugs can cause failed builds or remove recoverable backups.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
src/androidsdk/androidsdk.targets — Do not delete rollback backups after extraction failures View comment |
 |
src/androidsdk/androidsdk.targets — Validate the staged source before normalization View comment |
What changed in this PR
Updates Android SDK/NDK extraction to stage validated packages before atomic replacement, with retries, rollback, and cleanup support.
Changes:
- Adds directory replacement, retry, rollback, and backup cleanup tasks.
- Updates extraction targets for staging, validation, stamps, and incrementality.
- Adds task- and target-level test coverage.
| File | Description |
|---|---|
tests/Microsoft.Android.Build.BaseTasks-Tests/Resources/AndroidSdkTargetsTest.proj |
Test project configuration |
tests/Microsoft.Android.Build.BaseTasks-Tests/ReplaceDirectoryTests.cs |
Replacement and rollback tests |
tests/Microsoft.Android.Build.BaseTasks-Tests/Microsoft.Android.Build.BaseTasks-Tests.csproj |
Bootstrap task project reference |
tests/Microsoft.Android.Build.BaseTasks-Tests/AndroidSdkTargetsTests.cs |
Extraction and incrementality tests |
src/androidsdk/androidsdk.targets |
Staged extraction and cleanup orchestration |
build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/RetryingDirectoryTask.cs |
Shared retry and delete logic |
build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/ReplaceDirectory.cs |
Atomic replacement and rollback support |
build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/RemoveDirectoryBackups.cs |
Stale backup cleanup |
| </Target> | ||
|
|
||
| <Target Name="_CleanAndroidSdkPackageBackups" | ||
| AfterTargets="_ExtractAndroidSdkPackages" |
Comment on lines
+364
to
367
| <PropertyGroup> | ||
| <_InstallSourceDir Condition=" '$(HostOS)' == 'Windows' Or '$(_StripComponents)' == '0' ">$(_StagingDir)</_InstallSourceDir> | ||
| <_InstallSourceDir Condition=" '$(HostOS)' != 'Windows' And '$(_StripComponents)' == '1' ">@(_StagedTopDir)</_InstallSourceDir> | ||
| </PropertyGroup> |
Comment on lines
+51
to
+52
| <UsingTask AssemblyFile="$(BootstrapTasksAssembly)" TaskName="Xamarin.Android.Tools.BootstrapTasks.ReplaceDirectory" TaskFactory="TaskHostFactory" Runtime="NET" /> | ||
| <UsingTask AssemblyFile="$(BootstrapTasksAssembly)" TaskName="Xamarin.Android.Tools.BootstrapTasks.RemoveDirectoryBackups" TaskFactory="TaskHostFactory" Runtime="NET" /> |
Member
There was a problem hiding this comment.
Is there a way we could implement this without using two new MSBuild tasks? Can the built-in MSBuild ones work for this?
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.
Summary
source.propertiesis presentCleanRoot cause
Azure build 1592587, job
MSBuild Emulator Tests macOS > Tests > MSBuild+Emulator 5, log 1032 reached_ExtractAndroidSdkPackagesfor the image-providedplatforms/android-36.1. The hosted-image directory did not contain this repository's.extracted-<archive>-<hash>sentinel, so re-extracting the pinned archive was intentional. Target batching is per package and the package destinations are distinct; there is no parallel batch writing this destination.The failure came from recursively deleting the live destination before the replacement archive was staged.
RemoveDirpartially removed the existing tree, thenDirectory.Deletereporteddata/reswas still non-empty. The pipeline-level retries immediately retried the already-partially-deleted directory and failed again.The new flow stages and validates the archive first, renames the old directory to a sibling backup, and atomically moves the staged directory into place. A transient or concurrently changing old tree can no longer prevent the pinned package from being committed. Genuine unzip, validation, or commit failures still fail and restore the previous destination when possible.
References #12704.
Validation
dotnet test tests/Microsoft.Android.Build.BaseTasks-Tests/Microsoft.Android.Build.BaseTasks-Tests.csproj -v minimal(132 passed, 4 skipped)dotnet build build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks.csproj -v minimalsrc/androidsdk/androidsdk.csprojgit diff --check