fix: Allow to use environment executable java to launch servers #1263
Conversation
CsCherrYY
requested review from
Eskibear,
jdneo and
testforstephen
as code owners
extension/src/util/config.ts
Outdated
| return getConfigGradleJavaHome() || process.env.JAVA_HOME; | ||
| } | ||
|
|
||
| export async function checkEnvJavaExecutable(): Promise<boolean> { |
There was a problem hiding this comment.
I don't see any async statements in the body.
There was a problem hiding this comment.
Let me remove "async" of this function and getGradleServerEnv().
| const env = { ...process.env }; | ||
| if (javaHome) { | ||
| Object.assign(env, { | ||
| VSCODE_JAVA_HOME: javaHome, | ||
| }); | ||
| } else if (!(await checkEnvJavaExecutable())) { | ||
| return undefined; |
There was a problem hiding this comment.
what will happen if undefined is returned? compared with previous behavior where env is returned.
There was a problem hiding this comment.
prompt NO_JAVA_EXECUTABLE early before actually executing it?
There was a problem hiding this comment.
prompt NO_JAVA_EXECUTABLE early before actually executing it?
Yes. Please refer: https://github.com/microsoft/vscode-gradle/pull/1263/files#diff-3369ade313508c278005bfe83330cb8fb4852a75783f5f5843d8d89a46423fc6R43-R46, if we can't find a proper java executable, will return early.
The previous behavior is always to run the start script via cmd variable, in this way, Gradle will report an error if no java executable can be found.
| @@ -53,30 +54,24 @@ export async function startLanguageServer( | |||
| serverOptions = awaitServerConnection.bind(null, port); | |||
| } else { | |||
| // keep consistent with gRPC server | |||
| const javaHome = getConfigGradleJavaHome() || process.env.JAVA_HOME; | |||
| const javaHome = getJavaHome(); | |||
| let javaCommand; | |||
| if (!javaHome) { | |||
There was a problem hiding this comment.
Just 2 cents, flip the if-else branch would be more human-readable for me, because you don't assign a new javaHome in if !javaHome block..
Below makes more sense to me:
a)
if (!javaHome) {
javaHome = .....
// do something requiring javaHome
} else {
// do something requiring javaHome
}
b)
if (javaHome) {
// do something requiring javaHome
} else {
// do something unrelated with javaHome
}
fix #1249
This pr checks and keeps the consistent of the two servers (Gradle server and Gradle language server), and follow Gradle behavior.
We will check the following ones (with descent priority), Beside VS Code configuration, we use step 4-5 to keep consistent with Gradle itself behavior.