Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 11, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/Xamarin.Android.Build.Tasks/Tasks/ResolveJdkJvmPath.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ public class ResolveJdkJvmPath : AndroidTask
[Output]
public string JdkJvmPath { get; set; }

[Required]
public string MinimumSupportedJavaVersion { get; set; }

[Required]
public string LatestSupportedJavaVersion { get; set; }

public override bool RunTask ()
{
try {
Expand Down Expand Up @@ -54,12 +60,10 @@ string GetJvmPath ()
return cached;
}

JdkInfo info = null;
try {
info = new JdkInfo (JavaSdkPath);
} catch {
info = JdkInfo.GetKnownSystemJdkInfos (this.CreateTaskLogger ()).FirstOrDefault ();
}
var minVersion = Version.Parse (MinimumSupportedJavaVersion);
var maxVersion = Version.Parse (LatestSupportedJavaVersion);

JdkInfo info = MonoAndroidHelper.GetJdkInfo (this.CreateTaskLogger (), JavaSdkPath, minVersion, maxVersion);

if (info == null)
return null;
Expand Down
11 changes: 11 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Tasks/ResolveSdksTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ public class ResolveSdks : AndroidTask

public string CommandLineToolsVersion { get; set; }

[Required]
public string MinimumSupportedJavaVersion { get; set; }

[Required]
public string LatestSupportedJavaVersion { get; set; }

[Output]
public string CommandLineToolsPath { get; set; }

Expand Down Expand Up @@ -81,6 +87,11 @@ public override bool RunTask ()
MonoAndroidLibPath = MonoAndroidHelper.GetOSLibPath () + Path.DirectorySeparatorChar;
AndroidBinUtilsPath = MonoAndroidBinPath + "ndk" + Path.DirectorySeparatorChar;

var minVersion = Version.Parse (MinimumSupportedJavaVersion);
var maxVersion = Version.Parse (LatestSupportedJavaVersion);

JavaSdkPath = MonoAndroidHelper.GetJdkInfo (this.CreateTaskLogger (), JavaSdkPath, minVersion, maxVersion)?.HomePath;
Comment on lines +90 to +93
Copy link
Member

@jonathanpeppers jonathanpeppers May 6, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Member

@jonathanpeppers jonathanpeppers May 6, 2021

Choose a reason for hiding this comment

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

Here is where I saw JDK lookups taking time: e624434

Copy link
Contributor Author

@jonpryor jonpryor May 6, 2021

Choose a reason for hiding this comment

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

  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.

Copy link
Member

@jonathanpeppers jonathanpeppers May 7, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@jonpryor jonpryor May 7, 2021

Choose a reason for hiding this comment

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

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.


MonoAndroidHelper.RefreshSupportedVersions (ReferenceAssemblyPaths);

try {
Expand Down
4 changes: 2 additions & 2 deletions src/Xamarin.Android.Build.Tasks/Tasks/ValidateJavaVersion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,12 @@ public override bool RunTask ()
// `java -version` will produce values such as:
// java version "9.0.4"
// java version "1.8.0_77"
static readonly Regex JavaVersionRegex = new Regex (@"version ""(?<version>[\d\.]+)(_d+)?[^""]*""");
static readonly Regex JavaVersionRegex = new Regex (@"version ""(?<version>[\d\.]+)(_\d+)?[^""]*""");

// `javac -version` will produce values such as:
// javac 9.0.4
// javac 1.8.0_77
static readonly Regex JavacVersionRegex = new Regex (@"(?<version>[\d\.]+)(_d+)?");
internal static readonly Regex JavacVersionRegex = new Regex (@"(?<version>[\d\.]+)(_\d+)?");

bool ValidateJava ()
{
Expand Down
10 changes: 10 additions & 0 deletions ...marin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/ResolveSdksTaskTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ public class ResolveSdksTaskTests : BaseTest {
new ApiInfo () { Id = "Z", Level = 127, Name = "Z", FrameworkVersion = "v108.1.99", Stable = false },
};

// via Xamarin.Android.Common.props
const string MinimumSupportedJavaVersion = "1.6.0";
const string LatestSupportedJavaVersion = "11.0.99";

static object [] UseLatestAndroidSdkTestCases = new object [] {
new object[] {
/* buildtools */ "26.0.3",
Expand Down Expand Up @@ -167,6 +171,8 @@ public void UseLatestAndroidSdk (string buildtools, string jdk, ApiInfo[] apis,
AndroidSdkPath = androidSdkPath,
AndroidNdkPath = androidNdkPath,
JavaSdkPath = javaPath,
MinimumSupportedJavaVersion = MinimumSupportedJavaVersion,
LatestSupportedJavaVersion = LatestSupportedJavaVersion,
ReferenceAssemblyPaths = new [] {
Path.Combine (referencePath, "MonoAndroid"),
},
Expand Down Expand Up @@ -220,6 +226,8 @@ public void ResolveSdkTiming ()
AndroidSdkPath = androidSdkPath,
AndroidNdkPath = androidNdkPath,
JavaSdkPath = javaPath,
MinimumSupportedJavaVersion = MinimumSupportedJavaVersion,
LatestSupportedJavaVersion = LatestSupportedJavaVersion,
ReferenceAssemblyPaths = new [] {
Path.Combine (referencePath, "MonoAndroid"),
},
Expand Down Expand Up @@ -391,6 +399,8 @@ public void TargetFrameworkPairing (string description, ApiInfo[] androidSdk, Ap
AndroidSdkPath = androidSdkPath,
AndroidNdkPath = androidNdkPath,
JavaSdkPath = javaPath,
MinimumSupportedJavaVersion = MinimumSupportedJavaVersion,
LatestSupportedJavaVersion = LatestSupportedJavaVersion,
ReferenceAssemblyPaths = new [] {
Path.Combine (referencePath, "MonoAndroid"),
},
Expand Down
109 changes: 77 additions & 32 deletions src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/BaseTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,48 +389,93 @@ protected string CreateFauxReferencesDirectory (string path, ApiInfo [] versions

protected string CreateFauxJavaSdkDirectory (string path, string javaVersion, out string javaExe, out string javacExe)
{
javaExe = IsWindows ? "Java.cmd" : "java.bash";
javacExe = IsWindows ? "Javac.cmd" : "javac.bash";
var jarSigner = IsWindows ? "jarsigner.exe" : "jarsigner";
javaExe = IsWindows ? "java.cmd" : "java";
javacExe = IsWindows ? "javac.cmd" : "javac";

var javaPath = Path.Combine (Root, path);
var javaBinPath = Path.Combine (javaPath, "bin");
Directory.CreateDirectory (javaBinPath);

CreateFauxJavaExe (Path.Combine (javaBinPath, javaExe), javaVersion);
CreateFauxJavacExe (Path.Combine (javaBinPath, javacExe), javaVersion);
CreateFauxJdk (javaPath, javaVersion, javaVersion, javaVersion);

var jarSigner = IsWindows ? "jarsigner.exe" : "jarsigner";
var javaBinPath = Path.Combine (javaPath, "bin");
File.WriteAllText (Path.Combine (javaBinPath, jarSigner), "");

return javaPath;
}

void CreateFauxJavaExe (string javaExeFullPath, string version)
// https://github.com/xamarin/xamarin-android-tools/blob/683f37508b56c76c24b3287a5687743438625341/tests/Xamarin.Android.Tools.AndroidSdk-Tests/JdkInfoTests.cs#L60-L100
void CreateFauxJdk (string dir, string releaseVersion, string releaseBuildNumber, string javaVersion)
{
var sb = new StringBuilder ();
if (IsWindows) {
sb.AppendLine ("@echo off");
sb.AppendLine ($"echo java version \"{version}\"");
sb.AppendLine ($"echo Java(TM) SE Runtime Environment (build {version}-b13)");
sb.AppendLine ($"echo Java HotSpot(TM) 64-Bit Server VM (build 25.101-b13, mixed mode)");
} else {
sb.AppendLine ("#!/bin/bash");
sb.AppendLine ($"echo \"java version \\\"{version}\\\"\"");
sb.AppendLine ($"echo \"Java(TM) SE Runtime Environment (build {version}-b13)\"");
sb.AppendLine ($"echo \"Java HotSpot(TM) 64-Bit Server VM (build 25.101-b13, mixed mode)\"");
}
CreateFauxExecutable (javaExeFullPath, sb);
}

void CreateFauxJavacExe (string javacExeFullPath, string version)
Directory.CreateDirectory (dir);

using (var release = new StreamWriter (Path.Combine (dir, "release"))) {
release.WriteLine ($"JAVA_VERSION=\"{releaseVersion}\"");
release.WriteLine ($"BUILD_NUMBER={releaseBuildNumber}");
release.WriteLine ($"JUST_A_KEY");
}

var bin = Path.Combine (dir, "bin");
var inc = Path.Combine (dir, "include");
var jre = Path.Combine (dir, "jre");
var jli = Path.Combine (jre, "lib", "jli");

Directory.CreateDirectory (bin);
Directory.CreateDirectory (inc);
Directory.CreateDirectory (jli);
Directory.CreateDirectory (jre);

string quote = IsWindows ? "" : "\"";
string java = IsWindows
? $"echo java version \"{javaVersion}\"{Environment.NewLine}"
: $"echo java version '\"{javaVersion}\"'{Environment.NewLine}";
java = java +
$"echo Property settings:{Environment.NewLine}" +
$"echo {quote} java.home = {dir}{quote}{Environment.NewLine}" +
$"echo {quote} java.vendor = Xamarin.Android Unit Tests{quote}{Environment.NewLine}" +
$"echo {quote} java.version = {javaVersion}{quote}{Environment.NewLine}" +
$"echo {quote} xamarin.multi-line = line the first{quote}{Environment.NewLine}" +
$"echo {quote} line the second{quote}{Environment.NewLine}" +
$"echo {quote} .{quote}{Environment.NewLine}";

string javac =
$"echo javac {javaVersion}{Environment.NewLine}";

CreateShellScript (Path.Combine (bin, "jar"), "");
CreateShellScript (Path.Combine (bin, "java"), java);
CreateShellScript (Path.Combine (bin, "javac"), javac);
CreateShellScript (Path.Combine (jli, "libjli.dylib"), "");
CreateShellScript (Path.Combine (jre, "libjvm.so"), "");
CreateShellScript (Path.Combine (jre, "jvm.dll"), "");
}

// https://github.com/xamarin/xamarin-android-tools/blob/683f37508b56c76c24b3287a5687743438625341/tests/Xamarin.Android.Tools.AndroidSdk-Tests/JdkInfoTests.cs#L108-L132
void CreateShellScript (string path, string contents)
{
var sb = new StringBuilder ();
if (IsWindows) {
sb.AppendLine ("@echo off");
sb.AppendLine ($"echo javac {version}");
} else {
sb.AppendLine ("#!/bin/bash");
sb.AppendLine ($"echo \"javac {version}\"");
if (IsWindows && string.Compare (Path.GetExtension (path), ".dll", true) != 0)
path += ".cmd";
using (var script = new StreamWriter (path)) {
if (IsWindows) {
script.WriteLine ("@echo off");
}
else {
script.WriteLine ("#!/bin/sh");
}
script.WriteLine (contents);
}
CreateFauxExecutable (javacExeFullPath, sb);
if (IsWindows)
return;
var chmod = new ProcessStartInfo {
FileName = "chmod",
Arguments = $"+x \"{path}\"",
UseShellExecute = false,
RedirectStandardInput = false,
RedirectStandardOutput = true,
RedirectStandardError = true,
CreateNoWindow = true,
WindowStyle = ProcessWindowStyle.Hidden,
};
var p = Process.Start (chmod);
p.WaitForExit ();
}

void CreateFauxExecutable (string exeFullPath, StringBuilder sb) {
Expand Down
13 changes: 13 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,19 @@ public static void RefreshSupportedVersions (string[] referenceAssemblyPaths)
}
#endif // MSBUILD

public static JdkInfo GetJdkInfo (Action<TraceLevel, string> logger, string javaSdkPath, Version minSupportedVersion, Version maxSupportedVersion)
{
JdkInfo info = null;
try {
info = new JdkInfo (javaSdkPath);
} catch {
info = JdkInfo.GetKnownSystemJdkInfos (logger)
.Where (jdk => jdk.Version >= minSupportedVersion && jdk.Version <= maxSupportedVersion)
.FirstOrDefault ();
}
return info;
}

class SizeAndContentFileComparer : IEqualityComparer<FileInfo>
#if MSBUILD
, IEqualityComparer<ITaskItem>
Expand Down
4 changes: 4 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Xamarin.Android.Tooling.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ projects.
AndroidSdkPath="$(AndroidSdkDirectory)"
AndroidNdkPath="$(AndroidNdkDirectory)"
JavaSdkPath="$(JavaSdkDirectory)"
MinimumSupportedJavaVersion="$(MinimumSupportedJavaVersion)"
LatestSupportedJavaVersion="$(LatestSupportedJavaVersion)"
ReferenceAssemblyPaths="$(_XATargetFrameworkDirectories)">
<Output TaskParameter="CommandLineToolsPath" PropertyName="_AndroidToolsDirectory" />
<Output TaskParameter="AndroidNdkPath" PropertyName="AndroidNdkDirectory" Condition=" '$(AndroidNdkDirectory)' == '' " />
Expand All @@ -82,6 +84,8 @@ projects.
</ResolveSdks>
<ResolveJdkJvmPath
JavaSdkPath="$(_JavaSdkDirectory)"
MinimumSupportedJavaVersion="$(MinimumSupportedJavaVersion)"
LatestSupportedJavaVersion="$(LatestSupportedJavaVersion)"
Condition=" '$(DesignTimeBuild)' != 'True' And '$(_AndroidIsBindingProject)' != 'True' And '$(AndroidGenerateJniMarshalMethods)' == 'True' And '$(JdkJvmPath)' == '' ">
Copy link
Member

@jonathanpeppers jonathanpeppers May 6, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jonpryor jonpryor May 6, 2021

Choose a reason for hiding this comment

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

🤔

Maybe this also needs to be in VS as well?

Copy link
Contributor Author

@jonpryor jonpryor May 6, 2021

Choose a reason for hiding this comment

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

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.

<Output TaskParameter="JdkJvmPath" PropertyName="JdkJvmPath" />
</ResolveJdkJvmPath>
Expand Down