Skip to content

Allow setting JDK path via a java.home JSON setting.#234

Open
brown wants to merge 1 commit intogeorgewfraser:masterfrom
brown:allow-setting-java-home
Open

Allow setting JDK path via a java.home JSON setting.#234
brown wants to merge 1 commit intogeorgewfraser:masterfrom
brown:allow-setting-java-home

Conversation

@brown
Copy link
Contributor

@brown brown commented Jan 21, 2023

Currently, the language server always infers the location of the JDK, either from JAVA_HOME or by searching in various directories that depend on the operating system.

This pull request adds the ability to set the JDK location explicitly via JSON.

@brown brown force-pushed the allow-setting-java-home branch 3 times, most recently from 7f92e45 to e77a0c4 Compare January 21, 2023 22:50
@georgewfraser
Copy link
Owner

Can you add a test @brown ?

@brown brown force-pushed the allow-setting-java-home branch from e77a0c4 to 8b69428 Compare March 9, 2026 20:13
@brown
Copy link
Contributor Author

brown commented Mar 9, 2026

I would like to add a test, but it doesn't look straightforward. Please suggest an approach you would be happy with. Thanks!

@georgewfraser
Copy link
Owner

This looks directionally useful, but I don't think it is mergeable yet.

Changes I'd want to see before re-review:

  • Make java.home affect the actual JDK model used by the server, not just Docs/src.zip lookup. Right now platform classes still come from the running JVM via ScanClassPath.jdkTopLevelClasses(), so the configured JDK is only partially applied.
  • Remove or key the Docs src.zip cache by java.home. As written, the first src.zip lookup can be reused even after the configured JDK changes.
  • Add tests that prove a configured java.home changes behavior, not just default src.zip discovery. The current FindSrcZipTest still exercises only the default lookup path.

Happy to re-review once those pieces are addressed.

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

Successfully merging this pull request may close these issues.

3 participants