Skip to content

Conversation

@pjcollins
Copy link
Member

The NDK can now be installed by the sdkmanager tool in a side by side
fashion inside the Android SDK. Currently only the ndk;20.0.5594570
package exists, and it installs into $(AndroidSdk)/ndk/20.0.5594570.

We should re-use the logic used to look up and sort build-tools folders
to locate these new NDK installation locations. Additionally, we should
check both the ndk-bundle and new ndk/$(VERSION) paths on macOS if
we are otherwise unable to locate an NDK installation there.

@pjcollins
Copy link
Member Author

We'll need to consider #73 alongside these changes.

@pjcollins pjcollins requested review from joj and jonpryor August 20, 2019 18:29
The NDK can now be installed by the sdkmanager tool in a side by side
fashion inside the Android SDK. Currently only the `ndk;20.0.5594570`
package exists, and it installs into `$(AndroidSdk)/ndk/20.0.5594570`.

We should re-use the logic used to look up and sort `build-tools` folders
to locate these new NDK installation locations. Additionally, we should
check both the `ndk-bundle` and new `ndk/$(VERSION)` paths on macOS if
we are otherwise unable to locate an NDK installation there.
sdks.Add (AndroidSdkPath);

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

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.

return version;
}

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

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.

@pjcollins pjcollins added the do-not-merge Do not merge this PR label Sep 25, 2019
@pjcollins
Copy link
Member Author

I'm going to close this as the SDK Manager now allows installation of any recent NDK version into the default ndk-bundle path, and as a result the desire to support additional detection for versions in ndk/$(Version) seems greatly reduced.

@pjcollins pjcollins closed this Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge Do not merge this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants