Skip to content

Support side by side NDK installations in SDK #76

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pjcollins wants to merge 2 commits into from
Closed

Support side by side NDK installations in SDK #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
737f5cb
Support side by side NDK installations in SDK
pjcollins Aug 20, 2019
7d708a6
Apply feedback
pjcollins Aug 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@ src/*.dll
bin
**/obj
packages/
TestResult-*.xml
TestResult*.xml
.vs/
18 changes: 2 additions & 16 deletions src/Xamarin.Android.Tools.AndroidSdk/AndroidSdkInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,34 +46,20 @@ public IEnumerable<string> GetBuildToolsPaths ()
var buildTools = Path.Combine (AndroidSdkPath, "build-tools");
if (Directory.Exists (buildTools)) {
var preview = Directory.EnumerateDirectories (buildTools)
.Where(x => TryParseVersion (Path.GetFileName (x)) == null)
.Where(x => sdk.TryParseVersion (Path.GetFileName (x)) == null)
.Select(x => x);

foreach (var d in preview)
yield return d;

var sorted = from p in Directory.EnumerateDirectories (buildTools)
let version = TryParseVersion (Path.GetFileName (p))
where version != null
orderby version descending
select p;

foreach (var d in sorted)
foreach (var d in sdk.GetSortedToolDirectoryPaths (buildTools))
yield return d;
}
var ptPath = Path.Combine (AndroidSdkPath, "platform-tools");
if (Directory.Exists (ptPath))
yield return ptPath;
}

static Version TryParseVersion (string v)
{
Version version;
if (Version.TryParse (v, out version))
return version;
return null;
}

public IEnumerable<AndroidVersion> GetInstalledPlatformVersions (AndroidVersions versions)
{
if (versions == null)
Expand Down
48 changes: 47 additions & 1 deletion src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,36 @@ public virtual bool ValidateJavaSdkLocation (string loc)
}

