[Java.Interop.BootstrapTasks] Support single-digit versions #327

jonpryor · 2018-05-25T21:48:27Z

The <JdkInfo/> task sorts preferred JDK versions based on the
version number embedded into the directory name.

This unfortunately requires that the version number embedded into the
directory name contain two or more version parts. This is true on
macOS and Windows:

    # macOS
    $ ls -1tr /Library/Java/JavaVirtualMachines/
    1.6.0_65-b14-462.jdk
    jdk1.8.0_77.jdk
    jdk-9.0.4.jdk

But this is not necessarily the case on Linux:

    $ ls -1 /usr/lib/jvm
    java-8-openjdk-amd64
    java-8-oracle

Single-part version numbers aren't supported by Version.TryParse(),
so when $(JI_MAX_JDK) is set -- causing maxVersion to be non-null
-- while the version isn't extracted from the directory name, the
comparison can fail:

    The "JdkInfo" task failed unexpectedly.
    System.ArgumentNullException: Value cannot be null.
    Parameter name: v1
      at System.Version.op_LessThanOrEqual (System.Version v1, System.Version v2) [0x00003] in <2f83d2ff70e3444cb3582fe4e97bad63>:0
      at Java.Interop.BootstrapTasks.JdkInfo+<>c__DisplayClass31_0.<GetJavaHomePathFromMachine>b__2 (<>f__AnonymousType0`2[<Path>j__TPar,<Version>j__TPar] v) [0x00000] in …/xamarin-android/external/Java.Interop/src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs:172
      ...
      at Java.Interop.BootstrapTasks.JdkInfo.GetJavaHomePathFromMachine (System.Version maxVersion) [0x00015] in …/xamarin-android/external/Java.Interop/src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs:163
      at Java.Interop.BootstrapTasks.JdkInfo.Execute () [0x00176] in …/xamarin-android/external/Java.Interop/src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs:56

Fix this by allowing extraction of Version numbers from single-part
version codes as seen on Linux, allowing us to perform comparisons
against non-null versions.

jonathanpeppers

Ok, I think I see one other thing that could break here.

If we hit any of the null cases here: https://github.com/xamarin/java.interop/blob/ca27edd23f4dfa2700cf28deaabfe2d74fc72a93/src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs#L105-L115

Like if there was a random directory in there with no numbers.

I think we need a null check for Version somewhere in this LINQ expression: https://github.com/xamarin/java.interop/blob/ca27edd23f4dfa2700cf28deaabfe2d74fc72a93/src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs#L168-L175

jonpryor · 2018-05-26T02:27:05Z

Ok, I think I see one other thing that could break here.

Yup, that's correct.

There's another, closely related, problem:

What should we do about ties?

The original comment lays out the scenario, but doesn't ask (or answer!) the question:

Given this directory:

$ ls -1 /usr/lib/jvm
java-8-openjdk-amd64
java-8-oracle

Which directory should be preferred, by default?

I posed this question to @grendello, and he suggested using readlink /etc/alternatives/java. While plausible, this only answers the question on Linux (in which /etc/alternatives/java may exist). In other platforms...if you "somehow" have two "identical" versions, which should we use?

The pre- 4bd9297 answer was to use the directory with the most recent timestamp: https://github.com/xamarin/java.interop/blob/4bd9297f311b3e0fb3e07335507a38278b83d255^/build-tools/scripts/jdk.mk#L81:

_DARWIN_JDK_ROOT      := $(shell ls -dtr $(_DARWIN_JDK_FALLBACK_DIRS) | $(_VERSION_SORT))

(This was why we used ls in the first place!)

I suspect we should return to this behavior of preferring the directory with the most recent timestamp.

jonathanpeppers · 2018-05-26T02:33:42Z

It's good you already have LINQ:
OrderBy -> Version
ThenBy -> new DirectoryInfo (v.Path).LastWriteTimeUtc (or whatever it really is)

I think picking the highest version, then timestamp should still be doable.

radekdoulik · 2018-05-28T09:28:19Z

For equal versions, the most recent timestamps sounds good to me.

BTW, various linuxes differ and on my dated Fedora I see this:

ls -1 /usr/lib/jvm
java-1.7.0-openjdk-1.7.0.60-2.4.3.0.fc20.x86_64
java-1.8.0-openjdk-1.8.0.121-10.b14.fc25.x86_64
java-1.8.0-openjdk-1.8.0.121-8.b14.fc25.x86_64
java-1.8.0-openjdk-1.8.0.40-25.b25.fc21.x86_64
jre
jre-1.8.0
jre-1.8.0-openjdk
jre-1.8.0-openjdk-1.8.0.121-10.b14.fc25.x86_64
jre-openjdk

readlink /etc/alternatives/java
/usr/lib/jvm/java-1.8.0-openjdk-1.8.0.121-10.b14.fc25.x86_64/jre/bin/java

So using /etc/alternatives/java on Linux, where available, looks OK to me as well.

jonpryor · 2018-05-28T13:55:33Z

@radekdoulik: can you do the ls -l /usr/lib/jvm output? :-)

The regex we use for sorting only looks at the first match in a filename. As such, there will be 6 matches for 1.8.0 on your machine. Presumably if we order by timestamp then java-1.8.0-openjdk-1.8.0.121-10.b14.fc25.x86_64 or jre-1.8.0-openjdk-1.8.0.121-10.b14.fc25.x86_64 may match.

Do the jre-* directories even contain javac and libjvm.so?

We might also want to filter out directories which don't fulfill the requirements.

radekdoulik · 2018-05-28T19:06:39Z

I assume we use ctime here:

ls -lct /usr/lib/jvm
total 16
lrwxrwxrwx. 1 root root   35 Mar 31  2017 jre-1.8.0-openjdk -> /etc/alternatives/jre_1.8.0_openjdk
lrwxrwxrwx. 1 root root   27 Mar 31  2017 jre-1.8.0 -> /etc/alternatives/jre_1.8.0
lrwxrwxrwx. 1 root root   29 Mar 31  2017 jre-openjdk -> /etc/alternatives/jre_openjdk
lrwxrwxrwx. 1 root root   21 Mar 31  2017 jre -> /etc/alternatives/jre
lrwxrwxrwx. 1 root root   51 Mar 31  2017 jre-1.8.0-openjdk-1.8.0.121-10.b14.fc25.x86_64 -> java-1.8.0-openjdk-1.8.0.121-10.b14.fc25.x86_64/jre
drwxr-xr-x. 3 root root 4096 Mar 31  2017 java-1.8.0-openjdk-1.8.0.121-10.b14.fc25.x86_64
drwxr-xr-x. 3 root root 4096 Mar 30  2017 java-1.8.0-openjdk-1.8.0.121-8.b14.fc25.x86_64
drwxr-xr-x. 3 root root 4096 Apr 16  2015 java-1.8.0-openjdk-1.8.0.40-25.b25.fc21.x86_64
drwxr-xr-x. 3 root root 4096 Oct 25  2014 java-1.7.0-openjdk-1.7.0.60-2.4.3.0.fc20.x86_64

The jre-* tree does not contain javac. It contains libjvm.so.

find /usr/lib/jvm/java-1.8.0-openjdk-1.8.0.121-10.b14.fc25.x86_64/jre | grep jvm.so
/usr/lib/jvm/java-1.8.0-openjdk-1.8.0.121-10.b14.fc25.x86_64/jre/lib/amd64/server/libjvm.so

grendello · 2018-05-29T12:04:47Z

I find the "use the latest timestamp" approach to choose between Oracle Java and OpenJDK a bit dangerous. They are supposed to be compatible but, as we know from experience, they're not. Maybe we should, in this case, apply a simple check - look for oracle in the directory string - and use the "official" version. Also, all the Linux systems should export JAVA_HOME via /etc/profile.d/jvm.{sh,csh} file which is included in all shell instances (not just interactive ones). Linux build bot fails because it runs the build in a chroot and we need to bug @directhex to set it up properly. The alternatives mechanism should be the next approach we take with the heuristic/timestamp one becoming the very last resort (even perhaps after simply running javac -version and trying to figure out the version to use from there) On Mac the equvalent is to use /usr/libexec/java_home to locate the current default Java version.

jonpryor · 2018-05-29T17:33:40Z

New And Improved™!

Current version now tries the output of readlink /etc/alternatives/java and /usr/libexec/java_home before hitting the "fallback" path.

We also add a "JDK Validation" method to ensure that not only does e.g. $JAVA_HOME contain a valid version -- when $(JI_MAX_JDK) is set -- but also that it is otherwise valid: it contains jar, javac, a JVM library, and an include directory.

grendello · 2018-05-29T17:45:25Z

src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs

-					Log.LogMessage (MessageImportance.Low, $"  Skipping JAVA_HOME default value of `{java_home}` as it does not contain an `include` subdirectory.");
-					java_home = null;
-				}
+			if (java_home != null &&


the java_home != null check is redundant, since ValidateJdkPath repeats it. It might look neater if the null check is completely removed

grendello · 2018-05-29T17:46:36Z

src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs

+		bool ValidateJdkPath (Version maxVersion, string java_home)
+		{
+			return ValidateJdkPath (maxVersion, java_home,
+					out var jar, out var javac, out var jdk, out var include);


Looks like a perfect place to use C#7 discards :)

grendello · 2018-05-29T17:48:08Z

src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs

+					Log.LogMessage (MessageImportance.Low, $"    {e.Data}");
+					path = e.Data;
+			});
+			return path;


/etc/alternatives/java points to the java executable, not home path:

$ readlink /etc/alternatives/java /usr/lib/jvm/java-8-oracle/jre/bin/java

so more processing is necessary here to match output from e.g. macOS java_home

After the force-push update, this comment is now on the wrong method. ;-)

It has been addressed, by sending the output of readlink /etc/alternatives/java through GetJavaHomePathFromJava(), which runs java -XshowSettings:properties ....

The `<JdkInfo/>` task sorts preferred JDK versions based on the version number embedded into the directory name. This unfortunately requires that the version number embedded into the directory name contain two or more version parts. This is true on macOS and Windows: # macOS $ ls -1tr /Library/Java/JavaVirtualMachines/ 1.6.0_65-b14-462.jdk jdk1.8.0_77.jdk jdk-9.0.4.jdk But this is not necessarily the case on Linux: $ ls -1 /usr/lib/jvm java-8-openjdk-amd64 java-8-oracle Single-part version numbers aren't supported by `Version.TryParse()`, so when `$(JI_MAX_JDK)` is set -- causing `maxVersion` to be non-null -- while the version *isn't* extracted from the directory name, the [comparison can fail][0]: [0]: https://jenkins.mono-project.com/job/xamarin-anroid-linux-pr-builder/2917/console The "JdkInfo" task failed unexpectedly. System.ArgumentNullException: Value cannot be null. Parameter name: v1 at System.Version.op_LessThanOrEqual (System.Version v1, System.Version v2) [0x00003] in <2f83d2ff70e3444cb3582fe4e97bad63>:0 at Java.Interop.BootstrapTasks.JdkInfo+<>c__DisplayClass31_0.<GetJavaHomePathFromMachine>b__2 (<>f__AnonymousType0`2[<Path>j__TPar,<Version>j__TPar] v) [0x00000] in …/xamarin-android/external/Java.Interop/src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs:172 ... at Java.Interop.BootstrapTasks.JdkInfo.GetJavaHomePathFromMachine (System.Version maxVersion) [0x00015] in …/xamarin-android/external/Java.Interop/src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs:163 at Java.Interop.BootstrapTasks.JdkInfo.Execute () [0x00176] in …/xamarin-android/external/Java.Interop/src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs:56 Fix this by allowing extraction of Version numbers from single-part version codes as seen on Linux, allowing us to perform comparisons against non-null versions. If multiple directories *still* have matching version numbers, then we sort the output so that the location with the most recent write time is used. Additionally, check some OS-specific preferred locations: * On macOS, `/usr/libexec/java_home`, which is a program which prints out the preferred JDK location to stdout. * On Linux, `/usr/alternatives/java` is a symlink to the preferred JDK location, which we can read using `readlink`. Treat the OS-specific locations similar to `$JAVA_HOME`, and check them *before* attempting the `JdkInfo.JdksRoot` children.

