Skip to content

Make java.configuration.runtimes machine-overridable#2337

Merged
rgrunber merged 2 commits intoredhat-developer:masterfrom
thakkarparth007:runtimes-machine-overridable
Mar 8, 2022
Merged

Make java.configuration.runtimes machine-overridable#2337
rgrunber merged 2 commits intoredhat-developer:masterfrom
thakkarparth007:runtimes-machine-overridable

Conversation

@thakkarparth007
Copy link
Copy Markdown
Contributor

@thakkarparth007 thakkarparth007 commented Mar 2, 2022

Potential fix for #2001

I have no idea if changing this configuration's scope can cause unintentional side effects. If so, do educate me and I'd be happy to make some code changes if it is not a lot.

Signed-off-by: Parth <thakkarparth007@gmail.com>
@thakkarparth007 thakkarparth007 force-pushed the runtimes-machine-overridable branch from 70a4f47 to 1d84904 Compare March 2, 2022 20:42
@rgrunber
Copy link
Copy Markdown
Member

rgrunber commented Mar 2, 2022

After reconsidering, I think this approach might work. Before my main concern was that allowing a project/folder to set java.configuration.runtimes is a security risk in the same way allowing a project to dictate java.jdt.ls.java.home or java.jdt.ls.vmargs would be. But now we have workspace trust, and if we add the property to restrictedConfigurations it should handle that concern.

@testforstephen @fbricon , let me know if there's some case we're missing here.

@testforstephen
Copy link
Copy Markdown
Collaborator

i think it's OK, other than we need to add java.configuration.runtimes as untrusted via restrictedConfigurations.

vscode-java/package.json

Lines 13 to 20 in 083f580

"capabilities": {
"untrustedWorkspaces": {
"supported": "limited",
"restrictedConfigurations": [
"java.jdt.ls.java.home",
"java.home",
"java.jdt.ls.vmargs"
]

@Eskibear
Copy link
Copy Markdown
Contributor

Eskibear commented Mar 7, 2022

Regarding this, I had a feature request. In java.configuration.runtimes, we store mapping of language level and path to JDK.
Currently I'm unable to use different JDK distribution for different project because the setting scope.

E.g. I have two Maven projects with language level set to Java SE-1.8. But for Project A I want to use RedHat OpenJDK, and Micrsoft's OpenJDK for Project B. Currently I'm not able to do that with workspace level setting.

I'm wondering if we'll have this feature with this PR? Or do we need change something in jdtls, as I remember the backend setting is also per machine.

@rgrunber
Copy link
Copy Markdown
Member

rgrunber commented Mar 7, 2022

Regarding this, I had a feature request. In java.configuration.runtimes, we store mapping of language level and path to JDK. Currently I'm unable to use different JDK distribution for different project because the setting scope.

E.g. I have two Maven projects with language level set to Java SE-1.8. But for Project A I want to use RedHat OpenJDK, and Micrsoft's OpenJDK for Project B. Currently I'm not able to do that with workspace level setting.

I'm wondering if we'll have this feature with this PR? Or do we need change something in jdtls, as I remember the backend setting is also per machine.

I just tried out the PR and it allows me to override java.configuration.runtimes from .vscode/settings.json in a workspace folder. It seems in line with what https://code.visualstudio.com/api/references/contribution-points implies about machine-overridable.

I couldn't set a folder-specific setting on the client for a multi-root workspace, and get it to be sent to the language server, but maybe I'm not understanding how the setting should work. The problem I see is I don't think we even support a multi-root project. In JVMConfigurator, we call JavaRuntime.setDefaultVMInstall(..) to set the JVM, which says :

Sets a VM as the system-wide default VM, and notifies registered VM install

So this would only work with a single "project" per workspace.

@Eskibear
Copy link
Copy Markdown
Contributor

Eskibear commented Mar 8, 2022

So this would only work with a single "project" per workspace.

I think that's fine for me. I can always open different projects in different workspace for the moment.

But in Eclipse, when I import multiple projects, I can configure VMInstall per project. So I think it's at least feasible regardless of effort. In long-term plan, I believe we do want to fully support multi-root workspace, maybe we should think about how to refactor the JVMConfigurator then.

@rgrunber
Copy link
Copy Markdown
Member

rgrunber commented Mar 8, 2022

So this would only work with a single "project" per workspace.

I think that's fine for me. I can always open different projects in different workspace for the moment.

But in Eclipse, when I import multiple projects, I can configure VMInstall per project. So I think it's at least feasible regardless of effort. In long-term plan, I believe we do want to fully support multi-root workspace, maybe we should think about how to refactor the JVMConfigurator then.

Yup! The JDT API definitely has some way to set the JVM per project. We've likely only used the global setting for now. As long as we're fine with some functionality being unsupported for the time being, I think it should be fine to proceed with this.

@thakkarparth007 , can you update the PR based on #2337 (comment)

thakkarparth007 added a commit to thakkarparth007/vscode-java that referenced this pull request Mar 8, 2022
Change as per:

redhat-developer#2337 (comment)

Signed-off-by: Parth Thakkar <thakkar.parth.d@gmail.com>
@thakkarparth007 thakkarparth007 force-pushed the runtimes-machine-overridable branch from ee48a5a to 46f8e01 Compare March 8, 2022 07:45
@thakkarparth007
Copy link
Copy Markdown
Contributor Author

@rgrunber done.

@rgrunber
Copy link
Copy Markdown
Member

rgrunber commented Mar 8, 2022

If there's no opposition to this, or special case we've missed, I'll merge soon. Last remaining thing is to update https://github.com/redhat-developer/vscode-java/wiki/JDK-Requirements to briefly mention workspace settings can now take advantage of the setting.

@rgrunber rgrunber merged commit 8fbc1fa into redhat-developer:master Mar 8, 2022
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.

4 participants