[Xamarin.Android.Tools.AndroidSdk] More Microsoft Dist JDK Support #130
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
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1314263 Remember `AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()` (commit a4aad18)? It's supposed to "detect" the latest installed "preferred" JDK installation location, and set configuration values so that subsequent `new AndroidSdkInfo()` expressions will use that JDK location, instead of some other JDK location. The "preferred" JDK was a JDK that was "known good" for Xamarin.Android use, i.e. one that Visual Studio installed. In a4aad18, this was the `microsoft_dist_openjdk_*` version. When we (prematurely) removed support for the `microsoft_dist_openjdk_` build in commits 237642c and dca30d9, we updated `AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()` to instead prefer the [Microsoft OpenJDK][0] build, but this migration needed to be delayed until Visual Studio 17.0. Which means that the "current world order" is *wrong*: `AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()` tries to detect a JDK that is not currently installed by anything. Futhermore, Visual Studio for Mac will call `AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()` if no JDK is currently configured. The result is that on "clean" systems, Visual Studio for Mac can crash with a `NotSupportedException` because it's looking for a Microsoft OpenJDK installation, but only a `microsoft_dist_openjdk_` installation is available. Thus, no preferred JDK is found, and the IDE crashes. Fix this by adding a new (`internal`) `JdkInfo.GetPreferredJdkInfos()` method, which appropriately checks *both* Microsoft OpenJDK *and* `microsoft_dist_openjdk_` installation locations, and update `AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()` to now use `JdkInfo.GetPreferredJdkInfos()`. (We add the method to `JdkInfo` so that it is easier to keep `GetPreferredJdkInfos()` and `GetKnownSystemJdkInfos()` consistent with each other.) [0]: https://www.microsoft.com/openjdk
jonpryor
force-pushed
the
jonp-detect-should-use-all-jdks
branch
from
946cf74 to
f07372b
Compare
tondat
approved these changes
Aug 11, 2021
jonathanpeppers
approved these changes
Aug 11, 2021
Member
jonathanpeppers
left a comment
jonathanpeppers
left a comment
There was a problem hiding this comment.
The fix looks good to me.
Down the road, is there something we could do to test these changes better? Would mocking the file system, environment variable, and/or registry calls be doable?
Contributor
Author
|
We do have some code to create a "faux" JDK: https://github.com/xamarin/xamarin-android-tools/blob/f9c1b0d754050cef0a8339ff87a37882fa13a9f6/tests/Xamarin.Android.Tools.AndroidSdk-Tests/JdkInfoTests.cs#L60-L100 We also have support for "overriding" the default global locations: https://github.com/xamarin/xamarin-android-tools/blob/f9c1b0d754050cef0a8339ff87a37882fa13a9f6/src/Xamarin.Android.Tools.AndroidSdk/Jdks/MicrosoftDistJdkLocations.cs#L21 (Not "complete" support for overriding, but it's "something".) My "problem" with a unit test here is that it wouldn't help with the original bug: the unit test would just assert that the method does what it does, but wouldn't help ensure that Rephrased: even if there were tests, we'd likely have overlooked those tests as part of the whole "remove support for |
jonpryor
added a commit
that referenced
this pull request
Aug 11, 2021
) Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1314263 Remember `AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()` (commit a4aad18)? It's supposed to "detect" the latest installed "preferred" JDK installation location, and set configuration values so that subsequent `new AndroidSdkInfo()` expressions will use that JDK location, instead of some other JDK location. The "preferred" JDK was a JDK that was "known good" for Xamarin.Android use, i.e. one that Visual Studio installed. In a4aad18, this was the `microsoft_dist_openjdk_*` version. When we (prematurely) removed support for the `microsoft_dist_openjdk_` build in commits 237642c and dca30d9, we updated `AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()` to instead prefer the [Microsoft OpenJDK][0] build, but this migration needed to be delayed until Visual Studio 17.0. Which means that the "current world order" is *wrong*: `AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()` tries to detect a JDK that is not currently installed by anything. Futhermore, Visual Studio for Mac will call `AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()` if no JDK is currently configured. The result is that on "clean" systems, Visual Studio for Mac can crash with a `NotSupportedException` because it's looking for a Microsoft OpenJDK installation, but only a `microsoft_dist_openjdk_` installation is available. Thus, no preferred JDK is found, and the IDE crashes. Fix this by adding a new (`internal`) `JdkInfo.GetPreferredJdkInfos()` method, which appropriately checks *both* Microsoft OpenJDK *and* `microsoft_dist_openjdk_` installation locations, and update `AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()` to now use `JdkInfo.GetPreferredJdkInfos()`. (We add the method to `JdkInfo` so that it is easier to keep `GetPreferredJdkInfos()` and `GetKnownSystemJdkInfos()` consistent with each other.) [0]: https://www.microsoft.com/openjdk
jonpryor
added a commit
that referenced
this pull request
Aug 11, 2021
) Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1314263 Remember `AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()` (commit a4aad18)? It's supposed to "detect" the latest installed "preferred" JDK installation location, and set configuration values so that subsequent `new AndroidSdkInfo()` expressions will use that JDK location, instead of some other JDK location. The "preferred" JDK was a JDK that was "known good" for Xamarin.Android use, i.e. one that Visual Studio installed. In a4aad18, this was the `microsoft_dist_openjdk_*` version. When we (prematurely) removed support for the `microsoft_dist_openjdk_` build in commits 237642c and dca30d9, we updated `AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()` to instead prefer the [Microsoft OpenJDK][0] build, but this migration needed to be delayed until Visual Studio 17.0. Which means that the "current world order" is *wrong*: `AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()` tries to detect a JDK that is not currently installed by anything. Futhermore, Visual Studio for Mac will call `AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()` if no JDK is currently configured. The result is that on "clean" systems, Visual Studio for Mac can crash with a `NotSupportedException` because it's looking for a Microsoft OpenJDK installation, but only a `microsoft_dist_openjdk_` installation is available. Thus, no preferred JDK is found, and the IDE crashes. Fix this by adding a new (`internal`) `JdkInfo.GetPreferredJdkInfos()` method, which appropriately checks *both* Microsoft OpenJDK *and* `microsoft_dist_openjdk_` installation locations, and update `AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()` to now use `JdkInfo.GetPreferredJdkInfos()`. (We add the method to `JdkInfo` so that it is easier to keep `GetPreferredJdkInfos()` and `GetKnownSystemJdkInfos()` consistent with each other.) [0]: https://www.microsoft.com/openjdk
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1314263
Remember
AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()(commit a4aad18)? It's supposed to "detect" the latest installed
"preferred" JDK installation location, and set configuration values
so that subsequent
new AndroidSdkInfo()expressions will use thatJDK location, instead of some other JDK location.
The "preferred" JDK was a JDK that was "known good" for Xamarin.Android
use, i.e. one that Visual Studio installed. In a4aad18, this was the
microsoft_dist_openjdk_*version.When we (prematurely) removed support for the
microsoft_dist_openjdk_build in commits 237642c and dca30d9, we updated
AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()to insteadprefer the Microsoft OpenJDK build, but this migration needed to
be delayed until Visual Studio 17.0.
Which means that the "current world order" is wrong:
AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()tries todetect a JDK that is not currently installed by anything.
Futhermore, Visual Studio for Mac will call
AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()if no JDKis currently configured.
The result is that on "clean" systems, Visual Studio for Mac can
crash with a
NotSupportedExceptionbecause it's looking for aMicrosoft OpenJDK installation, but only a
microsoft_dist_openjdk_installation is available. Thus, no preferred JDK is found, and the
IDE crashes.
Fix this by adding a new (
internal)JdkInfo.GetPreferredJdkInfos()method, which appropriately checks both Microsoft OpenJDK and
microsoft_dist_openjdk_installation locations, and updateAndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest()to nowuse
JdkInfo.GetPreferredJdkInfos().(We add the method to
JdkInfoso that it is easier to keepGetPreferredJdkInfos()andGetKnownSystemJdkInfos()consistentwith each other.)