Add EmulatorRunner for emulator CLI operations#284
Conversation
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.
src/Xamarin.Android.Tools.AndroidSdk/Runners/AndroidEnvironmentHelper.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Tools.AndroidSdk/Runners/AndroidEnvironmentHelper.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Tools.AndroidSdk/Runners/AndroidToolRunner.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Tools.AndroidSdk/Runners/AndroidToolRunner.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Tools.AndroidSdk/Runners/AndroidToolRunner.cs
Outdated
Show resolved
Hide resolved
|
I'd like to get the System.Diagnostics.Process code unified like mentioned here: |
Addresses PR #284 feedback to use existing ProcessUtils instead of the removed AndroidToolRunner. Simplifies API: - Methods now throw InvalidOperationException on failure - Uses ProcessUtils.RunToolAsync() and StartToolBackground() - Removed complex ToolRunnerResult wrapper types Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
f1aa44f to
826d4aa
Compare
Addresses PR #283/#284 feedback to use existing ProcessUtils. Simplifies API by throwing exceptions on failure instead of returning result types with error states. Changes: - AdbRunner: Simplified using ProcessUtils.RunToolAsync() - EmulatorRunner: Uses ProcessUtils.StartToolBackground() - Removed duplicate AndroidDeviceInfo from Models directory Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
39617c8 to
5268300
Compare
1b10889 to
ee31e4b
Compare
ee31e4b to
3a788bb
Compare
Review feedback addressed — commit references
New files:
Modified:
Draft dotnet/android consumer PR to follow. |
jonathanpeppers
left a comment
There was a problem hiding this comment.
🤖 AI Review Summary
Found 7 issues: 1 correctness, 2 error handling, 1 API design, 1 code duplication, 1 code organization, 1 naming.
- Correctness:
StartAvdredirects stdout/stderr but never drains the pipes — OS buffer fill will deadlock the emulator process (EmulatorRunner.cs:74) - API design:
AdditionalArgsis a singlestring— will be treated as one argument byProcessUtils.ArgumentList, breaking multi-token args like-gpu swiftshader_indirect(EmulatorBootOptions.cs:14) - Error handling:
ListDevicesAsyncignores the exit code fromProcessUtils.StartProcesswhile sibling methods inAvdManagerRunnercheck it consistently (AdbRunner.cs:72) - Code duplication:
AvdManagerRunner.AvdManagerPathreimplements the cmdline-tools version scanning thatProcessUtils.FindCmdlineTool(added in this same PR) already provides (AvdManagerRunner.cs:33) - Error handling: Bare
catch { }swallows all exceptions without capturing them (AdbRunner.cs:107)
👍 Solid three-phase boot logic ported faithfully from dotnet/android. Good use of virtual on AdbRunner methods to enable clean test mocking. Thorough test coverage with 13+ unit tests covering parsing, edge cases, and the full boot flow. Nice extraction of AndroidEnvironmentHelper for shared env var setup.
This review was generated by the android-tools-reviewer skill based on review guidelines established by @jonathanpeppers.
src/Xamarin.Android.Tools.AndroidSdk/Models/EmulatorBootOptions.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Tools.AndroidSdk/Runners/AvdManagerRunner.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Tools.AndroidSdk/Runners/AvdManagerRunner.cs
Outdated
Show resolved
Hide resolved
This skill let's you say:
review this PR: #284
Some example code reviews:
* #283 (review)
* #284 (review)
This is built off a combination of previous code reviews, saved in
`docs/CODE_REVIEW_POSTMORTEM.md`, and the review rules in
`references/review-rules.md`.
27366b8 to
1b0488b
Compare
jonathanpeppers
left a comment
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict:
Found 3 issues: 1 warning, 2 suggestions.
⚠️ Exit code check: \ListAvdNamesAsync\ discards exit code, inconsistent with other runner methods (\EmulatorRunner.cs:103)- 💡 Error handling: \TryKillProcess\ empty catch could mask real failures (\EmulatorRunner.cs:249)
- 💡 API design: \RunShellCommandAsync\ passes command as a single string through device shell (\AdbRunner.cs:168)
Detailed Findings
⚠️ \EmulatorRunner.cs:103\ — Exit code not checked in \ListAvdNamesAsync\ProcessUtils.StartProcess\ returns an exit code here but it’s discarded. If \�mulator -list-avds\ fails (e.g., missing shared libraries, corrupt SDK install), this silently returns an empty list instead of throwing.
Every other runner method in this PR and in \AdbRunner\ checks exit codes — \ListDevicesAsync\ calls \ThrowIfFailed, \StopEmulatorAsync\ calls \ThrowIfFailed, \GetShellPropertyAsync\ checks \�xitCode == 0. This method should be consistent:
csharp var exitCode = await ProcessUtils.StartProcess (psi, stdout, null, cancellationToken, environmentVariables).ConfigureAwait (false); ProcessUtils.ThrowIfFailed (exitCode, "emulator -list-avds", stdout);
Rule: Check exit codes consistently (Postmortem #48)
💡 \EmulatorRunner.cs:249\ — Empty catch in \TryKillProcessThis empty \catch\ silently swallows all exceptions during process cleanup. While the intent is best-effort (the process may have already exited), an unexpected failure here (e.g., \AccessDeniedException\ on a system process) would be invisible.
Since \EmulatorRunner\ already has a \logger\ field, consider logging the exception:
csharp } catch (Exception ex) { // Best-effort: process may have already exited between check and cleanup logger?.Invoke (TraceLevel.Verbose, \$"Failed to stop emulator process: {ex.Message}"); }
Note: this would require making \TryKillProcess\ an instance method (to access \logger), or passing the logger in. If that’s too much churn, the comment is sufficient for now — but the bare \catch\ is a code smell per the review guidelines.
Rule: No empty catch blocks (Postmortem #11)
💡 \AdbRunner.cs:168\ — \RunShellCommandAsync\ single command string
\command\ is passed as a single argument to \�db shell, which means the device’s shell interprets it (shell expansion, pipes, semicolons all active). For the current internal usage ("pm path android") this is fine, but since this is a \public virtual\ method, a future caller could accidentally pass unsanitized input.
Consider documenting the shell-interpretation behavior in the XML doc, or offering a structured overload:
csharp // Overload for structured commands (no device-side shell interpretation): public virtual async Task<string?> RunShellCommandAsync ( string serial, string command, string[] args, CancellationToken ct = default)
When \�db shell\ receives multiple arguments, it \�xec()\s directly without shell interpretation. Low priority since all current callers are hardcoded strings.
Rule: Structured args, not string interpolation (Postmortem #49)
👍 What looks good
- Clean three-phase boot logic in \BootAvdAsync\ — check-if-online → check-if-running → launch-and-poll. Good separation of concerns.
- Correct \OperationCanceledException\ handling: \when (!cancellationToken.IsCancellationRequested)\ properly distinguishes timeout from caller cancellation.
- \EmulatorBootResult\ as a ecord\ type is exactly right for an immutable result.
- Proper pipe draining with \BeginOutputReadLine/\BeginErrorReadLine\ after redirect — prevents deadlock on long-running emulator.
- Good #if NET5_0_OR_GREATER\ guard for \process.Kill(entireProcessTree: true).
- Thorough test coverage with \MockAdbRunner\ — smart use of \�irtual\ methods for testability.
- Process cleanup on timeout/exception via \TryKillProcess\ prevents orphan emulator processes.
Review generated by android-tools-reviewer from review guidelines by @jonathanpeppers.
Adds EmulatorRunner with StartAvd, ListAvdNamesAsync, and BootAndWaitAsync. Adds virtual shell methods to AdbRunner for testability. Adds ConfigureEnvironment to AndroidEnvironmentHelper. 212/213 tests pass (1 pre-existing JDK failure). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
On Windows, the fake emulator was created as emulator.exe with batch script content (@echo off), which is not a valid PE binary. Process.Start() throws Win32Exception when trying to execute it, causing BootEmulator_AppearsAfterPolling to fail. Fix: - EmulatorPath now prefers .exe, falls back to .bat/.cmd on Windows (matching how older Android SDK tools ship batch wrappers) - Test fake creates emulator.bat instead of emulator.exe on Windows, with a proper idle command (ping -n 60) so the process stays alive during the polling test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…, NUnit constraints - EmulatorBootOptions.AdditionalArgs: string? → IEnumerable<string>? (prevents single-arg bug) - EmulatorBootResult: class → record with init properties, file-scoped namespace - EmulatorBootOptions: file-scoped namespace - StartAvd: drain redirected stdout/stderr with BeginOutputReadLine/BeginErrorReadLine - Cache Windows emulator extensions as static readonly array - Tests: replace null-forgiving '!' with Assert.That + Does.Contain - Tests: use ProcessUtils.CreateProcessStartInfo for chmod instead of raw ProcessStartInfo - Add PublicAPI.Unshipped.txt entries for all new public types Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Apply constructor environmentVariables to ProcessStartInfo in StartAvd - Move timeoutCts before Phase 2 so both Phase 2 (AVD already running) and Phase 3 (launch emulator) share a single boot timeout - Remove dead try-catch from WaitForFullBootAsync (callers handle timeout) - Use Process.Kill(entireProcessTree: true) on .NET 5+ to clean up child processes on Linux/macOS Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Improve public API naming for EmulatorRunner: - LaunchAvd: fire-and-forget process spawn, returns Process immediately - BootAvdAsync: full lifecycle — launch + poll until fully booted - Add comprehensive XML documentation explaining the behavioral difference between the two methods - Update PublicAPI surface files and all test references Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fix #1: Validate avdName in LaunchAvd (ArgumentException on null/empty) Fix #2: RunShellCommandAsync now returns full trimmed stdout (not just first line) Fix #3: Add 9 FirstNonEmptyLine parsing tests + 3 LaunchAvd validation tests Fix #5: Log stderr via logger on shell command failures (AdbRunner gets logger param) Fix #6: Remove TOCTOU HasExited check in TryKillProcess (rely on catch block) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Dispose the Process wrapper on the success path so file handles and native resources are released. The emulator OS process keeps running — only the .NET Process handle is freed. Failure/timeout paths already dispose via TryKillProcess. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- ListAvdNamesAsync: check exit code from emulator -list-avds - TryKillProcess: change bare catch to catch(Exception ex) with logger - TryKillProcess: make instance method to access logger field - RunShellCommandAsync: add XML doc warning about shell interpretation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
5956dc6 to
f19b014
Compare
…structured RunShellCommandAsync overload - Rename LaunchAvd to LaunchEmulator (fire-and-forget) - Rename BootAvdAsync to BootEmulatorAsync (full lifecycle) - Add RunShellCommandAsync(serial, command, args, ct) overload that passes args as separate tokens (exec, no shell interpretation) - Fix RS0026/RS0027: only the most-params overload has optional ct - Update all tests and PublicAPI files Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the 454-line BootAndroidEmulator implementation with a thin ~180-line wrapper that delegates to EmulatorRunner.BootEmulatorAsync() from Xamarin.Android.Tools.AndroidSdk. Key changes: - Remove all process management, polling, and boot detection logic - Delegate to EmulatorRunner.BootEmulatorAsync() for the full 3-phase boot: check online → check AVD running → launch + poll + wait - Map EmulatorBootResult errors to existing XA0143/XA0145 error codes - Virtual ExecuteBoot() method for clean test mocking - Update submodule to feature/emulator-runner (d8ee2d5) Tests updated from 9 to 10 (added ExtraArguments and UnknownError tests) using simplified mock pattern — MockBootAndroidEmulator overrides ExecuteBoot() to return canned EmulatorBootResult values. Depends on: dotnet/android-tools#284 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@jonathanpeppers Here's the dotnet/android consumer PR you requested: dotnet/android#10948 It replaces the 454-line The PR is in draft since it depends on this PR (#284) merging first — the submodule currently points to |
|
Note: the consumer PR was recreated after a branch rename — the correct link is now dotnet/android#10949 (the previous #10948 was auto-closed). |
Port additional test coverage from dotnet/android PR #10949: - AlreadyOnlinePhysicalDevice: physical device serial passthrough - AdditionalArgs_PassedToLaunchEmulator: verify extra args reach process - CancellationToken_AbortsBoot: cancellation during polling phase - ColdBoot_PassesNoSnapshotLoad: verify -no-snapshot-load flag - BootEmulatorAsync_NullAdbRunner_Throws: null guard validation - BootEmulatorAsync_EmptyDeviceName_Throws: empty string guard Total EmulatorRunner test count: 24 (18 existing + 6 new) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
src/Xamarin.Android.Tools.AndroidSdk/Models/EmulatorBootOptions.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Tools.AndroidSdk/Models/EmulatorBootOptions.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Tools.AndroidSdk/PublicAPI/net10.0/PublicAPI.Unshipped.txt
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Tools.AndroidSdk/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt
Outdated
Show resolved
Hide resolved
The 'adb emu avd name' console command returns empty output on emulator v36+ due to gRPC authentication requirements. This causes BootEmulatorAsync to never match the running emulator by AVD name, resulting in a perpetual polling loop and eventual timeout. Add a fallback to 'adb shell getprop ro.boot.qemu.avd_name' which reads the boot property set by the emulator kernel. This property is always available and doesn't require console authentication. The fix benefits all consumers of ListDevicesAsync/GetEmulatorAvdNameAsync, not just BootEmulatorAsync. Verified locally: BootEmulatorAsync now completes in ~3s (was timing out at 120s) on emulator v36.4.9 with API 36 image. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🔬 Definitive Proof:
|
| Method | Result |
|---|---|
adb -s emulator-5554 emu avd name |
EMPTY (exit code 0, no output) |
adb shell getprop ro.boot.qemu.avd_name |
MAUI_Emulator_API_36 ✅ |
echo "avd name" | nc localhost 5554 (raw telnet) |
MAUI_Emulator_API_36 ✅ |
| Console port 5554 | OPEN (nc -z succeeds) |
Analysis
- The console port IS accessible — raw telnet to 5554 returns the AVD name correctly
adb emureturns empty —adbuses a different protocol path than raw telnet, and something changed in emulator v36 that breaks it- The emulator warns:
The emulator now requires a signed jwt token for gRPC access!— while gRPC (port 8554) differs from telnet console (port 5554), this may affect howadbauthenticates to the console
Impact on dotnet/android
The original BootAndroidEmulator.GetRunningAvdName() on main uses the exact same command:
MonoAndroidHelper.RunProcess(adbPath, $"-s {serial} emu avd name", ...);This means FindRunningEmulatorForAvd would fail to match the AVD → WaitForEmulatorOnline would poll indefinitely → timeout after 120s. This is exactly the bug @jonathanpeppers reported.
Fix Validation
Our getprop ro.boot.qemu.avd_name fallback in AdbRunner.GetEmulatorAvdNameAsync:
- Completes in 13ms (vs infinite timeout)
BootEmulatorAsyncend-to-end: 2.8 seconds (vs 120s timeout)- All 259 existing tests pass
🔄 Correction: ADB v37 Regression (not emulator v36 issue)After deeper investigation, the root cause is more specific: The Real Issue: ADB v37.0.0 broke
|
📋 Research: ADB v37.0.0
|
| Evidence | Finding |
|---|---|
| Platform-tools 37.0.0 | ✅ Stable, public release on dl.google.com |
| GitHub Actions macOS 15 | ✅ Ships with pt 37.0.0 + emulator 36.4.9 |
| Google Issue Tracker | ✅ #251776353 — known open bug |
| dotnet/android CI | Uses pt 36.0.0 (pinned) — not yet affected |
getprop fallback |
Works on ALL ADB versions — forward-compatible fix |
Changes: - Convert EmulatorBootOptions from class to record with init properties - Change AdditionalArgs from IEnumerable to List for collection initializers - Remove REMOVED lines from PublicAPI.Unshipped.txt files - Remove local Log function, inline logger calls - Simplify while loop condition in WaitForFullBootAsync - Remove entireProcessTree from process termination Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace logger?.Invoke with logger.Invoke using a static NullLogger no-op delegate in EmulatorRunner, AdbRunner, and AvdManagerRunner. The constructor assigns logger ?? NullLogger so the field is never null. Static methods use logger ??= NullLogger at entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

EmulatorRunner: High-level emulator lifecycle management
Adds
EmulatorRunner— a managed wrapper over the Android SDKemulatorCLI binary, following the same pattern asAdbRunnerandAvdManagerRunner.API Surface
LaunchEmulator(avdName, options?)Processhandle. Caller owns the process lifetime. ValidatesavdNameis non-empty.BootEmulatorAsync(avdName, adb, options?, token?)adb devicesuntil boot completes or timeout. ReturnsEmulatorBootResultwith status and serial. Disposes Process handle on success (emulator keeps running).ListAvdNamesAsync(token?)emulator -list-avds. Checks exit code for failures.Key Design Decisions
LaunchEmulator(fire-and-forget) vsBootEmulatorAsync(full lifecycle) — clear verb distinction matching the emulator domainEmulatorRunnername (notAvdRunner) — follows convention of naming runners after their CLI binary (emulator→EmulatorRunner,adb→AdbRunner)LaunchEmulatorreturnsProcess(caller-owned);BootEmulatorAsyncdisposes handle on success (emulator keeps running as detached process), kills+disposes on failure/timeoutLaunchEmulatorcallsBeginOutputReadLine()/BeginErrorReadLine()afterStart()to prevent OS pipe buffer deadlockTryKillProcess: Instance method, uses typedcatch (Exception ex)with logger for diagnostics, usesKill(entireProcessTree: true)on .NET 5+AdbRunner Enhancements (in this PR)
Action? loggerparameter to constructorRunShellCommandAsync(serial, command, ct)— single-string shell command (RunShellCommandAsync(serial, command, args, ct)— NEW: structured overload that passes args as separate tokens, bypassing device shell interpretation viaexec(). Safer for dynamic input.GetShellPropertyAsyncreturns first non-empty line (forgetpropqueries)CancellationTokenGetEmulatorAvdNameAsyncnow falls back toadb shell getprop ro.boot.qemu.avd_namewhenadb emu avd namereturns empty (broken on emulator v36+ due to gRPC auth requirements)Models
EmulatorBootOptions— configurable timeout (default 120s), poll interval (default 2s), cold boot, extra args (IEnumerable?)EmulatorBootResult— immutablerecordwithinit-only properties:Status(enum),Serial,Message. Statuses:Success,AlreadyRunning,Timeout,ErrorBug Fix: AVD Name Detection on Emulator v36+
The
adb emu avd nameconsole command returns empty output on emulator v36+ due to gRPC authentication requirements. This causedBootEmulatorAsyncto never match the running emulator by AVD name, resulting in a perpetual polling loop and eventual timeout.Root cause:
GetEmulatorAvdNameAsyncrelied solely onadb -s <serial> emu avd name, which uses the emulator console protocol. Newer emulators require signed JWT tokens for gRPC/console access, causing the command to silently return empty.Fix: Added fallback to
adb shell getprop ro.boot.qemu.avd_name, which reads the boot property set by the emulator kernel. This property is always available via the standard adb shell interface without authentication.Verified:
BootEmulatorAsyncnow completes in ~3s (was timing out at 120s) on emulator v36.4.9 with API 36 image.Consumer PR
BootAndroidEmulatorMSBuild task (~454 lines) with a ~180-line wrapper delegating toEmulatorRunner.BootEmulatorAsync()Tests (24 EmulatorRunner + 9 AdbRunner = 33 total)
EmulatorRunner (24):
emulator -list-avdsoutput (empty, single, multiple, blank lines, Windows newlines) — 4 testsLaunchEmulatorargument validation (null, empty, whitespace AVD name) — 3 testsBootEmulatorAsynclifecycle: already online device, already running AVD, successful boot after polling, timeout, launch failure, cancellation token — 6 testsBootEmulatorAsyncvalidation: invalid timeout, invalid poll interval, null AdbRunner, empty device name — 4 testsBootAndroidEmulatorTests: physical device passthrough, AdditionalArgs forwarding, ColdBoot flag, cancellation abort — 4 testsAdbRunner (9):
FirstNonEmptyLineparsing (null, empty, whitespace, single value, multiline, mixed) — 9 testsReview Feedback Addressed
LaunchEmulatorvalidatesavdNameparameter (throwsArgumentException)LaunchEmulatordrains stdout/stderr pipes viaBeginOutputReadLine()/BeginErrorReadLine()RunShellCommandAsyncreturns full stdout (not just first line)RunShellCommandAsyncoverload (no shell interpretation)HasExitedguard fromTryKillProcessListAvdNamesAsyncchecks exit codeTryKillProcessuses typedcatch (Exception ex)with loggingRunShellCommandAsyncXML doc warns about shell interpretationEmulatorBootResultusesinit-only properties (immutable record)BootAndroidEmulatorTests