jonathanpeppers

So much code, to just "find java" on three platforms...

My only suggestion at this point might be to somehow move this code to a single cs file that could be reused in other projects without a dependency on MSBuild. Then, for example, I could add java.interop as a submodule to something like Embeddinator-4000 and even use the code within a Cake script. This could be done later, too.

radekdoulik

I like the idea of extracting the code later to own class/source file. I guess it will be handy later when running jinmarshalmethod-gen.exe on Windows.

jonpryor · 2018-05-30T00:25:53Z

In the interest of getting things ready for branching, we're merging now that it's green. :-)

That said:

I guess it will be handy later when running jinmarshalmethod-gen.exe on Windows.

I don't think this would be useful; xamarin-android already has that information. We simply need to provide that information to jnimarshalmethod-gen.exe, presumably via command-line options.

radekdoulik · 2018-05-30T08:27:45Z

Does XA have it when installed at a user machine, ie. not XA build time? What I remembered was that we need to introduce a way to get it at runtime. Maybe I remembered it wrong then. Where in the XA do we have it?

jonpryor · 2018-06-01T01:43:17Z

Does XA have it when installed at a user machine

The <JdkInfo/> task/Java.Interop.BootstrapTasks.dll assembly is not installed onto the end-user machine.

The Xamarin.Android <ResolveSdks/> task has similar functionality, but not quite identical: it won't probe a ton of directories trying to find a "best" JDK, it'll instead read from the config file/registry to find the previously set values, and if they're not set, error out.

