Skip to content

Commit 25e7711

Browse files
rmarinhoCopilot
andcommitted
Simplify AdbRunner constructor: require full adb path
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>
1 parent 90ef8b5 commit 25e7711

3 files changed

Lines changed: 50 additions & 92 deletions

File tree

src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs

Lines changed: 23 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
using System.Diagnostics;
77
using System.Globalization;
88
using System.IO;
9-
using System.Linq;
109
using System.Net.Sockets;
1110
using System.Text.RegularExpressions;
1211
using System.Threading;
@@ -20,52 +19,28 @@ namespace Xamarin.Android.Tools;
2019
/// </summary>
2120
public class AdbRunner
2221
{
23-
readonly Func<string?> getSdkPath;
24-
readonly Func<string?>? getJdkPath;
22+
readonly string adbPath;
23+
readonly IDictionary<string, string>? environmentVariables;
2524

2625
// Pattern to match device lines: <serial> <state> [key:value ...]
27-
// Requires 2+ spaces between serial and state (adb pads serials).
28-
// Matches known adb device states. Uses \s+ to handle both space and tab separators.
26+
// Uses \s+ to handle both space and tab separators.
2927
// Explicit state list prevents false positives from non-device lines.
3028
static readonly Regex AdbDevicesRegex = new Regex (
3129
@"^([^\s]+)\s+(device|offline|unauthorized|authorizing|no permissions|recovery|sideload|bootloader|connecting|host)\s*(.*)$",
3230
RegexOptions.Compiled | RegexOptions.IgnoreCase);
3331
static readonly Regex ApiRegex = new Regex (@"\bApi\b", RegexOptions.Compiled);
3432

35-
public AdbRunner (Func<string?> getSdkPath)
36-
: this (getSdkPath, null)
37-
{
38-
}
39-
40-
public AdbRunner (Func<string?> getSdkPath, Func<string?>? getJdkPath)
41-
{
42-
this.getSdkPath = getSdkPath ?? throw new ArgumentNullException (nameof (getSdkPath));
43-
this.getJdkPath = getJdkPath;
44-
}
45-
46-
public string? AdbPath {
47-
get {
48-
var sdkPath = getSdkPath ();
49-
if (!string.IsNullOrEmpty (sdkPath)) {
50-
var ext = OS.IsWindows ? ".exe" : "";
51-
var sdkAdb = Path.Combine (sdkPath, "platform-tools", "adb" + ext);
52-
if (File.Exists (sdkAdb))
53-
return sdkAdb;
54-
}
55-
return ProcessUtils.FindExecutablesInPath ("adb").FirstOrDefault ();
56-
}
57-
}
58-
59-
public bool IsAvailable => AdbPath is not null;
60-
61-
string RequireAdb ()
62-
{
63-
return AdbPath ?? throw new InvalidOperationException ("ADB not found.");
64-
}
65-
66-
IDictionary<string, string> GetEnvironmentVariables ()
33+
/// <summary>
34+
/// Creates a new AdbRunner with the full path to the adb executable.
35+
/// </summary>
36+
/// <param name="adbPath">Full path to the adb executable (e.g., "/path/to/sdk/platform-tools/adb").</param>
37+
/// <param name="environmentVariables">Optional environment variables to pass to adb processes.</param>
38+
public AdbRunner (string adbPath, IDictionary<string, string>? environmentVariables = null)
6739
{
68-
return AndroidEnvironmentHelper.GetEnvironmentVariables (getSdkPath (), getJdkPath?.Invoke ());
40+
if (string.IsNullOrWhiteSpace (adbPath))
41+
throw new ArgumentException ("Path to adb must not be empty.", nameof (adbPath));
42+
this.adbPath = adbPath;
43+
this.environmentVariables = environmentVariables;
6944
}
7045

7146
/// <summary>
@@ -74,12 +49,10 @@ IDictionary<string, string> GetEnvironmentVariables ()
7449
/// </summary>
7550
public async Task<IReadOnlyList<AdbDeviceInfo>> ListDevicesAsync (CancellationToken cancellationToken = default)
7651
{
77-
var adb = RequireAdb ();
78-
var envVars = GetEnvironmentVariables ();
7952
using var stdout = new StringWriter ();
8053
using var stderr = new StringWriter ();
81-
var psi = ProcessUtils.CreateProcessStartInfo (adb, "devices", "-l");
82-
var exitCode = await ProcessUtils.StartProcess (psi, stdout, stderr, cancellationToken, envVars).ConfigureAwait (false);
54+
var psi = ProcessUtils.CreateProcessStartInfo (adbPath, "devices", "-l");
55+
var exitCode = await ProcessUtils.StartProcess (psi, stdout, stderr, cancellationToken, environmentVariables).ConfigureAwait (false);
8356

8457
ProcessUtils.ThrowIfFailed (exitCode, "adb devices -l", stderr.ToString ());
8558

@@ -88,7 +61,7 @@ public async Task<IReadOnlyList<AdbDeviceInfo>> ListDevicesAsync (CancellationTo
8861
// For each emulator, try to get the AVD name
8962
foreach (var device in devices) {
9063
if (device.Type == AdbDeviceType.Emulator) {
91-
device.AvdName = await GetEmulatorAvdNameAsync (adb, device.Serial, cancellationToken).ConfigureAwait (false);
64+
device.AvdName = await GetEmulatorAvdNameAsync (device.Serial, cancellationToken).ConfigureAwait (false);
9265
device.Description = BuildDeviceDescription (device);
9366
}
9467
}
@@ -101,13 +74,12 @@ public async Task<IReadOnlyList<AdbDeviceInfo>> ListDevicesAsync (CancellationTo
10174
/// falling back to a direct emulator console TCP query if that fails.
10275
/// Ported from dotnet/android GetAvailableAndroidDevices.GetEmulatorAvdName.
10376
/// </summary>
104-
internal async Task<string?> GetEmulatorAvdNameAsync (string adbPath, string serial, CancellationToken cancellationToken = default)
77+
internal async Task<string?> GetEmulatorAvdNameAsync (string serial, CancellationToken cancellationToken = default)
10578
{
10679
try {
107-
var envVars = GetEnvironmentVariables ();
10880
using var stdout = new StringWriter ();
10981
var psi = ProcessUtils.CreateProcessStartInfo (adbPath, "-s", serial, "emu", "avd", "name");
110-
await ProcessUtils.StartProcess (psi, stdout, null, cancellationToken, envVars).ConfigureAwait (false);
82+
await ProcessUtils.StartProcess (psi, stdout, null, cancellationToken, environmentVariables).ConfigureAwait (false);
11183

11284
foreach (var line in stdout.ToString ().Split ('\n')) {
11385
var trimmed = line.Trim ();
@@ -187,14 +159,11 @@ public async Task WaitForDeviceAsync (string? serial = null, TimeSpan? timeout =
187159
if (effectiveTimeout <= TimeSpan.Zero)
188160
throw new ArgumentOutOfRangeException (nameof (timeout), effectiveTimeout, "Timeout must be a positive value.");
189161

190-
var adb = RequireAdb ();
191-
var envVars = GetEnvironmentVariables ();
192-
193162
var args = string.IsNullOrEmpty (serial)
194163
? new [] { "wait-for-device" }
195-
: new [] { "-s", serial!, "wait-for-device" };
164+
: new [] { "-s", serial, "wait-for-device" };
196165

197-
var psi = ProcessUtils.CreateProcessStartInfo (adb, args);
166+
var psi = ProcessUtils.CreateProcessStartInfo (adbPath, args);
198167

199168
using var cts = CancellationTokenSource.CreateLinkedTokenSource (cancellationToken);
200169
cts.CancelAfter (effectiveTimeout);
@@ -203,7 +172,7 @@ public async Task WaitForDeviceAsync (string? serial = null, TimeSpan? timeout =
203172
using var stderr = new StringWriter ();
204173

205174
try {
206-
var exitCode = await ProcessUtils.StartProcess (psi, stdout, stderr, cts.Token, envVars).ConfigureAwait (false);
175+
var exitCode = await ProcessUtils.StartProcess (psi, stdout, stderr, cts.Token, environmentVariables).ConfigureAwait (false);
207176
ProcessUtils.ThrowIfFailed (exitCode, "adb wait-for-device", stderr.ToString (), stdout.ToString ());
208177
} catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) {
209178
throw new TimeoutException ($"Timed out waiting for device after {effectiveTimeout.TotalSeconds}s.");
@@ -215,11 +184,9 @@ public async Task StopEmulatorAsync (string serial, CancellationToken cancellati
215184
if (string.IsNullOrWhiteSpace (serial))
216185
throw new ArgumentException ("Serial must not be empty.", nameof (serial));
217186

218-
var adb = RequireAdb ();
219-
var envVars = GetEnvironmentVariables ();
220-
var psi = ProcessUtils.CreateProcessStartInfo (adb, "-s", serial, "emu", "kill");
187+
var psi = ProcessUtils.CreateProcessStartInfo (adbPath, "-s", serial, "emu", "kill");
221188
using var stderr = new StringWriter ();
222-
var exitCode = await ProcessUtils.StartProcess (psi, null, stderr, cancellationToken, envVars).ConfigureAwait (false);
189+
var exitCode = await ProcessUtils.StartProcess (psi, null, stderr, cancellationToken, environmentVariables).ConfigureAwait (false);
223190
ProcessUtils.ThrowIfFailed (exitCode, $"adb -s {serial} emu kill", stderr.ToString ());
224191
}
225192

tests/Xamarin.Android.Tools.AndroidSdk-Tests/AdbRunnerTests.cs

Lines changed: 12 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
using System;
45
using System.Collections.Generic;
56
using System.IO;
67
using NUnit.Framework;
@@ -486,32 +487,21 @@ public void MergeDevicesAndEmulators_EmptyAdbDevices_ReturnsAllAvailable ()
486487
// Consumer: MAUI DevTools Adb provider (AdbPath, IsAvailable properties)
487488

488489
[Test]
489-
public void AdbPath_FindsInSdk ()
490+
public void Constructor_NullPath_ThrowsArgumentException ()
490491
{
491-
var tempDir = Path.Combine (Path.GetTempPath (), $"adb-test-{Path.GetRandomFileName ()}");
492-
var platformTools = Path.Combine (tempDir, "platform-tools");
493-
Directory.CreateDirectory (platformTools);
494-
495-
try {
496-
var adbName = OS.IsWindows ? "adb.exe" : "adb";
497-
File.WriteAllText (Path.Combine (platformTools, adbName), "");
498-
499-
var runner = new AdbRunner (() => tempDir);
492+
Assert.Throws<ArgumentException> (() => new AdbRunner (null!));
493+
}
500494

501-
Assert.IsNotNull (runner.AdbPath);
502-
Assert.IsTrue (runner.IsAvailable);
503-
Assert.IsTrue (runner.AdbPath!.Contains ("platform-tools"));
504-
} finally {
505-
Directory.Delete (tempDir, true);
506-
}
495+
[Test]
496+
public void Constructor_EmptyPath_ThrowsArgumentException ()
497+
{
498+
Assert.Throws<ArgumentException> (() => new AdbRunner (""));
507499
}
508500

509501
[Test]
510-
public void AdbPath_NullSdkPath_StillSearchesPath ()
502+
public void Constructor_WhitespacePath_ThrowsArgumentException ()
511503
{
512-
var runner = new AdbRunner (() => null);
513-
// Should not throw — falls back to PATH search
514-
_ = runner.AdbPath;
504+
Assert.Throws<ArgumentException> (() => new AdbRunner (" "));
515505
}
516506

517507
[Test]
@@ -627,15 +617,15 @@ public void MapAdbStateToStatus_Sideload_ReturnsUnknown ()
627617
[Test]
628618
public void WaitForDeviceAsync_NegativeTimeout_ThrowsArgumentOutOfRange ()
629619
{
630-
var runner = new AdbRunner (() => "/fake/sdk");
620+
var runner = new AdbRunner ("/fake/sdk/platform-tools/adb");
631621
Assert.ThrowsAsync<System.ArgumentOutOfRangeException> (
632622
async () => await runner.WaitForDeviceAsync (timeout: System.TimeSpan.FromSeconds (-1)));
633623
}
634624

635625
[Test]
636626
public void WaitForDeviceAsync_ZeroTimeout_ThrowsArgumentOutOfRange ()
637627
{
638-
var runner = new AdbRunner (() => "/fake/sdk");
628+
var runner = new AdbRunner ("/fake/sdk/platform-tools/adb");
639629
Assert.ThrowsAsync<System.ArgumentOutOfRangeException> (
640630
async () => await runner.WaitForDeviceAsync (timeout: System.TimeSpan.Zero));
641631
}

tests/Xamarin.Android.Tools.AndroidSdk-Tests/RunnerIntegrationTests.cs

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ public class RunnerIntegrationTests
2525
{
2626
static string sdkPath;
2727
static string jdkPath;
28+
static string adbPath;
2829
static SdkManager sdkManager;
2930
static string bootstrappedSdkPath;
3031

@@ -88,6 +89,12 @@ public async Task OneTimeSetUp ()
8889
sdkManager.JavaSdkPath = jdkPath;
8990
sdkManager.AndroidSdkPath = sdkPath;
9091
}
92+
93+
// Resolve the full path to adb for AdbRunner
94+
var adbExe = OS.IsWindows ? "adb.exe" : "adb";
95+
adbPath = Path.Combine (sdkPath, "platform-tools", adbExe);
96+
if (!File.Exists (adbPath))
97+
Assert.Ignore ($"adb not found at {adbPath}");
9198
}
9299

93100
[OneTimeTearDown]
@@ -112,19 +119,16 @@ public void OneTimeTearDown ()
112119
// ── AdbRunner integration ──────────────────────────────────────
113120

114121
[Test]
115-
public void AdbRunner_IsAvailable_WithSdk ()
122+
public void AdbRunner_Constructor_AcceptsValidPath ()
116123
{
117-
var runner = new AdbRunner (() => sdkPath);
118-
119-
Assert.IsTrue (runner.IsAvailable, "AdbRunner should find adb in SDK");
120-
Assert.IsNotNull (runner.AdbPath);
121-
Assert.IsTrue (File.Exists (runner.AdbPath), $"adb binary should exist at {runner.AdbPath}");
124+
var runner = new AdbRunner (adbPath);
125+
Assert.IsNotNull (runner);
122126
}
123127

124128
[Test]
125129
public async Task AdbRunner_ListDevicesAsync_ReturnsWithoutError ()
126130
{
127-
var runner = new AdbRunner (() => sdkPath);
131+
var runner = new AdbRunner (adbPath);
128132

129133
// On CI there are no physical devices or emulators, but the command
130134
// should succeed and return an empty (or non-null) list.
@@ -137,9 +141,7 @@ public async Task AdbRunner_ListDevicesAsync_ReturnsWithoutError ()
137141
[Test]
138142
public void AdbRunner_WaitForDeviceAsync_TimesOut_WhenNoDevice ()
139143
{
140-
var runner = new AdbRunner (() => sdkPath);
141-
142-
// With no devices connected, wait-for-device should time out
144+
var runner = new AdbRunner (adbPath);
143145
var ex = Assert.ThrowsAsync<TimeoutException> (async () =>
144146
await runner.WaitForDeviceAsync (timeout: TimeSpan.FromSeconds (5)));
145147

@@ -152,11 +154,10 @@ public void AdbRunner_WaitForDeviceAsync_TimesOut_WhenNoDevice ()
152154
[Test]
153155
public void AllRunners_ToolDiscovery_ConsistentWithSdk ()
154156
{
155-
var adb = new AdbRunner (() => sdkPath);
156-
157-
Assert.IsTrue (adb.IsAvailable, "adb should be available");
157+
var runner = new AdbRunner (adbPath);
158158

159159
// adb path should be under the SDK
160-
StringAssert.StartsWith (sdkPath, adb.AdbPath!);
160+
Assert.IsTrue (File.Exists (adbPath), $"adb should exist at {adbPath}");
161+
StringAssert.StartsWith (sdkPath, adbPath);
161162
}
162163
}

0 commit comments

Comments
 (0)