Skip to content

Conversation

@jonathanpeppers
Copy link
Member

In .NET 5, there is a single AndroidApiInfo.xml file:

<AndroidApiInfo>
  <Id>30</Id>
  <Level>30</Level>
  <Name>R</Name>
  <Version>v11.0</Version>
  <Stable>True</Stable>
</AndroidApiInfo>

And so if you call AndroidVersions.GetApiLevelFromId(29) you would
always get null returned. This is because KnownVersions does not
have API 29 or 30 listed.

To fix this and not update KnownVersions, we can make the various
GetApiLevelFromId methods return the incoming API level as a last
resort.

So if 29 comes in, 29 can be returned and assumed to be a valid API
level.

@jonathanpeppers jonathanpeppers requested a review from jonpryor June 29, 2020 21:13
@jonathanpeppers
Copy link
Member Author

I think there is an unrelated test failure on Windows:

     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"

In .NET 5, there is a single `AndroidApiInfo.xml` file:

    <AndroidApiInfo>
      <Id>30</Id>
      <Level>30</Level>
      <Name>R</Name>
      <Version>v11.0</Version>
      <Stable>True</Stable>
    </AndroidApiInfo>

And so if you call `AndroidVersions.GetApiLevelFromId(29)` you would
always get `null` returned. This is because `KnownVersions` does not
have API 29 or 30 listed.

To fix this and *not* update `KnownVersions`, we can make the various
`GetApiLevelFromId` methods return the incoming API level as a last
resort.

So if 29 comes in, 29 can be returned and assumed to be a valid API
level.
@jonathanpeppers jonathanpeppers marked this pull request as ready for review June 30, 2020 13:51
@jonpryor
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor
Copy link
Contributor

The "unrelated test failure" on Windows is because Windows is non-deterministic: the test asserts that given an Android SDK directory androidSdk which contains the file {androidSdk}\ndk-bundle\ndk-stack.cmd, then this:

var info = new AndroidSdkInfo (logger: null, androidSdkPath: androidSdk);

will return {androidSdk}\ndk-bundle as the info.AndroidNdkPath value.

The problem is that the info.AndroidNdkPath logic involves is doomed to fail. For starters, if the Registry has an NDK path configured, it is preferred:

https://github.com/xamarin/xamarin-android-tools/blob/3974fc38c0f25f943b5d3bf0a4e174532a2a60ee/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs#L61

If the Registry doesn't have a preferred NDK value, then we hit AndroidSdkWindows.GetAllAvailableAndroidNdks(), which involves a .Distinct() call:

https://github.com/xamarin/xamarin-android-tools/blob/3974fc38c0f25f943b5d3bf0a4e174532a2a60ee/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs#L224-L268

which means that the order of returned directories is unknowable and may (will?) vary from test run to test run.

This is, in short, a highly "unstable" algorithm which won't be consistent from one run to the next. Which explains why it sometimes fails, and sometimes doesn't. :-(

@jonpryor
Copy link
Contributor

Context: https://github.com/xamarin/xamarin-android/pull/4873
Context: https://github.com/xamarin/xamarin-android/pull/4873#issuecomment-651331337

When xamarin-android is built to support .NET 5, the .NET 5 install
directory contains a single `AndroidApiInfo.xml` file:

	<AndroidApiInfo>
	  <Id>30</Id>
	  <Level>30</Level>
	  <Name>R</Name>
	  <Version>v11.0</Version>
	  <Stable>True</Stable>
	</AndroidApiInfo>

`AndroidVersions`, meanwhile, is setup to read a *set* of
`AndroidApiInfo.xml` files (aad97f16) to "dynamically" compute mappings
between possible `$(TargetFrameworkVersion)` values, API-levels, and
IDs for those API levels.

When there is only one such file, if you call:

	int? apiLevel = androidVersions.GetApiLevelFromId (29);

then `apiLevel` will always be `null`, because (at present)
`AndroidVersions.KnownVersions` doesn't know about API-29 or API-30.

We *could* update `AndroidVersions.KnownVersions` to contain entries
for API-29 & API-30, but doing so means that we reintroduce the
scenario that `AndroidVersions` was added to help defend against: a
need/requirement to update `AndroidVersions.KnownVersions` *every time*
a new API level was released, lest ~everything break.

We *don't* want to require `AndroidVersions.KnownVersions` updates.

To allow a .NET 5-like environment to work *without* updating
`KnownVersions`, update the various `GetApiLevelFromId()` methods to
return the incoming API level as a fallback.  If `"29"` comes in, then
`29` can be returned and assumed to be a valid API level.

@jonpryor jonpryor merged commit 79a0141 into dotnet:master Jun 30, 2020
@jonathanpeppers jonathanpeppers deleted the unknown-api-levels branch June 30, 2020 19:29
jonpryor pushed a commit that referenced this pull request Jul 14, 2020
Fixes: #92

Context: #90 (comment)

The PR builds for #90 encountered an "unrelated test failure" 
in `AndroidSdkInfoTests.Ndk_PathInSdk()` on Windows, because Windows
is non-deterministic: the test asserts that given an Android SDK
directory `androidSdk` which contains the file
`{androidSdk}\ndk-bundle\ndk-stack.cmd`, then this:

	var info = new AndroidSdkInfo (logger: null, androidSdkPath: androidSdk);

will have `info.AndroidNdkPath`==`{androidSdk}\ndk-bundle`.

Instead, this test would occasionally fail on CI:

	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"

Here, the "preferred"/system-wide NDK is being chosen over the
`{androidSdk}\ndk-bundle` directory that the unit test created.

The wrong directory was chosen for two reasons: 

 1. `AndroidSdkBase.Initialize()` would use `PreferedAndroidNdkPath`
    when `androidNdkPath` was null, *first*, before we checked
    `{androidSdk}\ndk-bundle`.

 2. If `PreferedAndroidNdkPath` happened to be null, then
    `AndroidSdkBase.Initialize()` would try to use
    `AllAndroidNdks.FirstOrDefault()` as a default value, also before
    checking `{androidSdk}\ndk-bundle`.  The problem here is that the
    `AllAndrdoidNdks` property uses [`Enumerable.Distinct()`][0], which
    returns an *unordered* list of directories.

That the test ever worked at all is a minor miracle.

Additionally, the support for `{androidSdk}\ndk-bundle` was Windows-
specific; it didn't run on macOS.

Update `AndroidSdkInfo` so that `{androidSdk}/ndk-bundle` is supported
on macOS, and that `{androidSdk}/ndk-bundle` is *preferred* when the
`androidNdkPath` parameter is `null`, *before* checking any other
plausible default locations.

This allows the `AndroidSdkInfoTests.Ndk_PathInSdk()` test to run
everywhere, and work reliably.

[0]: https://docs.microsoft.com/en-us/dotnet/api/system.linq.enumerable.distinct?view=netcore-3.1
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.

2 participants