From 55ea8df3e744b3bd4e2362d898ecaa1d7e79860d Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Wed, 3 Jun 2020 16:03:50 -0400 Subject: [PATCH 1/2] [Xamarin.Android.Tools.AndroidSdk] Prefer JAVA_HOME MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: https://github.com/xamarin/xamarin-android/pull/4567 `JdkInfo.GetKnownSystemJdkInfos()` returns a list of "known JDK locations", and the order of the returned paths was, basically, "locations that Xamarin/Microsoft controls come first." `AndroidSdkWindows.GetJdkInfos()` reads the Windows registry and is "controlled by" Visual Studio (unless someone is editing the Registry by hand…). If not on Windows, `GetConfiguredJdks()` reads `monodroid-config.xml` (managed by Visual Studio for Mac); failing that we probe some "well known Microsoft-controlled" directory locations… …and only failing *that* do we try the `$JAVA_HOME` environment variable, the ["de-facto" way][0] to tell software where Java is installed, and if `$JAVA_HOME` isn't set we further fallback to checking directories in `$PATH` and other mechanisms. The problem with this approach is that it isn't overridable, which is a usefully important feature if you want to *test new JDK versions*, as is the case in xamarin/xamarin-androi#4567. The "obvious" way to "try out" a new JDK would be to export the `JAVA_HOME` environment variable to the location of the JDK to use, but *that won't work* because `JdkInfo.GetKnownSystemJdkInfos()` *explicitly prefers* locations that aren't easily controllable in a CI environment. We *could* add a new environment variable to override all the others, but this is needless complexity. Update `JdkInfo.GetKnownSystemJdkInfos()` to check `$JAVA_HOME` *first*, not 4th. This will allow CI to export the `JAVA_HOME` environment variable, allowing it to be preferred over others. [0]: https://docs.oracle.com/cd/E19182-01/821-0917/inst_jdk_javahome_t/index.html --- src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs b/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs index e82886d..abe5ab3 100644 --- a/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs +++ b/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs @@ -281,10 +281,10 @@ public static IEnumerable GetKnownSystemJdkInfos (Action Date: Wed, 3 Jun 2020 19:32:51 -0400 Subject: [PATCH 2/2] [Xamarin.Android.Tools.AndroidSdk] Prefer JI_JAVA_HOME Context: https://github.com/xamarin/xamarin-android-tools/pull/86#issuecomment-638446368 @jpobst wrote: > My concern is that we don't know who all is setting `JAVA_HOME`. > What if some installer out there is setting it to a Java distro > or version we don't support? This is almost certainly the case, and an excellent point. Instead of preferring the `JAVA_HOME` environment variable, prefer the `JI_HAVE_HOME` environment variable. This will allow CI to override *something* "reasonable" so that the JDK can be controlled, without stepping into an unknowable minefield of JDK installs which already set `JAVA_HOME` to something that won't work with Xamarin.Android. --- src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs | 9 +++++---- .../Sdks/AndroidSdkWindows.cs | 15 ++------------- .../AndroidSdkInfoTests.cs | 15 +++++++++------ .../JdkInfoTests.cs | 17 +++++++++++++++++ 4 files changed, 33 insertions(+), 23 deletions(-) diff --git a/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs b/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs index abe5ab3..6c4c8b8 100644 --- a/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs +++ b/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs @@ -281,10 +281,11 @@ public static IEnumerable GetKnownSystemJdkInfos (Action GetWindowsJdks (Action logger) return AndroidSdkWindows.GetJdkInfos (logger); } - static IEnumerable GetJavaHomeEnvironmentJdks (Action logger) + static IEnumerable GetEnvironmentVariableJdks (string envVar, Action logger) { - var java_home = Environment.GetEnvironmentVariable ("JAVA_HOME"); + var java_home = Environment.GetEnvironmentVariable (envVar); if (string.IsNullOrEmpty (java_home)) yield break; - var jdk = TryGetJdkInfo (java_home, logger, "$JAVA_HOME"); + var jdk = TryGetJdkInfo (java_home, logger, $"${envVar}"); if (jdk != null) yield return jdk; } diff --git a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs index c363703..8740798 100644 --- a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs +++ b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs @@ -108,7 +108,7 @@ protected override IEnumerable GetAllAvailableAndroidSdks () protected override string? GetJavaSdkPath () { - var jdk = GetJdkInfos (Logger).FirstOrDefault (); + var jdk = JdkInfo.GetKnownSystemJdkInfos (Logger).FirstOrDefault (); return jdk?.HomePath; } @@ -139,18 +139,7 @@ IEnumerable ToJdkInfos (IEnumerable paths, string locator) .Concat (ToJdkInfos (GetOpenJdkPaths (), "OpenJDK")) .Concat (ToJdkInfos (GetKnownOpenJdkPaths (), "Well-known OpenJDK paths")) .Concat (ToJdkInfos (GetOracleJdkPaths (), "Oracle JDK")) - .Concat (ToJdkInfos (GetEnvironmentJdkPaths (), "Environment Variables")); - } - - private static IEnumerable GetEnvironmentJdkPaths () - { - var environment = new [] { "JAVA_HOME" }; - foreach (var key in environment) { - var value = Environment.GetEnvironmentVariable (key); - if (!string.IsNullOrEmpty (value)) { - yield return value; - } - } + ; } private static IEnumerable GetPreferredJdkPaths () diff --git a/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidSdkInfoTests.cs b/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidSdkInfoTests.cs index 262a292..a02ffd6 100644 --- a/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidSdkInfoTests.cs +++ b/tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidSdkInfoTests.cs @@ -136,9 +136,13 @@ public void Constructor_SetValuesFromPath () } [Test] - [Ignore ("This test will only work locally if you rename/remove your Open JDK directory.")] - public void JdkDirectory_JavaHome () + public void JdkDirectory_JavaHome ([Values ("JI_JAVA_HOME", "JAVA_HOME")] string envVar) { + if (envVar.Equals ("JAVA_HOME", StringComparison.OrdinalIgnoreCase)) { + Assert.Ignore ("This test will only work locally if you rename/remove your Open JDK directory."); + return; + } + CreateSdks (out string root, out string jdk, out string ndk, out string sdk); JdkInfoTests.CreateFauxJdk (jdk, releaseVersion: "1.8.999", releaseBuildNumber: "9", javaVersion: "1.8.999-9"); @@ -150,16 +154,15 @@ public void JdkDirectory_JavaHome () string java_home = null; try { // We only set via JAVA_HOME - java_home = Environment.GetEnvironmentVariable ("JAVA_HOME", EnvironmentVariableTarget.Process); - Environment.SetEnvironmentVariable ("JAVA_HOME", jdk); + java_home = Environment.GetEnvironmentVariable (envVar, EnvironmentVariableTarget.Process); + Environment.SetEnvironmentVariable (envVar, jdk, EnvironmentVariableTarget.Process); var info = new AndroidSdkInfo (logger, androidSdkPath: sdk, androidNdkPath: ndk, javaSdkPath: ""); Assert.AreEqual (ndk, info.AndroidNdkPath, "AndroidNdkPath not preserved!"); Assert.AreEqual (sdk, info.AndroidSdkPath, "AndroidSdkPath not preserved!"); Assert.AreEqual (jdk, info.JavaSdkPath, "JavaSdkPath not preserved!"); } finally { - if (java_home != null) - Environment.SetEnvironmentVariable ("JAVA_HOME", java_home, EnvironmentVariableTarget.Process); + Environment.SetEnvironmentVariable (envVar, java_home, EnvironmentVariableTarget.Process); Directory.Delete (root, recursive: true); } } diff --git a/tests/Xamarin.Android.Tools.AndroidSdk-Tests/JdkInfoTests.cs b/tests/Xamarin.Android.Tools.AndroidSdk-Tests/JdkInfoTests.cs index 122deed..594c6d0 100644 --- a/tests/Xamarin.Android.Tools.AndroidSdk-Tests/JdkInfoTests.cs +++ b/tests/Xamarin.Android.Tools.AndroidSdk-Tests/JdkInfoTests.cs @@ -11,6 +11,23 @@ namespace Xamarin.Android.Tools.Tests [TestFixture] public class JdkInfoTests { + [Test] + public void GetKnownSystemJdkInfos_PrefersJiJavaHome () + { + var previous = Environment.GetEnvironmentVariable ("JI_JAVA_HOME", EnvironmentVariableTarget.Process); + try { + Environment.SetEnvironmentVariable ("JI_JAVA_HOME", FauxJdkDir, EnvironmentVariableTarget.Process); + + var defaultJdkDir = JdkInfo.GetKnownSystemJdkInfos () + .FirstOrDefault (); + Assert.IsNotNull (defaultJdkDir); + Assert.AreEqual (FauxJdkDir, defaultJdkDir.HomePath); + } + finally { + Environment.SetEnvironmentVariable ("JI_JAVA_HOME", previous, EnvironmentVariableTarget.Process); + } + } + [Test] public void Constructor_NullPath () {