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

Fixes for usage in combination with loadwatch #238

Closed
wants to merge 1 commit into from

Conversation

iMichka
Copy link

@iMichka iMichka commented Apr 7, 2016

This fixes the problems described in bug #230.

When using jline to create a new ConsoleReader object
in combination with loadwatch, the process never returned.

In jline1, a long time ago, Runtime.getRuntime().exec(cmd)
was used to execute commands. This was migrated to the
ProcessBuilder API later, which is the recommended way.

In the meantime, the commands (as plain strings) were split
manually with split(""), whereas Runtime.getRuntime().exec(cmd)
was using a StringTokenizer internally.

The java implementations here are different, and one needs to
be carefull when passing down stuff to the ProcessBuilder.
See also:
http://stackoverflow.com/questions/6856028/difference-between-processbuilder-and-runtime-exec

My implementation just uses explicit List calls everywhere
to make clear how arguments are split, which fixes the usage of
jline in combination with loadwatch.

This fixes the problems described in bug jline#230.

When using jline to create a new ConsoleReader object
in combination with loadwatch, the process never returned.

In jline1, a long time ago, Runtime.getRuntime().exec(cmd)
was used to execute commands. This was migrated to the
ProcessBuilder API later, which is the recommended way.

In the meantime, the commands (as plain strings) were split
manually with split(""), whereas Runtime.getRuntime().exec(cmd)
was using a StringTokenizer internally.

The java implementations here are different, and one needs to
be carefull when passing down stuff to the ProcessBuilder.
See also:
http://stackoverflow.com/questions/6856028/difference-between-processbuilder-and-runtime-exec

My implementation just uses explicit List<String> calls everywhere
to make clear how arguments are split, which fixes the usage of
jline in combination with loadwatch.
@iMichka
Copy link
Author

iMichka commented Apr 7, 2016

Two notes:

Loadwatch has probably some problems that also needs to be addressed (but my knowledge about terminal vodoo is probably too limited to understand what is going on here ...):
This will never return: loadwatch -h 10 -- stty -icanon
This works: loadwatch -h 10 -- "stty -icanon"
This works: loadwatch -h 10 -- ls -l

@gnodet
Copy link
Member

gnodet commented Sep 22, 2016

The PR looks wrong to me.
sh -c takes a single argument which may need to be quoted.
At least, on OS X, splitting the arguments results in those arguments not being passed in the command.

        ArrayList<String> cmdList = new ArrayList<String>();
        cmdList.add("sh");
        cmdList.add("-c");
        cmdList.addAll(Arrays.asList("/Users/gnodet/tmp/test.sh", "a", "b", "c", "d"));
        Process p = new ProcessBuilder(cmdList).start();
        String result = waitAndCapture(p);
        System.out.println(result);

With test.sh being:

echo 1$1
echo 2$2
echo 3$3
echo 4$4

The result is the following:

1
2
3
4

While the following code:

        ArrayList<String> cmdList = new ArrayList<String>();
        cmdList.add("sh");
        cmdList.add("-c");
        cmdList.add("/Users/gnodet/tmp/test.sh a b c d");
        Process p = new ProcessBuilder(cmdList).start();
        String result = waitAndCapture(p);
        System.out.println(result);

produces the expected result:

1a
2b
3c
4d

@iMichka
Copy link
Author

iMichka commented Dec 5, 2016

Sorry for the long delay. You are right, my fix is probably wrong. I wrote this some time ago so I forgot about the details. I will need to dive into the code again to see if I can find another solution.

What is clear is that at one point a regression was introduced in jline, and it would be great if it could be fixed. Maybe you could have a look too, you probably better know the code than me? Not sure when I will find time to start working on this again ...

@iMichka
Copy link
Author

iMichka commented Dec 12, 2016

Closing (see full discussion in #230)

@iMichka iMichka closed this Dec 12, 2016
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.

None yet

2 participants