jonpryor force-pushed the jonp-JdkInfo-single-digit-versions branch from 9a4c98c to efcea05 Compare May 25, 2018 21:56

jonpryor requested review from grendello, jonathanpeppers and radekdoulik May 25, 2018 22:05

jonathanpeppers reviewed May 26, 2018

View reviewed changes

jonpryor mentioned this pull request May 29, 2018

[build] Prepare Java.Runtime.Environment.Override.dllmap dotnet/android#1739

Closed

jonpryor force-pushed the jonp-JdkInfo-single-digit-versions branch from efcea05 to b01878e Compare May 29, 2018 17:31

grendello requested changes May 29, 2018

View reviewed changes

jonpryor force-pushed the jonp-JdkInfo-single-digit-versions branch from b01878e to d03ff8c Compare May 29, 2018 18:04

grendello approved these changes May 29, 2018

View reviewed changes

jonathanpeppers approved these changes May 29, 2018

View reviewed changes

radekdoulik approved these changes May 29, 2018

View reviewed changes

jonpryor merged commit bed2958 into dotnet:master May 30, 2018

github-actions bot locked and limited conversation to collaborators Apr 14, 2024

[Java.Interop.BootstrapTasks] Support single-digit versions #327

[Java.Interop.BootstrapTasks] Support single-digit versions #327

Uh oh!

Conversation

jonpryor commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonathanpeppers left a comment

Choose a reason for hiding this comment

Uh oh!

jonpryor commented May 26, 2018

Uh oh!

jonathanpeppers commented May 26, 2018

Uh oh!

radekdoulik commented May 28, 2018

Uh oh!

jonpryor commented May 28, 2018

Uh oh!

radekdoulik commented May 28, 2018

Uh oh!

grendello commented May 29, 2018

Uh oh!

jonpryor commented May 29, 2018

Uh oh!

grendello May 29, 2018

Choose a reason for hiding this comment

Uh oh!

grendello May 29, 2018

Choose a reason for hiding this comment

Uh oh!

grendello May 29, 2018

Choose a reason for hiding this comment

Uh oh!

jonpryor May 29, 2018

Choose a reason for hiding this comment

Uh oh!

jonathanpeppers left a comment

Choose a reason for hiding this comment

Uh oh!

radekdoulik left a comment

Choose a reason for hiding this comment

Uh oh!

jonpryor commented May 30, 2018

Uh oh!

radekdoulik commented May 30, 2018

Uh oh!

jonpryor commented Jun 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonpryor commented May 25, 2018 •

edited

Loading