Add AdbRunner for adb CLI operations#283
Conversation
|
I'd like to get the System.Diagnostics.Process code unified like mentioned here: |
Addresses PR #283 feedback to use existing ProcessUtils instead of the removed AndroidToolRunner. Simplifies API: - Methods now throw InvalidOperationException on failure - Uses ProcessUtils.RunToolAsync() for all tool invocations - Added AndroidDeviceInfo model - Removed complex ToolRunnerResult wrapper types Co-authored-by: Copilot <[email protected]>
bb947a5 to
b08dd3f
Compare
Addresses PR #283/#284 feedback to use existing ProcessUtils. Simplifies API by throwing exceptions on failure instead of returning result types with error states. Changes: - AdbRunner: Simplified using ProcessUtils.RunToolAsync() - EmulatorRunner: Uses ProcessUtils.StartToolBackground() - Removed duplicate AndroidDeviceInfo from Models directory Co-authored-by: Copilot <[email protected]>
d378294 to
ec0675f
Compare
923285f to
1cf8fc6
Compare
|
Implemented your suggested approach:
Next steps per your plan:
|
|
The dotnet/android side is now ready as a draft PR: dotnet/android#10880 It delegates Workflow:
|
src/Xamarin.Android.Tools.AndroidSdk/Runners/AndroidEnvironmentHelper.cs
Show resolved
Hide resolved
193203e to
5f9a212
Compare
…eout
- Broaden AdbDevicesRegex to match any device state (recovery, sideload, etc.)
using \s{2,} separator to avoid matching random text lines
- Skip daemon startup lines (starting with *) in ParseAdbDevicesOutput
- ListDevicesAsync now captures stderr and throws on non-zero exit code
- WaitForDeviceAsync now checks exit code and throws with stdout/stderr context
- Validate timeout: reject negative and zero TimeSpan values
- Add 6 tests: recovery/sideload parsing, state mapping, timeout validation
Co-authored-by: Copilot <[email protected]>
…eout
- Broaden AdbDevicesRegex to match any device state (recovery, sideload, etc.)
using \s{2,} separator to avoid matching random text lines
- Skip daemon startup lines (starting with *) in ParseAdbDevicesOutput
- ListDevicesAsync now captures stderr and throws on non-zero exit code
- WaitForDeviceAsync now checks exit code and throws with stdout/stderr context
- Validate timeout: reject negative and zero TimeSpan values
- Add 6 tests: recovery/sideload parsing, state mapping, timeout validation
Co-authored-by: Copilot <[email protected]>
be56ade to
1d82e36
Compare
Port adb device parsing, formatting, and merging logic from dotnet/android GetAvailableAndroidDevices MSBuild task into a shared AdbRunner class. New files: - AdbRunner.cs — ListDevicesAsync, WaitForDeviceAsync, StopEmulatorAsync, ParseAdbDevicesOutput, BuildDeviceDescription, FormatDisplayName, MapAdbStateToStatus, MergeDevicesAndEmulators - AdbDeviceInfo/AdbDeviceType/AdbDeviceStatus — device models - AndroidEnvironmentHelper — shared env var builder for all runners - ProcessUtils.ThrowIfFailed — shared exit code validation Modified files: - EnvironmentVariableNames — add ANDROID_USER_HOME, ANDROID_AVD_HOME - SdkManager.Process.cs — deduplicate env var logic via AndroidEnvironmentHelper Tests: - 43 unit tests (parsing, formatting, merging, path discovery, timeout) - 5 integration tests (CI-only, real SDK tools) Co-authored-by: Copilot <[email protected]>
- StopEmulatorAsync now captures stderr and calls ThrowIfFailed for consistency with ListDevicesAsync/WaitForDeviceAsync. - ThrowIfFailed changed from public to internal since it is only used within the library. - Remove inaccurate cmdline-tools bootstrap claim from integration test doc comment. Co-authored-by: Copilot <[email protected]>
Avoids allocating a joined string when callers already have individual lines (e.g., MSBuild LogEventsFromTextOutput). The existing string overload now delegates to the new one. Addresses review feedback from @jonathanpeppers on dotnet/android#10880. Co-authored-by: Copilot <[email protected]>
…esAndEmulators Accepts Action<TraceLevel, string> to route debug messages through the caller's logging infrastructure (e.g., MSBuild TaskLoggingHelper). Restores log messages lost when logic moved from dotnet/android to android-tools: AVD name formatting, running emulator detection, and non-running emulator additions. Follows the existing CreateTaskLogger pattern used by JdkInstaller. Co-authored-by: Copilot <[email protected]>
dotnet/android compiles the submodule as netstandard2.0 with WarningsAsErrors=Nullable. In netstandard2.0, string.IsNullOrEmpty lacks [NotNullWhen(false)], so the compiler doesn't narrow string? to string after null checks. Add null-forgiving operators where the preceding guard guarantees non-null. Fixes: CS8601 in AndroidEnvironmentHelper.cs (sdkPath, jdkPath) Fixes: CS8620 in AdbRunner.cs (serial in string[] array literal) Co-authored-by: Copilot <[email protected]>
- Replace \s{2,} with \s+ to handle tab-separated adb output
- Use explicit state list (device|offline|unauthorized|etc.) instead
of \S+ to prevent false positives from non-device lines
- Add ParseAdbDevicesOutput_TabSeparator test
Co-authored-by: Copilot <[email protected]>
Address review feedback: replace bare catch with catch(Exception ex) and log via Trace.WriteLine for debuggability. Co-authored-by: Copilot <[email protected]>
When 'adb emu avd name' fails (common on macOS), fall back to querying the emulator console directly via TCP on the console port extracted from the serial (emulator-XXXX -> port XXXX). This fixes duplicate device entries when running emulators can't be matched with their AVD definitions. Co-authored-by: Copilot <[email protected]>
Address review feedback (threads 41-43): replace Func<string?> getSdkPath constructor with string adbPath that takes the full path to the adb executable. Remove AdbPath property, IsAvailable property, RequireAdb(), PATH discovery fallback, and getSdkPath/getJdkPath fields. Callers are now responsible for resolving the adb path before constructing. Environment variables can optionally be passed via the constructor. Co-authored-by: Copilot <[email protected]>
…seAdbDevicesOutput Thread 46: Add ProcessUtils.ThrowIfFailed(int, string, StringWriter?, StringWriter?) overload that delegates to the string version. Update AdbRunner callers to pass StringWriter directly instead of calling .ToString() at each call site. Thread 47: Remove ParseAdbDevicesOutput(string) overload. Callers now split the string themselves and pass IEnumerable<string> directly. This removes the dual-signature confusion and aligns with dotnet/android's usage pattern. Co-authored-by: Copilot <[email protected]>
…pression patterns that give the compiler proper non-null flow on netstandard2.0. - Convert MapAdbStateToStatus from switch statement to switch expression. - Update copilot-instructions.md with both guidelines for future PRs. Co-authored-by: Copilot <[email protected]>
- RequireCi(): check for truthy value (case-insensitive 'true') instead of just variable presence - Remove SDK bootstrap fallback; Assert.Ignore when ANDROID_HOME missing to avoid flaky network-dependent CI runs - Remove section separator comments (region-style anti-pattern) - Fix regex comment to match actual \s+ behavior (1+ whitespace) - Replace null-forgiving ex! with ex?.Message pattern - Remove unused usings and bootstrappedSdkPath field Co-authored-by: Copilot <[email protected]>
e66b6d4 to
da3000c
Compare
There was a problem hiding this comment.
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.
Co-authored-by: Copilot <[email protected]>
Remove the TCP console fallback for AVD name queries as requested in review. The adb shell approach is sufficient; if it returns empty the method now simply returns null. Co-authored-by: Copilot <[email protected]>
…Async
Use 'serial is { Length: > 0 } s' pattern to avoid string?[] → string[]
nullability mismatch when building with dotnet/android WarningsAsErrors.
Co-authored-by: Copilot <[email protected]>
…g, ThrowIfFailed overload Apply review patterns from PR #283 (AdbRunner): - Constructor takes resolved 'string avdManagerPath' instead of Func<string?> delegates - Add IDictionary<string, string>? environmentVariables for ANDROID_HOME/JAVA_HOME - Remove AvdManagerPath property, IsAvailable, RequireAvdManagerPath(), ConfigureEnvironment() - Use 'is { Length: > 0 }' pattern matching for null/empty checks - Add ThrowIfFailed(StringWriter) overload to ProcessUtils - Change ThrowIfFailed/ValidateNotNullOrEmpty/FindCmdlineTool to internal visibility - Update tests: FindCmdlineTool tests replace AvdManagerPath tests, add constructor validation Co-authored-by: Copilot <[email protected]>
jonathanpeppers
left a comment
There was a problem hiding this comment.
🤖 AI Review Summary
Found 3 issues: 2 API design, 1 documentation accuracy.
- API design:
ParseAdbDevicesOutputandMergeDevicesAndEmulatorsreturnList<T>instead ofIReadOnlyList<T>(AdbRunner.cs:142,264) - Documentation:
GetEmulatorAvdNameAsyncdoc comment mentions TCP console fallback that isn't implemented (AdbRunner.cs:74)
👍 Positives:
- Excellent
OperationCanceledExceptionhandling — caught and rethrown before the generalcatch (Exception)inGetEmulatorAvdNameAsync, and thewhenclause inWaitForDeviceAsynccorrectly distinguishes timeout from caller cancellation. - Consistent exit code checking via
ProcessUtils.ThrowIfFailedacross all three async methods. - All process creation goes through
ProcessUtils.CreateProcessStartInfowith 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!. CancellationTokenproperly 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.
Resolves merge conflicts: keeps canonical AdbRunner from #283, adds shell methods and virtual ListDevicesAsync for EmulatorRunner BootAndWait, keeps updated AvdManagerRunner with resolved-path constructor from #282. Co-authored-by: Copilot <[email protected]>
Delegates adb devices parsing, description building, and device/emulator merging from GetAvailableAndroidDevices to AdbRunner in the shared xamarin-android-tools submodule. Removes ~200 lines of duplicated logic. - ParseAdbDevicesOutput accepts IEnumerable<string> to avoid string.Join - BuildDeviceDescription/MergeDevicesAndEmulators accept optional Action<TraceLevel, string> logger for MSBuild diagnostics - Tests updated to use AdbRunner/AdbDeviceInfo directly Depends on dotnet/android-tools#283. Co-authored-by: Copilot <[email protected]>
…ulators, fix stale doc - ParseAdbDevicesOutput: List<AdbDeviceInfo> → IReadOnlyList<AdbDeviceInfo> - MergeDevicesAndEmulators: List<AdbDeviceInfo> → IReadOnlyList<AdbDeviceInfo> - GetEmulatorAvdNameAsync: remove stale TCP fallback reference from doc comment - Tests: TrueForAll → LINQ All (IReadOnlyList compatible) Co-authored-by: Copilot <[email protected]>
jonathanpeppers
left a comment
There was a problem hiding this comment.
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.
Summary
Wraps
adbCLI operations for device management. Addresses #279.Parsing/formatting/merging logic ported from
dotnet/androidGetAvailableAndroidDevicesMSBuild task, enabling code sharing via theexternal/xamarin-android-toolssubmodule. See draft PR: dotnet/android#10880Public API
Internal methods (not part of public API):
GetEmulatorAvdNameAsync— queries AVD name viaadb emu avd namewith TCP console fallbackProcessUtils.ThrowIfFailed— shared exit code validation (string and StringWriter overloads)Key Design Decisions
string adbPath: Callers pass the resolved path; no lazyFunc<>indirection. OptionalenvironmentVariablesdictionary for ANDROID_HOME/JAVA_HOME/PATH.public staticsodotnet/androidcan call them without instantiatingAdbRunner(e.g.,GetAvailableAndroidDevicesMSBuild task passesList<string>toParseAdbDevicesOutput)IEnumerable<string>overload:dotnet/androidpassesList<string>directly from output linesBuildDeviceDescriptionandMergeDevicesAndEmulatorsacceptAction<TraceLevel, string>?—dotnet/androidpassesthis.CreateTaskLogger()for MSBuild trace output\s+separator to match one or more whitespace characters (spaces or tabs). Matches explicit known states withIgnoreCase. Daemon startup lines (*) are pre-filtered.ListDevicesAsync,WaitForDeviceAsync, andStopEmulatorAsyncthrowInvalidOperationExceptionwith stderr context on non-zero exit viaProcessUtils.ThrowIfFailed(internal)MapAdbStateToStatusas switch expression: Simple value mapping uses C# switch expression for concisenessis { Length: > 0 }patterns throughout for null checks onnetstandard2.0wherestring.IsNullOrEmpty()lacks[NotNullWhen(false)]ToTitleCaseto normalize mixed-case input (e.g., "PiXeL" → "Pixel")StartProcess: Runners pass env vars dictionary toProcessUtils.StartProcess.AndroidEnvironmentHelper.GetEnvironmentVariables()builds the dict.Tests
45 unit tests (
AdbRunnerTests.cs):4 integration tests (
RunnerIntegrationTests.cs):TF_BUILD=TrueorCI=true(case-insensitive truthy check), skipped locallyJAVA_HOME) and Android SDK (ANDROID_HOME) on CI agentAssert.IgnorewhenANDROID_HOMEmissing (no bootstrap/network dependency)ListDevicesAsync,WaitForDeviceAsynctimeout, tool discoveryReview Feedback Addressed
string adbPath25e7711Func<>withstring adbPath, remove lazy resolution2dac552,93aca4b,b9be955a42544fProcessUtils.ThrowIfFailed, visibility adjustmentseb63cb5\s+separator, explicit state list,IgnoreCasee689e82dotnet/androidWarningsAsErrors=Nullablecompatibilityb9be955Action<TraceLevel, string>?on BuildDeviceDescription + MergeDevicesAndEmulatorsIEnumerable<string>parse overload93aca4bList<string>directlyf68600dGetEmulatorAvdNameAsynclogs viaTrace.WriteLine90ef8b5adb emu avd namefailsThrowIfFailedStringWriter overloadb4d9a5fIEnumerable<string>parse method!with patterns850cb39is { Length: > 0 }patterns;MapAdbStateToStatus→ switch expressionda3000c// ── Section ──region-style commentsRequireCi()truthy checkda3000cstring.Equals("true", OrdinalIgnoreCase)instead of presence checkda3000cAssert.IgnorewhenANDROID_HOMEmissing — no network-dependent SDK downloadda3000c\s+behavior (1+ whitespace)72291e2is { Length: > 0 }in ProcessUtils, improved netstandard2.0 commentCo-authored-by: Copilot [email protected]