Skip to content

Conversation

@jonpryor
Copy link
Contributor

@jonpryor jonpryor commented Aug 3, 2018

Context: dotnet/android#2004 (comment)

There are a two (related) "usability" issues with
JdkInfo.GetKnownSystemJdkInfos():

  1. It returns duplicate paths, and
  2. There's no way of knowing where a path "came from"

For example:

$ csharp -r:bin/Debug/Xamarin.Android.Tools.AndroidSdk.dll
csharp> using Xamarin.Android.Tools;
csharp> JdkInfo.GetKnownSystemJdkInfos();
{
  JdkInfo(Version=1.8.0.77, Vendor="Oracle Corporation", HomePath="/Library/Java/JavaVirtualMachines/jdk1.8.0_77.jdk/Contents/Home"),
  Not a valid JDK directory: `/Library/Java/JavaVirtualMachines/1.6.0_65-b14-462.jdk/Contents/Home`
  System.ArgumentException: Could not find required file `jvm` within `/Library/Java/JavaVirtualMachines/1.6.0_65-b14-462.jdk/Contents/Home`; is this a valid JDK?
  Parameter name: homePath
    at Xamarin.Android.Tools.JdkInfo.ValidateFile (System.String name, System.String path) [0x00025] in <087d9a26808b4d2d9c5d2844064ae952>:0
    at Xamarin.Android.Tools.JdkInfo..ctor (System.String homePath, System.String locator) [0x00120] in <087d9a26808b4d2d9c5d2844064ae952>:0
    at Xamarin.Android.Tools.JdkInfo.TryGetJdkInfo (System.String path, System.Action`2[T1,T2] logger, System.String locator) [0x00004] in <087d9a26808b4d2d9c5d2844064ae952>:0
  JdkInfo(Version=1.8.0.77, Vendor="Oracle Corporation", HomePath="/Library/Java/JavaVirtualMachines/jdk1.8.0_77.jdk/Contents/Home"),
}

Notice that on my machine jdk1.8.0_77.jdk is listed twice.

Why is it listed twice? It's listed twice because it comes from two
different "locators": JdkInfo.GetPathEnvironmentJdks(), which reads
$PATH, and JdkInfo.GetLibexecJdks(), which parses the output of
/usr/libexec/java_home -X. If $JAVA_HOME were set to the same
location, it would be listed three times.

As-is, there's no way of knowing where a path is "coming from"; the
"locator" -- what found the path -- isn't known.

We could .Distinct() the output so that we don't have duplicates,
but I like the idea of duplicates, as it shows that we're using
multiple locators.

Then we hit xamarin-android PR #2004, in which the
xamarin-android-tools bump within xamarin-android would fail,
depending on which machine ran the unit tests:

Xamarin.Android.Common.targets(691,2): warning : Not a valid JDK directory: `/var/folders/19/hjm21bn17913_0ck7nh3thdr0000gn/T/tmp731daee6.tmp/microsoft_dist_openjdk_1.8.999`

Where did that path come from?!

Update JdkInfo.TryGetJdkInfo() so that it takes an additional
"locator" parameter, which is publicly visible as the new
JdkInfo.Locator property, so that when we encounter an invalid JDK
directory, we know where that path came from:

Xamarin.Android.Common.targets(691,2): warning : Not a valid JDK directory: `/var/folders/19/hjm21bn17913_0ck7nh3thdr0000gn/T/tmp731daee6.tmp/microsoft_dist_openjdk_1.8.999`; via locator: monodroid-config.xml

Now we can see that the invalid (bizarro!) JDK path is coming from
monodroid-config.xml, allowing us to deduce that
monodroid-config.xml is "bad" (because the unit tests in a4aad18
incorrectly restored the original monodroid-config.xml file, meaning
the state of the machine after running that unit test is forever
suspect!).

The additional inclusion of Locator information makes it much easier
to reason about things, and the inclusion of duplicates makes sense:

$ csharp -r:bin/Debug/Xamarin.Android.Tools.AndroidSdk.dll
csharp> using Xamarin.Android.Tools;
csharp> JdkInfo.GetKnownSystemJdkInfos();
{
  JdkInfo(Version=1.8.0.77, Vendor="Oracle Corporation", HomePath="/Library/Java/JavaVirtualMachines/jdk1.8.0_77.jdk/Contents/Home", Locator="$PATH"),
  Not a valid JDK directory: `/Library/Java/JavaVirtualMachines/1.6.0_65-b14-462.jdk/Contents/Home`
  System.ArgumentException: Could not find required file `jvm` within `/Library/Java/JavaVirtualMachines/1.6.0_65-b14-462.jdk/Contents/Home`; is this a valid JDK?
  Parameter name: homePath
    at Xamarin.Android.Tools.JdkInfo.ValidateFile (System.String name, System.String path) [0x00025] in <087d9a26808b4d2d9c5d2844064ae952>:0
    at Xamarin.Android.Tools.JdkInfo..ctor (System.String homePath, System.String locator) [0x00120] in <087d9a26808b4d2d9c5d2844064ae952>:0
    at Xamarin.Android.Tools.JdkInfo.TryGetJdkInfo (System.String path, System.Action`2[T1,T2] logger, System.String locator) [0x00004] in <087d9a26808b4d2d9c5d2844064ae952>:0
  JdkInfo(Version=1.8.0.77, Vendor="Oracle Corporation", HomePath="/Library/Java/JavaVirtualMachines/jdk1.8.0_77.jdk/Contents/Home", Locator="`/usr/libexec/java_home -X`")
}

Context: dotnet/android#2004 (comment)

There are a two (related) "usability" issues with
`JdkInfo.GetKnownSystemJdkInfos()`:

 1. It returns duplicate paths, and
 2. There's no way of knowing where a path "came from"

For example:

	$ csharp -r:bin/Debug/Xamarin.Android.Tools.AndroidSdk.dll
	csharp> using Xamarin.Android.Tools;
	csharp> JdkInfo.GetKnownSystemJdkInfos();
	{
	  JdkInfo(Version=1.8.0.77, Vendor="Oracle Corporation", HomePath="/Library/Java/JavaVirtualMachines/jdk1.8.0_77.jdk/Contents/Home"),
	  Not a valid JDK directory: `/Library/Java/JavaVirtualMachines/1.6.0_65-b14-462.jdk/Contents/Home`
	  System.ArgumentException: Could not find required file `jvm` within `/Library/Java/JavaVirtualMachines/1.6.0_65-b14-462.jdk/Contents/Home`; is this a valid JDK?
	  Parameter name: homePath
	    at Xamarin.Android.Tools.JdkInfo.ValidateFile (System.String name, System.String path) [0x00025] in <087d9a26808b4d2d9c5d2844064ae952>:0
	    at Xamarin.Android.Tools.JdkInfo..ctor (System.String homePath, System.String locator) [0x00120] in <087d9a26808b4d2d9c5d2844064ae952>:0
	    at Xamarin.Android.Tools.JdkInfo.TryGetJdkInfo (System.String path, System.Action`2[T1,T2] logger, System.String locator) [0x00004] in <087d9a26808b4d2d9c5d2844064ae952>:0
	  JdkInfo(Version=1.8.0.77, Vendor="Oracle Corporation", HomePath="/Library/Java/JavaVirtualMachines/jdk1.8.0_77.jdk/Contents/Home"),
	}

Notice that on my machine `jdk1.8.0_77.jdk` is listed *twice*.

Why is it listed twice?  It's listed twice because it comes from two
different "locators": `JdkInfo.GetPathEnvironmentJdks()`, which reads
`$PATH`, and `JdkInfo.GetLibexecJdks()`, which parses the output of
`/usr/libexec/java_home -X`.  If `$JAVA_HOME` were set to the same
location, it would be listed three times.

As-is, there's no way of knowing where a path is "coming from"; the
"locator" -- what found the path -- isn't known.

We could `.Distinct()` the output so that we don't have duplicates,
but I *like* the idea of duplicates, as it shows that we're using
multiple locators.

Then we hit [xamarin-android PR #2004][0], in which the
`xamarin-android-tools` bump within `xamarin-android` would fail,
*depending on which machine ran the unit tests*:

	Xamarin.Android.Common.targets(691,2): warning : Not a valid JDK directory: `/var/folders/19/hjm21bn17913_0ck7nh3thdr0000gn/T/tmp731daee6.tmp/microsoft_dist_openjdk_1.8.999`

Where did that path come from?!

Update `JdkInfo.TryGetJdkInfo()` so that it takes an additional
"locator" parameter, which is publicly visible as the new
`JdkInfo.Locator` property, so that when we encounter an invalid JDK
directory, we know where that path came from:

	Xamarin.Android.Common.targets(691,2): warning : Not a valid JDK directory: `/var/folders/19/hjm21bn17913_0ck7nh3thdr0000gn/T/tmp731daee6.tmp/microsoft_dist_openjdk_1.8.999`; via locator: monodroid-config.xml

*Now* we can see that the invalid (bizarro!) JDK path is coming from
`monodroid-config.xml`, allowing us to deduce that
`monodroid-config.xml` is "bad" (because the unit tests in a4aad18
incorrectly restored the original `monodroid-config.xml` file, meaning
the state of the machine after running that unit test is forever
suspect!).

The additional inclusion of Locator information makes it much easier
to reason about things, and the inclusion of duplicates makes sense:

	$ csharp -r:bin/Debug/Xamarin.Android.Tools.AndroidSdk.dll
	csharp> using Xamarin.Android.Tools;
	csharp> JdkInfo.GetKnownSystemJdkInfos();
	{
	  JdkInfo(Version=1.8.0.77, Vendor="Oracle Corporation", HomePath="/Library/Java/JavaVirtualMachines/jdk1.8.0_77.jdk/Contents/Home", Locator="$PATH"),
	  Not a valid JDK directory: `/Library/Java/JavaVirtualMachines/1.6.0_65-b14-462.jdk/Contents/Home`
	  System.ArgumentException: Could not find required file `jvm` within `/Library/Java/JavaVirtualMachines/1.6.0_65-b14-462.jdk/Contents/Home`; is this a valid JDK?
	  Parameter name: homePath
	    at Xamarin.Android.Tools.JdkInfo.ValidateFile (System.String name, System.String path) [0x00025] in <087d9a26808b4d2d9c5d2844064ae952>:0
	    at Xamarin.Android.Tools.JdkInfo..ctor (System.String homePath, System.String locator) [0x00120] in <087d9a26808b4d2d9c5d2844064ae952>:0
	    at Xamarin.Android.Tools.JdkInfo.TryGetJdkInfo (System.String path, System.Action`2[T1,T2] logger, System.String locator) [0x00004] in <087d9a26808b4d2d9c5d2844064ae952>:0
	  JdkInfo(Version=1.8.0.77, Vendor="Oracle Corporation", HomePath="/Library/Java/JavaVirtualMachines/jdk1.8.0_77.jdk/Contents/Home", Locator="`/usr/libexec/java_home -X`")
	}

[0]: dotnet/android#2004
@jonpryor jonpryor merged commit d3de054 into dotnet:master Aug 3, 2018
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Aug 16, 2018
grendello pushed a commit to dotnet/android that referenced this pull request Aug 17, 2018
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.

3 participants