Skip to content

search all installed JDKs when java.home not specified#1701

Merged
fbricon merged 4 commits intoredhat-developer:masterfrom
Eskibear:fix-jdk-detection
Nov 18, 2020
Merged

search all installed JDKs when java.home not specified#1701
fbricon merged 4 commits intoredhat-developer:masterfrom
Eskibear:fix-jdk-detection

Conversation

@Eskibear
Copy link
Copy Markdown
Contributor

@Eskibear Eskibear commented Nov 18, 2020

In macOS Big Sur, /usr is wronly recongnized as JDK homedir. See #1700 (comment)

It's caused by defect of upstream lib node-find-java-home. As a hotfix, here I copied a file from https://github.com/microsoft/vscode-java-pack/blob/fc6cb66f496fbbe5e8b2d9971c430e081f5b5460/src/java-runtime/utils/findJavaRuntime.ts

By validating all installed JDKs, this PR can also fix #1621

Now the JDK detection follows below order:

  • if java.home is specified, we validate it, and if it's invalid, we prompt error message as before
  • if java.home is not specified, we search all possible places(env.JAVA_HOME, env.PATH, Windows Registry, Common installation directories for popular distributions) for installed JDKs, pick a valid one (major version >= 11) if exists

@Eskibear
search all installed JDKs when java.home not specified
185e8c2 
Signed-off-by: Yan Zhang <yanzh@microsoft.com>
@Eskibear
Copy link
Copy Markdown
Contributor Author

Eskibear commented Nov 18, 2020

https://github.com/redhat-developer/vscode-java#setting-the-jdk should also be updated in this PR. I'll do that after you review it.

testforstephen
testforstephen reviewed Nov 18, 2020
View reviewed changes
testforstephen
testforstephen reviewed Nov 18, 2020
View reviewed changes
@Eskibear
check JDK_HOME first
ce0652c 
Signed-off-by: Yan Zhang <yanzh@microsoft.com>
testforstephen
testforstephen reviewed Nov 18, 2020
View reviewed changes
@Eskibear
align order of JDKs with README
eb1a82c 
Signed-off-by: Yan Zhang <yanzh@microsoft.com>
@Eskibear
reset unintended change
53c4d42 
Signed-off-by: Yan Zhang <yanzh@microsoft.com>
testforstephen
testforstephen approved these changes Nov 18, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@testforstephen testforstephen left a comment

Choose a reason for hiding this comment

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

LGTM.

@rgrunber @fbricon Since multiple users are reporting Java errors due to wrong JDK detection after upgrading to macOS Big Sur, can we merge this PR into Nov Middle release? WDYT.

@testforstephen testforstephen linked an issue Nov 18, 2020 that may be closed by this pull request
Error in every java file after updating to macOS Big Sur #1700
Closed
@rgrunber
Copy link
Copy Markdown
Member

rgrunber commented Nov 18, 2020

Yes, feel free to merge. We can update release documents afterwards.

@fbricon fbricon merged commit f4eea29 into redhat-developer:master Nov 18, 2020
@rgrunber
Copy link
Copy Markdown
Member

rgrunber commented Nov 19, 2020

I think there might be an issue with this change that ends up affecting Linux distros.

Have a look at https://github.com/redhat-developer/vscode-java/blob/master/src/findJavaRuntimes.ts#L247
In my case, 'proposed' was '/usr/lib/jvm/java-11-openjdk' . Clearly 'bin/javac' existed to get to this point. What failed for me was the '.isDirectory()' check. On my machine '/usr/lib/jvm/java-11-openjdk' is a symbolic link, so I'd assume the directory check fails.

It's fairly common for symbolic links in /usr/lib/jvm of the form java-11-openjdk -> java-11-openjdk-11.0.9.11-0.fc32.x86_64

@Eskibear
Copy link
Copy Markdown
Contributor Author

Eskibear commented Nov 19, 2020

Exactly. Let me check if it's safe to simply remove the isDirectory() check.

@Eskibear
Copy link
Copy Markdown
Contributor Author

Eskibear commented Nov 19, 2020

@rgrunber Is it a convention to install JDKs into /usr/lib/jvm ? If so, we can fallback to check this directory on Linux, if no JAVA_HOME specified..

@Eskibear Eskibear mentioned this pull request Nov 19, 2020
Merged
@Eskibear
Copy link
Copy Markdown
Contributor Author

Eskibear commented Nov 19, 2020

The isDirectory() check was originally added from this commit: jsdevel/node-find-java-home@9bd6daf#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R129

It was not intended to fix any edge case, and I just tried after removing it, no problems found. So I just created a PR to remove it.

@rgrunber
Copy link
Copy Markdown
Member

rgrunber commented Nov 19, 2020

@rgrunber Is it a convention to install JDKs into /usr/lib/jvm ? If so, we can fallback to check this directory on Linux, if no JAVA_HOME specified..

This can vary between distros, so I don't think it holds in call cases, but it could be a good fallback. It's certainly the case for Fedora and Ubuntu.

@Eskibear
Copy link
Copy Markdown
Contributor Author

Eskibear commented Nov 19, 2020

@rgrunber ok, I can add /usr/lib/jvm as fallback, to increase coverage.

@Eskibear Eskibear mentioned this pull request Nov 19, 2020
Merged
@Eskibear Eskibear deleted the fix-jdk-detection branch September 14, 2023 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@testforstephen testforstephen testforstephen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error in every java file after updating to macOS Big Sur potential failure of activation when JDK <11 is also installed

4 participants

@Eskibear @rgrunber @testforstephen @fbricon