Add EmulatorRunner for emulator CLI operations#284
Conversation
- AndroidToolRunner: Base class for running Android SDK CLI tools - AndroidEnvironmentHelper: Environment variable setup, ABI mapping - ToolRunnerResult: Generic result model for tool execution This provides the foundation for specific tool runners. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- EmulatorRunner: Start and manage Android emulators API: - StartEmulator(): Start an emulator for an AVD (returns Process) - StartEmulatorAsync(): Start emulator and wait for boot - WaitForBootAsync(): Wait for emulator to complete boot - ListRunningEmulatorsAsync(): List running emulator serials Addresses #278 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a new EmulatorRunner to Xamarin.Android.Tools.AndroidSdk intended to wrap Android emulator CLI operations, alongside new shared infrastructure for running Android SDK command-line tools with environment setup and result modeling.
Changes:
- Added
EmulatorRunnerto start an AVD, stop an emulator, and list available AVD names. - Added
AndroidToolRunnerutility to run SDK tools sync/async (with timeouts) and to start long-running background processes. - Added
AndroidEnvironmentHelperandToolRunnerResult/ToolRunnerResult<T>to standardize tool environment and execution results.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/Xamarin.Android.Tools.AndroidSdk/Runners/EmulatorRunner.cs | Introduces emulator wrapper methods (start/stop/list AVDs) built on the tool runner infrastructure. |
| src/Xamarin.Android.Tools.AndroidSdk/Runners/AndroidToolRunner.cs | Adds process execution helpers (sync/async + background) with timeout/output capture. |
| src/Xamarin.Android.Tools.AndroidSdk/Runners/AndroidEnvironmentHelper.cs | Adds env var setup and mapping helpers (ABI/API/tag display names). |
| src/Xamarin.Android.Tools.AndroidSdk/Models/ToolRunnerResult.cs | Adds a shared result model for tool execution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "google_apis_playstore" => "Google API's, Play Store", | ||
| "google_apis" => playStoreEnabled ? "Google API's, Play Store" : "Google API's", |
There was a problem hiding this comment.
The display strings use "Google API's" (possessive) where the intended plural is "Google APIs". This is user-visible output, so it should be corrected for grammar/branding consistency.
| "google_apis_playstore" => "Google API's, Play Store", | |
| "google_apis" => playStoreEnabled ? "Google API's, Play Store" : "Google API's", | |
| "google_apis_playstore" => "Google APIs, Play Store", | |
| "google_apis" => playStoreEnabled ? "Google APIs, Play Store" : "Google APIs", |
| public static string? MapAbiToArchitecture (string? abi) | ||
| { | ||
| return abi switch { | ||
| "arm64-v8a" => "arm64", | ||
| "armeabi-v7a" => "arm", | ||
| "x86_64" => "x64", | ||
| "x86" => "x86", | ||
| _ => abi | ||
| }; | ||
| } |
There was a problem hiding this comment.
AndroidEnvironmentHelper introduces several non-trivial mapping functions (ABI -> architecture, API level -> version, tag -> display name) but there are no unit tests validating the mappings. Given the repo already has a Xamarin.Android.Tools.AndroidSdk-Tests suite, adding targeted tests would help prevent regressions when mappings are updated.
| readonly AdbRunner adbRunner; | ||
|
|
||
| /// <summary> | ||
| /// Creates a new EmulatorRunner. | ||
| /// </summary> | ||
| /// <param name="getSdkPath">Function to get the Android SDK path.</param> | ||
| /// <param name="getJdkPath">Function to get the JDK path.</param> | ||
| public EmulatorRunner (Func<string?> getSdkPath, Func<string?> getJdkPath) | ||
| { | ||
| this.getSdkPath = getSdkPath ?? throw new ArgumentNullException (nameof (getSdkPath)); | ||
| this.getJdkPath = getJdkPath ?? throw new ArgumentNullException (nameof (getJdkPath)); | ||
| this.adbRunner = new AdbRunner (getSdkPath); | ||
| } |
There was a problem hiding this comment.
EmulatorRunner depends on AdbRunner, but there is no AdbRunner type in the repository (searching the repo for class AdbRunner returns no matches). This will not compile unless the missing runner is added in this PR or the dependency is included/updated.
| /// <summary> | ||
| /// Starts an AVD. | ||
| /// </summary> | ||
| /// <param name="avdName">Name of the AVD to start.</param> | ||
| /// <param name="coldBoot">Whether to perform a cold boot (no snapshot).</param> | ||
| /// <param name="waitForBoot">Whether to wait for the device to fully boot.</param> | ||
| /// <param name="additionalArgs">Additional emulator arguments.</param> | ||
| /// <param name="cancellationToken">Cancellation token.</param> | ||
| /// <returns>Result with the emulator process.</returns> | ||
| public async Task<ToolRunnerResult<Process?>> StartAvdAsync ( | ||
| string avdName, | ||
| bool coldBoot = false, | ||
| bool waitForBoot = false, | ||
| string? additionalArgs = null, | ||
| CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
The implemented public API (StartAvdAsync, StopEmulatorAsync, ListAvdNamesAsync) does not match the PR description / Issue #278 proposed surface (e.g., StartEmulator, StartEmulatorAsync, WaitForBootAsync, ListRunningEmulatorsAsync). Either update the implementation to match the described API or update the PR description/issue expectations so consumers know what to call.
| if (waitForBoot) { | ||
| var waitResult = await adbRunner.WaitForDeviceAsync ( | ||
| null, // Wait for any device | ||
| TimeSpan.FromMinutes (2), | ||
| cancellationToken | ||
| ).ConfigureAwait (false); |
There was a problem hiding this comment.
waitForBoot currently calls adbRunner.WaitForDeviceAsync(null, ...) ("wait for any device"). If there is already a physical device or another emulator connected, this can return success immediately without the started AVD booting. The runner needs to wait for the specific emulator instance it started (e.g., determine the new emulator serial and wait for that).
| public static ToolRunnerResult Run ( | ||
| string fileName, | ||
| string arguments = "", | ||
| string? workingDirectory = null, | ||
| Dictionary<string, string>? environmentVariables = null, | ||
| TimeSpan? timeout = null, | ||
| CancellationToken cancellationToken = default) | ||
| { |
There was a problem hiding this comment.
AndroidToolRunner.Run(...) accepts a CancellationToken but never observes it. Either remove the parameter or honor it (e.g., periodically check cancellationToken.IsCancellationRequested and kill the process / return a cancelled result) to avoid misleading API behavior.
| var completed = process.WaitForExit ((int) effectiveTimeout.TotalMilliseconds); | ||
|
|
||
| if (!completed) { | ||
| try { KillProcess (process); } catch { } | ||
| stopwatch.Stop (); | ||
| return new ToolRunnerResult { | ||
| ExitCode = -1, | ||
| StandardOutput = stdoutBuilder.ToString (), | ||
| StandardError = stderrBuilder.ToString (), | ||
| Duration = stopwatch.Elapsed, | ||
| TimedOut = true, | ||
| ErrorMessage = $"Process timed out after {effectiveTimeout.TotalSeconds}s" |
There was a problem hiding this comment.
process.WaitForExit((int) effectiveTimeout.TotalMilliseconds) will throw if effectiveTimeout.TotalMilliseconds is > int.MaxValue or < 0. Consider clamping to int.MaxValue (and/or validating timeout) to prevent unexpected exceptions for larger timeouts.
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true, |
There was a problem hiding this comment.
StartBackground sets RedirectStandardOutput/RedirectStandardError = true but does not start any readers. This can cause the emulator process to block once stdout/stderr buffers fill up. For long-running background processes, either do not redirect these streams by default or immediately begin async reads to drain them.
| RedirectStandardOutput = true, | |
| RedirectStandardError = true, | |
| RedirectStandardOutput = false, | |
| RedirectStandardError = false, |
|
I'd like to get the System.Diagnostics.Process code unified like mentioned here: |
Summary
Adds
EmulatorRunnerclass to wrapemulatorCLI operations for starting and managing Android emulators.Addresses #278
New API
Usage
Dependencies
Requires:
adb shell getpropCo-authored-by: Copilot 223556219+Copilot@users.noreply.github.com