[Xamarin.Android.Build.Tasks] Prefer supported JDK versions

[jonpryor]

Context: https://devdiv.visualstudio.com/DevDiv/_releaseProgress?releaseId=1029208&_a=release-environment-extension&environmentId=5321844&extensionId=ms.vss-test-web.test-result-in-release-environment-editor-tab&runId=21424746&resultId=100001&paneView=attachments

Context: dotnet/android-tools@237642c
Context: dotnet/android-tools@dca30d9
Context: dotnet/android-tools@d92fc3e
Context: dotnet/android-tools@2241489

xamarin/xamarin-android-tools was updated to check for JDK locations
in a variety of locations, checking for these locations (1) after
a "preferred" JDK location, but (2) before $JAVA_HOME and the
Visual Studio-installed "microsoft_dist_openjdk_1.8.0.25" package.
Consequently, when running on an environment in which multiple JDKs
are installed, and an unsupported version of a JDK is present,
it's possible for build failures to result, as Xamarin.Android
attempted to use an unsupported version.

This happened during one of our internal CI runs:

Project templates should have zero errors
Expected: <empty>
But was:  < "
[Line]: 248
[Description]: Building with JDK version 14.0.2 is not supported. Please install JDK version 11.0. See https://aka.ms/xamarin/jdk9-errors (XA0030)
[File]: Xamarin.Android.Legacy.targets

This particular machine had AdoptOpenJDK 14 installed, in addition
to other probed-for JDK locations, but because it was found first --
JdkInfo.GetKnownSystemJdkInfos() doesn't know or care about
Xamarin.Android JDK supported versions, and we're trying to deprecate
the "microsoft_dist_openjdk*" package, and thus it shouldn't be
preferred anyway… -- the Xamarin.Android build attempted to use
JDK 14, when JDK 14 isn't supported.

This could happen because the <ResolveSdks/> task calls
MonoAndroidHelper.RefreshAndroidSdk(), which calls
the AndroidSdkInfo constructor, which calls
AndroidSdkBase.Initialize(), which calls
AndroidSdkBase.GetJavaSdkPaths(). If $(JavaSdkDirectory)
isn't set (or is invalid), and a preferred JDK isn't set or is
invalid, then AndoridSdkInfo will implicitly call
JdkInfo.GetKnownSystemJdkInfos(), which doesn't know anything about
Xamarin.Android's JDK version requirements.

Fix this scenario by improving the <ResolveSdks/> and
<ResolveJdkJvmPath/> tasks to use $(MinimumSupportedJavaVersion)
and $(LatestSupportedJavaVersion) to constrain which JDK we use,
using the first "known system JDK" which is greater than or equal to
$(MinimumSupportedJavaVersion) and less than or equal to
$(LatestSupportedJavaVersion).

The $(JavaSdkDirectory) MSBuild property is still preferred, if set,
followed by the "preferred JDK location", if set. It's only if
neither of these locations is set that we'll check for all known
system JDKs, at which point we now filter them.

[jonathanpeppers]

How was a project template even hitting this? $(AndroidGenerateJniMarshalMethods) would not be True, right?

[jonpryor]

🤔

Maybe this also needs to be in VS as well?

[jonpryor]

We can't find any place within VS that would need fixing.

However, I did find a codepath wherein <ResolveSdks/> calls AndroidSdkInfo, which eventually calls JdkInfo.GetKnownSystemJdkInfos() if/when the JDK path provided to the AndroidSdkInfo constructor isn't valid, and this is likely responsible for the integration test failure originally reported.

[jonpryor]

Thanks to @GouriKumari for bringing this scenario to our attention.

[jonathanpeppers]

This task gets called per Android project on every build. So I could see this adding incremental build time, especially in a larger solution. I think I've seen the JdkInfo lookup take 100ms before.

Is there a way we could pass the min/max JDK version through here instead:

https://github.com/xamarin/xamarin-android/blob/57e1a37c443c90f3487a93a312fbfef3441d2769/src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs#L116

If we want something easier to backport, maybe hardcoding something in xamarin-android-tools would work, too?

[jonathanpeppers]

Here is where I saw JDK lookups taking time: e624434

[jonpryor]

1. Should we merge <ResolveJdkJvmPath/> and <ResolveSdks/>, "reverting" and also adding the BuildEngine4.RegisterTaskObject() invocations to <ResolveSdks/>?
2. I saw e624434 in passing, and I wonder what exactly was being measured? (Aside: why does JdkInfo take 47ms to construct?! Is it the Lazy<T> construction, used to parse -XshowSettings:properties -version output?)

Not that I'm saying you didn't see that improvement. I'm wondering about the environment of the measurement. In particular, was msbuild /p:JavaSdkDirectory=… used to capture that timing?

If $(JavaSdkDirectory) isn't set -- which I assume is the default and more common occurrence -- then we'll almost certainly "always" hit https://github.com/xamarin/xamarin-android-tools/blob/683f37508b56c76c24b3287a5687743438625341/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs#L69

JavaSdkPath    = GetValidPath (ValidateJavaSdkLocation,     javaSdkPath,    () => PreferedJavaSdkPath,      () => GetJavaSdkPaths ());

