-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8351435: Change the default Console implementation back to the built-in one in java.base module
#23993
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
Conversation
|
👋 Welcome back naoto! A progress list of the required criteria for merging this PR into |
|
@naotoj This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 277 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
|
/contributor add @lahodaj |
|
@naotoj |
| // check "expect" command availability | ||
| var expect = Paths.get("/usr/bin/expect"); | ||
| if (!Files.exists(expect) || !Files.isExecutable(expect)) { | ||
| System.out.println("'expect' command not found. Test ignored."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Naoto, I think throwing a jtreg.SkippedException might be better here so that it's clear that the test was skipped. There have been recent reporting improvements too which make it easier to notice such skipped tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Modified the piece (w/ some other minor changes)
java.base module
| var file = Path.of(System.getProperty("test.src", "."), "Output.java") | ||
| .toAbsolutePath().toString(); | ||
| var pb = ProcessTools.createTestJavaProcessBuilder("--enable-preview", file, mode); | ||
| var pb = ProcessTools.createTestJavaProcessBuilder("-Djdk.console=jdk.internal.le", "--enable-preview", file, mode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you looked into changing the test so that it runs twice, the second with the jline provider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The child java process Output.java calls System.console(), so the launcher has to be run with -Djdk.console=jdk.internal.le, otherwise NPE is thrown in Output.java.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, no console in the child process for this implementation.
|
Thanks for the reviews! |
|
Going to push as commit 4247744.
Your commit was automatically rebased without conflicts. |
JDK has been using JLine based Console implementation, in JDK20 as opt-in, then in JDK22 as the default. While it has been providing rich functionality for Console, it is increasingly difficult to maintain as a Console implementation. In light of the on-ramp feature (https://bugs.openjdk.org/browse/JDK-8344699), which proposes switching
java.lang.IOclass to useSystem.inandSystem.outinstead of Console, reverting the default Console implementation to JDK's built-in one in the java.base module is considered desirable. Some tests are modified along with this change, among them test/jdk/java/io/Console/ConsolePromptTest.java changes were contributed by @lahodaj (thanks!)Progress
Issues
java.basemodule (Enhancement - P4)java.basemodule (CSR)Reviewers
Contributors
<jlahoda@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23993/head:pull/23993$ git checkout pull/23993Update a local copy of the PR:
$ git checkout pull/23993$ git pull https://git.openjdk.org/jdk.git pull/23993/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23993View PR using the GUI difftool:
$ git pr show -t 23993Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23993.diff
Using Webrev
Link to Webrev Comment