Add AdbRunner for adb CLI operations

[rmarinho]

Summary

Wraps adb CLI operations for device management. Addresses #279.

Parsing/formatting/merging logic ported from dotnet/android GetAvailableAndroidDevices MSBuild task, enabling code sharing via the external/xamarin-android-tools submodule. See draft PR: dotnet/android#10880

Public API

public class AdbRunner
{
    // Constructor — requires full path to adb executable
    public AdbRunner (string adbPath, IDictionary<string, string>? environmentVariables = null);

    // Instance methods (async, invoke adb process)
    public Task<IReadOnlyList<AdbDeviceInfo>> ListDevicesAsync (CancellationToken ct = default);
    public Task WaitForDeviceAsync (string? serial = null, TimeSpan? timeout = null, CancellationToken ct = default);
    public Task StopEmulatorAsync (string serial, CancellationToken ct = default);

    // Static helpers — public so dotnet/android can call without instantiating AdbRunner
    public static List<AdbDeviceInfo> ParseAdbDevicesOutput (IEnumerable<string> lines);
    public static AdbDeviceStatus MapAdbStateToStatus (string adbState);
    public static string BuildDeviceDescription (AdbDeviceInfo device, Action<TraceLevel, string>? logger = null);
    public static string FormatDisplayName (string avdName);
    public static List<AdbDeviceInfo> MergeDevicesAndEmulators (IReadOnlyList<AdbDeviceInfo> adbDevices, IReadOnlyList<string> availableEmulators, Action<TraceLevel, string>? logger = null);
}

Internal methods (not part of public API):

• GetEmulatorAvdNameAsync — queries AVD name via adb emu avd name with TCP console fallback
• ProcessUtils.ThrowIfFailed — shared exit code validation

Key Design Decisions

• Constructor takes string adbPath: Callers pass the resolved path; no lazy Func<string?> indirection. Optional environmentVariables dictionary for ANDROID_HOME/JAVA_HOME/PATH.
• Static parsing methods are public static so dotnet/android can call them without instantiating AdbRunner (e.g., GetAvailableAndroidDevices MSBuild task passes List<string> to ParseAdbDevicesOutput)
• IEnumerable<string> overload: dotnet/android passes List<string> directly from output lines
• Logger parameter: BuildDeviceDescription and MergeDevicesAndEmulators accept Action<TraceLevel, string>? — dotnet/android passes this.CreateTaskLogger() for MSBuild trace output
• Regex with explicit state list: Uses \s+ separator to match one or more whitespace characters (spaces or tabs). Matches explicit known states with IgnoreCase. Daemon startup lines (*) are pre-filtered.
• Exit code checking: ListDevicesAsync, WaitForDeviceAsync, and StopEmulatorAsync throw InvalidOperationException with stderr context on non-zero exit via ProcessUtils.ThrowIfFailed (internal)
• MapAdbStateToStatus as switch expression: Simple value mapping uses C# switch expression for conciseness
• Property patterns instead of null-forgiving: Uses is { Length: > 0 } patterns throughout for null checks on netstandard2.0 where string.IsNullOrEmpty() lacks [NotNullWhen(false)]
• FormatDisplayName: Lowercases before ToTitleCase to normalize mixed-case input (e.g., "PiXeL" → "Pixel")
• Environment variables via StartProcess: Runners pass env vars dictionary to ProcessUtils.StartProcess. AndroidEnvironmentHelper.GetEnvironmentVariables() builds the dict.

Tests

45 unit tests (AdbRunnerTests.cs):

• ParseAdbDevicesOutput: real-world data, empty output, single/multiple devices, mixed states, daemon messages, IP:port, Windows newlines, recovery/sideload, tab-separated output
• FormatDisplayName: underscores, title case, API capitalization, mixed case, special chars, empty
• MapAdbStateToStatus: all known states + unknown (recovery, sideload)
• MergeDevicesAndEmulators: no emulators, no running, mixed, case-insensitive dedup, sorting
• Constructor: valid path, null/empty throws, sets properties
• WaitForDeviceAsync: timeout validation (negative, zero)

4 integration tests (RunnerIntegrationTests.cs):

