Fix sort ordering for ndk-bundle, add macOS support #91

jonathanpeppers · 2020-06-30T13:39:58Z

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.

jonathanpeppers · 2020-06-30T19:39:01Z

Wait this isn't green:

1) Failed : Xamarin.Android.Tools.Tests.AndroidSdkInfoTests.Constructor_SetValuesFromPath
  AndroidNdkPath not set from $PATH!
  Expected string length 68 but was 44. Strings differ at index 1.
  Expected: "/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/tmp11587757...."
  But was:  "/Users/runner/Library/Android/sdk/ndk-bundle"
  ------------^
  at Xamarin.Android.Tools.Tests.AndroidSdkInfoTests.Constructor_SetValuesFromPath () [0x00000] in <4e091f45935d4073b64b6415de873d6b>:0

jonpryor · 2020-06-30T20:16:47Z

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

What should be the semantics of AndroidSdkInfo.AndroidNdkPath relative to .AndroidSdkPath when AndroidSdkInfo is constructed with androidSdkPath set (and valid), but androidNdkPath not set?

Should AndroidSdkInfo.AndroidNdkPath be the default system-wide path? Or should it be {androidSdkPath}/ndk-bundle?

Given that the local androidNdkPath variable/parameter is set before AllAndroidNdks/GetAllAvailableAndroidNdks() is accessed, it seems that the former would be the behavior, even with the current state of the PR.

I believe that what is more "reasonable" is the latter behavior: that if androidSdkPath is set, and it contains an ndk-bundle, then {androidSdkPath}/ndk-bundle should be preferred over an existing "default" system-wide preferred NDK dir.

In order for that to happen, I think we'd need to remove AndroidSdkInfo.cs:61. I think that would be "fine".

Relatedly, AllAndroidNdks/GetAllAvailableAndroidNdks() should return PreferedAndroidNdkPath "first" (or "second", after {androidSdkPath}/ndk-bundle}, should {androidSdkPath}/ndk-bundle` exist).

jonpryor · 2020-06-30T20:19:51Z

Related: #92

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.

Fixes: dotnet#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

jonathanpeppers · 2020-07-02T12:39:33Z

src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs

+					dirs.AddRange (GetAllAvailableAndroidSdks ());
+					allAndroidSdks = dirs.Where (d => ValidateAndroidSdkLocation (d))
+						.Select (d => d!)
+						.Distinct ()


@jonpryor will Distinct() lose the ordering again? or does the ordering not matter here?

Yes, it will lose the ordering again, but this is only accessed from AndroidSdkInfo.AllAndroidSdks, which "nobody" should be using, and which always had .Distinct() semantics, so anybody using that method shouldn't see anything bizarre.

I hope.

Confirmed we're only using GetAllAvailableAndroidSdks (and also every time we use it we're using Distinct too :)

jonpryor · 2020-07-14T20:23:44Z

Fixes: https://github.com/xamarin/xamarin-android-tools/issues/92

Context: https://github.com/xamarin/xamarin-android-tools/pull/90#issuecomment-651375196

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

jonathanpeppers requested a review from jonpryor June 30, 2020 13:39

jonathanpeppers marked this pull request as draft June 30, 2020 19:10

jonathanpeppers force-pushed the windows-ndk-bundle branch 2 times, most recently from ea98ace to 1a81057 Compare June 30, 2020 19:22

jonathanpeppers changed the title ~~[windows] prefer AndroidSdkPath\ndk-bundle~~ Fix sort ordering for ndk-bundle, add macOS support Jun 30, 2020

jonathanpeppers marked this pull request as ready for review June 30, 2020 19:38

jonathanpeppers marked this pull request as draft June 30, 2020 19:38

jonathanpeppers force-pushed the windows-ndk-bundle branch from 1a81057 to eefaef6 Compare June 30, 2020 20:10

jonathanpeppers force-pushed the windows-ndk-bundle branch from eefaef6 to ab45fd0 Compare June 30, 2020 20:16

jonathanpeppers force-pushed the windows-ndk-bundle branch from ab45fd0 to f8c43c9 Compare June 30, 2020 20:33

jonathanpeppers marked this pull request as ready for review June 30, 2020 20:46

jonpryor added 2 commits July 1, 2020 17:58

Merge remote-tracking branch 'origin/master' into windows-ndk-bundle

9232161

jonpryor requested a review from joj July 2, 2020 11:49

jonathanpeppers commented Jul 2, 2020

View reviewed changes

joj approved these changes Jul 2, 2020

View reviewed changes

jonpryor merged commit 5718cd2 into dotnet:master Jul 14, 2020

jonathanpeppers deleted the windows-ndk-bundle branch July 14, 2020 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix sort ordering for ndk-bundle, add macOS support #91

Fix sort ordering for ndk-bundle, add macOS support #91

Uh oh!

jonathanpeppers commented Jun 30, 2020 •

edited

Loading

Uh oh!

jonathanpeppers commented Jun 30, 2020

Uh oh!

jonpryor commented Jun 30, 2020

Uh oh!

jonpryor commented Jun 30, 2020

Uh oh!

jonathanpeppers Jul 2, 2020

Uh oh!

jonpryor Jul 2, 2020 •

edited

Loading

Uh oh!

joj Jul 2, 2020

Uh oh!

jonpryor commented Jul 14, 2020

Uh oh!

Uh oh!

Fix sort ordering for ndk-bundle, add macOS support #91

Fix sort ordering for ndk-bundle, add macOS support #91

Uh oh!

Conversation

jonathanpeppers commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonathanpeppers commented Jun 30, 2020

Uh oh!

jonpryor commented Jun 30, 2020

Uh oh!

jonpryor commented Jun 30, 2020

Uh oh!

jonathanpeppers Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

jonpryor Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joj Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

jonpryor commented Jul 14, 2020

Uh oh!

Uh oh!

jonathanpeppers commented Jun 30, 2020 •

edited

Loading

jonpryor Jul 2, 2020 •

edited

Loading