Skip to content

Add AdbRunner for adb CLI operations#283

Merged
jonathanpeppers merged 17 commits intomainfrom
feature/adb-runner
Mar 6, 2026
Merged

Add AdbRunner for adb CLI operations#283
jonathanpeppers merged 17 commits intomainfrom
feature/adb-runner

Conversation

@rmarinho
Copy link
Member

@rmarinho rmarinho commented Feb 23, 2026

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 (string and StringWriter overloads)

Key Design Decisions

  • Constructor takes string adbPath: Callers pass the resolved path; no lazy Func<> 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
  • WaitForDeviceAsync: timeout validation (negative, zero)

4 integration tests (RunnerIntegrationTests.cs):

  • Run only when TF_BUILD=True or CI=true (case-insensitive truthy check), 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 25e7711 Replace Func<> with string adbPath, remove lazy resolution
Port device listing from dotnet/android 2dac552, 93aca4b, b9be955 ParseAdbDevicesOutput, BuildDeviceDescription, FormatDisplayName, MergeDevicesAndEmulators
Exit code check, internal visibility a42544f ProcessUtils.ThrowIfFailed, visibility adjustments
Fix regex: explicit states + tab support eb63cb5 \s+ separator, explicit state list, IgnoreCase
Fix nullable reference type warnings e689e82 For dotnet/android WarningsAsErrors=Nullable compatibility
Optional logger callback b9be955 Action<TraceLevel, string>? on BuildDeviceDescription + MergeDevicesAndEmulators
IEnumerable<string> parse overload 93aca4b dotnet/android passes List<string> directly
Log exception in bare catch f68600d GetEmulatorAvdNameAsync logs via Trace.WriteLine
Emulator console fallback for AVD name 90ef8b5 TCP console query when adb emu avd name fails
ThrowIfFailed StringWriter overload b4d9a5f Delegates to string version; single IEnumerable<string> parse method
Replace null-forgiving ! with patterns 850cb39 is { Length: > 0 } patterns; MapAdbStateToStatus → switch expression
Remove section separator comments da3000c Removed // ── Section ── region-style comments
Tighten RequireCi() truthy check da3000c string.Equals("true", OrdinalIgnoreCase) instead of presence check
Remove bootstrap fallback in tests da3000c Assert.Ignore when ANDROID_HOME missing — no network-dependent SDK download
Fix regex comment accuracy da3000c Comment matches \s+ behavior (1+ whitespace)
Apply review suggestions (pattern matching, comment) 72291e2 is { Length: > 0 } in ProcessUtils, improved netstandard2.0 comment

Co-authored-by: Copilot [email protected]

This comment was marked as resolved.

@rmarinho rmarinho added the copilot `copilot-cli` or other AIs were used to author this label Feb 23, 2026
@jonathanpeppers
Copy link
Member

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

rmarinho added a commit that referenced this pull request Feb 24, 2026
Addresses PR #283 feedback to use existing ProcessUtils instead of
the removed AndroidToolRunner. Simplifies API:

- Methods now throw InvalidOperationException on failure
- Uses ProcessUtils.RunToolAsync() for all tool invocations
- Added AndroidDeviceInfo model
- Removed complex ToolRunnerResult wrapper types

Co-authored-by: Copilot <[email protected]>
rmarinho added a commit that referenced this pull request Feb 24, 2026
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 <[email protected]>
@rmarinho rmarinho force-pushed the feature/adb-runner branch 4 times, most recently from d378294 to ec0675f Compare March 2, 2026 11:42
@rmarinho rmarinho force-pushed the feature/adb-runner branch 3 times, most recently from 923285f to 1cf8fc6 Compare March 3, 2026 14:35
@rmarinho
Copy link
Member Author

rmarinho commented Mar 3, 2026

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
Copy link
Member Author

rmarinho commented Mar 3, 2026

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

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@rmarinho rmarinho force-pushed the feature/adb-runner branch from 193203e to 5f9a212 Compare March 3, 2026 18:23
rmarinho added a commit that referenced this pull request Mar 3, 2026
…eout