• Run only when TF_BUILD=True or CI=true (case-insensitive), skipped locally
• Require pre-installed JDK (JAVA_HOME) and Android SDK (ANDROID_HOME) on CI agent
• Assert.Ignore when ANDROID_HOME missing (no bootstrap/network dependency)
• Cover: constructor, ListDevicesAsync, WaitForDeviceAsync timeout, tool discovery

Review Feedback Addressed

Feedback	Commit	Details
Constructor: require string adbPath	b68bea9	Replace Func<string?> with string adbPath, remove lazy resolution
Port device listing from dotnet/android	669121a, c142198, ca1b3a2	ParseAdbDevicesOutput, BuildDeviceDescription, FormatDisplayName, MergeDevicesAndEmulators
ThrowIfFailed overload + delete string ParseAdbDevicesOutput	1c79b8b	StringWriter overload delegates to string version; single IEnumerable<string> parse method
Replace null-forgiving ! with property patterns	3cadb58	is { Length: > 0 } patterns in AdbRunner + AndroidEnvironmentHelper; MapAdbStateToStatus → switch expression
Remove section separator comments	e66b6d4	Removed // ── Section ── region-style comments
Tighten RequireCi() to check truthy value	e66b6d4	string.Equals("true", OrdinalIgnoreCase) instead of presence check
Remove bootstrap fallback in tests	e66b6d4	Assert.Ignore when ANDROID_HOME missing — no network-dependent SDK download
Fix regex comment accuracy	e66b6d4	Comment matches \s+ behavior (1+ whitespace, not "2+ spaces")
Log exceptions in bare catch	7801c18	GetEmulatorAvdNameAsync logs via Trace.WriteLine
Update copilot-instructions.md	3cadb58	Added rules: no null-forgiving !, prefer switch expressions

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[jonathanpeppers]

I'd like to get the System.Diagnostics.Process code unified like mentioned here:

• Add base Android tool runner infrastructure #281 (comment)

[rmarinho]

Implemented your suggested approach:

• Ported the device listing logic from dotnet/android's GetAvailableAndroidDevices MSBuild task into AdbRunner in feature/adb-runner branch
• AdbDeviceInfo now has all the same fields: Serial, Description, Type (enum), Status (enum), AvdName, Model, Product, Device, TransportId
• Ported ParseAdbDevicesOutput (same regex pattern), BuildDeviceDescription (same priority order), FormatDisplayName (title case + API capitalization), MapAdbStateToStatus, and MergeDevicesAndEmulators (dedup + sorting)
• Added GetEmulatorAvdNameAsync (async version of GetEmulatorAvdName)
• 33 unit tests ported from the dotnet/android test cases (parsing, display name formatting, status mapping, merging/dedup, path discovery)

Next steps per your plan:

1. ✅ feature/adb-runner has the ported logic (pushed)
2. ⬜ Open a draft PR in dotnet/android that updates the submodule + rewrites GetAvailableAndroidDevices.cs to consume the new shared API
3. ⬜ Review/merge android-tools first, then dotnet/android

[rmarinho]

The dotnet/android side is now ready as a draft PR: dotnet/android#10880

It delegates GetAvailableAndroidDevices parsing/formatting/merging to the shared AdbRunner methods from this PR, removing ~200 lines of duplicated code. All 33 existing tests are preserved and updated to use AdbRunner/AdbDeviceInfo directly (no more reflection).

Workflow:

1. Merge this PR first
2. Update the dotnet/android submodule pointer from feature/adb-runner to main
3. Take Use shared AdbRunner from android-tools for device listing android#10880 out of draft and merge

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.

[jonathanpeppers]

🤖 AI Review Summary

Found 1 issue: error handling.

• Error handling: Bare catch block in GetEmulatorAvdNameAsync silently swallows exceptions without logging (AdbRunner.cs:119)

👍 Excellent PR overall — thorough test coverage (44 unit + 9 integration tests), consistent use of ProcessUtils for all process creation, proper CancellationToken propagation throughout, correct OperationCanceledException rethrow before the bare catch, good use of EnvironmentVariableNames constants, and clean code organization (one type per file, file-scoped namespaces). The refactoring of SdkManager.GetEnvironmentVariables into the shared AndroidEnvironmentHelper is a nice DRY improvement.

---

This review was generated by the android-tools-reviewer skill based on review guidelines established by @jonathanpeppers.

[rmarinho]

