Skip to content
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

metals-vscode incorrectly assumes java home directory. #1528

Open
megri opened this issue Aug 28, 2024 · 4 comments
Open

metals-vscode incorrectly assumes java home directory. #1528

megri opened this issue Aug 28, 2024 · 4 comments

Comments

@megri
Copy link
Contributor

megri commented Aug 28, 2024

Describe the bug

Using asdf to manage jvm installs, vs code incorrectly assumes that a java reference found on $PATH has a home directory under <detected-java-path>/../..
This happens here:

const possibleJavaHome = path.dirname(path.dirname(realJavaPath));

It then blindly tries to run <detected-java-path>/../../bin/java which fails.

To Reproduce Steps to reproduce the behavior:

  1. Install asdf: https://asdf-vm.com/guide/getting-started.html
  2. asdf plugin add java
  3. asdf install java zulu-21.36.19
  4. asdf global java zulu-21.36.19
  5. Open a Scala project and switch to the Output/Metals tab
  6. Output will look something like
Metals version: 1.3.5
Using coursier located at /opt/homebrew/bin/coursier
Searching for Java on PATH. Found java executable under /Users/you/.asdf/shims/java that resolves to /Users/you/.asdf/shims/java
failed while running /Users/you/.asdf/bin/java -version
Error: spawn /Users/you/.asdf/bin/java ENOENT
Java version doesn't match the required one of 17
No installed java with version 17 found. Will fetch one using coursier:
	openjdk version "17" 2021-09-14
	OpenJDK Runtime Environment Temurin-17+35 (build 17+35)
	OpenJDK 64-Bit Server VM Temurin-17+35 (build 17+35, mixed mode)

Expected behavior

If the actual JAVA_HOME is needed it could be found using something like this very hacky method I just pieced together:

echo 'System.getProperty("java.home")' | jshell -q 2>&1 | sed -n '1p' | sed -ne 's/^.*==> //p'

The jshell command should be located on $PATH as previously. Note that this imposes a delay of ~1.5 seconds on my setup.

Screenshots

Installation:

  • Operating system: macOS
  • VSCode version: 1.92.2
  • VSCode extension version: 1.39.0
  • Metals version: 1.3.5

Additional context

Search terms

java, jvm, javaHome, JAVA_HOME

@tgodzik
Copy link
Contributor

tgodzik commented Aug 28, 2024

Thanks for reporting! How does the directory structure look like for /Users/you/.asdf ? shims/java is only the binary? I wonder how that actually work 🤔

@megri
Copy link
Contributor Author

megri commented Aug 28, 2024

Yeah, shims/ contains a bunch of scripts that forwards to somewhere else. shims/java looks like

#!/usr/bin/env bash
# asdf-plugin: java graalvm-22.3.1+java17
# asdf-plugin: java zulu-11.64.19
# asdf-plugin: java zulu-11.66.19
# asdf-plugin: java zulu-11.74.15
# asdf-plugin: java zulu-17.34.19
# asdf-plugin: java zulu-17.50.19
# asdf-plugin: java zulu-21.30.15
# asdf-plugin: java zulu-21.36.19
exec /opt/homebrew/opt/asdf/libexec/bin/asdf exec "java" "$@" # asdf_allow: ' asdf '

Running the jshell snippet from above gives "/Users/you/.asdf/installs/java/zulu-21.36.19/zulu-21.jdk/Contents/Home".

@megri
Copy link
Contributor Author

megri commented Aug 28, 2024

If JAVA_HOME is actually not needed then running java -version with realJavaPath should be fine. Not sure when/if it is actually appropriate to back out of the binary's path and re-enter through bin/.

@tgodzik
Copy link
Contributor

tgodzik commented Aug 28, 2024

We should be fine just fixing the logic to not assume we're in bin/java. We could possibly do the fix for Java sources inside metals server as a fallback. So we wouldn't need to invoke anything before actually starting metals.

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

No branches or pull requests

2 participants