- Broaden AdbDevicesRegex to match any device state (recovery, sideload, etc.)
  using \s{2,} separator to avoid matching random text lines
- Skip daemon startup lines (starting with *) in ParseAdbDevicesOutput
- ListDevicesAsync now captures stderr and throws on non-zero exit code
- WaitForDeviceAsync now checks exit code and throws with stdout/stderr context
- Validate timeout: reject negative and zero TimeSpan values
- Add 6 tests: recovery/sideload parsing, state mapping, timeout validation

Co-authored-by: Copilot <[email protected]>
rmarinho added a commit that referenced this pull request Mar 3, 2026
…eout

- Broaden AdbDevicesRegex to match any device state (recovery, sideload, etc.)
  using \s{2,} separator to avoid matching random text lines
- Skip daemon startup lines (starting with *) in ParseAdbDevicesOutput
- ListDevicesAsync now captures stderr and throws on non-zero exit code
- WaitForDeviceAsync now checks exit code and throws with stdout/stderr context
- Validate timeout: reject negative and zero TimeSpan values
- Add 6 tests: recovery/sideload parsing, state mapping, timeout validation

Co-authored-by: Copilot <[email protected]>
@rmarinho rmarinho force-pushed the feature/adb-runner branch from be56ade to 1d82e36 Compare March 3, 2026 19:18
@rmarinho rmarinho requested a review from Copilot March 4, 2026 09:24

This comment was marked as outdated.

rmarinho and others added 12 commits March 5, 2026 15:37
Port adb device parsing, formatting, and merging logic from
dotnet/android GetAvailableAndroidDevices MSBuild task into a
shared AdbRunner class.

New files:
- AdbRunner.cs — ListDevicesAsync, WaitForDeviceAsync, StopEmulatorAsync,
  ParseAdbDevicesOutput, BuildDeviceDescription, FormatDisplayName,
  MapAdbStateToStatus, MergeDevicesAndEmulators
- AdbDeviceInfo/AdbDeviceType/AdbDeviceStatus — device models
- AndroidEnvironmentHelper — shared env var builder for all runners
- ProcessUtils.ThrowIfFailed — shared exit code validation

Modified files:
- EnvironmentVariableNames — add ANDROID_USER_HOME, ANDROID_AVD_HOME
- SdkManager.Process.cs — deduplicate env var logic via AndroidEnvironmentHelper

Tests:
- 43 unit tests (parsing, formatting, merging, path discovery, timeout)
- 5 integration tests (CI-only, real SDK tools)

Co-authored-by: Copilot <[email protected]>
- StopEmulatorAsync now captures stderr and calls ThrowIfFailed
  for consistency with ListDevicesAsync/WaitForDeviceAsync.

- ThrowIfFailed changed from public to internal since it is only
  used within the library.

- Remove inaccurate cmdline-tools bootstrap claim from integration
  test doc comment.

Co-authored-by: Copilot <[email protected]>
Avoids allocating a joined string when callers already have individual
lines (e.g., MSBuild LogEventsFromTextOutput). The existing string
overload now delegates to the new one.

Addresses review feedback from @jonathanpeppers on dotnet/android#10880.

Co-authored-by: Copilot <[email protected]>
…esAndEmulators

Accepts Action<TraceLevel, string> to route debug messages through the
caller's logging infrastructure (e.g., MSBuild TaskLoggingHelper).
Restores log messages lost when logic moved from dotnet/android to
android-tools: AVD name formatting, running emulator detection, and
non-running emulator additions.

Follows the existing CreateTaskLogger pattern used by JdkInstaller.

Co-authored-by: Copilot <[email protected]>
dotnet/android compiles the submodule as netstandard2.0 with
WarningsAsErrors=Nullable. In netstandard2.0, string.IsNullOrEmpty
lacks [NotNullWhen(false)], so the compiler doesn't narrow string?
to string after null checks. Add null-forgiving operators where
the preceding guard guarantees non-null.

