Skip to content

Commit c56c66b

Browse files
rmarinhoCopilot
andcommitted
Address PR #282 review feedback
- Fix argument validation: CreateAvdAsync/DeleteAvdAsync now throw ArgumentException for empty strings, ArgumentNullException for null, and validate before RequireAvdManagerPath() - Make AndroidEnvironmentHelper internal, remove unused GetToolEnvironment - Add ANDROID_USER_HOME to ConfigureEnvironment for consistent AVD location across tools (matches SdkManager behavior) - Add AndroidUserHome constant to EnvironmentVariableNames - Fix hard-coded AVD directory: use GetAvdRootDirectory() that respects ANDROID_AVD_HOME and ANDROID_USER_HOME env vars - Re-list after CreateAvdAsync to get actual path from avdmanager - Add 5 new validation tests for Create/DeleteAvdAsync Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 0b54ff1 commit c56c66b

4 files changed

Lines changed: 93 additions & 32 deletions

File tree

src/Xamarin.Android.Tools.AndroidSdk/EnvironmentVariableNames.cs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,5 +40,11 @@ internal static class EnvironmentVariableNames
4040
/// Executable file extensions (Windows).
4141
/// </summary>
4242
public const string PathExt = "PATHEXT";
43+
44+
/// <summary>
45+
/// Overrides the default location for Android user-specific data
46+
/// (AVDs, preferences, etc.). Defaults to $HOME/.android.
47+
/// </summary>
48+
public const string AndroidUserHome = "ANDROID_USER_HOME";
4349
}
4450
}

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

