Improved Java Home Detection #121
Conversation
No, you don't have to do this for backwards-compatible changes in the settings structure. |
server/src/Settings.ts
Outdated
| + "Please install one and/or manually specify it in the Gobra settings."; | ||
| reject(msg); | ||
| } else { | ||
| const javaHome = javaHomes[0]; |
There was a problem hiding this comment.
Minor: Maybe it is worth warning the user that we arbitrarily picked one of the available Java installations.
There was a problem hiding this comment.
Would you log a warning? I'm a bit hesitant to just insert a Log.hint here as I do not want to spam the user with popup. Also, this function seems to get called when checking the settings and when starting ViperServer, thus a potential popup might appear multiple times (that also why error messages do not directly get logged but returned as a promise failure)
There was a problem hiding this comment.
The scenario I have in mind is one that actually happened on one of our customer's laptop some time ago. Assume the user has multiple Java installations, e.g. one Java 8 64-bit without the server component and one Java 8 64-bit with the server component. If the IDE automatically picks the former, ViperServer startup command will crash with a not-100%-obvious stack trace. The user might then try running the java commend in their command line, but that might actually succeed if the right java version is on $PATH. This would seem to the user as a bug in the IDE, whereas (in my view) it's actually their fault since they didn't specify which Java version the IDE should use. Therefore, in such a scenario I would think it's good practice to be as explicit as possible, even showing a popup that explains how to change the settings in such a way that would solve the problem and the warning would not appear anymore.
How about something like this:
Warning: multiple Java installations detected:
/usr/lib/java [automatically selected]
/User/Library/Oracle/Java 11/Home
To suppress this warning, please define
"viper.javaSettings.javaBinary" in your settings file.
At the very least, I suggest logging this information via Log.log(msg, LogLevel.Info). However, I don't fully understand why the popup would appear multiple times, precisely because this should only be checked at IDE startup time.
There was a problem hiding this comment.
I've now implemented it by extending the settings check and display a corresponding warning in such cases. As it turns out, my own setup is affected by the ambiguity and thus I've checked that the warning is correctly displayed :P
@aterga You might want to have a quick look at the checkSettings rewrite, which is now async and hopefully a bit easier to read
There was a problem hiding this comment.
Great, thanks! I will merge this PR very soon.
Is there an easy way to link the Settings button in the Java-installation-warning popup to lead to the JSON version of the settings, rather than the GUI? GUI settings aren't currently supported by Viper IDE, so ideally we should direct the user to the JSON file (if VS Code API provides this feature).
There was a problem hiding this comment.
Not sure whether this would be possible. One thing that might complicate things is that to the best of my knowledge there can be different places to store the settings file (user global vs. workspace settings). Thus, I don't think we should attempt to reimplement this logic
There was a problem hiding this comment.
I am not suggesting to reimplement the logic. But currently the button already leads to some GUI wrapper over a JSON file. Ideally, it should lead to the file itself, not the GUI, because users who are unfamiliar with VS Code might not know how to get to the JSON in which they can control Viper's settings.
Switches to
viperproject/locate-java-hometo find a suitable java home.@aterga sorry for large changeset, I've updated some NPM dependencies, which resulted in some changes and replaced all
requiresby properimportstatements.One question: There is now an additional configuration field
javaBinary. Do I have to increase the version hash (even though it is fully backwards compatible)?