Uh oh!
There was an error while loading. Please reload this page.
Add simple stdin support#130
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| using System.Collections.Generic; | ||
| namespace Octopus.Shellfish; | ||
| public interface IInputSource | ||
| { | ||
| IEnumerable<string> GetInput(); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -26,6 +26,7 @@ public class ShellCommand | ||
| List<IOutputTarget>? stdOutTargets; | ||
| List<IOutputTarget>? stdErrTargets; | ||
| IInputSource? stdInSource; | ||
| public ShellCommand(string executable) | ||
| { | ||
| @@ -138,18 +139,24 @@ public ShellCommand WithStdErrTarget(IOutputTarget target) | ||
| return this; | ||
| } | ||
| public ShellCommand WithStdInSource(IInputSource source) | ||
| { | ||
| stdInSource = source; | ||
| return this; | ||
| } | ||
| /// <summary> | ||
| /// Launches the process and synchronously waits for it to exit. | ||
| /// </summary> | ||
| public ShellCommandResult Execute(CancellationToken cancellationToken = default) | ||
| { | ||
| using var process = new Process(); | ||
| ConfigureProcess(process, out var shouldBeginOutputRead, out var shouldBeginErrorRead, out var redirectingStdIn); | ||
| var exitedEvent = AttachProcessExitedManualResetEvent(process, cancellationToken); | ||
| process.Start(); | ||
| BeginIoStreams(process, shouldBeginOutputRead, shouldBeginErrorRead, redirectingStdIn); | ||
| try | ||
| { | ||
| @@ -185,12 +192,12 @@ public ShellCommandResult Execute(CancellationToken cancellationToken = default) | ||
| public async Task<ShellCommandResult> ExecuteAsync(CancellationToken cancellationToken = default) | ||
| { | ||
| using var process = new Process(); | ||
| ConfigureProcess(process, out var shouldBeginOutputRead, out var shouldBeginErrorRead, out var redirectingStdIn); | ||
| var exitedTask = AttachProcessExitedTask(process, cancellationToken); | ||
| process.Start(); | ||
| BeginIoStreams(process, shouldBeginOutputRead, shouldBeginErrorRead, redirectingStdIn); | ||
| try | ||
| { | ||
| @@ -216,7 +223,7 @@ public async Task<ShellCommandResult> ExecuteAsync(CancellationToken cancellatio | ||
| } | ||
| // sets standard flags on the Process that apply for both Execute and ExecuteAsync | ||
| void ConfigureProcess(Process process, out bool shouldBeginOutputRead, out bool shouldBeginErrorRead, out bool redirectingStdIn) | ||
| { | ||
| process.StartInfo.FileName = executable; | ||
| @@ -264,7 +271,7 @@ void ConfigureProcess(Process process, out bool shouldBeginOutputRead, out bool | ||
| } | ||
| } | ||
| shouldBeginOutputRead = shouldBeginErrorRead = redirectingStdIn = false; | ||
| if (stdOutTargets is { Count: > 0 }) | ||
| { | ||
| process.StartInfo.RedirectStandardOutput = true; | ||
| @@ -292,15 +299,41 @@ void ConfigureProcess(Process process, out bool shouldBeginOutputRead, out bool | ||
| foreach (var target in targets) target.WriteLine(e.Data); | ||
| }; | ||
| } | ||
| if (stdInSource is not null) | ||
| { | ||
| process.StartInfo.RedirectStandardInput = true; | ||
| redirectingStdIn = true; | ||
| } | ||
| } | ||
| // Common code for Execute and ExecuteAsync to handle stdin and stdout streaming | ||
| void BeginIoStreams(Process process, | ||
| bool shouldBeginOutputRead, | ||
| bool shouldBeginErrorRead, | ||
| bool redirectingStdIn) | ||
| { | ||
| if (shouldBeginOutputRead) process.BeginOutputReadLine(); | ||
| if (shouldBeginErrorRead) process.BeginErrorReadLine(); | ||
| if (redirectingStdIn) | ||
| { | ||
| try | ||
| { | ||
| foreach (var val in stdInSource!.GetInput()) | ||
Contributor There was a problem hiding this comment. In the first iteration of my stdin one, I effectively wrote this; I just had an array of strings for stdin, and I wrote them all in a foreach loop exactly like this one. I found though, that when I ran the test that asks for more than one line of input, it didn't work. What appeared to happen was that the first line of text would get sent to the first prompt, the second line of text would get dropped because the shell script hadn't yet asked for any input, and then shortly later when it did ask, it was too late. I see you've brought that test across in this PR. Does it work for you? If so my hypothesis above must have been wrong in some way. Anyway, while this should work well for simple programs, there are two things which give me pause
| ||
| { | ||
| process.StandardInput.Write(val); | ||
| } | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| // gracefully handle cancellation of the enumerator | ||
| } | ||
| finally | ||
| { | ||
| process.StandardInput.Close(); | ||
| } | ||
| } | ||
| } | ||
| static async Task FinalWaitForExitAsync(Process process, CancellationToken cancellationToken) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| using System.Collections.Generic; | ||
| namespace Octopus.Shellfish; | ||
| class StringInputSource(string value) : IInputSource | ||
| { | ||
| public IEnumerable<string> GetInput() => [value]; | ||
| } | ||
| public static class StringInputSourceExtensions | ||
| { | ||
| public static ShellCommand WithStdInSource(this ShellCommand shellCommand, string input) | ||
| { | ||
| shellCommand.WithStdInSource(new StringInputSource(input)); | ||
| return shellCommand; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| using System; | ||
| using System.IO; | ||
| using System.Runtime.InteropServices; | ||
| namespace Tests.Plumbing; | ||
| public static class TempScript | ||
| { | ||
| // Some interactions such as stdout or encoding codepages require things that don't work with an inline cmd /c or bash -c command | ||
| // This helper writes a script file into the temp directory so we can exercise more complex scenarios | ||
| public static Handle Create(string cmd, string sh) | ||
| { | ||
| if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| { | ||
| var tempFile = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N") + ".cmd"); | ||
| File.WriteAllText(tempFile, cmd); | ||
| return new Handle(tempFile); | ||
| } | ||
| else | ||
| { | ||
| var tempFile = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N") + ".sh"); | ||
| File.WriteAllText(tempFile, sh.Replace("\r\n", "\n")); | ||
| return new Handle(tempFile); | ||
| } | ||
| } | ||
| public class Handle(string scriptPath) : IDisposable | ||
| { | ||
| public string ScriptPath { get; } = scriptPath; | ||
| public void Dispose() | ||
| { | ||
| try | ||
| { | ||
| File.Delete(ScriptPath); | ||
| } | ||
| catch | ||
| { | ||
| // nothing to do if we can't delete the temp file | ||
| } | ||
| } | ||
| // Returns the host application which will run the script. Either cmd.exe or bash | ||
| public string GetHostExecutable() | ||
| { | ||
| return RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "cmd.exe" : "bash"; | ||
| } | ||
| // Returns the command line args to get the host application to run the script | ||
| // For cmd.exe, returns ["/c", ScriptPath] as it needs /c | ||
| // For bash, returns [ScriptPath] as it doesn't need any preamble | ||
| public string[] GetCommandArgs() | ||
| { | ||
| // when running cmd.exe we need /c to tell it to run the script; bash doesn't want any preamble for a script file | ||
| return RuntimeInformation.IsOSPlatform(OSPlatform.Windows) | ||
| ? ["/c", ScriptPath] | ||
| : [ScriptPath]; | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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, I see what you're going for here.
My one basically using IObservable... IObservable is the mirror-image of IEnumerable so you end up with the same approach, just synchronous instead of async.
That said, while sync is much simpler, I'm not sure it's safe or correct 🤔 Will be fun to chat through it.