Skip to content

Conversation

@jonpryor
Copy link
Contributor

@jonpryor jonpryor commented Apr 16, 2021

Context: https://www.azul.com/downloads/zulu-community/

Commit 237642c removed support for the ancient
microsoft_dist_openjdk_* versions, replacing it with the newer
Microsoft OpenJDK.

One problem with this change is one of compatibility:
microsoft_dist_openjdk_ was a build of JDK 1.8, while
Microsoft OpenJDK only provides builds for JDK 11 and later.
This is a problem, as the Visual Studio Android Designer
currently requires JDK 1.8, and is not yet compatible with JDK 11.

Add support to probe for for Azul Zulu OpenJDK installation
directories, as Zulu provides updated JDK 1.8 installers.

Additionally, refactor and centralize the JDK Location logic.
Previously, JdkInfo.GetKnownSystemJdkInfos() would return all of
the Windows installation locations before the macOS/Linux/etc.
locations, which made it possible for Windows and non-Windows
platforms to return paths in an inconsistent order. Introduce a new
JdkLocations helper, and add new subclasses for each specific JDK
location, so that JdkInfo.GetKnownSystemJdkInfos() can be unified
across macOS/Linux & Windows, increasing consistency.

Add a new tools/ls-jdks tool, to make it easier to print all probed
JDK locations.

@jonpryor
Copy link
Contributor Author

TODO: write a better commit message. (It's a draft!)

"Primary" motivation is to add support for the Azul Zulu JDK.

"Secondary" motivation -- which is most of the diff, by far -- is to bring a degree of sanity here. Previously, JdkInfo.GetKnownSystemJdkInfos() was spread across Windows support, localized within AndroidSdkWindows, and non-Windows (macOS, Linux) support, within JdkInfo itself.

For me, at least, this makes understanding the lookups "weird".

Unify these systems, creating 6 "JDK Location" classes which know where to look for one and only one "JDK", for both macOS & Windows, and use these unified "finders" within JdkInfo.GetKnownSystemJdkInfos().

I find this a tad "cleaner".

Keep in mind that we'll be wanting to add Yet Another JDK location next week, for Adopt OpenJDK, so I find making things "more understandable" to be a benefit.

Copy link
Contributor

@grendello grendello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I wonder if we should have a class that also looks in ~/android-toolchain/jdk without having to put that location in JAVA_HOME? I know it would work only on systems where one builds Xamarin.Android, but it should be harmless on other machines while making it easier to use XA JDK as the main one on developer's machine.

@jonpryor jonpryor marked this pull request as ready for review April 19, 2021 16:15
@jonpryor jonpryor changed the title icanhaz Zulu support? [Xamarin.Android.Tools.AndroidSdk] Probe for Zulu JDK Apr 19, 2021

if (!string.IsNullOrEmpty (currentVersion)) {

// No matter what the CurrentVersion is, look for 1.6 or 1.7 or 1.8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is outdated

Context: https://www.azul.com/downloads/zulu-community/

Commit 237642c removed support for the ancient
`microsoft_dist_openjdk_*` versions, replacing it with the newer
[Microsoft OpenJDK][0].

One problem with this change is one of *compatibility*:
`microsoft_dist_openjdk_` was a build of JDK 1.8, while
Microsoft OpenJDK only provides builds for JDK 11 and later.
This is a problem, as the [Visual Studio Android Designer][1]
currently requires JDK 1.8, and is not yet compatible with JDK 11.

Add support to probe for for [Azul Zulu OpenJDK][2] installation
directories, as Zulu provides updated JDK 1.8 installers.

Additionally, refactor and centralize the JDK Location logic.
Previously, `JdkInfo.GetKnownSystemJdkInfos()` would return all of
the Windows installation locations before the macOS/Linux/etc.
locations, which made it possible for Windows and non-Windows
platforms to return paths in an inconsistent order.  Introduce a new
`JdkLocations` helper, and add new subclasses for each specific JDK
location, so that `JdkInfo.GetKnownSystemJdkInfos()` can be *unified*
across macOS/Linux & Windows, increasing consistency.

Add a new `tools/ls-jdks` tool, to make it easier to print all probed
JDK locations.

[0]: https://www.microsoft.com/openjdk
[1]: https://docs.microsoft.com/en-us/xamarin/android/user-interface/android-designer/designer-walkthrough?tabs=windows
[2]: https://www.azul.com/downloads/zulu-community/?package=jdk
@jonpryor jonpryor merged commit dca30d9 into dotnet:main Apr 19, 2021
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.

4 participants