[Xamarin.Android.Tools.AndroidSdk] Prefer JAVA_HOME #86
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Context: dotnet/android#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
|
My concern is that we don't know who all is setting It feels like it might be safer to introduce a |
We could, if we used I should certainly add a test for whatever env var is used. |
Hilarious. I should remove that. :-) |
Context: dotnet#86 (comment) @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.
|
The "extra" All comments should thus be resolved (I hope). |
jonathanpeppers
approved these changes
Jun 4, 2020
jonpryor
added a commit
that referenced
this pull request
Jun 4, 2020
Context: dotnet/android#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 dotnet/android#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. Given that *existing convention* is for JDK installs to set the `JAVA_HOME` environment variable -- and thus `JAVA_HOME` may very well refer to a JDK which Xamarin.Android doesn't support -- we are leery of making `JAVA_HOME` the "primary" override. Instead, add support for a new `JI_JAVA_HOME` environment variable which, if set, is the *preferred* JDK to use within Xamarin.Android (unless otherwise overridden by e.g. `$(JavaSdkDirectory)`). This will allow CI to export the `JAVA_HOME` environment variable, allowing it to be preferred over others. Additionally, remove some `JAVA_HOME` "duplication" between `JdkInfo` and `AndroidSdkWindows`, so that things are easier to reason about. [0]: https://docs.oracle.com/cd/E19182-01/821-0917/inst_jdk_javahome_t/index.html
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: dotnet/android#4567
JdkInfo.GetKnownSystemJdkInfos()returns a list of "known JDKlocations", 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()readsmonodroid-config.xml(managed by Visual Studio for Mac); failingthat we probe some "well known Microsoft-controlled" directory
locations…
…and only failing that do we try the
$JAVA_HOMEenvironmentvariable, the "de-facto" way to tell software where Java is
installed, and if
$JAVA_HOMEisn't set we further fallback tochecking directories in
$PATHand 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 dotnet/android#4567. The "obvious" way to
"try out" a new JDK would be to export the
JAVA_HOMEenvironmentvariable to the location of the JDK to use, but that won't work
because
JdkInfo.GetKnownSystemJdkInfos()explicitly preferslocations that aren't easily controllable in a CI environment.
Given that existing convention is for JDK installs to set the
JAVA_HOMEenvironment variable -- and thusJAVA_HOMEmay verywell refer to a JDK which Xamarin.Android doesn't support -- we are
leery of making
JAVA_HOMEthe "primary" override.Instead, add support for a new
JI_JAVA_HOMEenvironment variablewhich, if set, is the preferred JDK to use within Xamarin.Android
(unless otherwise overridden by e.g.
$(JavaSdkDirectory)).This will allow CI to export the
JAVA_HOMEenvironment variable,allowing it to be preferred over others.
Additionally, remove some
JAVA_HOME"duplication" betweenJdkInfoand
AndroidSdkWindows, so that things are easier to reason about.