From cc1e063e8ea619cfb7089a6fa5f3c71e26850dc2 Mon Sep 17 00:00:00 2001 From: Rui Marinho Date: Wed, 18 Mar 2026 18:36:35 +0000 Subject: [PATCH 1/2] Fix AdbRunner AVD detection: use getprop first, skip offline emulators Two fixes for AdbRunner emulator detection reliability: 1. Swap AVD name detection order in GetEmulatorAvdNameAsync: use 'adb shell getprop ro.boot.qemu.avd_name' first (reliable on all modern emulators), fall back to 'adb emu avd name' for older versions. The 'emu avd name' command returns empty output on emulator 36.4.10+. 2. Skip AVD name queries for offline emulators in ListDevicesAsync: neither getprop nor 'emu avd name' works on offline devices, causing unnecessary delays during boot polling loops. Context: dotnet/android#10965 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Runners/AdbRunner.cs | 51 +++--- .../AdbRunnerTests.cs | 145 ++++++++++++++++++ 2 files changed, 173 insertions(+), 23 deletions(-) diff --git a/src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs b/src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs index 0dd60476..9f91b2e3 100644 --- a/src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs +++ b/src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbRunner.cs @@ -36,7 +36,7 @@ public class AdbRunner /// /// Full path to the adb executable (e.g., "/path/to/sdk/platform-tools/adb"). /// Optional environment variables to pass to adb processes. - /// Optional logger callback for diagnostic messages. + /// Optional logger callback receiving a and message string. public AdbRunner (string adbPath, IDictionary? environmentVariables = null, Action? logger = null) { if (string.IsNullOrWhiteSpace (adbPath)) @@ -48,7 +48,8 @@ public AdbRunner (string adbPath, IDictionary? environmentVariab /// /// Lists connected devices using 'adb devices -l'. - /// For emulators, queries the AVD name using 'adb -s <serial> emu avd name'. + /// For online emulators, queries the AVD name via getprop / emu avd name. + /// Offline emulators are included but without AVD names (querying them would fail). /// public virtual async Task> ListDevicesAsync (CancellationToken cancellationToken = default) { @@ -61,11 +62,15 @@ public virtual async Task> ListDevicesAsync (Cancel var devices = ParseAdbDevicesOutput (stdout.ToString ().Split ('\n')); - // For each emulator, try to get the AVD name + // For each online emulator, try to get the AVD name. + // Skip offline emulators — neither getprop nor 'emu avd name' work on them + // and attempting these commands causes unnecessary delays during boot polling. foreach (var device in devices) { - if (device.Type == AdbDeviceType.Emulator) { + if (device.Type == AdbDeviceType.Emulator && device.Status == AdbDeviceStatus.Online) { device.AvdName = await GetEmulatorAvdNameAsync (device.Serial, cancellationToken).ConfigureAwait (false); device.Description = BuildDeviceDescription (device); + } else if (device.Type == AdbDeviceType.Emulator) { + logger.Invoke (TraceLevel.Verbose, $"Skipping AVD name query for {device.Status} emulator {device.Serial}"); } } @@ -74,15 +79,26 @@ public virtual async Task> ListDevicesAsync (Cancel /// /// Queries the emulator for its AVD name. - /// Tries adb -s <serial> emu avd name first (emulator console protocol), - /// then falls back to adb shell getprop ro.boot.qemu.avd_name which reads the - /// boot property set by the emulator kernel. The fallback is needed because - /// emu avd name can return empty output on some adb/emulator version - /// combinations (observed with adb v36). + /// Tries adb shell getprop ro.boot.qemu.avd_name first (reliable on all modern + /// emulators), then falls back to adb -s <serial> emu avd name (emulator + /// console protocol) for older emulator versions. The emu avd name command returns + /// empty output on emulator 36.4.10+ (observed with adb v36), so getprop is the + /// preferred method. /// internal async Task GetEmulatorAvdNameAsync (string serial, CancellationToken cancellationToken = default) { - // Try 1: Console command (fast, works on most emulator versions) + // Try 1: Shell property (reliable on modern emulators, always set by the emulator kernel) + try { + var avdName = await GetShellPropertyAsync (serial, "ro.boot.qemu.avd_name", cancellationToken).ConfigureAwait (false); + if (avdName is { Length: > 0 } name && !string.IsNullOrWhiteSpace (name)) + return name.Trim (); + } catch (OperationCanceledException) { + throw; + } catch (Exception ex) { + logger.Invoke (TraceLevel.Warning, $"GetEmulatorAvdNameAsync: getprop failed for {serial}: {ex.Message}"); + } + + // Try 2: Console command (fallback for older emulators where getprop may not be available) try { using var stdout = new StringWriter (); var psi = ProcessUtils.CreateProcessStartInfo (adbPath, "-s", serial, "emu", "avd", "name"); @@ -97,19 +113,8 @@ public virtual async Task> ListDevicesAsync (Cancel } } catch (OperationCanceledException) { throw; - } catch { - // Fall through to getprop fallback - } - - // Try 2: Shell property (fallback when 'adb emu avd name' returns empty on some adb/emulator versions) - try { - var avdName = await GetShellPropertyAsync (serial, "ro.boot.qemu.avd_name", cancellationToken).ConfigureAwait (false); - if (avdName is { Length: > 0 } name && !string.IsNullOrWhiteSpace (name)) - return name.Trim (); - } catch (OperationCanceledException) { - throw; - } catch { - // Both methods failed + } catch (Exception ex) { + logger.Invoke (TraceLevel.Warning, $"GetEmulatorAvdNameAsync: both methods failed for {serial}: {ex.Message}"); } return null; diff --git a/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AdbRunnerTests.cs b/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AdbRunnerTests.cs index c9536cf9..f97f92c4 100644 --- a/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AdbRunnerTests.cs +++ b/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AdbRunnerTests.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.IO; using System.Linq; +using System.Threading.Tasks; using NUnit.Framework; namespace Xamarin.Android.Tools.Tests; @@ -714,4 +715,148 @@ public void FirstNonEmptyLine_PmPathOutput () var output = "package:/system/framework/framework-res.apk\n"; Assert.AreEqual ("package:/system/framework/framework-res.apk", AdbRunner.FirstNonEmptyLine (output)); } + + // --- GetEmulatorAvdNameAsync + ListDevicesAsync tests --- + // These tests use a fake 'adb' script to control process output, + // verifying AVD detection order and offline emulator handling. + + static string CreateFakeAdb (string scriptBody) + { + if (OS.IsWindows) + Assert.Ignore ("Fake adb tests use bash scripts and are not supported on Windows."); + + var dir = Path.Combine (Path.GetTempPath (), $"fake-adb-{Guid.NewGuid ():N}"); + Directory.CreateDirectory (dir); + var path = Path.Combine (dir, "adb"); + File.WriteAllText (path, "#!/bin/bash\n" + scriptBody); + + var psi = new System.Diagnostics.ProcessStartInfo ("chmod") { + ArgumentList = { "+x", path }, + }; + System.Diagnostics.Process.Start (psi)?.WaitForExit (); + + return path; + } + + static void CleanupFakeAdb (string adbPath) + { + var dir = Path.GetDirectoryName (adbPath); + File.Delete (adbPath); + if (dir != null) + Directory.Delete (dir); + } + + [Test] + public async Task GetEmulatorAvdNameAsync_PrefersGetprop () + { + // getprop returns a value — should be used, emu avd name should NOT be needed + var adbPath = CreateFakeAdb (""" + if [[ "$3" == "shell" && "$4" == "getprop" ]]; then + echo "My_AVD_Name" + exit 0 + fi + if [[ "$3" == "emu" ]]; then + echo "WRONG_NAME" + echo "OK" + exit 0 + fi + exit 1 + """); + + try { + var runner = new AdbRunner (adbPath); + var name = await runner.GetEmulatorAvdNameAsync ("emulator-5554"); + Assert.AreEqual ("My_AVD_Name", name, "Should return getprop result"); + } finally { + CleanupFakeAdb (adbPath); + } + } + + [Test] + public async Task GetEmulatorAvdNameAsync_FallsBackToEmuAvdName () + { + // getprop returns empty — should fall back to emu avd name + var adbPath = CreateFakeAdb (""" + if [[ "$3" == "shell" && "$4" == "getprop" ]]; then + echo "" + exit 0 + fi + if [[ "$3" == "emu" ]]; then + echo "Fallback_AVD" + echo "OK" + exit 0 + fi + exit 1 + """); + + try { + var runner = new AdbRunner (adbPath); + var name = await runner.GetEmulatorAvdNameAsync ("emulator-5554"); + Assert.AreEqual ("Fallback_AVD", name, "Should fall back to emu avd name"); + } finally { + CleanupFakeAdb (adbPath); + } + } + + [Test] + public async Task GetEmulatorAvdNameAsync_BothFail_ReturnsNull () + { + // Both getprop and emu avd name return empty + var adbPath = CreateFakeAdb (""" + echo "" + exit 0 + """); + + try { + var runner = new AdbRunner (adbPath); + var name = await runner.GetEmulatorAvdNameAsync ("emulator-5554"); + Assert.IsNull (name, "Should return null when both methods fail"); + } finally { + CleanupFakeAdb (adbPath); + } + } + + [Test] + public async Task ListDevicesAsync_SkipsAvdQueryForOfflineEmulators () + { + // adb devices returns one online emulator and one offline emulator. + // Only the online one should get an AVD name query. + var adbPath = CreateFakeAdb (""" + if [[ "$1" == "devices" ]]; then + echo "List of devices attached" + echo "emulator-5554 device product:sdk_gphone64_arm64 model:sdk_gphone64_arm64 device:emu64a transport_id:1" + echo "emulator-5556 offline" + exit 0 + fi + if [[ "$1" == "-s" && "$2" == "emulator-5554" && "$3" == "shell" && "$4" == "getprop" ]]; then + echo "Online_AVD" + exit 0 + fi + if [[ "$1" == "-s" && "$2" == "emulator-5556" ]]; then + # This should NOT be called for offline emulators. + # Return a name anyway so we can detect if it was incorrectly queried. + echo "OFFLINE_SHOULD_NOT_APPEAR" + exit 0 + fi + exit 1 + """); + + try { + var runner = new AdbRunner (adbPath); + var devices = await runner.ListDevicesAsync (); + + Assert.AreEqual (2, devices.Count, "Should return both emulators"); + + var online = devices.First (d => d.Serial == "emulator-5554"); + var offline = devices.First (d => d.Serial == "emulator-5556"); + + Assert.AreEqual (AdbDeviceStatus.Online, online.Status); + Assert.AreEqual ("Online_AVD", online.AvdName, "Online emulator should have AVD name"); + + Assert.AreEqual (AdbDeviceStatus.Offline, offline.Status); + Assert.IsNull (offline.AvdName, "Offline emulator should NOT have AVD name queried"); + } finally { + CleanupFakeAdb (adbPath); + } + } } From efbd59e19f717033f83be2c9c588c3f85187db01 Mon Sep 17 00:00:00 2001 From: Rui Marinho Date: Thu, 19 Mar 2026 15:37:03 +0000 Subject: [PATCH 2/2] Address review feedback: use FileUtil.Chmod, remove null-forgiving operators - Replace Process.Start("chmod") with FileUtil.Chmod(path, 0x1ED) which uses managed File.SetUnixFileMode on .NET 7+ and the chmod DllImport on older runtimes - Replace null-forgiving operator on Path.GetDirectoryName() with is { Length: > 0 } property pattern per repo conventions - Reorder CleanupFakeAdb to delete file only when dir is non-null Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AdbRunnerTests.cs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AdbRunnerTests.cs b/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AdbRunnerTests.cs index f97f92c4..49b56db6 100644 --- a/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AdbRunnerTests.cs +++ b/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AdbRunnerTests.cs @@ -729,11 +729,7 @@ static string CreateFakeAdb (string scriptBody) Directory.CreateDirectory (dir); var path = Path.Combine (dir, "adb"); File.WriteAllText (path, "#!/bin/bash\n" + scriptBody); - - var psi = new System.Diagnostics.ProcessStartInfo ("chmod") { - ArgumentList = { "+x", path }, - }; - System.Diagnostics.Process.Start (psi)?.WaitForExit (); + FileUtil.Chmod (path, 0x1ED); // 0755 return path; } @@ -741,9 +737,10 @@ static string CreateFakeAdb (string scriptBody) static void CleanupFakeAdb (string adbPath) { var dir = Path.GetDirectoryName (adbPath); - File.Delete (adbPath); - if (dir != null) + if (dir is { Length: > 0 }) { + File.Delete (adbPath); Directory.Delete (dir); + } } [Test]