Skip to content

expose the api to get preferred jdk infos #142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 27, 2021

Conversation

tondat
Copy link
Contributor

@tondat tondat commented Oct 26, 2021

  • VisualStudio needs to know the list of preferred jdk infos to set the default jdk on the android settings property page

@tondat tondat requested review from mcumming and jonpryor October 26, 2021 20:16
Copy link
Member

@pjcollins pjcollins left a comment

Choose a reason for hiding this comment

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

@tondat
Copy link
Contributor Author

tondat commented Oct 26, 2021

I think this is supposed to be internal for a reason, are you able to use this instead?

https://github.com/xamarin/xamarin-android-tools/blob/4c2e36c757c8a8d7da48ca1ffa97240f3602ec85/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs#L284

no, this is the error, the explanation is in the bug https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1415418

{
logger = logger ?? AndroidSdkInfo.DefaultConsoleLogger;

Choose a reason for hiding this comment

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

Is there a file logger that can be used? What if the process doesn't have a console?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

client is responsible to set the logger, in our case XamarinVS caller.

I have added it to be compatible with all the methods in this class that check the logger or assign the default if it is a null parameter

@jonpryor
Copy link
Contributor

@tondat
Copy link
Contributor Author

tondat commented Oct 27, 2021

@tondat : I would prefer that XamarinVS call AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest():

https://github.com/xamarin/xamarin-android-tools/blob/34e98e2b65917d105169f868b5648f67e68b6784/src/Xamarin.Android.Tools.AndroidSdk/AndroidSdkInfo.cs#L181

we cannot do that, we have a dialog (UI) and I need this value, when user accept it I changed the value and I need to refresh the AndroidSDK of androditools. So I need this api public, to get the installed JDK and I cannot use the GetKnownSystemJdkInfos because it gives priority to the preferred path and it can be a custom path defined for the user and I need the default installed by willow. Full explanation in the bug: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1415418

@jonpryor
Copy link
Contributor

@tondat: I don't follow. VSMac also has a dialog (UI), and does use AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest().

Now, VSMac doesn't show what the path will be changed to; it just has a Reset to Defaults button.

…though now that I look at VSMac, it's wrong, in that the Reset to Defaults button is disabled if the value hasn't changed! (Meaning if you change the JDK path to anything, quit VSMac, restart VSMac, and return to the Android Locations tab, the button is disabled. Lol/sigh.)

@jonpryor
Copy link
Contributor

@tondat: Given that the VSMac UX is hilariously bad, I'll concede that we do need a new method.

Thus, naming: we've "overloaded" the adjective "preferred"; we're using it as:

  • The path to use by default; the value stored in the Windows Registry or in monodroid-config.xml. See e.g. AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest(), JdkLocations.GetPreferredJdks(), etc.
  • The path to use if no preferred path is set.

Just writing the above is confusing, actually.

Thus, yes, we should add a new method, but GetPreferredJdkInfos() is not a great name.

How does GetSupportedJdkInfos() sound? The point is to return paths that we "know" will work, right?

@tondat
Copy link
Contributor Author

tondat commented Oct 27, 2021

@jonpryor yes this is the main goal (for XamainVS is the JDK installed by willow for example), this name is ok for me.

- VisualStudio needs to know this list to set the default jdk on the android settings property page
@tondat tondat force-pushed the dev/jmt/exposeGetPreferredJDKs branch from 10c0ed2 to 27d4606 Compare October 27, 2021 15:34
@tondat
Copy link
Contributor Author

tondat commented Oct 27, 2021

@jonpryor please see the name change

@jonpryor jonpryor merged commit fc976d8 into dotnet:main Oct 27, 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.

5 participants