Fixes: CS8601 in AndroidEnvironmentHelper.cs (sdkPath, jdkPath)
Fixes: CS8620 in AdbRunner.cs (serial in string[] array literal)

Co-authored-by: Copilot <[email protected]>
- Replace \s{2,} with \s+ to handle tab-separated adb output
- Use explicit state list (device|offline|unauthorized|etc.) instead
  of \S+ to prevent false positives from non-device lines
- Add ParseAdbDevicesOutput_TabSeparator test

Co-authored-by: Copilot <[email protected]>
Address review feedback: replace bare catch with catch(Exception ex)
and log via Trace.WriteLine for debuggability.

Co-authored-by: Copilot <[email protected]>
When 'adb emu avd name' fails (common on macOS), fall back to
querying the emulator console directly via TCP on the console port
extracted from the serial (emulator-XXXX -> port XXXX).

This fixes duplicate device entries when running emulators can't
be matched with their AVD definitions.

Co-authored-by: Copilot <[email protected]>
Address review feedback (threads 41-43): replace Func<string?> getSdkPath
constructor with string adbPath that takes the full path to the adb
executable. Remove AdbPath property, IsAvailable property, RequireAdb(),
PATH discovery fallback, and getSdkPath/getJdkPath fields.

Callers are now responsible for resolving the adb path before constructing.
Environment variables can optionally be passed via the constructor.

Co-authored-by: Copilot <[email protected]>
…seAdbDevicesOutput

Thread 46: Add ProcessUtils.ThrowIfFailed(int, string, StringWriter?, StringWriter?)
overload that delegates to the string version. Update AdbRunner callers to pass
StringWriter directly instead of calling .ToString() at each call site.

Thread 47: Remove ParseAdbDevicesOutput(string) overload. Callers now split
the string themselves and pass IEnumerable<string> directly. This removes
the dual-signature confusion and aligns with dotnet/android's usage pattern.

Co-authored-by: Copilot <[email protected]>
…pression

  patterns that give the compiler proper non-null flow on netstandard2.0.
- Convert MapAdbStateToStatus from switch statement to switch expression.
- Update copilot-instructions.md with both guidelines for future PRs.

Co-authored-by: Copilot <[email protected]>
- RequireCi(): check for truthy value (case-insensitive 'true') instead
  of just variable presence
- Remove SDK bootstrap fallback; Assert.Ignore when ANDROID_HOME missing
  to avoid flaky network-dependent CI runs
- Remove section separator comments (region-style anti-pattern)
- Fix regex comment to match actual \s+ behavior (1+ whitespace)
- Replace null-forgiving ex! with ex?.Message pattern
- Remove unused usings and bootstrappedSdkPath field

Co-authored-by: Copilot <[email protected]>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

rmarinho and others added 3 commits March 5, 2026 17:47
Remove the TCP console fallback for AVD name queries as requested
in review. The adb shell approach is sufficient; if it returns empty
the method now simply returns null.

Co-authored-by: Copilot <[email protected]>
…Async

Use 'serial is { Length: > 0 } s' pattern to avoid string?[] → string[]
nullability mismatch when building with dotnet/android WarningsAsErrors.

Co-authored-by: Copilot <[email protected]>
rmarinho added a commit that referenced this pull request Mar 5, 2026
…g, ThrowIfFailed overload

Apply review patterns from PR #283 (AdbRunner):
- Constructor takes resolved 'string avdManagerPath' instead of Func<string?> delegates
- Add IDictionary<string, string>? environmentVariables for ANDROID_HOME/JAVA_HOME
- Remove AvdManagerPath property, IsAvailable, RequireAvdManagerPath(), ConfigureEnvironment()
- Use 'is { Length: > 0 }' pattern matching for null/empty checks
- Add ThrowIfFailed(StringWriter) overload to ProcessUtils
- Change ThrowIfFailed/ValidateNotNullOrEmpty/FindCmdlineTool to internal visibility
- Update tests: FindCmdlineTool tests replace AvdManagerPath tests, add constructor validation

Co-authored-by: Copilot <[email protected]>
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI Review Summary

