AdbRunner: follow-ups from PR #351
- Dominant language
- C#
- Stars
- 2.1k
- Forks
- 579
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 257
Description
Tracking the items that were deliberately deferred from dotnet/android-tools#351 (`[AdbRunner] Add ADB forward port management`) so they don't get lost.
## Background
PR dotnet/android-tools#351 added `ForwardPortAsync` / `RemoveForwardPortAsync` / `RemoveAllForwardPortsAsync` / `ListForwardPortsAsync` to `AdbRunner`, mirroring the reverse-port API added in dotnet/android-tools#305. During review, [jonathanpeppers raised the question](https://github.com/dotnet/android-tools/pull/351#discussion_r3228624449) of whether these should use the in-process `AdbClient` from dotnet/android-tools#327 instead of shelling out to `adb`. The decision was to keep shelling out for dotnet/android-tools#351 (to stay symmetric with the merged reverse implementation, and to avoid coupling dotnet/android-tools#351 to an unmerged dependency), and capture the broader migration here.
## Items
### 1. Stdout parity for reverse-port methods
`ReversePortAsync`, `RemoveReversePortAsync`, and `RemoveAllReversePortsAsync` (`src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs`, lines ~267, 289, 304) still pass `null` for stdout to `ProcessUtils.StartProcess` and only `stderr` to `ProcessUtils.ThrowIfFailed`. After dotnet/android-tools#351, the parallel forward methods capture stdout and pass it to `ThrowIfFailed` as well, which matches the repo convention ("Include stdout in error diagnostics") and yields richer failure messages because `adb` sometimes writes errors to stdout. Bring the three reverse methods in line.
### 2. Migrate `AdbRunner` to use `AdbClient` once dotnet/android-tools#327 merges
After dotnet/android-tools#327 (in-process `AdbClient` for the ADB daemon socket protocol) lands, do a single consistent pass that switches the existing process-based call sites in `AdbRunner` to use `AdbClient`:
- `ListDevicesAsync` → `host:devices-l` (already used by `AdbDeviceTracker`)
- `GetShellPropertyAsync` / `GetEmulatorAvdNameAsync` → `host:transport:` + `shell:`
- Forward operations → `host-serial::forward:…` / `killforward` / `killforward-all` / `host:list-forward`
- Reverse operations → `host:transport:` + `reverse:forward:…` / `killforward` / `killforward-all` / `list-forward`
Open question to resolve as part of the migration: how to preserve the implicit `adb start-server` behaviour that shelling out gives us today. A bare `connect("127.0.0.1", 5037)` against a cold daemon will throw `SocketException`. Options include shelling out to `adb start-server` once on `AdbClient` failure, or documenting that callers must ensure the daemon is running.
### 3. Cross-platform failure-path tests
The current `AdbRunner` failure-path tests (e.g. `GetEmulatorAvdNameAsync_*`) rely on a fake `adb` bash script and are skipped on Windows (4 skips). The migration in item 2 enables driving these tests with an in-memory loopback `TcpListener` instead, which would run on all platforms. Add tests covering at minimum:
- `ForwardPortAsync` failure surfaces stdout in the thrown `InvalidOperationException.Message` (lock-in for the change made in dotnet/android-tools#351).
- The reverse equivalents once item 1 lands.
cc @jonathanpeppers @rmarinho
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.