Addressed the bare catch feedback in 9368682: GetEmulatorAvdNameAsync now catches Exception ex and logs via Trace.WriteLine instead of silently swallowing. Also updated PR description to accurately reflect the current API surface (including IEnumerable<string> overload, logger parameters, internal methods).

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

---

You can also share your feedback on Copilot code review. Take the survey.

[jonathanpeppers]

What is going on here? Why are we using a TcpClient?!?

[rmarinho]

The TcpClient fallback exists because adb -s emulator-XXXX emu avd name returns exit code 0 but empty output on macOS. We verified this with both:

android-tools AdbRunner (empty AvdName without fallback)
dotnet/android GetAvailableAndroidDevices MSBuild task on main (same bug — Description=sdk gphone64 arm64 with no AVD name)
Without the AVD name, MergeDevicesAndEmulators can't match running emulators with AVD definitions from emulator -list-avds, leading to duplicates in UI device pickers.

The TCP console approach connects to the emulator's console port (emulator-{port}) and sends avd name — this works reliably because the emulator console protocol is stable and documented.

An alternative is process scanning (lsof -i :port → PID → ps -o command= → parse -avd arg), which avoids potential auth token requirements. Happy to switch to that approach if preferred, or keep TCP as the simpler cross-platform option. Let me know which you prefer.

[jonathanpeppers]

The BootAndroidEmulator MSBuild task doesn't have this, so why do we need it here?

• https://github.com/dotnet/android/blob/main/src/Xamarin.Android.Build.Tasks/Tasks/BootAndroidEmulator.cs

[jonathanpeppers]

Again, I would avoid introducing TcpClient code if we don't need it. It doesn't seem like we should be opening network connections for this.

[rmarinho]

Feedback addressed in 8e4be7a

Threads 41-43: Simplify AdbRunner constructor

Replaced Func<string?> getSdkPath with string adbPath — the constructor now takes the full path to the adb executable. Removed:

• AdbPath property
• IsAvailable property
• RequireAdb()
• PATH discovery fallback (FindExecutablesInPath)
• getSdkPath/getJdkPath fields

Environment variables can optionally be passed via the constructor parameter IDictionary<string, string>? environmentVariables.

Thread 44: TcpClient AVD name fallback — analysis

The TcpClient fallback exists because adb -s emulator-XXXX emu avd name returns exit code 0 but empty output on macOS. We verified this with both:

• android-tools AdbRunner (empty AvdName without fallback)
• dotnet/android GetAvailableAndroidDevices MSBuild task on main (same bug — Description=sdk gphone64 arm64 with no AVD name)

Without the AVD name, MergeDevicesAndEmulators can't match running emulators with AVD definitions from emulator -list-avds, leading to duplicates in UI device pickers.

The TCP console approach connects to the emulator's console port (emulator-{port}) and sends avd name — this works reliably because the emulator console protocol is stable and documented.

An alternative is process scanning (lsof -i :port → PID → ps -o command= → parse -avd arg), which avoids potential auth token requirements. Happy to switch to that approach if preferred, or keep TCP as the simpler cross-platform option. Let me know which you prefer.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

---

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

In QueryAvdNameViaConsoleAsync, if the Task.Delay(..., cts.Token) completes due to cancellation/timeout, the method returns null without honoring cancellationToken (external cancellation should propagate as OperationCanceledException). Also, when returning early, connectTask may still fault later without being observed. Consider explicitly handling cancellation vs timeout, and ensure any non-awaited connectTask has its exceptions observed (or is awaited/continued) before returning.

Suggested change

	var connectTask = client.ConnectAsync ("127.0.0.1", port);
	var completed = await Task.WhenAny (connectTask, Task.Delay (3000, cts.Token)).ConfigureAwait (false);
	if (completed != connectTask) {
	var connectTask = client.ConnectAsync ("127.0.0.1", port);
	_ = connectTask.ContinueWith (
	t => {
	// Observe exceptions when the task is not awaited (e.g. timeout path)
	var _ = t.Exception;
	},
	CancellationToken.None,
	TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously,
	TaskScheduler.Default);
	var completed = await Task.WhenAny (connectTask, Task.Delay (3000, cts.Token)).ConfigureAwait (false);
	if (completed != connectTask) {
	if (cancellationToken.IsCancellationRequested)
	throw new OperationCanceledException (cancellationToken);