Lines changed: 8 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -11,35 +11,12 @@ namespace Xamarin.Android.Tools;
1111
/// <summary>
1212
/// Helper for setting up environment variables for Android SDK tools.
1313
/// </summary>
14-
public static class AndroidEnvironmentHelper
14+
internal static class AndroidEnvironmentHelper
1515
{
16-
/// <summary>
17-
/// Creates environment variables for running Android SDK tools.
18-
/// </summary>
19-
public static Dictionary<string, string>? GetToolEnvironment (string? sdkPath, string? jdkPath)
20-
{
21-
if (string.IsNullOrEmpty (sdkPath) && string.IsNullOrEmpty (jdkPath))
22-
return null;
23-
24-
var env = new Dictionary<string, string> (StringComparer.OrdinalIgnoreCase);
25-
26-
if (!string.IsNullOrEmpty (sdkPath))
27-
env [EnvironmentVariableNames.AndroidHome] = sdkPath;
28-
29-
if (!string.IsNullOrEmpty (jdkPath)) {
30-
env [EnvironmentVariableNames.JavaHome] = jdkPath;
31-
var jdkBin = Path.Combine (jdkPath, "bin");
32-
var currentPath = Environment.GetEnvironmentVariable (EnvironmentVariableNames.Path) ?? "";
33-
env [EnvironmentVariableNames.Path] = string.IsNullOrEmpty (currentPath) ? jdkBin : jdkBin + Path.PathSeparator + currentPath;
34-
}
35-
36-
return env;
37-
}
38-
3916
/// <summary>
4017
/// Configures environment variables on a ProcessStartInfo for running Android SDK tools.
4118
/// </summary>
42-
public static void ConfigureEnvironment (ProcessStartInfo psi, string? sdkPath, string? jdkPath)
19+
internal static void ConfigureEnvironment (ProcessStartInfo psi, string? sdkPath, string? jdkPath)
4320
{
4421
if (!string.IsNullOrEmpty (sdkPath))
4522
psi.EnvironmentVariables [EnvironmentVariableNames.AndroidHome] = sdkPath;
@@ -50,5 +27,11 @@ public static void ConfigureEnvironment (ProcessStartInfo psi, string? sdkPath,
5027
var currentPath = psi.EnvironmentVariables [EnvironmentVariableNames.Path] ?? "";
5128
psi.EnvironmentVariables [EnvironmentVariableNames.Path] = string.IsNullOrEmpty (currentPath) ? jdkBin : jdkBin + Path.PathSeparator + currentPath;
5229
}
30+
31+
// Set ANDROID_USER_HOME for consistent AVD location across tools (matches SdkManager behavior)
32+
if (!psi.EnvironmentVariables.ContainsKey (EnvironmentVariableNames.AndroidUserHome)) {
33+
psi.EnvironmentVariables [EnvironmentVariableNames.AndroidUserHome] = Path.Combine (
34+
Environment.GetFolderPath (Environment.SpecialFolder.UserProfile), ".android");
35+
}
5336
}
5437
}

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

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -91,11 +91,16 @@ public async Task<IReadOnlyList<AvdInfo>> ListAvdsAsync (CancellationToken cance
9191
public async Task<AvdInfo> CreateAvdAsync (string name, string systemImage, string? deviceProfile = null,
9292
bool force = false, CancellationToken cancellationToken = default)
9393
{
94-
var avdManagerPath = RequireAvdManagerPath ();
95-
if (string.IsNullOrEmpty (name))
94+
if (name is null)
9695
throw new ArgumentNullException (nameof (name));
97-
if (string.IsNullOrEmpty (systemImage))
96+
if (name.Length == 0)
97+
throw new ArgumentException ("Value cannot be an empty string.", nameof (name));
98+
if (systemImage is null)
9899
throw new ArgumentNullException (nameof (systemImage));
100+
if (systemImage.Length == 0)
101+
throw new ArgumentException ("Value cannot be an empty string.", nameof (systemImage));
102+
103+
var avdManagerPath = RequireAvdManagerPath ();
99104

100105
// Check if AVD already exists — return it instead of failing
101106
if (!force) {
@@ -106,9 +111,7 @@ public async Task<AvdInfo> CreateAvdAsync (string name, string systemImage, stri
106111
}
107112

108113
// Detect orphaned AVD directory (folder exists without .ini registration).
109-
var avdDir = Path.Combine (
110-
Environment.GetFolderPath (Environment.SpecialFolder.UserProfile),
111-
".android", "avd", $"{name}.avd");
114+
var avdDir = Path.Combine (GetAvdRootDirectory (), $"{name}.avd");
112115
if (Directory.Exists (avdDir))
113116
force = true;
114117

@@ -142,15 +145,27 @@ public async Task<AvdInfo> CreateAvdAsync (string name, string systemImage, stri
142145
throw new InvalidOperationException ($"Failed to create AVD '{name}': {errorOutput}");
143146
}
144147

148+
// Re-list to get the actual path from avdmanager (respects ANDROID_USER_HOME/ANDROID_AVD_HOME)
149+
var avds = await ListAvdsAsync (cancellationToken).ConfigureAwait (false);
150+
var created = avds.FirstOrDefault (a => string.Equals (a.Name, name, StringComparison.OrdinalIgnoreCase));
151+
if (created is not null)
152+
return created;
153+
154+
// Fallback if re-list didn't find it
145155
return new AvdInfo {
146156
Name = name,
147157
DeviceProfile = deviceProfile,
148-
Path = avdDir,
158+
Path = Path.Combine (GetAvdRootDirectory (), $"{name}.avd"),
149159
};
150160
}
151161

152162
public async Task DeleteAvdAsync (string name, CancellationToken cancellationToken = default)
153163
{
164+
if (name is null)
165+
throw new ArgumentNullException (nameof (name));
166+
if (name.Length == 0)
167+
throw new ArgumentException ("Value cannot be an empty string.", nameof (name));
168+
154169
var avdManagerPath = RequireAvdManagerPath ();
155170

156171
using var stderr = new StringWriter ();
@@ -186,5 +201,26 @@ internal static List<AvdInfo> ParseAvdListOutput (string output)
186201

187202
return avds;
188203
}
204+
205+
/// <summary>
206+
/// Resolves the AVD root directory, respecting ANDROID_AVD_HOME and ANDROID_USER_HOME.
207+
/// </summary>
208+
static string GetAvdRootDirectory ()
209+
{
210+
// ANDROID_AVD_HOME takes highest priority
211+
var avdHome = Environment.GetEnvironmentVariable ("ANDROID_AVD_HOME");
212+
if (!string.IsNullOrEmpty (avdHome))
213+
return avdHome;
214+
215+
// ANDROID_USER_HOME/avd is the next option
216+
var userHome = Environment.GetEnvironmentVariable (EnvironmentVariableNames.AndroidUserHome);
217+
if (!string.IsNullOrEmpty (userHome))
218+
return Path.Combine (userHome, "avd");
219+
220+
// Default: ~/.android/avd
221+
return Path.Combine (
222+
Environment.GetFolderPath (Environment.SpecialFolder.UserProfile),
223+
".android", "avd");
224+
}
189225
}
190226

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

Lines changed: 36 additions & 0 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.IO;
56
using NUnit.Framework;
67

@@ -160,4 +161,39 @@ public void AvdManagerPath_MissingSdk_ReturnsNull ()
160161
var runner = new AvdManagerRunner (() => "/nonexistent/path", null);
161162
Assert.IsNull (runner.AvdManagerPath);
162163
}
164+
165+
[Test]
166+
public void CreateAvdAsync_NullName_ThrowsArgumentNullException ()
167+
{
168+
var runner = new AvdManagerRunner (() => "/fake/sdk", null);
169+
Assert.ThrowsAsync<ArgumentNullException> (() => runner.CreateAvdAsync (null!, "system-image"));
170+
}
171+
172+
[Test]
173+
public void CreateAvdAsync_EmptyName_ThrowsArgumentException ()
174+
{
175+
var runner = new AvdManagerRunner (() => "/fake/sdk", null);
176+
Assert.ThrowsAsync<ArgumentException> (() => runner.CreateAvdAsync ("", "system-image"));
177+
}
178+
179+
[Test]
180+
public void CreateAvdAsync_EmptySystemImage_ThrowsArgumentException ()
181+
{
182+
var runner = new AvdManagerRunner (() => "/fake/sdk", null);
183+
Assert.ThrowsAsync<ArgumentException> (() => runner.CreateAvdAsync ("test-avd", ""));
184+
}
185+
186+
[Test]
187+
public void DeleteAvdAsync_NullName_ThrowsArgumentNullException ()
188+
{
189+
var runner = new AvdManagerRunner (() => "/fake/sdk", null);
190+
Assert.ThrowsAsync<ArgumentNullException> (() => runner.DeleteAvdAsync (null!));
191+
}
192+
193+
[Test]
194+
public void DeleteAvdAsync_EmptyName_ThrowsArgumentException ()
195+
{
196+
var runner = new AvdManagerRunner (() => "/fake/sdk", null);
197+
Assert.ThrowsAsync<ArgumentException> (() => runner.DeleteAvdAsync (""));
198+
}
163199
}

0 commit comments

Comments
 (0)