javaSdkPath will be null, necessitating that we first try () => PreferedJavaSdkPath.

For most customer machines, this should return a valid value. This (presumably) is a net-win, and would be a reason to look into updating AndroidSdkInfo to have version constraints.

However, if () => PreferedJavaSdkPath fails -- which presumably is the case on this CI machine -- then we hit the () => GetJavaSdkPaths() codepath, which is likely why we're finding and attempting to use an unsupported JDK version.

---

Which I suppose leaves two questions:

1. Would BuildEngine4.RegisterTaskObject() within <ResolveSdks/> fix the problem?
2. Do we know what about JdkJvmPath lookup is slow? The directory traversal?

In retrospect, the "odd" thing about the pre-e624434 world order is that ResolveSdks.GetJvmPath() even existed in the first place, as constructed; once we have a JDK path, why would GetJvmPath() need to call JdkInfo.GetKnownSystemJdkInfos() again?

Looks like that was always the case, though:

1a2eb95#diff-ec49e6de47ef6cb5b59095d242a56acce5830d39402bf075722fce7dd1326612R105

Also odd that I kinda/sorta mentioned the "oddness" of having a JdkInfo.GetKnownSystemJdkInfos() invocation, then forgot about how odd that was: #2153 (comment)

I don't see why, given a valid JDK path, JdkJvmPath lookup should be so slow; via csharp:

using Xamarin.Android.Tools;
using System.Diagnostics;
using System.IO
var validJdkPath=Path.Combine(Environment.GetEnvironmentVariable("HOME"), "Library/Developer/Xamarin/jdk/microsoft_dist_openjdk_1.8.0.25");
var s = Stopwatch.StartNew(); var p = new JdkInfo(validJdkPath).JdkJvmPath; s.Stop();
s.ElapsedMilliseconds
// 26 on first invocation, 0 on subsequent iterations.  Behold the JIT!

So maybe JdkJvmPath is slow, because of JIT overheads (ugh), and no amount of caching can fix things when we're concerned about design-time builds on the first build. It doesn't matter if subsequent builds are faster (yay JIT!), when we care about the Design-Time build, which won't need JdkJvmPath…

But what about the JDK itself?

using Xamarin.Android.Tools;
using System.Diagnostics;
using System.IO
var s = Stopwatch.StartNew(); var p = new AndroidSdkInfo().JavaSdkPath; s.Stop();
s.ElapsedMilliseconds;
print(p);

This is "differently fun"; if I edit $HOME/.config/xbuild/monodroid-config.xml and remove the <java-sdk/> element -- removing the "preferred JDK" location -- then the above takes 81ms to run, while if I have a <java-sdk/> element -- providing a "preferred JDK" location -- then the above takes 55ms to run, a savings of 26ms when the preferred JDK location is set and exists.

[jonathanpeppers]

When I was testing e624434, I probably didn't have JavaSdkDirectory set at all and was testing from the command-line.

Let me just test this change tomorrow on Windows/.NET framework with a Release build and see how long it takes.

Were your numbers above a Release build?

[jonpryor]

My numbers above were for a Debug build of Xamarin.Android.Tools.AndroidSdk.dll, run at the command-line via csharp:

$ csharp -r:bin/Debug/netstandard2.0/Xamarin.Android.Tools.AndroidSdk.dll
Mono C# Shell, type "help;" for help

Enter statements below.
csharp>

I then pasted the commands above, hence the Stopwatch use. I avoided the csharp> "prefix" in the above pastes so that you could copy & paste the commands into the terminal.

[jonathanpeppers]

I just tried a Release build of Xamarin.Android on Windows, and the changes here don't seem to be any worse with default settings:

> .\bin\Release\bin\xabuild .\samples\HelloWorld\HelloWorld.csproj -bl
...
Task ResolveSdks 20ms
...
Took: 00:00:00.0033017
...
Task ResolveSdks 3ms
...
Took: 00:00:00.0008511

So it would only be slower in the case this new catch block runs, which should be rare?

My timing was:

+                       Stopwatch w = new Stopwatch();
+                       w.Start();
                        var minVersion      = Version.Parse (MinimumSupportedJavaVersion);
                        var maxVersion      = Version.Parse (LatestSupportedJavaVersion);

                        JavaSdkPath         = MonoAndroidHelper.GetJdkInfo (this.CreateTaskLogger (), JavaSdkPath, minVersion, maxVersion)?.HomePath;
+                       w.Stop();
+                       Log.LogDebugMessage("Took: " + w.Elapsed);

[jonathanpeppers]

Oh, maybe these tests fail now, though:

[jonpryor]

Oh, maybe these tests fail now, though:

Hopefully fixed in my latest force-push.

[jonathanpeppers]

I think different ones are failing now (or I didn't notice them before):

[jonpryor]

@jonathanpeppers commented:

I think different ones are failing now (or I didn't notice them before):

This was because ResolveSdksTaskTests.cs directly used <ResolveSdks/>, and wasn't setting the newly added (and now required) MinimumSupportedJavaVersion and LatestSupportedJavaVersion properties.

The unit test has been updated.