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

Remove placeholder jshell jdk8 module #8264

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mbien
Copy link
Member

@mbien mbien commented Feb 20, 2025

  • remove lib.nbjshell module
  • rename lib.nbjshell9 to lib.nbjshell, effectively replacing the original placeholder module
  • bump module requirements to JDK 11

implemented analog to #7677

wip

 - remove lib.nbjshell module
 - rename lib.nbjshell9 to lib.nbjshell, effectively replacing the
   original placeholder module
 - bump module requirements to JDK 11
@mbien mbien added Code cleanup Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) ci:all-tests [ci] enable all tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Feb 20, 2025
@mbien mbien added this to the NB26 milestone Feb 20, 2025
@mbien mbien marked this pull request as draft February 21, 2025 01:02
@mbien mbien removed this from the NB26 milestone Feb 21, 2025
@lahodaj
Copy link
Contributor

lahodaj commented Feb 26, 2025

I think this will be a good cleanup.

@mbien
Copy link
Member Author

mbien commented Feb 27, 2025

@lahodaj originally I only intended to replace lib.nbjshell with lib.nbjshell9, but it turned out that both modules can likely be removed.

while testing I noticed that jshell doesn't work if NB runtime JDK < configured jshell JDK. I am not sure if this was a requirement or not. Thats why I set it back to draft. (this is the case in NB 25 already, with or without this PR. If NB runs on JDK 17 it can't run jshell on 21.

@matthiasblaesing
Copy link
Contributor

The cross-jdk access indeed seems to be problematic. NB running on JDK 23 can use target JDK 23 and 21, but not 17. Running this by hand shows the problem:

matthias@enterprise:~$ /usr/lib/jvm/java-17-openjdk-amd64/bin/java -classpath /home/matthias/bin/netbeans-dev/java/modules/ext/nb-mod-jshell-probe.jar: -Xrunjdwp:transport=dt_socket,address=localhost:53633,suspend=y,includevirtualthreads=n org.netbeans.lib.jshell.agent.AgentWorker 35645
ERROR: JDWP option syntax error: -agentlib:jdwp=transport=dt_socket,address=localhost:53633,suspend=y,includevirtualthreads=n
matthias@enterprise:~$

The new parameter includevirtualthreads breaks the connection setup.

In the opposite direction I see a problem when running on JDK 17 and choosing JDK 21 as platform. If I read ShellSession.java correctly, the code asks the runtime JDK to create a javac with target of the target platform. This must go wrong.

So I think this is not a regression, just a feature, that is broken when not used in default configuration (target JDK != runtime JDK).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:all-tests [ci] enable all tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Code cleanup Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants