Tyrrrz / CliWrap

Library for running command-line processes
MIT License
4.32k stars 264 forks source link

Add WithExitCondition feature #242

Closed dmilosz closed 4 months ago

dmilosz commented 4 months ago

Closes #241

Example usage:

var cmd = Cli.Wrap("powershell")
    .WithArguments("Start-Process -NoNewWindow -FilePath \"powershell\" -ArgumentList \"'sleep 1000'\"; exit 0")
    .WithWaitingForOutputProcessing(false)
    .WithStandardOutputPipe(PipeTarget.ToDelegate(line => Console.WriteLine($"STDOUT {line}")))
    .WithStandardErrorPipe(PipeTarget.ToDelegate(line => Console.WriteLine($"STDERR {line}")));

var result = await cmd.ExecuteAsync();

There is no breaking change. Current implementations will get true by default and will go with old, standard flow.

Tyrrrz commented 4 months ago

I wonder if there's a way to implement this in another way -- by somehow instructing the child process not to share the stdout/err/in streams with its children. I wonder if it's possible? Is this issue specific to any OS?

dmilosz commented 4 months ago

I'm not aware of any method to tell grand children what streams to use. I think only the direct parent is capable of doing that. The issue can also be reproduced with Ubuntu and bash, using nohup and background processing. Currently:

using CliWrap;

Console.WriteLine($"Program started at: {DateTime.Now.ToLongTimeString()}");

var cmd = Cli.Wrap("bash")
    .WithArguments(["-c", "nohup bash -c \"sleep 5; exit 0\" &"])
    .WithStandardOutputPipe(PipeTarget.ToDelegate(line => Console.WriteLine($"STDOUT {line}")))
    .WithStandardErrorPipe(PipeTarget.ToDelegate(line => Console.WriteLine($"STDERR {line}")));

var result = await cmd.ExecuteAsync();

Console.WriteLine($"Process started at: {result.StartTime.DateTime.ToLongTimeString()}");
Console.WriteLine($"Process finished with exit code: {result.ExitCode}");
Console.WriteLine($"Process finished at: {result.ExitTime.DateTime.ToLongTimeString()}");

Console.WriteLine($"Program finished at: {DateTime.Now.ToLongTimeString()}");

Output:

Program started at: 10:59:18
Process started at: 10:59:18
Process finished with exit code: 0
Process finished at: 10:59:18
Program finished at: 10:59:23

As you can see, the parent was finished almost instantly, but CliWrap waited for child process.

Now, after the change, using WithExitCondition(CommandExitCondition.ProcessExited):


using CliWrap;

Console.WriteLine($"Program started at: {DateTime.Now.ToLongTimeString()}");

var cmd = Cli.Wrap("bash")
    .WithArguments(["-c", "nohup bash -c \"sleep 5; exit 0\" &"])
    .WithExitCondition(CommandExitCondition.ProcessExited)
    .WithStandardOutputPipe(PipeTarget.ToDelegate(line => Console.WriteLine($"STDOUT {line}")))
    .WithStandardErrorPipe(PipeTarget.ToDelegate(line => Console.WriteLine($"STDERR {line}")));

var result = await cmd.ExecuteAsync();

Console.WriteLine($"Process started at: {result.StartTime.DateTime.ToLongTimeString()}");
Console.WriteLine($"Process finished with exit code: {result.ExitCode}");
Console.WriteLine($"Process finished at: {result.ExitTime.DateTime.ToLongTimeString()}");

Console.WriteLine($"Program finished at: {DateTime.Now.ToLongTimeString()}");

Output:

Program started at: 11:09:01
Process started at: 11:09:01
Process finished with exit code: 0
Process finished at: 11:09:01
Program finished at: 11:09:01

If you check processes after the program is finished, you can verify the bash child process is still running, after the program exits.

dmilosz commented 4 months ago

Can you please also add a test that fails with the current behavior, but works with the new option? You can probably use CliWrap.Tests.Dummy to spawn a child process.

I've added 2 unit tests. They are not perfect, because the may fail unexpectedly when the machine is slow, since they depend on execution time. I don't have a better idea for a test which is 100% stable. However, child process is set to be running for 3 seconds so should be fine for most environments.

In the seconds test, if you remove .WithExitCondition(CommandExitCondition.ProcessExited); line, it will fail because child process will be definitely finished after the main process called by CLiWrap is done.

dmilosz commented 4 months ago

I've implemented better solution in #243