Repository navigation
[Microsoft.Android.Build.Tasks] Move InstallAndroidDependencies into MABT - #13520
simonrozsival wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A trailing separator in a valid JdkPath places staging inside the JDK directory, causing RemoveJdk() to delete both the existing JDK and newly extracted files before the move.
1 open issue
1 file not reviewed
src/Xamarin.Installer.AndroidSDK/Properties/Resources.Designer.cs: Generated file
What changed in this PR
This PR improves process and resource ownership in the legacy Android SDK installer, addressing the process-lifetime audit tracked in #12981.
Changes:
- Replaces license busy-polling with prompt-driven answers, cancellable input, concurrent output capture, and bounded cleanup.
- Moves Java extraction to native process capture with structured arguments, a deadline, diagnostics, and staging cleanup.
- Removes unused command helpers and adds standalone regression coverage.
| File | Description |
|---|---|
tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidSdkInstallerTests.cs |
Adds license lifecycle and Java extraction regression tests. |
src/Xamarin.Installer.Common/Xamarin.Installer.Common/CommonUtilities.cs |
Removes unused RunCommand overloads. |
src/Xamarin.Installer.AndroidSDK/Xamarin.Installer.AndroidSDK/JavaDependencyInstaller.cs |
Adds bounded extraction, layout checks, and staging cleanup. |
src/Xamarin.Installer.AndroidSDK/Xamarin.Installer.AndroidSDK/Common/AndroidLicensesStorage.cs |
Owns license process input, output, cancellation, and log lifetimes. |
src/Xamarin.Installer.AndroidSDK/Properties/Resources.resx |
Adds localized timeout and extraction error messages. |
src/Xamarin.Installer.AndroidSDK/Properties/Resources.Designer.cs |
Exposes the new resource strings. |
.github/skills/tests/references/test-catalog.md |
Documents the focused installer test command. |
Review effort: Balanced
d4f9f1c to
a235b37
Compare
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 1 ❌ error, 0 JdkPath, and committing an otherwise valid archive deletes both the installed JDK and its replacement. The inline comment describes the normalization and regression coverage needed.
The license changes improve ownership substantially: prompt-driven selective answers replace busy polling, stdout/stderr are consumed concurrently, sink access is synchronized, and process-exit/output-EOF handling has explicit deadlines. The new fixtures cover cancellation, blocked stdin, late diagnostics, sink failures, and repeated native-capture reader disposal. I reviewed all seven changed files, the affected callers, and the project target frameworks against the stacked PR diff.
CI: All 40 returned GitHub checks are completed/successful, including the aggregate dotnet-android check for build 1677892. Independent Azure build/timeline verification was blocked by Azure CLI initialization/authentication errors, so that verification remains incomplete.
Local validation: dotnet test tests/Xamarin.Android.Tools.AndroidSdk-Tests/Xamarin.Android.Tools.AndroidSdk-Tests.csproj -v minimal --filter FullyQualifiedName~AndroidSdkInstallerTests could not start: .NET CLI initialization failed creating the NuGet-Migrations mutex because its runtime directory was unavailable. No tests executed locally; the installed SDK also reports 10.0.403 while these projects target net11.0.
Generated by Android PR Reviewer for #13520 · copilot · gpt61 · 32.3 AIC · ⌖ 4.72 AIC · ⊞ 20.2K
Comment /review to run again
b4ba1ba to
7d5da70
Compare
…MABT Context: #12981 Context: https://devblogs.microsoft.com/dotnet/process-api-improvements-in-dotnet-11/ Context: https://learn.microsoft.com/visualstudio/msbuild/update-task-multithreaded After #13513, `InstallAndroidDependencies` installs SDK packages with Google's Android CLI, but it still lived in the netstandard2.0 `Xamarin.Installer.Build.Tasks.dll`. There it had to start the Android CLI, `/usr/bin/tar`, and `chmod` through `ProcessUtils`, and it could not run in multithreaded MSBuild (`-mt`). Move the task, `JavaDependencyInstaller`, and `OpenJdkSignatureVerifier` into `Microsoft.Android.Build.Tasks.dll`, which runs on .NET: * Register the task with the paired Full/Core `Runtime="NET"` `<UsingTask/>`. MABT links the existing localized messages and deploys `Xamarin.Installer.Common.props` and `.targets` to `tools`, so `Xamarin.Android.Common.targets` and the localization files are unchanged. Remove `Xamarin.Installer.Build.Tasks.csproj`. * Run the Android CLI with `Process.ReadAllLinesAsync()` and `WaitForExitAsync()`, keeping only the tail of its output as before, and kill its process tree when the task fails or is canceled. Extract JDK `.tar.gz` archives with `TarFile`, and mark the downloaded Android CLI executable with `File.SetUnixFileMode()`. * Derive from `MultithreadedAndroidTask` with `[MSBuildMultiThreadableTask]` and `ICancelableTask`. `RunTask()` runs the existing async workflow on the thread pool and forwards log messages to the thread that called `Execute()`, as `AsyncTask` did. Environment variables, the child process start info, and all paths come from `TaskEnvironment`; resolved paths are passed around as `AbsolutePath`. The default SDK and JDK locations come from the task's `LOCALAPPDATA` or `HOME`. Three analyzer findings are suppressed with justifications: `Process.Start()` (its `ProcessStartInfo` comes from `TaskEnvironment.GetProcessStartInfo()`), `Path.GetTempFileName()` (no `TaskEnvironment` alternative yet), and `Environment.GetFolderPath(ProgramFiles)` (a machine location). `OpenJdkSignatureTests` and the unit tests that create the task move to the net12.0 `Microsoft.Android.Build.Tasks.Tests`. The fake Android CLI test now also checks that a relative `AndroidSdkPath` resolves against the project directory and that messages are logged on the `Execute()` thread. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
7d5da70 to
c545363
Compare
…ependenciesTests Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Validation incomplete — no blocking code findings
Reviewed all 15 changed files, including the full moved implementations, project/packaging wiring, task registrations, and test coverage. Findings: 0 errors, 0 warnings, 2 non-blocking suggestions, posted inline for deterministic extraction and process-lifecycle coverage.
The migration keeps signature/hash verification intact, resolves project-relative installation paths through TaskEnvironment, uses structured process arguments, and preserves calling-thread logging. Linking the installer resources into MABT and retaining the shipped props/targets is consistent with the surrounding migration pattern.
CI: All 40 GitHub checks for head 07c58d96 report success, including the aggregate dotnet-android check and Windows/Linux/macOS lanes. I could not independently verify Azure build 1679429: the Azure CLI required authentication and the direct public REST read was denied. Consequently, this is not an unconditional CI-green/LGTM verdict.
Local validation: Attempted dotnet test src/Microsoft.Android.Build.Tasks/Tests/Microsoft.Android.Build.Tasks.Tests/Microsoft.Android.Build.Tasks.Tests.csproj --filter 'FullyQualifiedName~InstallAndroidDependenciesTests|FullyQualifiedName~OpenJdkSignatureTests' -v minimal; the .NET CLI failed during NuGet first-use initialization before building or executing tests. No local test pass is claimed. The worktree was left unchanged.
Generated by Android PR Reviewer for #13520 · copilot · gpt61 · 48.3 AIC · ⌖ 7.51 AIC · ⊞ 19.5K
Comment /review to run again
| throw new Exception ($"Failed to extract temp Java SDK archive {archive.DownloadedFilePath}:\n{extractResult}"); | ||
| using var file = File.OpenRead (downloadedFile); | ||
| using var gzip = new GZipStream (file, CompressionMode.Decompress); | ||
| await TarFile.ExtractToDirectoryAsync (gzip, unzipDestination, overwriteFiles: false, cancellationToken); |
There was a problem hiding this comment.
🤖 💡 Testing — Consider adding a small, offline .tar.gz fixture for this replacement of /usr/bin/tar. The moved signature tests do not exercise extraction, and InstallMicrosoftJdk is an explicit network test. A fixture containing an executable bin/java and a relative symlink could assert that installation preserves both, replaces an existing JDK, and removes staging; include a destination ending in a directory separator. This would give the new managed extraction path deterministic regression coverage on Unix without downloading a full JDK.
{Rule: Test behavior changes and filesystem edge cases}
| output.WriteLine (line.Content); | ||
| await process.WaitForExitAsync (cancellationToken); | ||
| } catch { | ||
| process.Kill (entireProcessTree: true); |
There was a problem hiding this comment.
🤖 💡 Testing — Consider extending the fake-CLI coverage to cancel an installation after the child has actually started. The current test covers argument forwarding and the logging thread, while the existing zero-timeout integration case can cancel before reaching this new process-reading/kill path. A deterministic test could have the helper signal readiness and then wait, call Cancel(), and assert that Execute() returns false with cancellation diagnostics and that the helper has exited. A separate nonzero-exit case emitting to both stdout and stderr would protect the retained XA5308 output-tail diagnostics.
{Rule: Exercise cancellation, process cleanup, and error paths}
There was a problem hiding this comment.
⚠️ Validation incomplete — no blocking code findings
Reviewed all 15 changed files, including the full moved implementations, project/packaging wiring, task registrations, and test coverage. Findings: 0 errors, 0 warnings, 2 non-blocking suggestions, posted inline for deterministic extraction and process-lifecycle coverage.
The migration keeps signature/hash verification intact, resolves project-relative installation paths through TaskEnvironment, uses structured process arguments, and preserves calling-thread logging. Linking the installer resources into MABT and retaining the shipped props/targets is consistent with the surrounding migration pattern.
CI: All 40 GitHub checks for head 07c58d96 report success, including the aggregate dotnet-android check and Windows/Linux/macOS lanes. I could not independently verify Azure build 1679429: the Azure CLI required authentication and the direct public REST read was denied. Consequently, this is not an unconditional CI-green/LGTM verdict.
Local validation: Attempted dotnet test src/Microsoft.Android.Build.Tasks/Tests/Microsoft.Android.Build.Tasks.Tests/Microsoft.Android.Build.Tasks.Tests.csproj --filter 'FullyQualifiedName~InstallAndroidDependenciesTests|FullyQualifiedName~OpenJdkSignatureTests' -v minimal; the .NET CLI failed during NuGet first-use initialization before building or executing tests. No local test pass is claimed. The worktree was left unchanged.
Generated by Android PR Reviewer for #13520 · copilot · gpt61 · 48.3 AIC · ⌖ 7.51 AIC · ⊞ 19.5K
Comment /review to run again
| throw new Exception ($"Failed to extract temp Java SDK archive {archive.DownloadedFilePath}:\n{extractResult}"); | ||
| using var file = File.OpenRead (downloadedFile); | ||
| using var gzip = new GZipStream (file, CompressionMode.Decompress); | ||
| await TarFile.ExtractToDirectoryAsync (gzip, unzipDestination, overwriteFiles: false, cancellationToken); |
There was a problem hiding this comment.
🤖 💡 Testing — Consider adding a small, offline .tar.gz fixture for this replacement of /usr/bin/tar. The moved signature tests do not exercise extraction, and InstallMicrosoftJdk is an explicit network test. A fixture containing an executable bin/java and a relative symlink could assert that installation preserves both, replaces an existing JDK, and removes staging; include a destination ending in a directory separator. This would give the new managed extraction path deterministic regression coverage on Unix without downloading a full JDK.
{Rule: Test behavior changes and filesystem edge cases}
| output.WriteLine (line.Content); | ||
| await process.WaitForExitAsync (cancellationToken); | ||
| } catch { | ||
| process.Kill (entireProcessTree: true); |
There was a problem hiding this comment.
🤖 💡 Testing — Consider extending the fake-CLI coverage to cancel an installation after the child has actually started. The current test covers argument forwarding and the logging thread, while the existing zero-timeout integration case can cancel before reaching this new process-reading/kill path. A deterministic test could have the helper signal readiness and then wait, call Cancel(), and assert that Execute() returns false with cancellation diagnostics and that the helper has exited. A separate nonzero-exit case emitting to both stdout and stderr would protect the retained XA5308 output-tail diagnostics.
{Rule: Exercise cancellation, process cleanup, and error paths}
Context: #12981
Context: https://devblogs.microsoft.com/dotnet/process-api-improvements-in-dotnet-11/
Context: https://learn.microsoft.com/visualstudio/msbuild/update-task-multithreaded
After #13513,
InstallAndroidDependenciesinstalls SDK packages with Google's Android CLI, but it still lived in the netstandard2.0Xamarin.Installer.Build.Tasks.dll. There it had to start the Android CLI,/usr/bin/tar, andchmodthroughProcessUtils, and it could not run in multithreaded MSBuild (-mt).Move the task,
JavaDependencyInstaller, andOpenJdkSignatureVerifierintoMicrosoft.Android.Build.Tasks.dll, which runs on .NET:Runtime="NET"<UsingTask/>. MABT links the existing localized messages and deploysXamarin.Installer.Common.propsand.targetstotools, soXamarin.Android.Common.targetsand the localization files are unchanged. RemoveXamarin.Installer.Build.Tasks.csproj./usr/bin/tar -xzffor JDK.tar.gzarchives is replaced byGZipStreamandTarFile.ExtractToDirectoryAsync()..ziparchives already usedZipFile.chmod 755for the downloaded Android CLI is replaced byFile.SetUnixFileMode(), so there is no child process or exit code to check.Process.ReadAllLinesAsync()andWaitForExitAsync()instead ofProcessUtils, keeps only the tail of its output as before, and has its process tree killed when the task fails or is canceled.Mono.Unixdependency. The installer'sMono.Unixusage went away with the libraries removed in [Xamarin.Installer.Build.Tasks] Use Android CLI to install SDK packages #13513.MultithreadedAndroidTaskwith[MSBuildMultiThreadableTask]andICancelableTask.RunTask()runs the existing async workflow on the thread pool and forwards log messages to the thread that calledExecute(), asAsyncTaskdid. Environment variables, the child process start info, and all paths come fromTaskEnvironment; resolved paths are passed around asAbsolutePath. The default SDK and JDK locations come from the task'sLOCALAPPDATAorHOME.Three analyzer findings are suppressed with justifications:
Process.Start()(itsProcessStartInfocomes fromTaskEnvironment.GetProcessStartInfo()),Path.GetTempFileName()(noTaskEnvironmentalternative yet), andEnvironment.GetFolderPath(ProgramFiles)(a machine location).OpenJdkSignatureTestsand the unit tests that create the task move to the net12.0Microsoft.Android.Build.Tasks.Tests. The fake Android CLI test now also checks that a relativeAndroidSdkPathresolves against the project directory and that messages are logged on theExecute()thread.Validation on macOS arm64 (head
07c58d96, on #135151d71269f):./dotnet-local.sh build src/Microsoft.Android.Build.Tasks/Tests/Microsoft.Android.Build.Tasks.Tests/Microsoft.Android.Build.Tasks.Tests.csproj: succeeded, including the task-authoring analyzers../dotnet-local.sh test src/Microsoft.Android.Build.Tasks/Tests/Microsoft.Android.Build.Tasks.Tests/Microsoft.Android.Build.Tasks.Tests.csproj --no-build --filter 'FullyQualifiedName~InstallAndroidDependenciesTests|FullyQualifiedName~OpenJdkSignatureTests': passed 57/57.make prepare && make leeroy: succeeded../dotnet-local.sh test bin/TestDebug/net11.0/Xamarin.Android.Build.Tests.dll --filter 'FullyQualifiedName~MultiThreadedMigrationTests|FullyQualifiedName~AndroidDependenciesTests': passed 26/26.dotnet new androidproject,dotnet-local.sh build -t:InstallAndroidDependencies -m -mt -nr:false -warnaserror:MSB4286,MSB4287,MSB4288 -p:AndroidSdkDirectory=$HOME/Library/Android/sdk -p:AcceptAndroidSDKLicenses=true: succeeded and installedcmdline-tools/21.0with the Android CLI.Windows and Linux were not run locally.