Skip to content
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.

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

[NDK] Locate and select only compatible NDK versions #103

Merged
merged 1 commit into from
Jan 20, 2021

Conversation

grendello
Copy link
Contributor

Context: dotnet/android#5499
Context: dotnet/android#5526
Context: android/ndk#1427
Context: https://developer.android.com/studio/command-line/variables#envar

Xamarin.Android is not (yet) compatible with the recently released
Android NDK r22 version. Azure build images have recently rolled out an
update which includes NDK r22 and, thus, it breaks builds for customers
using any form of Xamarin.Android AOT build.

In attempt to detect broken/incompatible NDK versions as well as select
the "best one", this commit adds code to scan the known NDK locations in
search of the preferred version. The search is conducted as follows:

  1. If the user selected a preferred NDK location, it is always used.
  2. Locations specified in the ANDROID_{HOME,SDK_ROOT} environment
    variables are returned next.
  3. Directories in the PATH environment variable are examined to find
    a valid NDK location.
  4. OS-specific known NDK locations are considered.

For each of the returned locations, we now look for the Android SDK
packages containing the NDK. There are two kinds of such packages:

  • ndk-bundle is the older package which allows for installation of
    only one NDK inside the SDK directory
  • ndk/* is a newer package which allows for installation of several
    NDK versions in parallel. Each subdirectory of ndk is an X.Y.Z
    version number of the NDK.

In each of these directories we look for the source.properties file
from which we then extract the NDK version and then we sort thus
discovered NDK instances using their version as the key, in the
descending order. The latest compatible (currently: less than 22 and
more than 15) version is selected and its path returned to the caller.

Comment on lines +15 to +19
static readonly char[] SourcePropertiesKeyValueSplit = new char[] { '=' };

// Per https://developer.android.com/studio/command-line/variables#envar
protected static readonly string[] AndroidSdkEnvVars = {"ANDROID_HOME", "ANDROID_SDK_ROOT"};
Copy link
Member

Choose a reason for hiding this comment

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

So this introduces $ANDROID_HOME as a place for finding both the Android SDK & NDK, correct?

Are we worried this could break existing users? I could see users having $ANDROID_HOME set from Android Studio or something else, but Xamarin.Android is currently using the Android SDK installed by Visual Studio . A different SDK/NDK could get selected?

If we do this, should we put the env var checks last?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$ANDROID_HOME and $ANDROID_SDK_ROOT are checked, yes. Without these I wasn't able to make it detect the SDK instance I wanted it to use - it searched the $PATH. This addition will not affect users who set their preferred NDK location from the IDE, as that is still the first path checked. Furthermore, we now collect all the NDK paths and check their versions, then return the newest compatible one - that's the main goal of this PR. Environment variables should, generally, be considered before anything else, but it might break backward compatibility - it was an oversight in the original implementation of the code.

Note also that, currently, the order in which Windows and Unix look for the NDK is different:

  1. Preferred location set by the IDE (both Windows and Unix)
  2. Environment variables, with this PR (both Windows and Unix)
  3. Other locations
    1. $PATH directories (Unix)
    2. Known hard-coded locations (Windows)
  4. Other locations
    1. Known hard-coded locations (Unix, only mac really)
    2. $PATH directories (Windows)
  5. All discovered SDK locations (Windows)

This needs to be fixed in some future PR, but for now - not sure where the environment variable check would have to be placed? I feel it needs to be before $PATH, since that variable is usually set account or system wide (e.g. in system preferences for the user/machine on Windows, and in /etc and user's shell init scripts on Unix) and it's not very comfortable to manipulate it ad-hoc, unlike the easy way to set the supported environment variables.

return String.Empty;
}

return ndkInstances.First ().Value;
Copy link
Member

Choose a reason for hiding this comment

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

What does this return? Won't it be the lowest found version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, look at the instantiation of the SortedDictionary - it uses a comparer that sorts in descending order

}
}

string FindBestNDK (string androidSdkPath)
Copy link
Member

Choose a reason for hiding this comment

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

Nit-pick, and I'm not sure I should be saying this, but

I'd prefer if this were "more pieces" with "clearer ordering":

IEnumerable<string> GetSdkNdkPaths (string androidSdkPath)
{
    var ndk = Path.Combine (androidSdkPath, "ndk");
    if (Directory.Exists (ndk)) {
        foreach (string ndkPath in Directory.EnumerateDirectories (ndk, SearchOption.TopDirectoryOnly)) {
            yield return ndkDir;
        }
    }
    ndk = Path.Combine (androidSdkPath, "ndk-bundle");
    if (Directory.Exists (ndk))
        yield return ndk;
}

IEnumerable<string> OrderNdks (IEnumerable<string> ndkDirs)
{
    return from ndk in ndkDirs
        let ndkVersion = LoadNDKVersion (ndk)
        where ndkVersion > MaximumCompatibleNDKMajorVersion && ndkVersion < MinimumCompatibleNDKMajorVersion
        orderby ndkVersion descending
        select ndk;
}

string GetBestSdkNdkPath (string androidSdkPath)
{
    return OrderNdks (GetSdkNdkPaths (androidSdkPath)).FirstOrDefault ();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's not do this right now. This entire code needs to be revised and refactored - most of the stuff can be done in the base class, for both Unix and Windows (and should be). Let's postpone it for some near future

@jonpryor
Copy link
Member

This should also add a test to tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidSdkInfoTests.cs.

@grendello
Copy link
Contributor Author

Please don't merge yet, working on adding one more test

@grendello
Copy link
Contributor Author

OK, ready

Context: dotnet/android#5499
Context: dotnet/android#5526
Context: android/ndk#1427
Context: https://developer.android.com/studio/command-line/variables#envar

Xamarin.Android is not (yet) compatible with the recently released
Android NDK r22 version.  Azure build images have recently rolled out an
update which includes NDK r22 and, thus, it breaks builds for customers
using any form of Xamarin.Android AOT build.

In attempt to detect broken/incompatible NDK versions as well as select
the "best one", this commit adds code to scan the known NDK locations in
search of the preferred version.  The search is conducted as follows:

  1. If the user selected a preferred NDK location, it is always used.
  2. Locations specified in the `ANDROID_{HOME,SDK_ROOT}` environment
     variables are returned next.
  3. Directories in the `PATH` environment variable are examined to find
     a valid NDK location.
  4. OS-specific known NDK locations are considered.

For each of the returned locations, we now look for the Android SDK
packages containing the NDK.  There are two kinds of such packages:

  * `ndk-bundle` is the older package which allows for installation of
    only one NDK inside the SDK directory
  * `ndk/*` is a newer package which allows for installation of several
    NDK versions in parallel.  Each subdirectory of `ndk` is an `X.Y.Z`
    version number of the NDK.

In each of these directories we look for the `source.properties` file
from which we then extract the NDK version and then we sort thus
discovered NDK instances using their version as the key, in the
descending order.  The latest compatible (currently: less than 22 and
more than 15) version is selected and its path returned to the caller.
@jonpryor
Copy link
Member

jonpryor commented Jan 20, 2021

Context: https://github.com/actions/virtual-environments/issues/2420
Context: https://github.com/xamarin/xamarin-android/issues/5499
Context: https://github.com/xamarin/xamarin-android/issues/5526
Context: https://github.com/android/ndk/issues/1427
Context: https://developer.android.com/studio/command-line/variables#envar

Xamarin.Android is not (yet) compatible with the recently released
Android NDK r22 version.  Azure build images have recently rolled out
an update which includes NDK r22 and, thus, it breaks builds for
customers using any form of Xamarin.Android AOT build.

In an attempt to detect broken/incompatible NDK versions as well as
to select the "best one", this commit adds code to scan the known NDK
locations in search of the preferred version.

Given an `AndroidSdkInfo` creation of:

	var info = new AndroidSdkInfo (logger:logger,
	        androidSdkPath: sdkPath,
	        androidNdkPath: ndkPath,
	        javaSdkPath: javaPath);
	var usedNdkPath = info.AndroidNdkPath;

If `ndkPath` is not `null` and otherwise valid, then `usedNdkPath`
is `ndkPath`.

If `ndkPath` is `null` or is otherwise invalid (missing `ndk-stack`,
etc.), then we search for an `info.AndroidNdkPath` value as follows:

 1. If `androidSdkPath` is not `null` and valid, then we check for
    Android SDK-relative NDK locations, in:

      * `{androidSdkPath}/ndk/*`
      * `{androidSdkPath}/ndk-bundle`

    For each found SDK-relative NDK directory, we filter out NDKs for
    which we cannot determine the package version, as well as those
    which are "too old" (< `MinimumCompatibleNDKMajorVersion`) or
    "too new" (> `MaximumCompatibleNDKMajorVersion`), currently r22.

    We prefer the NDK location with the highest version number. 

 2. If `androidSdkPath` is not `null` and valid and if there are no
    Android SDK-relative NDK locations, then we use the user-selected
    "preferred NDK location".  See also
    `AndroidSdkInfo.SetPreferredAndroidNdkPath()`.

 3. If `androidSdkPath` is not `null` and valid and if the preferred
    NDK location isn't set or is invalid, then we check directories
    specified in `$PATH`, and use the directory which contains
    `ndk-stack`.

 4. If `androidSdkPath` is not `null` and valid and `$PATH` didn't
    contain `ndk-stack`, then we continue looking for NDK locations
    within the Android SDK locations specified by the `$ANDROID_HOME`
    and `$ANDROID_SDK_ROOT` environment variables.
    As with (1), these likewise look for e.g. `${ANDROID_HOME}/ndk/*`
    or `${ANDROID_SDK_ROOT}/ndk-bundle` directories and select the
    NDK with the highest supported version.

 5. If `androidSdkPath` is `null`, then *first* we try to find a
    valid Android SDK directory, using on Unix:

     a. The preferred Android SDK directory; see also
        `AndroidSdkInfo.SetPreferredAndroidSdkPath().

     b. The `$ANDROID_HOME` and `ANDROID_SDK_ROOT`
        environment variables.

     c. Directories within `$PATH` that contain `adb`.

    Once an Android SDK is found, steps (1)…(4) are performed.

In (1) and (4), we now look for the Android SDK packages containing
the NDK.  There are two kinds of such packages:

  * `ndk-bundle` is the older package which allows for installation of
    only one NDK inside the SDK directory
  * `ndk/*` is a newer package which allows for installation of several
    NDK versions in parallel.  Each subdirectory of `ndk` is an `X.Y.Z`
    version number of the NDK.

In each of these directories we look for the `source.properties` file
from which we then extract the NDK version and then we sort thus
discovered NDK instances using their version as the key, in the
descending order.  The latest compatible (currently: less than 22 and
more than 15) version is selected and its path returned to the caller.

@jonpryor jonpryor merged commit b2d9fdf into dotnet:main Jan 20, 2021
@grendello grendello deleted the ndk-search branch January 20, 2021 21:19
jonpryor pushed a commit that referenced this pull request Jan 20, 2021
Context: actions/runner-images#2420
Context: dotnet/android#5499
Context: dotnet/android#5526
Context: android/ndk#1427
Context: https://developer.android.com/studio/command-line/variables#envar

Xamarin.Android is not (yet) compatible with the recently released
Android NDK r22 version.  Azure build images have recently rolled out
an update which includes NDK r22 and, thus, it breaks builds for
customers using any form of Xamarin.Android AOT build.

In an attempt to detect broken/incompatible NDK versions as well as
to select the "best one", this commit adds code to scan the known NDK
locations in search of the preferred version.

Given an `AndroidSdkInfo` creation of:

	var info = new AndroidSdkInfo (logger:logger,
	        androidSdkPath: sdkPath,
	        androidNdkPath: ndkPath,
	        javaSdkPath: javaPath);
	var usedNdkPath = info.AndroidNdkPath;

If `ndkPath` is not `null` and otherwise valid, then `usedNdkPath`
is `ndkPath`.

If `ndkPath` is `null` or is otherwise invalid (missing `ndk-stack`,
etc.), then we search for an `info.AndroidNdkPath` value as follows:

 1. If `androidSdkPath` is not `null` and valid, then we check for
    Android SDK-relative NDK locations, in:

      * `{androidSdkPath}/ndk/*`
      * `{androidSdkPath}/ndk-bundle`

    For each found SDK-relative NDK directory, we filter out NDKs for
    which we cannot determine the package version, as well as those
    which are "too old" (< `MinimumCompatibleNDKMajorVersion`) or
    "too new" (> `MaximumCompatibleNDKMajorVersion`), currently r22.

    We prefer the NDK location with the highest version number. 

 2. If `androidSdkPath` is not `null` and valid and if there are no
    Android SDK-relative NDK locations, then we use the user-selected
    "preferred NDK location".  See also
    `AndroidSdkInfo.SetPreferredAndroidNdkPath()`.

 3. If `androidSdkPath` is not `null` and valid and if the preferred
    NDK location isn't set or is invalid, then we check directories
    specified in `$PATH`, and use the directory which contains
    `ndk-stack`.

 4. If `androidSdkPath` is not `null` and valid and `$PATH` didn't
    contain `ndk-stack`, then we continue looking for NDK locations
    within the Android SDK locations specified by the `$ANDROID_HOME`
    and `$ANDROID_SDK_ROOT` environment variables.
    As with (1), these likewise look for e.g. `${ANDROID_HOME}/ndk/*`
    or `${ANDROID_SDK_ROOT}/ndk-bundle` directories and select the
    NDK with the highest supported version.

 5. If `androidSdkPath` is `null`, then *first* we try to find a
    valid Android SDK directory, using on Unix:

     a. The preferred Android SDK directory; see also
        `AndroidSdkInfo.SetPreferredAndroidSdkPath().

     b. The `$ANDROID_HOME` and `ANDROID_SDK_ROOT`
        environment variables.

     c. Directories within `$PATH` that contain `adb`.

    Once an Android SDK is found, steps (1)…(4) are performed.

In (1) and (4), we now look for the Android SDK packages containing
the NDK.  There are two kinds of such packages:

  * `ndk-bundle` is the older package which allows for installation of
    only one NDK inside the SDK directory
  * `ndk/*` is a newer package which allows for installation of several
    NDK versions in parallel.  Each subdirectory of `ndk` is an `X.Y.Z`
    version number of the NDK.

In each of these directories we look for the `source.properties` file
from which we then extract the NDK version and then we sort thus
discovered NDK instances using their version as the key, in the
descending order.  The latest compatible (currently: less than 22 and
more than 15) version is selected and its path returned to the caller.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants