Skip to content

Conversation

@jonpryor
Copy link
Contributor

@jonpryor jonpryor commented Jul 26, 2018

@jonpryor jonpryor force-pushed the jonp-bump-xat-75530565 branch from 6225d60 to f050504 Compare July 26, 2018 22:26
jonpryor added a commit to jonpryor/xamarin-android-tools that referenced this pull request Jul 27, 2018
Context: dotnet/android#2004
Context: https://jenkins.mono-project.com/job/xamarin-android-pr-builder/3656/testReport/junit/Xamarin.Android.Build.Tests/ResolveSdksTaskTests/ResolveSdkTiming___Debug/

Before commit 7553056, the only validation performed on the
`AndroidSdkInfo`'s `javaSdkPath` parameter was "does
`{javaSdkPath}/bin/jarsigner` exist?

This is the world the [xamarin-android unit tests assumed][0], as it
would create `{javaSdkPath}/bin/jarsigner` and
`{javaSdkPath}/bin/javac.bash` (among others), but *not*
`{javaSdkPath/bin/javac`.

Then came commit 7553056, which altered
`AndroidSdkUnix.ValidateJavaSdkLocation()` semantics so that it now
*required* the presence of `{javaSdkPath}/bin/javac`.

This changed caused the xamarin-android unit tests to fail:

	JavaSdkPath should be /Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/TestDebug/temp/ResolveSdkTiming/jdk
	Expected string length 63 but was 115. Strings differ at index 1.
	Expected: "/Library/Java/JavaVirtualMachines/jdk1.8.0_91.jdk/Contents/Home"
	But was:  "/Users/builder/jenkins/workspace/xamarin-android-pr-builder/x..."
	------------^

The "expected" and "actual" values are reversed; the actual problem is
that the `javaSdkPath` value provided to the `AndroidSdkInfo`
constructor --
`/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/TestDebug/temp/ResolveSdkTiming/jdk`
-- is "ignored", because it doesn't pass validation,
prompting `AndroidSdkInfo` to call `AndroidSdkUnix.GetJavaSdkPath()`,
which returns a "system" location,
`/Library/Java/JavaVirtualMachines/jdk1.8.0_91.jdk/Contents/Home`.

As such, the original `javaSdkPath` value is not preserved and
returned by the `AndroidSdkInfo.JavaSdkPath` property.

Fix this by *removing* the `AndroidSdkUnix.ValidateJavaSdkLocation()`
method: it doesn't appear to actually be useful in this instance, as
the base `AndroidSdkBase.ValidateJavaSdkLocation()`  already checks
for the presence of `{javaSdkPath}/bin/jarsigner`.

[0]: https://github.com/xamarin/xamarin-android/blob/997b8904889d371610026c1802a8e3403d7d2c96/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/BaseTest.cs#L220-L235
jonpryor added a commit to dotnet/android-tools that referenced this pull request Jul 27, 2018
Context: dotnet/android#2004
Context: https://jenkins.mono-project.com/job/xamarin-android-pr-builder/3656/testReport/junit/Xamarin.Android.Build.Tests/ResolveSdksTaskTests/ResolveSdkTiming___Debug/

Before commit 7553056, the only validation performed on the
`AndroidSdkInfo`'s `javaSdkPath` parameter was "does
`{javaSdkPath}/bin/jarsigner` exist?

This is the world the [xamarin-android unit tests assumed][0], as it
would create `{javaSdkPath}/bin/jarsigner` and
`{javaSdkPath}/bin/javac.bash` (among others), but *not*
`{javaSdkPath/bin/javac`.

Then came commit 7553056, which altered
`AndroidSdkUnix.ValidateJavaSdkLocation()` semantics so that it now
*required* the presence of `{javaSdkPath}/bin/javac`.

This changed caused the xamarin-android unit tests to fail:

	JavaSdkPath should be /Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/TestDebug/temp/ResolveSdkTiming/jdk
	Expected string length 63 but was 115. Strings differ at index 1.
	Expected: "/Library/Java/JavaVirtualMachines/jdk1.8.0_91.jdk/Contents/Home"
	But was:  "/Users/builder/jenkins/workspace/xamarin-android-pr-builder/x..."
	------------^

The "expected" and "actual" values are reversed; the actual problem is
that the `javaSdkPath` value provided to the `AndroidSdkInfo`
constructor --
`/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/TestDebug/temp/ResolveSdkTiming/jdk`
-- is "ignored", because it doesn't pass validation,
prompting `AndroidSdkInfo` to call `AndroidSdkUnix.GetJavaSdkPath()`,
which returns a "system" location,
`/Library/Java/JavaVirtualMachines/jdk1.8.0_91.jdk/Contents/Home`.

As such, the original `javaSdkPath` value is not preserved and
returned by the `AndroidSdkInfo.JavaSdkPath` property.

Fix this by *removing* the `AndroidSdkUnix.ValidateJavaSdkLocation()`
method: it doesn't appear to actually be useful in this instance, as
the base `AndroidSdkBase.ValidateJavaSdkLocation()`  already checks
for the presence of `{javaSdkPath}/bin/jarsigner`.

[0]: https://github.com/xamarin/xamarin-android/blob/997b8904889d371610026c1802a8e3403d7d2c96/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/BaseTest.cs#L220-L235
@jonpryor jonpryor force-pushed the jonp-bump-xat-75530565 branch from f050504 to ec1777c Compare July 27, 2018 19:48
@jonpryor
Copy link
Contributor Author

@jonpryor jonpryor force-pushed the jonp-bump-xat-75530565 branch from ec1777c to b2f64fd Compare August 2, 2018 00:55
@jonpryor jonpryor requested a review from dellis1972 as a code owner August 2, 2018 00:55
@jonpryor jonpryor changed the title Bump to xamarin/xamarin-android-tools:master@75530565 Bump to xamarin/xamarin-android-tools:master@917d3b3c Aug 2, 2018
@jonpryor
Copy link
Contributor Author

jonpryor commented Aug 2, 2018

If I'm very lucky, the CheckSignApk() failures are because the NDK couldn't be determined from $PATH, fixed in dotnet/android-tools@511d580.

However, that doesn't really make sense as we should always be explicitly providing the NDK location, so I doubt I'm that lucky.

In the meantime, update the assertions within CheckSignApk() so that I can at least know which assertion is failing.

@jonpryor
Copy link
Contributor Author

jonpryor commented Aug 2, 2018