/// <summary>
/// Checks that a value is the location of an Android SDK.
/// Searches all Android SDK locations for nested NDK installations.
/// </summary>
protected IEnumerable<string> FindNdkPathsInAndroidSdkPaths ()
{
string ndk;
var sdks = new List<string> ();
if (!string.IsNullOrEmpty (AndroidSdkPath))
sdks.Add (AndroidSdkPath);

sdks.AddRange (AllAndroidSdks);
foreach (var sdk in sdks.Distinct ()) {
Copy link
Contributor

@jonpryor jonpryor Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this provide an IEqualityComparer<string>, e.g.

foreach (var sdk in sdks.Distinct (StringComparer.OrdinalIgnoreCase)) {

If (somehow) sdks contains case-differing paths on Windows, you'll have duplicates if you don't use StringComparer.OrdinalIgnoreCase. This may or may not matter.

// Check for the "ndk-bundle" directory inside the SDK directories
if (Directory.Exists (ndk = Path.Combine (sdk, "ndk-bundle"))) {
if (ValidateAndroidNdkLocation (ndk)) {
yield return ndk;
}
}
// Check for any versioned "ndk/$(VERSION)" directory inside the SDK directories
if (Directory.Exists (ndk = Path.Combine (sdk, "ndk"))) {
foreach (var versionedNdk in GetSortedToolDirectoryPaths (ndk)) {
if (ValidateAndroidNdkLocation (versionedNdk)) {
yield return versionedNdk;
}
}
}
}
}

/// <summary>
/// Checks that a value is the location of an Android NDK.
/// </summary>
public bool ValidateAndroidNdkLocation (string loc)
{
Expand All @@ -167,6 +196,23 @@ static string GetExecutablePath (string dir, string exe)
return e;
return exe;
}

public Version TryParseVersion (string v)
{
Version.TryParse (v, out Version version);
return version;
}

public IEnumerable<string> GetSortedToolDirectoryPaths (string topToolDirectory)
Copy link
Contributor

@jonpryor jonpryor Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the sorting here is being done at the wrong "scope". For example, consider AndroidSdkBase.FindNdkPathsInAndroidSdkPaths(): this enumerates over an unbounded list of ndk paths, and for each such path calls GetSortedToolDirectoryPaths(). The result is a partial ordering, not a total ordering.

Is this a problem? ¯_(ツ)_/¯

(It's also likely a rare scenario, but ignoring that...)

For example, FindNdkPathsInAndroidSdkPaths() will return ndk-bundle before the ndk/* directories. This implies that if ndk-bundle is NDK r19 while ndk/20.0.0 is NDK r20, we'll be returning r19 before r20. Is that correct?

Similarly, if (though some bizarro happenstance) there are multiple Android SDKs present in $PATH, FindNdkPathsInAndroidSdkPaths() will return the NDKs in SDK enumeration order, not any form of "total" order.

Again, is this a problem? I don't know. I think I'd want to know.

If it is a problem, things get slightly more complicated. You can't just use the directory name to infer the version, as ndk-bundle contains no version information. You'd thus need to parse source.properties to determine the actual version, and sort on that. This would allow ndk-bundle to be properly sorted alongside the ndk/* directories, if necessary.

However, that then implies that the sorting is done at the wrong level; it needs to be done within e.g. FindNdkPathsInAndroidSdkPaths(), not here.

{
var sorted = from p in Directory.EnumerateDirectories (topToolDirectory)
let version = TryParseVersion (Path.GetFileName (p))
where version != null
orderby version descending
select p;

return sorted;
}
}
}

4 changes: 4 additions & 0 deletions src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,10 @@ protected override IEnumerable<string> GetAllAvailableAndroidNdks ()
if (ValidateAndroidNdkLocation (ndkDir))
yield return ndkDir;
}

// Check for NDK directories inside the SDK directories
foreach (var ndk in FindNdkPathsInAndroidSdkPaths ())
yield return ndk;
}

protected override string GetShortFormPath (string path)
Expand Down
14 changes: 3 additions & 11 deletions src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,17 +237,9 @@ protected override IEnumerable<string> GetAllAvailableAndroidNdks ()

Logger (TraceLevel.Info, "Looking for Android NDK...");

// 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))
yield return ndk;
// Check for NDK directories inside the SDK directories
foreach (var ndk in FindNdkPathsInAndroidSdkPaths ())
yield return ndk;

// Check for the key the user gave us in the VS/addin options
foreach (var root in roots)
Expand Down
49 changes: 42 additions & 7 deletions src/Xamarin.Android.Tools.AndroidSdk/Tests/AndroidSdkInfoTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,8 @@ 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 oldPath = Environment.GetEnvironmentVariable ("PATH");

var logs = new StringWriter();
Action<TraceLevel, string> logger = (level, message) => {
Expand All @@ -79,21 +77,58 @@ public void Ndk_PathInSdk()

try
{
var ndkPath = Path.Combine(sdk, "ndk-bundle");
Directory.CreateDirectory(ndkPath);
Directory.CreateDirectory(Path.Combine(ndkPath, "toolchains"));
File.WriteAllText(Path.Combine(ndkPath, "ndk-stack.cmd"), "");
if (!OS.IsWindows)
Environment.SetEnvironmentVariable ("PATH", "dummypath");

var ndkPath = Path.Combine(sdk, "ndk-bundle");
Directory.CreateDirectory (ndkPath);
CreateFauxAndroidNdkDirectory (ndkPath);
var info = new AndroidSdkInfo(logger, androidSdkPath: sdk, androidNdkPath: null, javaSdkPath: jdk);

Assert.AreEqual(ndkPath, info.AndroidNdkPath, "AndroidNdkPath not found inside sdk!");
}
finally
{
if (!OS.IsWindows)
Environment.SetEnvironmentVariable ("PATH", oldPath);

Directory.Delete(root, recursive: true);
}
}

[Test]
public void VersionedNdk_PathInSdk()
{
CreateSdks (out string root, out string jdk, out string ndk, out string sdk);
var oldPath = Environment.GetEnvironmentVariable ("PATH");

var logs = new StringWriter ();
Action<TraceLevel, string> logger = (level, message) => {
logs.WriteLine ($"[{level}] {message}");
};

try
{
if (!OS.IsWindows)
Environment.SetEnvironmentVariable ("PATH", "dummypath");

var ndkPath = Path.Combine (sdk, "ndk", "20.0.5594570");
Directory.CreateDirectory (ndkPath);
Directory.CreateDirectory (Path.Combine (sdk, "ndk", "noversion"));
CreateFauxAndroidNdkDirectory (ndkPath);
var info = new AndroidSdkInfo (logger, androidSdkPath: sdk, androidNdkPath: null, javaSdkPath: jdk);
Assert.AreEqual (ndkPath, info.AndroidNdkPath, "Versioned AndroidNdkPath not found inside sdk!");
Assert.AreNotEqual (ndkPath, ndk, "The default NDK path created should not match the nested versioned NDK path!");
}
finally
{
if (!OS.IsWindows)
Environment.SetEnvironmentVariable ("PATH", oldPath);

Directory.Delete (root, recursive: true);
}
}

[Test]
public void Constructor_SetValuesFromPath ()
{
Expand Down