This repository was archived by the owner on Jul 13, 2026. It is now read-only.

Add AdbRunner for adb CLI operations - #283

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

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

Conversation

@rmarinho

@rmarinhormarinho commented Feb 23, 2026

Copy link
Copy Markdown
Member

Summary

Wraps adb CLI operations for device management. Addresses dotnet/android#12071.

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

Public API

publicclassAdbRunner{// Constructor — requires full path to adb executablepublicAdbRunner(stringadbPath,IDictionary<string,string>?environmentVariables=null);// Instance methods (async, invoke adb process)publicTask<IReadOnlyList<AdbDeviceInfo>>ListDevicesAsync(CancellationTokenct=default);publicTaskWaitForDeviceAsync(string?serial=null,TimeSpan?timeout=null,CancellationTokenct=default);publicTaskStopEmulatorAsync(stringserial,CancellationTokenct=default);// Static helpers — public so dotnet/android can call without instantiating AdbRunnerpublicstaticList<AdbDeviceInfo>ParseAdbDevicesOutput(IEnumerable<string>lines);publicstaticAdbDeviceStatusMapAdbStateToStatus(stringadbState);publicstaticstringBuildDeviceDescription(AdbDeviceInfodevice,Action<TraceLevel,string>?logger=null);publicstaticstringFormatDisplayName(stringavdName);publicstaticList<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

FeedbackCommitDetails
Constructor: require string adbPath25e7711Replace Func<> with string adbPath, remove lazy resolution
Port device listing from dotnet/android2dac552, 93aca4b, b9be955ParseAdbDevicesOutput, BuildDeviceDescription, FormatDisplayName, MergeDevicesAndEmulators
Exit code check, internal visibilitya42544fProcessUtils.ThrowIfFailed, visibility adjustments
Fix regex: explicit states + tab supporteb63cb5\s+ separator, explicit state list, IgnoreCase
Fix nullable reference type warningse689e82For dotnet/androidWarningsAsErrors=Nullable compatibility
Optional logger callbackb9be955Action<TraceLevel, string>? on BuildDeviceDescription + MergeDevicesAndEmulators
IEnumerable<string> parse overload93aca4bdotnet/android passes List<string> directly
Log exception in bare catchf68600dGetEmulatorAvdNameAsync logs via Trace.WriteLine
Emulator console fallback for AVD name90ef8b5TCP console query when adb emu avd name fails
ThrowIfFailed StringWriter overloadb4d9a5fDelegates to string version; single IEnumerable<string> parse method
Replace null-forgiving ! with patterns850cb39is { Length: > 0 } patterns; MapAdbStateToStatus → switch expression
Remove section separator commentsda3000cRemoved // ── Section ── region-style comments
Tighten RequireCi() truthy checkda3000cstring.Equals("true", OrdinalIgnoreCase) instead of presence check
Remove bootstrap fallback in testsda3000cAssert.Ignore when ANDROID_HOME missing — no network-dependent SDK download
Fix regex comment accuracyda3000cComment matches \s+ behavior (1+ whitespace)
Apply review suggestions (pattern matching, comment)72291e2is { Length: > 0 } in ProcessUtils, improved netstandard2.0 comment

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

CopilotAI review requested due to automatic review settings February 23, 2026 17:39

This comment was marked as resolved.

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

Copy link
Copy Markdown
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 4 times, most recently from d378294 to ec0675fCompareMarch 2, 2026 11:42
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 3 times, most recently from 923285f to 1cf8fc6CompareMarch 3, 2026 14:35
@rmarinho

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Models/AdbDeviceInfo.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from 193203e to 5f9a212CompareMarch 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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from be56ade to 1d82e36CompareMarch 3, 2026 19:18
@rmarinho
rmarinho requested a review from CopilotMarch 4, 2026 09:24

This comment was marked as outdated.

rmarinhoand others added 9 commits March 5, 2026 15:37
…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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>
Address review feedback: replace bare catch with catch(Exception ex)
and log via Trace.WriteLine for debuggability.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/ProcessUtils.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
rmarinhoand others added 3 commits March 5, 2026 17:47
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…Async
Use 'serial is { Length: > 0 } s' pattern to avoid string?[] → string[]
nullability mismatch when building with dotnet/android WarningsAsErrors.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
rmarinhoand 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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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 mainMar 6, 2026
2 checks passed
@jonathanpeppers
jonathanpeppers deleted the feature/adb-runner branch March 6, 2026 14:59
jonathanpeppers added a commit to dotnet/android that referenced this pull request Jul 13, 2026
This skill lets you say:
review this PR: dotnet/android-tools#284
Some example code reviews:
* dotnet/android-tools#283 (review)
* dotnet/android-tools#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`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

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

@rmarinho@jonathanpeppers
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks"); } } catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); } })(); (function(){ try { var __m = "github.com"; var __re = new RegExp('^' + "github\\.com" + '
Skip to content
This repository was archived by the owner on Jul 13, 2026. It is now read-only.

Add AdbRunner for adb CLI operations - #283

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

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

Conversation

@rmarinho

@rmarinhormarinho commented Feb 23, 2026

Copy link
Copy Markdown
Member

Summary

Wraps adb CLI operations for device management. Addresses dotnet/android#12071.

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

Public API

publicclassAdbRunner{// Constructor — requires full path to adb executablepublicAdbRunner(stringadbPath,IDictionary<string,string>?environmentVariables=null);// Instance methods (async, invoke adb process)publicTask<IReadOnlyList<AdbDeviceInfo>>ListDevicesAsync(CancellationTokenct=default);publicTaskWaitForDeviceAsync(string?serial=null,TimeSpan?timeout=null,CancellationTokenct=default);publicTaskStopEmulatorAsync(stringserial,CancellationTokenct=default);// Static helpers — public so dotnet/android can call without instantiating AdbRunnerpublicstaticList<AdbDeviceInfo>ParseAdbDevicesOutput(IEnumerable<string>lines);publicstaticAdbDeviceStatusMapAdbStateToStatus(stringadbState);publicstaticstringBuildDeviceDescription(AdbDeviceInfodevice,Action<TraceLevel,string>?logger=null);publicstaticstringFormatDisplayName(stringavdName);publicstaticList<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

FeedbackCommitDetails
Constructor: require string adbPath25e7711Replace Func<> with string adbPath, remove lazy resolution
Port device listing from dotnet/android2dac552, 93aca4b, b9be955ParseAdbDevicesOutput, BuildDeviceDescription, FormatDisplayName, MergeDevicesAndEmulators
Exit code check, internal visibilitya42544fProcessUtils.ThrowIfFailed, visibility adjustments
Fix regex: explicit states + tab supporteb63cb5\s+ separator, explicit state list, IgnoreCase
Fix nullable reference type warningse689e82For dotnet/androidWarningsAsErrors=Nullable compatibility
Optional logger callbackb9be955Action<TraceLevel, string>? on BuildDeviceDescription + MergeDevicesAndEmulators
IEnumerable<string> parse overload93aca4bdotnet/android passes List<string> directly
Log exception in bare catchf68600dGetEmulatorAvdNameAsync logs via Trace.WriteLine
Emulator console fallback for AVD name90ef8b5TCP console query when adb emu avd name fails
ThrowIfFailed StringWriter overloadb4d9a5fDelegates to string version; single IEnumerable<string> parse method
Replace null-forgiving ! with patterns850cb39is { Length: > 0 } patterns; MapAdbStateToStatus → switch expression
Remove section separator commentsda3000cRemoved // ── Section ── region-style comments
Tighten RequireCi() truthy checkda3000cstring.Equals("true", OrdinalIgnoreCase) instead of presence check
Remove bootstrap fallback in testsda3000cAssert.Ignore when ANDROID_HOME missing — no network-dependent SDK download
Fix regex comment accuracyda3000cComment matches \s+ behavior (1+ whitespace)
Apply review suggestions (pattern matching, comment)72291e2is { Length: > 0 } in ProcessUtils, improved netstandard2.0 comment

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

CopilotAI review requested due to automatic review settings February 23, 2026 17:39

This comment was marked as resolved.

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

Copy link
Copy Markdown
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 4 times, most recently from d378294 to ec0675fCompareMarch 2, 2026 11:42
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 3 times, most recently from 923285f to 1cf8fc6CompareMarch 3, 2026 14:35
@rmarinho

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Models/AdbDeviceInfo.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from 193203e to 5f9a212CompareMarch 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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from be56ade to 1d82e36CompareMarch 3, 2026 19:18
@rmarinho
rmarinho requested a review from CopilotMarch 4, 2026 09:24

This comment was marked as outdated.

rmarinhoand others added 9 commits March 5, 2026 15:37
…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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>
Address review feedback: replace bare catch with catch(Exception ex)
and log via Trace.WriteLine for debuggability.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/ProcessUtils.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
rmarinhoand others added 3 commits March 5, 2026 17:47
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…Async
Use 'serial is { Length: > 0 } s' pattern to avoid string?[] → string[]
nullability mismatch when building with dotnet/android WarningsAsErrors.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
rmarinhoand 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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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 mainMar 6, 2026
2 checks passed
@jonathanpeppers
jonathanpeppers deleted the feature/adb-runner branch March 6, 2026 14:59
jonathanpeppers added a commit to dotnet/android that referenced this pull request Jul 13, 2026
This skill lets you say:
review this PR: dotnet/android-tools#284
Some example code reviews:
* dotnet/android-tools#283 (review)
* dotnet/android-tools#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`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

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

@rmarinho@jonathanpeppers
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Jul 13, 2026. It is now read-only.

Add AdbRunner for adb CLI operations - #283

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

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

Conversation

@rmarinho

@rmarinhormarinho commented Feb 23, 2026

Copy link
Copy Markdown
Member

Summary

Wraps adb CLI operations for device management. Addresses dotnet/android#12071.

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

Public API

publicclassAdbRunner{// Constructor — requires full path to adb executablepublicAdbRunner(stringadbPath,IDictionary<string,string>?environmentVariables=null);// Instance methods (async, invoke adb process)publicTask<IReadOnlyList<AdbDeviceInfo>>ListDevicesAsync(CancellationTokenct=default);publicTaskWaitForDeviceAsync(string?serial=null,TimeSpan?timeout=null,CancellationTokenct=default);publicTaskStopEmulatorAsync(stringserial,CancellationTokenct=default);// Static helpers — public so dotnet/android can call without instantiating AdbRunnerpublicstaticList<AdbDeviceInfo>ParseAdbDevicesOutput(IEnumerable<string>lines);publicstaticAdbDeviceStatusMapAdbStateToStatus(stringadbState);publicstaticstringBuildDeviceDescription(AdbDeviceInfodevice,Action<TraceLevel,string>?logger=null);publicstaticstringFormatDisplayName(stringavdName);publicstaticList<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

FeedbackCommitDetails
Constructor: require string adbPath25e7711Replace Func<> with string adbPath, remove lazy resolution
Port device listing from dotnet/android2dac552, 93aca4b, b9be955ParseAdbDevicesOutput, BuildDeviceDescription, FormatDisplayName, MergeDevicesAndEmulators
Exit code check, internal visibilitya42544fProcessUtils.ThrowIfFailed, visibility adjustments
Fix regex: explicit states + tab supporteb63cb5\s+ separator, explicit state list, IgnoreCase
Fix nullable reference type warningse689e82For dotnet/androidWarningsAsErrors=Nullable compatibility
Optional logger callbackb9be955Action<TraceLevel, string>? on BuildDeviceDescription + MergeDevicesAndEmulators
IEnumerable<string> parse overload93aca4bdotnet/android passes List<string> directly
Log exception in bare catchf68600dGetEmulatorAvdNameAsync logs via Trace.WriteLine
Emulator console fallback for AVD name90ef8b5TCP console query when adb emu avd name fails
ThrowIfFailed StringWriter overloadb4d9a5fDelegates to string version; single IEnumerable<string> parse method
Replace null-forgiving ! with patterns850cb39is { Length: > 0 } patterns; MapAdbStateToStatus → switch expression
Remove section separator commentsda3000cRemoved // ── Section ── region-style comments
Tighten RequireCi() truthy checkda3000cstring.Equals("true", OrdinalIgnoreCase) instead of presence check
Remove bootstrap fallback in testsda3000cAssert.Ignore when ANDROID_HOME missing — no network-dependent SDK download
Fix regex comment accuracyda3000cComment matches \s+ behavior (1+ whitespace)
Apply review suggestions (pattern matching, comment)72291e2is { Length: > 0 } in ProcessUtils, improved netstandard2.0 comment

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

CopilotAI review requested due to automatic review settings February 23, 2026 17:39

This comment was marked as resolved.

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

Copy link
Copy Markdown
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 4 times, most recently from d378294 to ec0675fCompareMarch 2, 2026 11:42
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 3 times, most recently from 923285f to 1cf8fc6CompareMarch 3, 2026 14:35
@rmarinho

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Models/AdbDeviceInfo.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from 193203e to 5f9a212CompareMarch 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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from be56ade to 1d82e36CompareMarch 3, 2026 19:18
@rmarinho
rmarinho requested a review from CopilotMarch 4, 2026 09:24

This comment was marked as outdated.

rmarinhoand others added 9 commits March 5, 2026 15:37
…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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>
Address review feedback: replace bare catch with catch(Exception ex)
and log via Trace.WriteLine for debuggability.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/ProcessUtils.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
rmarinhoand others added 3 commits March 5, 2026 17:47
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…Async
Use 'serial is { Length: > 0 } s' pattern to avoid string?[] → string[]
nullability mismatch when building with dotnet/android WarningsAsErrors.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
rmarinhoand 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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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 mainMar 6, 2026
2 checks passed
@jonathanpeppers
jonathanpeppers deleted the feature/adb-runner branch March 6, 2026 14:59
jonathanpeppers added a commit to dotnet/android that referenced this pull request Jul 13, 2026
This skill lets you say:
review this PR: dotnet/android-tools#284
Some example code reviews:
* dotnet/android-tools#283 (review)
* dotnet/android-tools#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`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

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

@rmarinho@jonathanpeppers
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length \u003e 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Jul 13, 2026. It is now read-only.

Add AdbRunner for adb CLI operations - #283

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

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

Conversation

@rmarinho

@rmarinhormarinho commented Feb 23, 2026

Copy link
Copy Markdown
Member

Summary

Wraps adb CLI operations for device management. Addresses dotnet/android#12071.

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

Public API

publicclassAdbRunner{// Constructor — requires full path to adb executablepublicAdbRunner(stringadbPath,IDictionary<string,string>?environmentVariables=null);// Instance methods (async, invoke adb process)publicTask<IReadOnlyList<AdbDeviceInfo>>ListDevicesAsync(CancellationTokenct=default);publicTaskWaitForDeviceAsync(string?serial=null,TimeSpan?timeout=null,CancellationTokenct=default);publicTaskStopEmulatorAsync(stringserial,CancellationTokenct=default);// Static helpers — public so dotnet/android can call without instantiating AdbRunnerpublicstaticList<AdbDeviceInfo>ParseAdbDevicesOutput(IEnumerable<string>lines);publicstaticAdbDeviceStatusMapAdbStateToStatus(stringadbState);publicstaticstringBuildDeviceDescription(AdbDeviceInfodevice,Action<TraceLevel,string>?logger=null);publicstaticstringFormatDisplayName(stringavdName);publicstaticList<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

FeedbackCommitDetails
Constructor: require string adbPath25e7711Replace Func<> with string adbPath, remove lazy resolution
Port device listing from dotnet/android2dac552, 93aca4b, b9be955ParseAdbDevicesOutput, BuildDeviceDescription, FormatDisplayName, MergeDevicesAndEmulators
Exit code check, internal visibilitya42544fProcessUtils.ThrowIfFailed, visibility adjustments
Fix regex: explicit states + tab supporteb63cb5\s+ separator, explicit state list, IgnoreCase
Fix nullable reference type warningse689e82For dotnet/androidWarningsAsErrors=Nullable compatibility
Optional logger callbackb9be955Action<TraceLevel, string>? on BuildDeviceDescription + MergeDevicesAndEmulators
IEnumerable<string> parse overload93aca4bdotnet/android passes List<string> directly
Log exception in bare catchf68600dGetEmulatorAvdNameAsync logs via Trace.WriteLine
Emulator console fallback for AVD name90ef8b5TCP console query when adb emu avd name fails
ThrowIfFailed StringWriter overloadb4d9a5fDelegates to string version; single IEnumerable<string> parse method
Replace null-forgiving ! with patterns850cb39is { Length: > 0 } patterns; MapAdbStateToStatus → switch expression
Remove section separator commentsda3000cRemoved // ── Section ── region-style comments
Tighten RequireCi() truthy checkda3000cstring.Equals("true", OrdinalIgnoreCase) instead of presence check
Remove bootstrap fallback in testsda3000cAssert.Ignore when ANDROID_HOME missing — no network-dependent SDK download
Fix regex comment accuracyda3000cComment matches \s+ behavior (1+ whitespace)
Apply review suggestions (pattern matching, comment)72291e2is { Length: > 0 } in ProcessUtils, improved netstandard2.0 comment

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

CopilotAI review requested due to automatic review settings February 23, 2026 17:39

This comment was marked as resolved.

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

Copy link
Copy Markdown
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 4 times, most recently from d378294 to ec0675fCompareMarch 2, 2026 11:42
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 3 times, most recently from 923285f to 1cf8fc6CompareMarch 3, 2026 14:35
@rmarinho

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Models/AdbDeviceInfo.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from 193203e to 5f9a212CompareMarch 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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from be56ade to 1d82e36CompareMarch 3, 2026 19:18
@rmarinho
rmarinho requested a review from CopilotMarch 4, 2026 09:24

This comment was marked as outdated.

rmarinhoand others added 9 commits March 5, 2026 15:37
…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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>
Address review feedback: replace bare catch with catch(Exception ex)
and log via Trace.WriteLine for debuggability.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/ProcessUtils.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
rmarinhoand others added 3 commits March 5, 2026 17:47
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…Async
Use 'serial is { Length: > 0 } s' pattern to avoid string?[] → string[]
nullability mismatch when building with dotnet/android WarningsAsErrors.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
rmarinhoand 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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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 mainMar 6, 2026
2 checks passed
@jonathanpeppers
jonathanpeppers deleted the feature/adb-runner branch March 6, 2026 14:59
jonathanpeppers added a commit to dotnet/android that referenced this pull request Jul 13, 2026
This skill lets you say:
review this PR: dotnet/android-tools#284
Some example code reviews:
* dotnet/android-tools#283 (review)
* dotnet/android-tools#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`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

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

@rmarinho@jonathanpeppers
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content
This repository was archived by the owner on Jul 13, 2026. It is now read-only.

Add AdbRunner for adb CLI operations - #283

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

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

Conversation

@rmarinho

@rmarinhormarinho commented Feb 23, 2026

Copy link
Copy Markdown
Member

Summary

Wraps adb CLI operations for device management. Addresses dotnet/android#12071.

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

Public API

publicclassAdbRunner{// Constructor — requires full path to adb executablepublicAdbRunner(stringadbPath,IDictionary<string,string>?environmentVariables=null);// Instance methods (async, invoke adb process)publicTask<IReadOnlyList<AdbDeviceInfo>>ListDevicesAsync(CancellationTokenct=default);publicTaskWaitForDeviceAsync(string?serial=null,TimeSpan?timeout=null,CancellationTokenct=default);publicTaskStopEmulatorAsync(stringserial,CancellationTokenct=default);// Static helpers — public so dotnet/android can call without instantiating AdbRunnerpublicstaticList<AdbDeviceInfo>ParseAdbDevicesOutput(IEnumerable<string>lines);publicstaticAdbDeviceStatusMapAdbStateToStatus(stringadbState);publicstaticstringBuildDeviceDescription(AdbDeviceInfodevice,Action<TraceLevel,string>?logger=null);publicstaticstringFormatDisplayName(stringavdName);publicstaticList<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

FeedbackCommitDetails
Constructor: require string adbPath25e7711Replace Func<> with string adbPath, remove lazy resolution
Port device listing from dotnet/android2dac552, 93aca4b, b9be955ParseAdbDevicesOutput, BuildDeviceDescription, FormatDisplayName, MergeDevicesAndEmulators
Exit code check, internal visibilitya42544fProcessUtils.ThrowIfFailed, visibility adjustments
Fix regex: explicit states + tab supporteb63cb5\s+ separator, explicit state list, IgnoreCase
Fix nullable reference type warningse689e82For dotnet/androidWarningsAsErrors=Nullable compatibility
Optional logger callbackb9be955Action<TraceLevel, string>? on BuildDeviceDescription + MergeDevicesAndEmulators
IEnumerable<string> parse overload93aca4bdotnet/android passes List<string> directly
Log exception in bare catchf68600dGetEmulatorAvdNameAsync logs via Trace.WriteLine
Emulator console fallback for AVD name90ef8b5TCP console query when adb emu avd name fails
ThrowIfFailed StringWriter overloadb4d9a5fDelegates to string version; single IEnumerable<string> parse method
Replace null-forgiving ! with patterns850cb39is { Length: > 0 } patterns; MapAdbStateToStatus → switch expression
Remove section separator commentsda3000cRemoved // ── Section ── region-style comments
Tighten RequireCi() truthy checkda3000cstring.Equals("true", OrdinalIgnoreCase) instead of presence check
Remove bootstrap fallback in testsda3000cAssert.Ignore when ANDROID_HOME missing — no network-dependent SDK download
Fix regex comment accuracyda3000cComment matches \s+ behavior (1+ whitespace)
Apply review suggestions (pattern matching, comment)72291e2is { Length: > 0 } in ProcessUtils, improved netstandard2.0 comment

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

CopilotAI review requested due to automatic review settings February 23, 2026 17:39

This comment was marked as resolved.

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

Copy link
Copy Markdown
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 4 times, most recently from d378294 to ec0675fCompareMarch 2, 2026 11:42
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 3 times, most recently from 923285f to 1cf8fc6CompareMarch 3, 2026 14:35
@rmarinho

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Models/AdbDeviceInfo.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from 193203e to 5f9a212CompareMarch 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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from be56ade to 1d82e36CompareMarch 3, 2026 19:18
@rmarinho
rmarinho requested a review from CopilotMarch 4, 2026 09:24

This comment was marked as outdated.

rmarinhoand others added 9 commits March 5, 2026 15:37
…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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>
Address review feedback: replace bare catch with catch(Exception ex)
and log via Trace.WriteLine for debuggability.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/ProcessUtils.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
rmarinhoand others added 3 commits March 5, 2026 17:47
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…Async
Use 'serial is { Length: > 0 } s' pattern to avoid string?[] → string[]
nullability mismatch when building with dotnet/android WarningsAsErrors.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
rmarinhoand 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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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 mainMar 6, 2026
2 checks passed
@jonathanpeppers
jonathanpeppers deleted the feature/adb-runner branch March 6, 2026 14:59
jonathanpeppers added a commit to dotnet/android that referenced this pull request Jul 13, 2026
This skill lets you say:
review this PR: dotnet/android-tools#284
Some example code reviews:
* dotnet/android-tools#283 (review)
* dotnet/android-tools#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`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

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

@rmarinho@jonathanpeppers
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Jul 13, 2026. It is now read-only.

Add AdbRunner for adb CLI operations - #283

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

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

Conversation

@rmarinho

@rmarinhormarinho commented Feb 23, 2026

Copy link
Copy Markdown
Member

Summary

Wraps adb CLI operations for device management. Addresses dotnet/android#12071.

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

Public API

publicclassAdbRunner{// Constructor — requires full path to adb executablepublicAdbRunner(stringadbPath,IDictionary<string,string>?environmentVariables=null);// Instance methods (async, invoke adb process)publicTask<IReadOnlyList<AdbDeviceInfo>>ListDevicesAsync(CancellationTokenct=default);publicTaskWaitForDeviceAsync(string?serial=null,TimeSpan?timeout=null,CancellationTokenct=default);publicTaskStopEmulatorAsync(stringserial,CancellationTokenct=default);// Static helpers — public so dotnet/android can call without instantiating AdbRunnerpublicstaticList<AdbDeviceInfo>ParseAdbDevicesOutput(IEnumerable<string>lines);publicstaticAdbDeviceStatusMapAdbStateToStatus(stringadbState);publicstaticstringBuildDeviceDescription(AdbDeviceInfodevice,Action<TraceLevel,string>?logger=null);publicstaticstringFormatDisplayName(stringavdName);publicstaticList<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

FeedbackCommitDetails
Constructor: require string adbPath25e7711Replace Func<> with string adbPath, remove lazy resolution
Port device listing from dotnet/android2dac552, 93aca4b, b9be955ParseAdbDevicesOutput, BuildDeviceDescription, FormatDisplayName, MergeDevicesAndEmulators
Exit code check, internal visibilitya42544fProcessUtils.ThrowIfFailed, visibility adjustments
Fix regex: explicit states + tab supporteb63cb5\s+ separator, explicit state list, IgnoreCase
Fix nullable reference type warningse689e82For dotnet/androidWarningsAsErrors=Nullable compatibility
Optional logger callbackb9be955Action<TraceLevel, string>? on BuildDeviceDescription + MergeDevicesAndEmulators
IEnumerable<string> parse overload93aca4bdotnet/android passes List<string> directly
Log exception in bare catchf68600dGetEmulatorAvdNameAsync logs via Trace.WriteLine
Emulator console fallback for AVD name90ef8b5TCP console query when adb emu avd name fails
ThrowIfFailed StringWriter overloadb4d9a5fDelegates to string version; single IEnumerable<string> parse method
Replace null-forgiving ! with patterns850cb39is { Length: > 0 } patterns; MapAdbStateToStatus → switch expression
Remove section separator commentsda3000cRemoved // ── Section ── region-style comments
Tighten RequireCi() truthy checkda3000cstring.Equals("true", OrdinalIgnoreCase) instead of presence check
Remove bootstrap fallback in testsda3000cAssert.Ignore when ANDROID_HOME missing — no network-dependent SDK download
Fix regex comment accuracyda3000cComment matches \s+ behavior (1+ whitespace)
Apply review suggestions (pattern matching, comment)72291e2is { Length: > 0 } in ProcessUtils, improved netstandard2.0 comment

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

CopilotAI review requested due to automatic review settings February 23, 2026 17:39

This comment was marked as resolved.

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

Copy link
Copy Markdown
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 4 times, most recently from d378294 to ec0675fCompareMarch 2, 2026 11:42
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 3 times, most recently from 923285f to 1cf8fc6CompareMarch 3, 2026 14:35
@rmarinho

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Models/AdbDeviceInfo.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from 193203e to 5f9a212CompareMarch 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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from be56ade to 1d82e36CompareMarch 3, 2026 19:18
@rmarinho
rmarinho requested a review from CopilotMarch 4, 2026 09:24

This comment was marked as outdated.

rmarinhoand others added 9 commits March 5, 2026 15:37
…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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>
Address review feedback: replace bare catch with catch(Exception ex)
and log via Trace.WriteLine for debuggability.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/ProcessUtils.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
rmarinhoand others added 3 commits March 5, 2026 17:47
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…Async
Use 'serial is { Length: > 0 } s' pattern to avoid string?[] → string[]
nullability mismatch when building with dotnet/android WarningsAsErrors.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
rmarinhoand 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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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 mainMar 6, 2026
2 checks passed
@jonathanpeppers
jonathanpeppers deleted the feature/adb-runner branch March 6, 2026 14:59
jonathanpeppers added a commit to dotnet/android that referenced this pull request Jul 13, 2026
This skill lets you say:
review this PR: dotnet/android-tools#284
Some example code reviews:
* dotnet/android-tools#283 (review)
* dotnet/android-tools#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`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

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

@rmarinho@jonathanpeppers
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Jul 13, 2026. It is now read-only.

Add AdbRunner for adb CLI operations - #283

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

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

Conversation

@rmarinho

@rmarinhormarinho commented Feb 23, 2026

Copy link
Copy Markdown
Member

Summary

Wraps adb CLI operations for device management. Addresses dotnet/android#12071.

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

Public API

publicclassAdbRunner{// Constructor — requires full path to adb executablepublicAdbRunner(stringadbPath,IDictionary<string,string>?environmentVariables=null);// Instance methods (async, invoke adb process)publicTask<IReadOnlyList<AdbDeviceInfo>>ListDevicesAsync(CancellationTokenct=default);publicTaskWaitForDeviceAsync(string?serial=null,TimeSpan?timeout=null,CancellationTokenct=default);publicTaskStopEmulatorAsync(stringserial,CancellationTokenct=default);// Static helpers — public so dotnet/android can call without instantiating AdbRunnerpublicstaticList<AdbDeviceInfo>ParseAdbDevicesOutput(IEnumerable<string>lines);publicstaticAdbDeviceStatusMapAdbStateToStatus(stringadbState);publicstaticstringBuildDeviceDescription(AdbDeviceInfodevice,Action<TraceLevel,string>?logger=null);publicstaticstringFormatDisplayName(stringavdName);publicstaticList<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

FeedbackCommitDetails
Constructor: require string adbPath25e7711Replace Func<> with string adbPath, remove lazy resolution
Port device listing from dotnet/android2dac552, 93aca4b, b9be955ParseAdbDevicesOutput, BuildDeviceDescription, FormatDisplayName, MergeDevicesAndEmulators
Exit code check, internal visibilitya42544fProcessUtils.ThrowIfFailed, visibility adjustments
Fix regex: explicit states + tab supporteb63cb5\s+ separator, explicit state list, IgnoreCase
Fix nullable reference type warningse689e82For dotnet/androidWarningsAsErrors=Nullable compatibility
Optional logger callbackb9be955Action<TraceLevel, string>? on BuildDeviceDescription + MergeDevicesAndEmulators
IEnumerable<string> parse overload93aca4bdotnet/android passes List<string> directly
Log exception in bare catchf68600dGetEmulatorAvdNameAsync logs via Trace.WriteLine
Emulator console fallback for AVD name90ef8b5TCP console query when adb emu avd name fails
ThrowIfFailed StringWriter overloadb4d9a5fDelegates to string version; single IEnumerable<string> parse method
Replace null-forgiving ! with patterns850cb39is { Length: > 0 } patterns; MapAdbStateToStatus → switch expression
Remove section separator commentsda3000cRemoved // ── Section ── region-style comments
Tighten RequireCi() truthy checkda3000cstring.Equals("true", OrdinalIgnoreCase) instead of presence check
Remove bootstrap fallback in testsda3000cAssert.Ignore when ANDROID_HOME missing — no network-dependent SDK download
Fix regex comment accuracyda3000cComment matches \s+ behavior (1+ whitespace)
Apply review suggestions (pattern matching, comment)72291e2is { Length: > 0 } in ProcessUtils, improved netstandard2.0 comment

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

CopilotAI review requested due to automatic review settings February 23, 2026 17:39

This comment was marked as resolved.

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

Copy link
Copy Markdown
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 4 times, most recently from d378294 to ec0675fCompareMarch 2, 2026 11:42
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 3 times, most recently from 923285f to 1cf8fc6CompareMarch 3, 2026 14:35
@rmarinho

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Models/AdbDeviceInfo.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from 193203e to 5f9a212CompareMarch 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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from be56ade to 1d82e36CompareMarch 3, 2026 19:18
@rmarinho
rmarinho requested a review from CopilotMarch 4, 2026 09:24

This comment was marked as outdated.

rmarinhoand others added 9 commits March 5, 2026 15:37
…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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>
Address review feedback: replace bare catch with catch(Exception ex)
and log via Trace.WriteLine for debuggability.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/ProcessUtils.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
rmarinhoand others added 3 commits March 5, 2026 17:47
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…Async
Use 'serial is { Length: > 0 } s' pattern to avoid string?[] → string[]
nullability mismatch when building with dotnet/android WarningsAsErrors.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
rmarinhoand 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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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 mainMar 6, 2026
2 checks passed
@jonathanpeppers
jonathanpeppers deleted the feature/adb-runner branch March 6, 2026 14:59
jonathanpeppers added a commit to dotnet/android that referenced this pull request Jul 13, 2026
This skill lets you say:
review this PR: dotnet/android-tools#284
Some example code reviews:
* dotnet/android-tools#283 (review)
* dotnet/android-tools#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`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

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

@rmarinho@jonathanpeppers
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content
This repository was archived by the owner on Jul 13, 2026. It is now read-only.

Add AdbRunner for adb CLI operations - #283

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

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

Conversation

@rmarinho

@rmarinhormarinho commented Feb 23, 2026

Copy link
Copy Markdown
Member

Summary

Wraps adb CLI operations for device management. Addresses dotnet/android#12071.

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

Public API

publicclassAdbRunner{// Constructor — requires full path to adb executablepublicAdbRunner(stringadbPath,IDictionary<string,string>?environmentVariables=null);// Instance methods (async, invoke adb process)publicTask<IReadOnlyList<AdbDeviceInfo>>ListDevicesAsync(CancellationTokenct=default);publicTaskWaitForDeviceAsync(string?serial=null,TimeSpan?timeout=null,CancellationTokenct=default);publicTaskStopEmulatorAsync(stringserial,CancellationTokenct=default);// Static helpers — public so dotnet/android can call without instantiating AdbRunnerpublicstaticList<AdbDeviceInfo>ParseAdbDevicesOutput(IEnumerable<string>lines);publicstaticAdbDeviceStatusMapAdbStateToStatus(stringadbState);publicstaticstringBuildDeviceDescription(AdbDeviceInfodevice,Action<TraceLevel,string>?logger=null);publicstaticstringFormatDisplayName(stringavdName);publicstaticList<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

FeedbackCommitDetails
Constructor: require string adbPath25e7711Replace Func<> with string adbPath, remove lazy resolution
Port device listing from dotnet/android2dac552, 93aca4b, b9be955ParseAdbDevicesOutput, BuildDeviceDescription, FormatDisplayName, MergeDevicesAndEmulators
Exit code check, internal visibilitya42544fProcessUtils.ThrowIfFailed, visibility adjustments
Fix regex: explicit states + tab supporteb63cb5\s+ separator, explicit state list, IgnoreCase
Fix nullable reference type warningse689e82For dotnet/androidWarningsAsErrors=Nullable compatibility
Optional logger callbackb9be955Action<TraceLevel, string>? on BuildDeviceDescription + MergeDevicesAndEmulators
IEnumerable<string> parse overload93aca4bdotnet/android passes List<string> directly
Log exception in bare catchf68600dGetEmulatorAvdNameAsync logs via Trace.WriteLine
Emulator console fallback for AVD name90ef8b5TCP console query when adb emu avd name fails
ThrowIfFailed StringWriter overloadb4d9a5fDelegates to string version; single IEnumerable<string> parse method
Replace null-forgiving ! with patterns850cb39is { Length: > 0 } patterns; MapAdbStateToStatus → switch expression
Remove section separator commentsda3000cRemoved // ── Section ── region-style comments
Tighten RequireCi() truthy checkda3000cstring.Equals("true", OrdinalIgnoreCase) instead of presence check
Remove bootstrap fallback in testsda3000cAssert.Ignore when ANDROID_HOME missing — no network-dependent SDK download
Fix regex comment accuracyda3000cComment matches \s+ behavior (1+ whitespace)
Apply review suggestions (pattern matching, comment)72291e2is { Length: > 0 } in ProcessUtils, improved netstandard2.0 comment

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

CopilotAI review requested due to automatic review settings February 23, 2026 17:39

This comment was marked as resolved.

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

Copy link
Copy Markdown
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 4 times, most recently from d378294 to ec0675fCompareMarch 2, 2026 11:42
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch 3 times, most recently from 923285f to 1cf8fc6CompareMarch 3, 2026 14:35
@rmarinho

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Models/AdbDeviceInfo.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from 193203e to 5f9a212CompareMarch 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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
@rmarinho
rmarinhoforce-pushed the feature/adb-runner branch from be56ade to 1d82e36CompareMarch 3, 2026 19:18
@rmarinho
rmarinho requested a review from CopilotMarch 4, 2026 09:24

This comment was marked as outdated.

rmarinhoand others added 9 commits March 5, 2026 15:37
…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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>
Address review feedback: replace bare catch with catch(Exception ex)
and log via Trace.WriteLine for debuggability.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
…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 <223556219+Copilot@users.noreply.github.com>
- 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 <223556219+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/ProcessUtils.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
rmarinhoand others added 3 commits March 5, 2026 17:47
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
…Async
Use 'serial is { Length: > 0 } s' pattern to avoid string?[] → string[]
nullability mismatch when building with dotnet/android WarningsAsErrors.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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.

Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
Comment threadsrc/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs Outdated
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
rmarinhoand 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 <223556219+Copilot@users.noreply.github.com>

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
Member

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 mainMar 6, 2026
2 checks passed
@jonathanpeppers
jonathanpeppers deleted the feature/adb-runner branch March 6, 2026 14:59
jonathanpeppers added a commit to dotnet/android that referenced this pull request Jul 13, 2026
This skill lets you say:
review this PR: dotnet/android-tools#284
Some example code reviews:
* dotnet/android-tools#283 (review)
* dotnet/android-tools#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`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

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

@rmarinho@jonathanpeppers