Conversation
10b4527 to
1a94a8d
Compare
tests/Xamarin.Android.Tools.AndroidSdk-Tests/SdkManagerTests.cs
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
930f96d to
8bc3209
Compare
5565656 to
bdeeb7f
Compare
src/Xamarin.Android.Tools.AndroidSdk/EnvironmentVariableNames.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Tools.AndroidSdk/EnvironmentVariableNames.cs
Outdated
Show resolved
Hide resolved
Split the 1200-line SdkManager.cs into focused partial class files: - SdkManager.cs: Core class, properties, constructor, dispose (~95 lines) - SdkManager.Manifest.cs: Manifest download and parsing (~141 lines) - SdkManager.Bootstrap.cs: SDK bootstrap/download/extract (~188 lines) - SdkManager.Packages.cs: Package list/install/uninstall/update (~224 lines) - SdkManager.Licenses.cs: License accept/parse/check (~220 lines) - SdkManager.Process.cs: Process execution, elevation, download (~310 lines) - SdkManager.FileUtils.cs: File permissions, copy, elevation check (~108 lines) No behavioral changes - pure structural refactor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move IsElevated, Chmod, CopyDirectoryRecursive, and SetExecutablePermissions to the shared FileUtil class for reuse across JdkInstaller and SdkManager. Delete SdkManager.FileUtils.cs partial and update callers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add ThrowIfDisposed() calls to all public async methods in SdkManager - Add explicit AndroidSdkPath null check in AcceptLicensesAsync(licenseIds) - Fix null-coalescing for Environment.GetFolderPath in RequiresElevation - Wrap cancellation registration dispose in try-catch for safety Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace empty catch blocks with proper exception logging - Add SdkManagerTimeout property (default 30min) replacing hardcoded 10min - Log exception details in RequiresElevation write test - Log license check non-zero exits at Verbose level Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
39bfbd5 to
e3d82d8
Compare
- Prefer internal over public for new APIs - Always use ProcessUtils instead of System.Diagnostics.Process - Use ArgumentList.Add() instead of string interpolation for args - Use FileUtil for file operations - Concise XML docs, no restating method names - netstandard2.0 workarounds for cancellation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add TryDeleteFiles batch helper to FileUtil - Use FileUtil.TryDeleteFiles/TryDeleteDirectory in SdkManager cleanup - Replace direct Process.Start in SetExecutablePermissions with ProcessUtils - Add XML doc remarks explaining why elevated flow uses Process.Start directly - Simplify RunSdkManagerElevatedAsync temp file management - Use single GUID for all temp files in elevated flow Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Check exitCode from sdkmanager --licenses - Only treat non-zero as non-fatal when output matches known 'already accepted' cases - Throw InvalidOperationException for genuine failures (missing Java, permissions, etc.) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… ManifestSource property Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace duplicated path-writability check with existing FileUtil helper. Direct Process.Start in elevated path is intentional (UAC requires UseShellExecute). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add environment variables and UseShellExecute support to ProcessUtils.StartProcess - Replace SdkManager.VerifyChecksum with DownloadUtils.VerifyChecksum - Replace SdkManager.DownloadFileAsync with DownloadUtils.DownloadFileAsync - Refactor RunSdkManagerElevatedAsync to use ProcessUtils - Remove ~60 lines of duplicated process/download code Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e AndroidEnvironmentHelper, reorder methods - Replace raw ProcessStartInfo with ProcessUtils.CreateProcessStartInfo in SdkManager.Process and Licenses - Merge two VerifyChecksum overloads into single method with default parameter - Restore AndroidEnvironmentHelper (consumed by maui DevTools CLI) - Move public AreLicensesAccepted before internal/private methods in Licenses Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ckupPath, fix XML docs - Change RunSdkManagerAsync to accept string[] instead of single string - Pass packages as individual args to avoid ArgumentList quoting issues - Escape arguments in elevated cmd.exe script to prevent injection - Fix httpClient field indentation in SdkManager.cs - Null-guard backupPath in Bootstrap.cs cleanup - Split duplicate XML doc blocks in ProcessUtils.cs - Split cmd.exe args properly: /c and script path as separate args Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…unner - Validate package arguments reject shell-special characters (&|><^) - Apply sanitization to env vars and arguments in elevated script Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Replace async ProcessUtils.StartProcess().Result with synchronous Process.Start/WaitForExit to avoid thread-pool starvation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace single 'echo y|' with a for-loop that generates 100 'y' responses, matching the non-elevated path's continuous feeding behavior. This ensures multi-license operations complete successfully. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dkManagerFailure helpers - Extract RequireSdkManagerPath() combining ThrowIfDisposed + FindSdkManagerPath - Extract ThrowOnSdkManagerFailure() to consolidate error logging + throw pattern - Simplify GetEnvironmentVariables() with collection initializer - Remove verbose XML doc comments on private methods - Net reduction: 39 lines across 4 files Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| var script = $""" | ||
| @echo off | ||
| {envBlock} | ||
| {licenseInput}"{sdkManagerPath}" {escapedArgs} > "{stdoutFile}" 2> "{stderrFile}" |
There was a problem hiding this comment.
In the generated elevated .cmd script, "{sdkManagerPath}" {escapedArgs} will typically point to sdkmanager.bat. Calling a .bat from within another .cmd file without call terminates the wrapper script early, so the echo %ERRORLEVEL% > "{exitCodeFile}" line never runs and failures can be reported as success. Prefix the sdkmanager invocation with call (or invoke it via cmd /c) so the wrapper continues and captures the real exit code.
| {licenseInput}"{sdkManagerPath}" {escapedArgs} > "{stdoutFile}" 2> "{stderrFile}" | |
| {licenseInput}call "{sdkManagerPath}" {escapedArgs} > "{stdoutFile}" 2> "{stderrFile}" |
| await Task.Run (() => { | ||
| if (!process.WaitForExit ((int) timeout.TotalMilliseconds)) { | ||
| KillProcess (process); | ||
| throw new TimeoutException ($"Process timed out after {timeout.TotalMinutes} minutes."); | ||
| } | ||
| }, cancellationToken).ConfigureAwait (false); |
There was a problem hiding this comment.
StartShellExecuteProcessAsync() passes the CancellationToken to Task.Run, but if cancellation occurs while waiting, the task can be canceled without killing the spawned process. This can leave an elevated cmd.exe (and child sdkmanager) running after the caller cancels. Consider registering the token to KillProcess(process) (similar to StartProcess) and ensure cancellation triggers process termination before returning/throwing.
| await Task.Run (() => { | |
| if (!process.WaitForExit ((int) timeout.TotalMilliseconds)) { | |
| KillProcess (process); | |
| throw new TimeoutException ($"Process timed out after {timeout.TotalMinutes} minutes."); | |
| } | |
| }, cancellationToken).ConfigureAwait (false); | |
| using (cancellationToken.Register (() => KillProcess (process))) { | |
| await Task.Run (() => { | |
| if (!process.WaitForExit ((int) timeout.TotalMilliseconds)) { | |
| KillProcess (process); | |
| throw new TimeoutException ($"Process timed out after {timeout.TotalMinutes} minutes."); | |
| } | |
| }).ConfigureAwait (false); | |
| } |
src/Xamarin.Android.Tools.AndroidSdk/AndroidEnvironmentHelper.cs
Outdated
Show resolved
Hide resolved
…nownVersions, use ProcessUtils in FileUtil - Fix double-enumeration in InstallAsync/UninstallAsync by materializing once - Derive ApiLevelToVersionMap from AndroidVersions.KnownVersions instead of hardcoding - Use ProcessUtils.StartProcess in SetExecutablePermissions instead of raw Process - Reword copilot-instructions to clarify ProcessUtils wraps ArgumentList Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Rename to SetExecutablePermissionsAsync with CancellationToken support - Remove .GetAwaiter().GetResult() blocking call - Properly await ProcessUtils.StartProcess in bootstrap flow Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The sdkmanager shell script uses save()/eval which concatenates individually-quoted arguments into a single string. On macOS/Linux, pass arguments as a single Arguments string instead of ArgumentList to work around this shell script bug. Also fix double-enumeration in UninstallAsync by materializing the packages collection upfront. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Adds SDK bootstrap and
sdkmanagerwrapper toXamarin.Android.Tools.AndroidSdk, enabling programmatic Android SDK setup from scratch.Closes #271
Architecture
Two-phase approach:
cmdline-tools/<version>/sdkmanagerCLI for all package operationsFile Structure
SdkManageris split into focused partial classes for maintainability:SdkManager.cs- Core class, properties, constructor, dispose (~84 lines)SdkManager.Manifest.cs- Manifest download and XML parsing (~141 lines)SdkManager.Bootstrap.cs- SDK bootstrap/download/extract flow (~175 lines)SdkManager.Packages.cs- Package list/install/uninstall/update + output parsing (~228 lines)SdkManager.Licenses.cs- License accept/parse/check operations (~246 lines)SdkManager.Process.cs- Process execution, elevation, download, checksum, env config (~307 lines)Supporting files:
EnvironmentVariableNames.cs- Centralized constants forANDROID_HOME,JAVA_HOME, etc.FileUtil.cs- Extended with chmod, recursive copy, executable permissions, elevation checkModels/Sdk/*.cs- Data models:SdkPackage,SdkLicense,SdkManifestComponent,SdkBootstrapProgress.github/copilot-instructions.md- Updated coding conventionsSdkManagerTests.cs- 534 lines of testsKey Features
~/.androidso sdkmanager writes install properties in user-writable locationDirectory.Movefalls back to recursive copyFileUtil.cs- no duplication across domain classesTests
89 tests pass (4 skipped). Covers manifest parsing, list output parsing, license parsing, error cases.
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com