From f8c43c9a055650722d2eb3e917dff6119a1027f8 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Tue, 30 Jun 2020 08:36:16 -0500 Subject: [PATCH 1/2] Fix sort ordering for ndk-bundle, add macOS support Context: https://build.azdo.io/xamarin/public/21545 An environment change on Azure DevOps seems to cause the test failure on Windows: Ndk_PathInSdk AndroidNdkPath not found inside sdk! Expected string length 71 but was 53. Strings differ at index 3. Expected: "C:\Users\VssAdministrator\AppData\Local\Temp\tmpAE78.tmp\sdk\..." But was: "C:\Program Files (x86)\Android\android-sdk\ndk-bundle" It appears that `xamarin-android-tools` is preferring an `ndk-bundle found on the system instead of the value passed in for `AndroidSdkPath`. To make matters worse, use of `Distinct()` returns an unordered sequence, and so the sort ordering is unknown: https://docs.microsoft.com/dotnet/api/system.linq.enumerable.distinct To fix this, let's not use `ToList()` or add `AndroidSdkPath` to the list at all. We can make an explicit check for it before the `foreach` loop. Inside the loop we should skip `AndroidSdkPath`. I also added the same code for the macOS/linux side in `AndroidSdkUnix`. I updated the `Ndk_PathInSdk` test so it will run on all platforms. --- .../Sdks/AndroidSdkUnix.cs | 18 +++++++++++++++++ .../Sdks/AndroidSdkWindows.cs | 20 ++++++++++++------- .../AndroidSdkInfoTests.cs | 10 ++++++---- 3 files changed, 37 insertions(+), 11 deletions(-) diff --git a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs index 12bd536..571ba1a 100644 --- a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs +++ b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs @@ -115,6 +115,14 @@ protected override IEnumerable GetAllAvailableAndroidSdks () protected override IEnumerable GetAllAvailableAndroidNdks () { + string ndk; + + if (!string.IsNullOrEmpty (AndroidSdkPath) && + Directory.Exists (ndk = Path.Combine (AndroidSdkPath, "ndk-bundle"))) { + if (ValidateAndroidNdkLocation (ndk)) + yield return ndk; + } + var preferedNdkPath = PreferedAndroidNdkPath; if (!string.IsNullOrEmpty (preferedNdkPath)) yield return preferedNdkPath!; @@ -125,6 +133,16 @@ protected override IEnumerable GetAllAvailableAndroidNdks () if (ValidateAndroidNdkLocation (ndkDir)) yield return ndkDir; } + + // Check for the "ndk-bundle" directory inside the SDK directories + foreach (var sdk in GetAllAvailableAndroidSdks ()) { + if (sdk == AndroidSdkPath) + continue; + if (Directory.Exists (ndk = Path.Combine (sdk, "ndk-bundle"))) { + if (ValidateAndroidNdkLocation (ndk)) + yield return ndk; + } + } } protected override string GetShortFormPath (string path) diff --git a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs index bf02640..06270b8 100644 --- a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs +++ b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs @@ -232,14 +232,20 @@ protected override IEnumerable GetAllAvailableAndroidNdks () // Check for the "ndk-bundle" directory inside the SDK directories string ndk; - var sdks = GetAllAvailableAndroidSdks().ToList(); - if (!string.IsNullOrEmpty(AndroidSdkPath)) - sdks.Add (AndroidSdkPath!); - - foreach(var sdk in sdks.Distinct()) - if (Directory.Exists(ndk = Path.Combine(sdk, "ndk-bundle"))) - if (ValidateAndroidNdkLocation(ndk)) + if (!string.IsNullOrEmpty (AndroidSdkPath) && + Directory.Exists (ndk = Path.Combine (AndroidSdkPath, "ndk-bundle"))) { + if (ValidateAndroidNdkLocation (ndk)) + yield return ndk; + } + + foreach (var sdk in GetAllAvailableAndroidSdks ()) { + if (sdk == AndroidSdkPath) + continue; + if (Directory.Exists (ndk = Path.Combine (sdk, "ndk-bundle"))) { + if (ValidateAndroidNdkLocation (ndk)) yield return ndk; + } + } // Check for the key the user gave us in the VS/addin options foreach (var root in roots) diff --git a/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidSdkInfoTests.cs b/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidSdkInfoTests.cs index a02ffd6..9a30516 100644 --- a/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidSdkInfoTests.cs +++ b/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidSdkInfoTests.cs @@ -67,9 +67,6 @@ public void Constructor_Paths () [Test] public void Ndk_PathInSdk() { - if (!OS.IsWindows) - Assert.Ignore("This only works in Windows"); - CreateSdks(out string root, out string jdk, out string ndk, out string sdk); var logs = new StringWriter(); @@ -77,12 +74,16 @@ public void Ndk_PathInSdk() logs.WriteLine($"[{level}] {message}"); }; + var oldPath = Environment.GetEnvironmentVariable ("PATH", EnvironmentVariableTarget.Process); try { + Environment.SetEnvironmentVariable ("PATH", "", EnvironmentVariableTarget.Process); + + var extension = OS.IsWindows ? ".cmd" : ""; var ndkPath = Path.Combine(sdk, "ndk-bundle"); Directory.CreateDirectory(ndkPath); Directory.CreateDirectory(Path.Combine(ndkPath, "toolchains")); - File.WriteAllText(Path.Combine(ndkPath, "ndk-stack.cmd"), ""); + File.WriteAllText(Path.Combine(ndkPath, $"ndk-stack{extension}"), ""); var info = new AndroidSdkInfo(logger, androidSdkPath: sdk, androidNdkPath: null, javaSdkPath: jdk); @@ -90,6 +91,7 @@ public void Ndk_PathInSdk() } finally { + Environment.SetEnvironmentVariable ("PATH", oldPath, EnvironmentVariableTarget.Process); Directory.Delete(root, recursive: true); } } From 2917561877668c5ae96bd1d9d66e68cab123feb7 Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Wed, 1 Jul 2020 18:44:28 -0400 Subject: [PATCH 2/2] [Xamarin.Android.Tools.AndroidSdk] Cleanup path validation logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: https://github.com/xamarin/xamarin-android-tools/issues/92 What we want *sounds* straightforward: given: var info = new AndroidSdkInfo (androidSdkPath: validSdkDir); then *if* `{validSdkDir}/ndk-bundle` exists, then `info.AndroidNdkPath` should be `{validSdkDir}/ndk-bundle`. Unfortunately, this is Doomed™, for two reasons: 1. `AndroidSdkBase.Initialize()` uses `AndroidSdkBase.PreferedAndroidNdkPath` if the `androidNdkPath` parameter is `null`, which will be the case here, and 2. If by chance `AndroidSdkBase.PreferedAndroidNdkPath` isn't set, we still hit `AllAndroidNdks.FirstOrDefault()`, which involves a `.Distinct()` call. The problem with [`Enumerable.Distinct()`][0] is that order is not preserved: > returns an unordered sequence that contains no duplicate values Note *unordered* sequence. Thus, the order should be *considered* to be effectively random. At which point I throw up my arms and say "this is too complicated," and start ripping out unnecessary functionality. (More should be removed, for that matter.) In particular, instead of this "semantically unworkable" code: androidNdkPath = androidNdkPath ?? PreferedAndroidNdkPath AndroidNdkPath = ValidateAndroidNdkLocation (androidNdkPath) ? androidNdkPath : AllAndroidNdks.FirstOrDefault (); which results in a priority order of: 1. `androidNdkPath` 2. `PreferedAndroidNdkPath` 3. The first `AllAndroidNdks` value, in indeterminate order, because of the not immediately obvious `.Distinct()` in `AllAndroidNdks`. we instead clean this up to be more explicit: AndroidNdkPath = GetValidNdkPath (androidNdkPath); Whereupon `GetValidNdkPath()` can have our intended order: 1. `androidNdkPath` 2. `{androidSdkPath}/ndk-bundle` 3. `PreferedAndroidNdkPath` 4. The first valid "other" location. "While we're at it," centralize error checking. We had calls to `ValidateAndroidSdkLocation()` and `ValidateAndroidNdkLocation()` strewn *everywhere*. These calls can be moved into `GetValidPath()` and `GetValidNdkPath()`, simplifying the logic of `GetAllAvailableAndroidSdks()`/etc. but also ensuring that we *can't forget* a validation call either. The result is that more code is removed than added. [0]: https://docs.microsoft.com/en-us/dotnet/api/system.linq.enumerable.distinct?view=netcore-3.1 --- .../Sdks/AndroidSdkBase.cs | 87 +++++++++++++++---- .../Sdks/AndroidSdkUnix.cs | 46 ++-------- .../Sdks/AndroidSdkWindows.cs | 32 ++----- .../AndroidSdkInfoTests.cs | 4 - 4 files changed, 80 insertions(+), 89 deletions(-) diff --git a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs index 1b10df1..820c81c 100644 --- a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs +++ b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs @@ -10,22 +10,21 @@ namespace Xamarin.Android.Tools abstract class AndroidSdkBase { string[]? allAndroidSdks; - string[]? allAndroidNdks; public string[] AllAndroidSdks { get { - if (allAndroidSdks == null) - allAndroidSdks = GetAllAvailableAndroidSdks ().Distinct ().ToArray (); + if (allAndroidSdks == null) { + var dirs = new List (); + dirs.Add (AndroidSdkPath); + dirs.AddRange (GetAllAvailableAndroidSdks ()); + allAndroidSdks = dirs.Where (d => ValidateAndroidSdkLocation (d)) + .Select (d => d!) + .Distinct () + .ToArray (); + } return allAndroidSdks; } } - public string[] AllAndroidNdks { - get { - if (allAndroidNdks == null) - allAndroidNdks = GetAllAvailableAndroidNdks ().Distinct ().ToArray (); - return allAndroidNdks; - } - } public readonly Action Logger; @@ -57,13 +56,10 @@ public AndroidSdkBase (Action logger) public virtual void Initialize (string? androidSdkPath = null, string? androidNdkPath = null, string? javaSdkPath = null) { - androidSdkPath = androidSdkPath ?? PreferedAndroidSdkPath; - androidNdkPath = androidNdkPath ?? PreferedAndroidNdkPath; - javaSdkPath = javaSdkPath ?? PreferedJavaSdkPath; + AndroidSdkPath = GetValidPath (ValidateAndroidSdkLocation, androidSdkPath, () => PreferedAndroidSdkPath, () => GetAllAvailableAndroidSdks ()); + JavaSdkPath = GetValidPath (ValidateJavaSdkLocation, javaSdkPath, () => PreferedJavaSdkPath, () => GetJavaSdkPaths ()); - AndroidSdkPath = ValidateAndroidSdkLocation (androidSdkPath) ? androidSdkPath : AllAndroidSdks.FirstOrDefault (); - AndroidNdkPath = ValidateAndroidNdkLocation (androidNdkPath) ? androidNdkPath : AllAndroidNdks.FirstOrDefault (); - JavaSdkPath = ValidateJavaSdkLocation (javaSdkPath) ? javaSdkPath : GetJavaSdkPath (); + AndroidNdkPath = GetValidNdkPath (androidNdkPath); if (!string.IsNullOrEmpty (JavaSdkPath)) { JavaBinPath = Path.Combine (JavaSdkPath, "bin"); @@ -93,11 +89,60 @@ public virtual void Initialize (string? androidSdkPath = null, string? androidNd NdkStack = GetExecutablePath (AndroidNdkPath, NdkStack); } + static string? GetValidPath (Func pathValidator, string? ctorParam, Func getPreferredPath, Func> getAllPaths) + { + if (pathValidator (ctorParam)) + return ctorParam; + ctorParam = getPreferredPath (); + if (pathValidator (ctorParam)) + return ctorParam; + foreach (var path in getAllPaths ()) { + if (pathValidator (path)) + return path; + } + return null; + } + + string? GetValidNdkPath (string? ctorParam) + { + if (ValidateAndroidNdkLocation (ctorParam)) + return ctorParam; + if (AndroidSdkPath != null) { + string bundle = Path.Combine (AndroidSdkPath, "ndk-bundle"); + if (Directory.Exists (bundle) && ValidateAndroidNdkLocation (bundle)) + return bundle; + } + ctorParam = PreferedAndroidNdkPath; + if (ValidateAndroidNdkLocation (ctorParam)) + return ctorParam; + foreach (var path in GetAllAvailableAndroidNdks ()) { + if (ValidateAndroidNdkLocation (path)) + return path; + } + return null; + } + protected abstract IEnumerable GetAllAvailableAndroidSdks (); - protected abstract IEnumerable GetAllAvailableAndroidNdks (); - protected abstract string? GetJavaSdkPath (); protected abstract string GetShortFormPath (string path); + protected virtual IEnumerable GetAllAvailableAndroidNdks () + { + // Look in PATH + foreach (var ndkStack in ProcessUtils.FindExecutablesInPath (NdkStack)) { + var ndkDir = Path.GetDirectoryName (ndkStack); + if (ndkDir == null) + continue; + yield return ndkDir; + } + + // Check for the "ndk-bundle" directory inside other SDK directories + foreach (var sdk in GetAllAvailableAndroidSdks ()) { + if (sdk == AndroidSdkPath) + continue; + yield return Path.Combine (sdk, "ndk-bundle"); + } + } + public abstract void SetPreferredAndroidSdkPath (string? path); public abstract void SetPreferredJavaSdkPath (string? path); public abstract void SetPreferredAndroidNdkPath (string? path); @@ -108,6 +153,12 @@ public string NdkHostPlatform { get { return IsNdk64Bit ? NdkHostPlatform64Bit : NdkHostPlatform32Bit; } } + IEnumerable GetJavaSdkPaths () + { + return JdkInfo.GetKnownSystemJdkInfos (Logger) + .Select (jdk => jdk.HomePath); + } + /// /// Checks that a value is the location of an Android SDK. /// diff --git a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs index 571ba1a..34836f3 100644 --- a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs +++ b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs @@ -98,51 +98,15 @@ protected override IEnumerable GetAllAvailableAndroidSdks () // Strip off "platform-tools" var dir = Path.GetDirectoryName (path); - if (ValidateAndroidSdkLocation (dir)) - yield return dir; + if (dir == null) + continue; + + yield return dir; } // Check some hardcoded paths for good measure var macSdkPath = Path.Combine (Environment.GetFolderPath (Environment.SpecialFolder.Personal), "Library", "Android", "sdk"); - if (ValidateAndroidSdkLocation (macSdkPath)) - yield return macSdkPath; - } - - protected override string? GetJavaSdkPath () - { - return JdkInfo.GetKnownSystemJdkInfos (Logger).FirstOrDefault ()?.HomePath; - } - - protected override IEnumerable GetAllAvailableAndroidNdks () - { - string ndk; - - if (!string.IsNullOrEmpty (AndroidSdkPath) && - Directory.Exists (ndk = Path.Combine (AndroidSdkPath, "ndk-bundle"))) { - if (ValidateAndroidNdkLocation (ndk)) - yield return ndk; - } - - var preferedNdkPath = PreferedAndroidNdkPath; - if (!string.IsNullOrEmpty (preferedNdkPath)) - yield return preferedNdkPath!; - - // Look in PATH - foreach (var ndkStack in ProcessUtils.FindExecutablesInPath (NdkStack)) { - var ndkDir = Path.GetDirectoryName (ndkStack); - if (ValidateAndroidNdkLocation (ndkDir)) - yield return ndkDir; - } - - // Check for the "ndk-bundle" directory inside the SDK directories - foreach (var sdk in GetAllAvailableAndroidSdks ()) { - if (sdk == AndroidSdkPath) - continue; - if (Directory.Exists (ndk = Path.Combine (sdk, "ndk-bundle"))) { - if (ValidateAndroidNdkLocation (ndk)) - yield return ndk; - } - } + yield return macSdkPath; } protected override string GetShortFormPath (string path) diff --git a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs index 06270b8..59b2082 100644 --- a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs +++ b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs @@ -102,14 +102,7 @@ protected override IEnumerable GetAllAvailableAndroidSdks () }; foreach (var basePath in paths) if (Directory.Exists (basePath)) - if (ValidateAndroidSdkLocation (basePath)) - yield return basePath; - } - - protected override string? GetJavaSdkPath () - { - var jdk = JdkInfo.GetKnownSystemJdkInfos (Logger).FirstOrDefault (); - return jdk?.HomePath; + yield return basePath; } internal static IEnumerable GetJdkInfos (Action logger) @@ -223,30 +216,13 @@ private static IEnumerable GetOracleJdkPaths () protected override IEnumerable GetAllAvailableAndroidNdks () { + var roots = new[] { RegistryEx.CurrentUser, RegistryEx.LocalMachine }; var wow = RegistryEx.Wow64.Key32; var regKey = GetMDRegistryKey (); Logger (TraceLevel.Info, "Looking for Android NDK..."); - // Check for the "ndk-bundle" directory inside the SDK directories - string ndk; - - if (!string.IsNullOrEmpty (AndroidSdkPath) && - Directory.Exists (ndk = Path.Combine (AndroidSdkPath, "ndk-bundle"))) { - if (ValidateAndroidNdkLocation (ndk)) - yield return ndk; - } - - foreach (var sdk in GetAllAvailableAndroidSdks ()) { - if (sdk == AndroidSdkPath) - continue; - if (Directory.Exists (ndk = Path.Combine (sdk, "ndk-bundle"))) { - if (ValidateAndroidNdkLocation (ndk)) - yield return ndk; - } - } - // Check for the key the user gave us in the VS/addin options foreach (var root in roots) if (CheckRegistryKeyForExecutable (root, regKey, MDREG_ANDROID_NDK, wow, ".", NdkStack)) @@ -271,6 +247,10 @@ protected override IEnumerable GetAllAvailableAndroidNdks () foreach (var dir in Directory.GetDirectories (basePath, "android-ndk-r*")) if (ValidateAndroidNdkLocation (dir)) yield return dir; + + foreach (var dir in base.GetAllAvailableAndroidNdks ()) { + yield return dir; + } } protected override string GetShortFormPath (string path) diff --git a/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidSdkInfoTests.cs b/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidSdkInfoTests.cs index 9a30516..28aad14 100644 --- a/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidSdkInfoTests.cs +++ b/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidSdkInfoTests.cs @@ -74,11 +74,8 @@ public void Ndk_PathInSdk() logs.WriteLine($"[{level}] {message}"); }; - var oldPath = Environment.GetEnvironmentVariable ("PATH", EnvironmentVariableTarget.Process); try { - Environment.SetEnvironmentVariable ("PATH", "", EnvironmentVariableTarget.Process); - var extension = OS.IsWindows ? ".cmd" : ""; var ndkPath = Path.Combine(sdk, "ndk-bundle"); Directory.CreateDirectory(ndkPath); @@ -91,7 +88,6 @@ public void Ndk_PathInSdk() } finally { - Environment.SetEnvironmentVariable ("PATH", oldPath, EnvironmentVariableTarget.Process); Directory.Delete(root, recursive: true); } }