@dellis1972 noted the problem: the warning that is causing the unit test failures is in the console output and build logs:

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` [/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/tests/TestRunner.NUnit/TestRunner.NUnit.csproj] [/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/build-tools/scripts/RunTests.targets]

That explains why an assertion that no warnings are reported is failing.

What I don't understand is how this is happening: the path it's complaining about is a path within $TEMP, e.g. from this unit test:

https://github.com/xamarin/xamarin-android-tools/blob/917d3b3ce455eed6a4a0e4271d34661bdf0b261d/src/Xamarin.Android.Tools.AndroidSdk/Tests/AndroidSdkInfoTests.cs#L270

The problem is:

  1. The "override path" is an AppDomain.SetData() value, and
  2. This unit test isn't run.

I am thus having a problem understanding how this temporary path -- with a directory name matching what our unexecuted unit tests create -- is causing a problem here.

¯_(ツ)_/¯

@jonpryor jonpryor force-pushed the jonp-bump-xat-75530565 branch 2 times, most recently from 86b0803 to 55e0206 Compare August 2, 2018 19:02
@jonpryor
Copy link
Contributor Author

jonpryor commented Aug 2, 2018

macOS PR Build 3711 succeeded.

I am thus faced with the prospect that this is in fact a machine-specific failure, as it failed on xam-mac-mini-6, but passed on xam-mac-mini-7.

(insert crying here.)

@jonpryor jonpryor force-pushed the jonp-bump-xat-75530565 branch from 55e0206 to d69c219 Compare August 2, 2018 21:21
jonpryor added a commit that referenced this pull request Aug 2, 2018
Context: https://paper.dropbox.com/doc/OpenJDK-and-You--AH1yWKdVXgno~uXYfmcUAZTwAg-NoECAe2XkBQeoxFfGL6ea
Context: dotnet/android-tools#29 (comment)
Context: #2004 (comment)

Fixes: http://work.devdiv.io/646086
Fixes: http://work.devdiv.io/652760
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/597752
Fixes: dotnet/android-tools#26
Fixes: dotnet/android-tools#39

Update & export `%JAVA_HOME%`, `%PATH%` on Windows.

We are [observing some unit test failures in this bump][0], which
[appear to be machine-specific][1].  We still don't understand the
cause of the unit test failures, so to assist in the eventual tracking
of this issue there is some additional, conditional, debug spew.

[0]: #2004 (comment)
[1]: #2004 (comment)
@jonpryor jonpryor force-pushed the jonp-bump-xat-75530565 branch from d69c219 to bd58afc Compare August 2, 2018 21:23
jonpryor added a commit that referenced this pull request Aug 2, 2018
Context: https://paper.dropbox.com/doc/OpenJDK-and-You--AH1yWKdVXgno~uXYfmcUAZTwAg-NoECAe2XkBQeoxFfGL6ea
Context: dotnet/android-tools#29 (comment)
Context: #2004 (comment)
Context: https://github.com/xamarin/xamarin-android-tools/blob/917d3b3ce455eed6a4a0e4271d34661bdf0b261d/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs#L256

Fixes: http://work.devdiv.io/646086
Fixes: http://work.devdiv.io/652760
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/597752
Fixes: dotnet/android-tools#26
Fixes: dotnet/android-tools#39

Update & export `%JAVA_HOME%`, `%PATH%` on Windows.

We are [observing some unit test failures in this bump][0], which
[appear to be machine-specific][1].  We still don't understand the
cause of the unit test failures, so to assist in the eventual tracking
of this issue there is some additional, conditional, debug spew.

[0]: #2004 (comment)
[1]: #2004 (comment)
jonpryor added a commit to dotnet/android-tools that referenced this pull request Aug 2, 2018
Context: dotnet/android#2004
Context: https://jenkins.mono-project.com/job/xamarin-android-pr-builder/3656/testReport/junit/Xamarin.Android.Build.Tests/ResolveSdksTaskTests/ResolveSdkTiming___Debug/

Before commit 7553056, the only validation performed on the
`AndroidSdkInfo`'s `javaSdkPath` parameter was "does
`{javaSdkPath}/bin/jarsigner` exist?

This is the world the [xamarin-android unit tests assumed][0], as it
would create `{javaSdkPath}/bin/jarsigner` and
`{javaSdkPath}/bin/javac.bash` (among others), but *not*
`{javaSdkPath/bin/javac`.

Then came commit 7553056, which altered
`AndroidSdkUnix.ValidateJavaSdkLocation()` semantics so that it now
*required* the presence of `{javaSdkPath}/bin/javac`.

This changed caused the xamarin-android unit tests to fail:

	JavaSdkPath should be /Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/TestDebug/temp/ResolveSdkTiming/jdk
	Expected string length 63 but was 115. Strings differ at index 1.
	Expected: "/Library/Java/JavaVirtualMachines/jdk1.8.0_91.jdk/Contents/Home"
	But was:  "/Users/builder/jenkins/workspace/xamarin-android-pr-builder/x..."
	------------^

The "expected" and "actual" values are reversed; the actual problem is
that the `javaSdkPath` value provided to the `AndroidSdkInfo`
constructor --
`/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/TestDebug/temp/ResolveSdkTiming/jdk`
-- is "ignored", because it doesn't pass validation,
prompting `AndroidSdkInfo` to call `AndroidSdkUnix.GetJavaSdkPath()`,
which returns a "system" location,
`/Library/Java/JavaVirtualMachines/jdk1.8.0_91.jdk/Contents/Home`.

As such, the original `javaSdkPath` value is not preserved and
returned by the `AndroidSdkInfo.JavaSdkPath` property.

Fix this by *removing* the `AndroidSdkUnix.ValidateJavaSdkLocation()`
method: it doesn't appear to actually be useful in this instance, as
the base `AndroidSdkBase.ValidateJavaSdkLocation()`  already checks
for the presence of `{javaSdkPath}/bin/jarsigner`.

[0]: https://github.com/xamarin/xamarin-android/blob/997b8904889d371610026c1802a8e3403d7d2c96/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/BaseTest.cs#L220-L235
jonpryor added a commit to dotnet/android-tools that referenced this pull request Aug 3, 2018
Context: dotnet/android#2004

Sometimes the xamarin-android-tools bump on xamarin-android works.

Sometimes it doesn't.

(Insert gnashing of teeth and tearing of hair here.)

Even "better", the path it's complaining about is entirely
non-sensical, and not reproducible except on certain machines:

	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`

That path *looks* like a path that `AndroidSdkInfoTests.cs` would
generate, except that test *isn't executed* -- or built! -- so
*where is that path coming from?!*

Surely it comes from `AppDomain.GetData()`, says I.  So I add some
logging to print out the value of the `AppDomain.GetData()` value,
and...it has no value.

So ***REALLY***, where is `/var/folders/...` coming from?

To answer *that* question, update `JdkInfo.TryGetJdkInfo()` to print
out a "category" for what directory we're loading.  With luck, this
bit of contextual information will help us narrow down where it's
coming from.  Maybe.
jonpryor added a commit to dotnet/android-tools that referenced this pull request Aug 3, 2018
Context: dotnet/android#2004

Sometimes the xamarin-android-tools bump on xamarin-android works.

Sometimes it doesn't.

(Insert gnashing of teeth and tearing of hair here.)

Even "better", the path it's complaining about is entirely
non-sensical, and not reproducible except on certain machines:

	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`

That path *looks* like a path that `AndroidSdkInfoTests.cs` would
generate, except that test *isn't executed* -- or built! -- so
*where is that path coming from?!*

Surely it comes from `AppDomain.GetData()`, says I.  So I add some
logging to print out the value of the `AppDomain.GetData()` value,
and...it has no value.

So ***REALLY***, where is `/var/folders/...` coming from?

To answer *that* question, update `JdkInfo.TryGetJdkInfo()` to print
out a "category" for what directory we're loading.  With luck, this
bit of contextual information will help us narrow down where it's
coming from.  Maybe.
@jonpryor jonpryor added the do-not-merge PR should not be merged. label Aug 3, 2018
@jonpryor jonpryor force-pushed the jonp-bump-xat-75530565 branch from bd58afc to 143ddac Compare August 3, 2018 02:21
jonpryor added a commit that referenced this pull request Aug 3, 2018
…c890a35ce3a8751316c1f829566e57

Context: https://paper.dropbox.com/doc/OpenJDK-and-You--AH1yWKdVXgno~uXYfmcUAZTwAg-NoECAe2XkBQeoxFfGL6ea
Context: dotnet/android-tools#29 (comment)
Context: #2004 (comment)
Context: https://github.com/xamarin/xamarin-android-tools/blob/917d3b3ce455eed6a4a0e4271d34661bdf0b261d/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs#L256

Fixes: http://work.devdiv.io/646086
Fixes: http://work.devdiv.io/652760
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/597752
Fixes: dotnet/android-tools#26
Fixes: dotnet/android-tools#39

Update & export `%JAVA_HOME%`, `%PATH%` on Windows.

We are [observing some unit test failures in this bump][0], which
[appear to be machine-specific][1].  We still don't understand the
cause of the unit test failures, so to assist in the eventual tracking
of this issue there is some additional, conditional, debug spew.

[0]: #2004 (comment)
[1]: #2004 (comment)

Also, MOAR INFO IN JDK ERROR MESSAGES.
@jonpryor
Copy link
Contributor Author

jonpryor commented Aug 3, 2018

The story so far...

To assist investigation, I created a new (to be deleted) xamarin-android-tools/jonp-dump-xa-pr-2004 branch, which changed JdkInfo.TryGetJdkInfo() to provide a "locator" in the warning message:

		static JdkInfo TryGetJdkInfo (string path, Action<TraceLevel, string> logger, string locator)
		{
			JdkInfo jdk = null;
			try {
				jdk = new JdkInfo (path, locator);
			}
			catch (Exception e) {
				logger (TraceLevel.Warning, $"Not a valid JDK directory: `{path}`; via locator: {locator}");
				logger (TraceLevel.Verbose, e.ToString ());
			}
			return jdk;
		}

(I should make a proper PR and merge this.)

The locator information lets us know where the path came from. After running on xam-mac-mini-7, it passed all unit tests, which was not unexpected. Rebuild on xam-mac-mini-6 (yay luck!), and it predictably failed, providing us the updated warning message:

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

Success! We can now piece together what happened.

The original unit tests for AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest() would backup the original monodroid-config.xml, implicitly create a new one, validate the contents of the new one, then restore the original file:

if (File.Exists (UnixConfigPath))
    File.Move (UnixConfigPath, backupConfig);
// ...
if (File.Exists (backupConfig))
    File.Move (backupConfig, UnixConfigPath)

However, this logic was faulty: File.Move() will not overwrite the target file if it already exists. As the target file already existed -- it was "implicitly" created by calling AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest() -- the File.Move() threw an (unreported?!) exception, meaning from that point onward the monodroid-config.xml on that PR builder was wrong.

And this was on the xamarin-android-tools PR builder Job, not the xamarin-android PR builder Job, though they share the same machines.

This bug was later fixed, but the damage had already been done. (The fix was later made redundant by altering the unit tests so that they ever used the "system" location for these tests, which we arguably should have done in the first place.)

The fix? We need to connect to xam-mac-mini-6 and delete monodroid-config.xml/restore the previously backed up "good" version.

jonpryor added a commit to jonpryor/xamarin-android-tools that referenced this pull request 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"

	$ 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 added a commit to jonpryor/xamarin-android-tools that referenced this pull request 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][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
Copy link
Contributor Author

jonpryor commented Aug 3, 2018

(I should make a proper PR and merge this.)

Which is here: dotnet/android-tools#43

@jonpryor jonpryor force-pushed the jonp-bump-xat-75530565 branch from 143ddac to c78d52a Compare August 3, 2018 13:39
@jonpryor jonpryor merged commit 84c5757 into master Aug 3, 2018
jonpryor added a commit to dotnet/android-tools that referenced this pull request 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][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 added a commit to jonpryor/xamarin-android that referenced this pull request Aug 13, 2018
@jonpryor jonpryor deleted the jonp-bump-xat-75530565 branch June 11, 2019 21:35
@github-actions github-actions bot locked and limited conversation to collaborators Feb 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

do-not-merge PR should not be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants