Re: RFR: 8304498: JShell does not switch to raw mode when there is no /bin/test [v2]
On Tue, 28 Mar 2023 18:33:46 GMT, Andrey Turbanov wrote: > @lahodaj was the issue reported to the upstream? I've just filled: https://github.com/jline/jline3/issues/839 - PR Comment: https://git.openjdk.org/jdk/pull/13100#issuecomment-1488819982
Re: RFR: 8304498: JShell does not switch to raw mode when there is no /bin/test [v2]
On Wed, 22 Mar 2023 18:38:08 GMT, Jan Lahoda wrote: >> If JShell is run on a system that does not have `/bin/test` (which is, >> apparently, possible for some systems, which only have `/usr/bin/test`), it >> won't switch the terminal into the raw mode, and the input will not work >> properly. >> >> The proposed fix herein is to detect whether `test` existing in >> `/usr/bin/test`, and if yes, use that location. Use the existing `/bin/test` >> otherwise. > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Checking the executable flags instead of running the program, as suggested. @lahodaj was the issue reported to the upstream? - PR Comment: https://git.openjdk.org/jdk/pull/13100#issuecomment-1487419384
Re: RFR: 8304498: JShell does not switch to raw mode when there is no /bin/test [v2]
On Wed, 22 Mar 2023 18:38:08 GMT, Jan Lahoda wrote: >> If JShell is run on a system that does not have `/bin/test` (which is, >> apparently, possible for some systems, which only have `/usr/bin/test`), it >> won't switch the terminal into the raw mode, and the input will not work >> properly. >> >> The proposed fix herein is to detect whether `test` existing in >> `/usr/bin/test`, and if yes, use that location. Use the existing `/bin/test` >> otherwise. > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Checking the executable flags instead of running the program, as suggested. looks sensible - Marked as reviewed by vromero (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13100#pullrequestreview-1359340541
Re: RFR: 8304498: JShell does not switch to raw mode when there is no /bin/test [v2]
On Wed, 22 Mar 2023 18:38:08 GMT, Jan Lahoda wrote: >> If JShell is run on a system that does not have `/bin/test` (which is, >> apparently, possible for some systems, which only have `/usr/bin/test`), it >> won't switch the terminal into the raw mode, and the input will not work >> properly. >> >> The proposed fix herein is to detect whether `test` existing in >> `/usr/bin/test`, and if yes, use that location. Use the existing `/bin/test` >> otherwise. > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Checking the executable flags instead of running the program, as suggested. LGTM the test call seems to be only used in one method where JLine attempts to test the tty fd value (AFAICT) - they're hacking into java.lang.ProcessBuilder$RedirectPipeImpl for that purpose. wonder if there's anything else the JDK libs can offer. https://github.com/openjdk/jdk/blob/master/src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/exec/ExecTerminalProvider.java#L120 - Marked as reviewed by coffeys (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13100#pullrequestreview-1354582322
Re: RFR: 8304498: JShell does not switch to raw mode when there is no /bin/test [v2]
> If JShell is run on a system that does not have `/bin/test` (which is, > apparently, possible for some systems, which only have `/usr/bin/test`), it > won't switch the terminal into the raw mode, and the input will not work > properly. > > The proposed fix herein is to detect whether `test` existing in > `/usr/bin/test`, and if yes, use that location. Use the existing `/bin/test` > otherwise. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Checking the executable flags instead of running the program, as suggested. - Changes: - all: https://git.openjdk.org/jdk/pull/13100/files - new: https://git.openjdk.org/jdk/pull/13100/files/159ef1a0..f7631210 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13100&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13100&range=00-01 Stats: 9 lines in 1 file changed: 2 ins; 6 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13100.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13100/head:pull/13100 PR: https://git.openjdk.org/jdk/pull/13100
Re: RFR: 8304498: JShell does not switch to raw mode when there is no /bin/test
On Mon, 20 Mar 2023 12:10:04 GMT, Jan Lahoda wrote:
> If JShell is run on a system that does not have `/bin/test` (which is,
> apparently, possible for some systems, which only have `/usr/bin/test`), it
> won't switch the terminal into the raw mode, and the input will not work
> properly.
>
> The proposed fix herein is to detect whether `test` existing in
> `/usr/bin/test`, and if yes, use that location. Use the existing `/bin/test`
> otherwise.
src/jdk.internal.le/share/classes/jdk/internal/org/jline/utils/OSUtils.java
line 108:
> 106: private static boolean isTestCommandValid(String command) {
> 107: try {
> 108: Process p = new ProcessBuilder(command, "-z",
> "").inheritIO().start();
is there a reason why you chose to spin up a process here ? Would a test for an
executable file be sufficient ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13100#discussion_r1144922827
Re: RFR: 8304498: JShell does not switch to raw mode when there is no /bin/test
On Mon, 20 Mar 2023 12:10:04 GMT, Jan Lahoda wrote: > If JShell is run on a system that does not have `/bin/test` (which is, > apparently, possible for some systems, which only have `/usr/bin/test`), it > won't switch the terminal into the raw mode, and the input will not work > properly. > > The proposed fix herein is to detect whether `test` existing in > `/usr/bin/test`, and if yes, use that location. Use the existing `/bin/test` > otherwise. As a general point, we probably need to change jline to not create sub-processes. If we needs to introspect the environment then it will need to do it in the current VM. - PR: https://git.openjdk.org/jdk/pull/13100
