Skip to content

Commit

Permalink
Leverage native ProcessStartInfo.ArgumentList API instead of string-b…
Browse files Browse the repository at this point in the history 
…ased

API on newer frameworks.

fix #95
  • Loading branch information
@madelson
madelson committed Feb 25, 2023
1 parent 8774b77 commit 879dbbd
Show file tree
Hide file tree
Showing 6 changed files with 90 additions and 33 deletions.
27 changes: 26 additions & 1 deletion MedallionShell.Tests/GeneralTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -488,9 +488,14 @@ void TestHelper(bool disposeOnExit)
}
else
{
#if NETCOREAPP
Assert.That(command1.Process.StartInfo.ArgumentList, Does.Contain("--id1"));
Assert.That(command2.Process.StartInfo.ArgumentList, Does.Contain("--id2"));
#else
command1.Process.StartInfo.Arguments.ShouldContain("--id1");
command1.Processes.SequenceEqual(new[] { command1.Process });
command2.Process.StartInfo.Arguments.ShouldContain("--id2");
#endif
command1.Processes.SequenceEqual(new[] { command1.Process });
command2.Processes.SequenceEqual(new[] { command2.Process }).ShouldEqual(true);
pipeCommand.Process.ShouldEqual(command2.Process);
pipeCommand.Processes.SequenceEqual(new[] { command1.Process, command2.Process }).ShouldEqual(true);
Expand Down Expand Up @@ -557,6 +562,10 @@ public void TestToString()
var command6 = command5.RedirectTo(new StringWriter());
command6.Wait();
command6.ToString().ShouldEqual($"{command5} > {new StringWriter()}");

var command7 = TestShell.Run(SampleCommand);
command7.Wait();
command7.ToString().ShouldEqual(sampleCommandString);
}

[Test]
Expand Down Expand Up @@ -605,6 +614,22 @@ public void TestProcessKeepsWritingAfterOutputIsClosed()
}
}

[Test]
[Obsolete("Tests obsolete code")]
public async Task TestCustomCommandLineSyntaxIsUsed()
{
using var command = TestShell.Run(
SampleCommand,
new object[] { "exit", 0 },
options: o => o.Syntax((CommandLineSyntax)Activator.CreateInstance(PlatformCompatibilityHelper.DefaultCommandLineSyntax.GetType())!)
.DisposeOnExit(false)
);

Assert.That(command.Process.StartInfo.Arguments, Does.EndWith("exit 0"));

await command.Task;
}

private IEnumerable<string> ErrorLines()
{
yield return "1";
Expand Down
7 changes: 1 addition & 6 deletions MedallionShell.Tests/UnitTestHelpers.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,7 @@ public static T ShouldNotEqual<T>(this T @this, T that, string? message = null)

public static string ShouldContain(this string haystack, string needle, string? message = null)
{
Assert.IsNotNull(haystack, $"Expected: contains '{needle}'. Was: NULL{(message != null ? $" ({message})" : string.Empty)}");
if (!haystack.Contains(needle))
{
Assert.Fail($"Expected: contains '{needle}'. Was: '{haystack}'{(message != null ? $" ({message})" : string.Empty)}");
}

Assert.That(haystack, Does.Contain(needle), message);
return haystack;
}
}
Expand Down
12 changes: 4 additions & 8 deletions MedallionShell/PlatformCompatibilityHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,10 @@ internal static class PlatformCompatibilityHelper

public static bool IsWindows => RuntimeInformation.IsOSPlatform(OSPlatform.Windows);

public static CommandLineSyntax GetDefaultCommandLineSyntax()
{
if (IsMono && !IsWindows)
{
return new MonoUnixCommandLineSyntax();
}
return new WindowsCommandLineSyntax();
}
public static readonly CommandLineSyntax DefaultCommandLineSyntax =
IsMono && !IsWindows
? new MonoUnixCommandLineSyntax()
: new WindowsCommandLineSyntax();

public static Stream WrapStandardInputStreamIfNeeded(Stream stream)
{
Expand Down
19 changes: 16 additions & 3 deletions MedallionShell/ProcessCommand.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ internal ProcessCommand(
{
this.disposeOnExit = disposeOnExit;
this.fileName = startInfo.FileName;
this.arguments = startInfo.Arguments;
this.arguments = GetArgumentsString(startInfo);
this.process = new Process { StartInfo = startInfo, EnableRaisingEvents = true };

var processMonitoringTask = CreateProcessMonitoringTask(this.process);
Expand Down Expand Up @@ -78,6 +78,18 @@ internal ProcessCommand(
this.task = this.CreateCombinedTask(processTask, ioTasks);
}

private static string GetArgumentsString(ProcessStartInfo startInfo)
{
#if NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1
if (startInfo.ArgumentList.Count > 0)
{
return PlatformCompatibilityHelper.DefaultCommandLineSyntax.CreateArgumentString(startInfo.ArgumentList);
}
#endif

return startInfo.Arguments;
}

private async Task<CommandResult> CreateCombinedTask(Task<int> processTask, IReadOnlyList<Task> ioTasks)
{
int exitCode;
Expand All @@ -100,7 +112,7 @@ private async Task<CommandResult> CreateCombinedTask(Task<int> processTask, IRea
}

private readonly Process process;
public override System.Diagnostics.Process Process
public override Process Process
{
get
{
Expand Down Expand Up @@ -147,7 +159,8 @@ public override int ProcessId
private readonly Task<CommandResult> task;
public override Task<CommandResult> Task { get { return this.task; } }

public override string ToString() => this.fileName + " " + this.arguments;
public override string ToString() =>
this.arguments.Length == 0 ? this.fileName : this.fileName + " " + this.arguments;

public override void Kill()
{
Expand Down
49 changes: 35 additions & 14 deletions MedallionShell/Shell.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,19 +40,16 @@ public Command Run(string executable, IEnumerable<object>? arguments = null, Act

var processStartInfo = new ProcessStartInfo
{
Arguments = arguments != null
? finalOptions.CommandLineSyntax.CreateArgumentString(arguments.Select(
arg => Convert.ToString(arg, CultureInfo.InvariantCulture)
?? throw new ArgumentException("must not contain null", nameof(arguments))
))
: string.Empty,
CreateNoWindow = true,
FileName = executable,
RedirectStandardError = true,
RedirectStandardInput = true,
RedirectStandardOutput = true,
UseShellExecute = false,
};

PopulateArguments(processStartInfo, arguments, finalOptions);

if (finalOptions.ProcessStreamEncoding != null)
{
processStartInfo.StandardOutputEncoding = processStartInfo.StandardErrorEncoding = finalOptions.ProcessStreamEncoding;
Expand All @@ -76,6 +73,30 @@ public Command Run(string executable, IEnumerable<object>? arguments = null, Act
return command;
}

private static void PopulateArguments(ProcessStartInfo processStartInfo, IEnumerable<object?>? arguments, Options options)
{
if (arguments is null) { return; }

var argumentStrings = arguments.Select(
arg => Convert.ToString(arg, CultureInfo.InvariantCulture)
?? throw new ArgumentException("must not contain null", nameof(arguments))
);

#if NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1
// If using the default command line syntax, prefer to use ArgumentsList rather than encoding ourselves (#95)
if (options.CommandLineSyntax == PlatformCompatibilityHelper.DefaultCommandLineSyntax)
{
foreach (var argumentString in argumentStrings)
{
processStartInfo.ArgumentList.Add(argumentString);
}
return;
}
#endif

processStartInfo.Arguments = options.CommandLineSyntax.CreateArgumentString(argumentStrings);
}

/// <summary>
/// Tries to attach to an already running process, given its <paramref name="processId"/>,
/// giving <paramref name="attachedCommand" /> representing the process and returning
Expand Down Expand Up @@ -143,14 +164,14 @@ public Command Run(string executable, params object[] arguments)

return this.Run(executable, arguments.AsEnumerable());
}
#endregion
#endregion

#region ---- Static API ----
#region ---- Static API ----
/// <summary>
/// A <see cref="Shell"/> that uses default options
/// </summary>
public static Shell Default { get; } = new Shell(_ => { });
#endregion
#endregion

private Options GetOptions(Action<Options>? additionalConfiguration)
{
Expand All @@ -160,7 +181,7 @@ private Options GetOptions(Action<Options>? additionalConfiguration)
return builder;
}

#region ---- Options ----
#region ---- Options ----
/// <summary>
/// Provides a builder interface for configuring the options for creating and executing
/// a <see cref="Medallion.Shell.Command"/>
Expand All @@ -181,15 +202,15 @@ internal Options()
internal Encoding? ProcessStreamEncoding { get; private set; }
internal CancellationToken ProcessCancellationToken { get; private set; }

#region ---- Builder methods ----
#region ---- Builder methods ----
/// <summary>
/// Restores all settings to the default value
/// </summary>
public Options RestoreDefaults()
{
this.StartInfoInitializers = new List<Action<ProcessStartInfo>>();
this.CommandInitializers = new List<Func<Command, Command>>();
this.CommandLineSyntax = PlatformCompatibilityHelper.GetDefaultCommandLineSyntax();
this.CommandLineSyntax = PlatformCompatibilityHelper.DefaultCommandLineSyntax;
this.ThrowExceptionOnError = false;
this.DisposeProcessOnExit = true;
this.ProcessTimeout = System.Threading.Timeout.InfiniteTimeSpan;
Expand Down Expand Up @@ -345,9 +366,9 @@ public Options CancellationToken(CancellationToken cancellationToken)
this.ProcessCancellationToken = cancellationToken;
return this;
}
#endregion
#endregion
}
#endregion
#endregion

private static bool IsIgnorableAttachingException(Exception exception)
{
Expand Down
9 changes: 8 additions & 1 deletion SampleCommand/PlatformCompatibilityTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,14 @@ private static string GetSampleCommandPath()
// on .net core, you can't run .net exes directly so instead we invoke them through dotnet
if (si.FileName == SampleCommandPath)
{
si.Arguments = !string.IsNullOrEmpty(si.Arguments) ? $"{si.FileName} {si.Arguments}" : si.FileName;
if (!string.IsNullOrEmpty(si.Arguments))
{
si.Arguments = $"{si.FileName} {si.Arguments}";
}
else
{
si.ArgumentList.Insert(0, si.FileName);
}
si.FileName = DotNetPath;
}
}));
Expand Down

0 comments on commit 879dbbd

Please sign in to comment.