scalameta / metals-vscode

Visual Studio Code extension for Metals
https://marketplace.visualstudio.com/items?itemName=scalameta.metals#overview
Apache License 2.0
297 stars 74 forks source link

Java home not recognised #1464

Closed Arthurm1 closed 4 months ago

Arthurm1 commented 4 months ago

Describe the bug

Metals doesn't recognise my JAVA_HOME setting.

To Reproduce Steps to reproduce the behavior:

1) Set your machine's environment settings JAVA_HOME to an already downloaded jdk 21. 2) Open the latest nightly VSCode Metals extension.

Metals starts up and immediately Coursier downloads a JDK 17 which then overwrites the JAVA_HOME on my machine and adds this jdk 17 to my machines path. This is pretty invasive and stops any apps that require jdk 21 from working on my machine.

I see there is a new Java Version setting in Metals which defaults to 17. I can change this to 21 but coursier will still download a JDK instead of using my own. It does download a version 21 though.

Metals now seems to ignore the VSCode "Java home" setting - I can't get that to affect any of this.

Not sure if this is a Windows only issue and it can't detect I have a JAVA_HOME setting. The release version of Metals works fine.

If the idea is for Metals to download a JDK version if one is not present then maybe the 2 VSCode settings (java home & java version) could be merged into something like... 1) Use JAVA_HOME (default) 2) Use specific JDK dir : enter jdk dir here 3) Download jdk 11 3) Download jdk 17 3) Download jdk 21

Expected behavior

I'd expect Metals to recognise my JAVA_HOME and use that be default.

The changes to my machines JAVA_HOME and path are worrying.

Installation:

Search terms

java_home

tgodzik commented 4 months ago

We should use the JAVA_HOME and only download if it's not available. Seems that something is wrong there. Maybe, it was due to the default being Java 17, which forced us to download JDK 17 since 21 was not matching what we would expect?

We can allow using any higher than the default.

tgodzik commented 4 months ago

@kasiaMarek could you take a look?

tgodzik commented 4 months ago

Also looks like it doesn't show progress (at least to me). We also should probably use --setup flag to avoid setting JAVA_HOME for the user.

kasiaMarek commented 4 months ago

Metals now seems to ignore the VSCode "Java home" setting - I can't get that to affect any of this.

Yes, that setting is now meant to point to java home you want to use for your project. Java version refers to the one that is used to start metals with. I guess it should be described a bit better.

I can change this to 21 but coursier will still download a JDK instead of using my own.

That's a bug, it should use your JAVA_HOME if the version matches. (We can change this to allow any higher as well.) Could you share your metals plugin output?

kasiaMarek commented 4 months ago

We also should probably use --setup flag to avoid setting JAVA_HOME for the user.

Quite the contrary I think. We are using the flag and we shouldn't.

Arthurm1 commented 4 months ago

My system JAVA_HOME env var was c:\jdk-21 in this test. My VSCode settings for Metals are only: "metals.javaHome": "c:\\jdk-21"

Here's the log output...

Metals version: 1.2.2
Using coursier located at C:\Users\arthurm\AppData\Local\Coursier\data\bin\cs.bat
No installed java with version 17 found. Will fetch one using coursier.
Coursier: Checking if the JAVA_HOME, PATH user environment variable(s) need(s) updating.
Coursier: Some global environment variables were updated. It is recommended to close this terminal once the setup command is done, and open a new one for the changes to be taken into account.
Using Java Home: C:\Users\arthurm\AppData\Local\Coursier\cache\arc\https\github.com\adoptium\temurin17-binaries\releases\download\jdk-17.0.8%252B7\OpenJDK17U-jdk_x64_windows_hotspot_17.0.8_7.zip\jdk-17.0.8+7
tgodzik commented 4 months ago

We also should probably use --setup flag to avoid setting JAVA_HOME for the user.

Quite the contrary I think. We are using the flag and we shouldn't.

I was actually trying to write the opposite, but I lost the "n't" :sweat_smile:

kasiaMarek commented 4 months ago

I made some changes that should already be on the newest snapshot. It won't probably resolve the issue of discovering your JAVA_HOME but it should provide more logging information and won't mess-up your JAVA_HOME and PATH variables anymore. So if you're willing to try it I'm hoping the additional logs will help us debug this further.

Arthurm1 commented 4 months ago

It doesn't change my env vars now - which is good. But still downloads jdk 17 and uses that.

My only VSCode settings are "metals.javaHome": "c:\\jdk-21"

Logs...

Metals version: 1.2.2
Using coursier located at C:\Users\arthurm\AppData\Local\Coursier\data\bin\cs.bat
C:\jdk-21\bin\java -version:
openjdk version "21" 2023-09-19 LTS
OpenJDK Runtime Environment Temurin-21+35 (build 21+35-LTS)
C:\jdk-21\bin\java -version:
OpenJDK 64-Bit Server VM Temurin-21+35 (build 21+35-LTS, mixed mode, sharing)
No installed java with version 17 found. Will fetch one using coursier.
Coursier: openjdk version "17.0.8" 2023-07-18
OpenJDK Runtime Environment Temurin-17.0.8+7 (build 17.0.8+7)
Coursier: OpenJDK 64-Bit Server VM Temurin-17.0.8+7 (build 17.0.8+7, mixed mode, sharing)
Using Java Home: C:\Users\arthurm\AppData\Local\Coursier\cache\arc\https\github.com\adoptium\temurin17-binaries\releases\download\jdk-17.0.8%252B7\OpenJDK17U-jdk_x64_windows_hotspot_17.0.8_7.zip\jdk-17.0.8+7

If I add the setting "metals.javaVersion": "21" then it still downloads a JDK...

etals version: 1.2.2
Using coursier located at C:\Users\arthurm\AppData\Local\Coursier\data\bin\cs.bat
C:\jdk-21\bin\java -version:
openjdk version "21" 2023-09-19 LTS
C:\jdk-21\bin\java -version:
OpenJDK Runtime Environment Temurin-21+35 (build 21+35-LTS)
OpenJDK 64-Bit Server VM Temurin-21+35 (build 21+35-LTS, mixed mode, sharing)
No installed java with version 21 found. Will fetch one using coursier.
Coursier: openjdk version "21.0.2" 2024-01-16 LTS
OpenJDK Runtime Environment Temurin-21.0.2+13 (build 21.0.2+13-LTS)
Coursier: OpenJDK 64-Bit Server VM Temurin-21.0.2+13 (build 21.0.2+13-LTS, mixed mode, sharing)
Using Java Home: C:\Users\arthurm\AppData\Local\Coursier\cache\arc\https\github.com\adoptium\temurin21-binaries\releases\download\jdk-21.0.2%252B13\OpenJDK21U-jdk_x64_windows_hotspot_21.0.2_13.zip\jdk-21.0.2+13
kasiaMarek commented 4 months ago

Yeah, thanks of doing this. So the issue is that we check java version by running java -version and looking for something matching /\d+\.\d+\.\d+/ and it just shows "21" in your case.