Found 3 issues: 2 API design, 1 documentation accuracy.

  • API design: ParseAdbDevicesOutput and MergeDevicesAndEmulators return List<T> instead of IReadOnlyList<T> (AdbRunner.cs:142,264)
  • Documentation: GetEmulatorAvdNameAsync doc comment mentions TCP console fallback that isn't implemented (AdbRunner.cs:74)

👍 Positives:

  • Excellent OperationCanceledException handling — caught and rethrown before the general catch (Exception) in GetEmulatorAvdNameAsync, and the when clause in WaitForDeviceAsync correctly distinguishes timeout from caller cancellation.
  • Consistent exit code checking via ProcessUtils.ThrowIfFailed across all three async methods.
  • All process creation goes through ProcessUtils.CreateProcessStartInfo with separate argument strings — no string interpolation into commands.
  • Clean one-type-per-file organization with file-scoped namespaces.
  • Property patterns (is { Length: > 0 }) used throughout instead of null-forgiving !.
  • CancellationToken properly propagated to every downstream async call.
  • Solid test coverage (45 unit + 4 integration) with real-world adb output data.

Review generated by android-tools-reviewer from review guidelines by @jonathanpeppers.

rmarinho added a commit that referenced this pull request Mar 5, 2026
Resolves merge conflicts: keeps canonical AdbRunner from #283, adds shell
methods and virtual ListDevicesAsync for EmulatorRunner BootAndWait, keeps
updated AvdManagerRunner with resolved-path constructor from #282.

Co-authored-by: Copilot <[email protected]>
rmarinho added a commit to dotnet/android that referenced this pull request Mar 6, 2026
Delegates adb devices parsing, description building, and device/emulator
merging from GetAvailableAndroidDevices to AdbRunner in the shared
xamarin-android-tools submodule. Removes ~200 lines of duplicated logic.

- ParseAdbDevicesOutput accepts IEnumerable<string> to avoid string.Join
- BuildDeviceDescription/MergeDevicesAndEmulators accept optional
  Action<TraceLevel, string> logger for MSBuild diagnostics
- Tests updated to use AdbRunner/AdbDeviceInfo directly

Depends on dotnet/android-tools#283.

Co-authored-by: Copilot <[email protected]>
rmarinho and others added 2 commits March 6, 2026 09:04
…ulators, fix stale doc

- ParseAdbDevicesOutput: List<AdbDeviceInfo> → IReadOnlyList<AdbDeviceInfo>
- MergeDevicesAndEmulators: List<AdbDeviceInfo> → IReadOnlyList<AdbDeviceInfo>
- GetEmulatorAvdNameAsync: remove stale TCP fallback reference from doc comment
- Tests: TrueForAll → LINQ All (IReadOnlyList compatible)

Co-authored-by: Copilot <[email protected]>
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the changes upstream in dotnet/android. I had this setup an emulator running + device attached:

> adb devices
List of devices attached
0A041FDD400327  device
emulator-5554   device

> emulator -list-avds
Pixel_9_Pro_XL
pixel_7_-_api_29
pixel_7_-_api_36

> adb -s emulator-5554 emu avd name
Pixel_9_Pro_XL
OK

Selection looks OK (donut is alias for dotnet-local.cmd):

> donut run -bl
Restore complete (0.5s)

Build succeeded in 0.5s
    info NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy

Build succeeded in 0.4s
Select a device to run on:

  0A041FDD400327 - Pixel 5
> emulator-5554 - Pixel 9 Pro Xl
  pixel_7_-_api_29 - Pixel 7 - API 29 (Not Running)
  pixel_7_-_api_36 - Pixel 7 - API 36 (Not Running)

Type to search

I was able to deploy to emulator and device.

@jonathanpeppers jonathanpeppers merged commit d3c269d into main Mar 6, 2026
2 checks passed
@jonathanpeppers jonathanpeppers deleted the feature/adb-runner branch March 6, 2026 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

copilot `copilot-cli` or other AIs were used to author this

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants