vezel-dev / cathode

A terminal-centric replacement for the .NET console APIs.
https://docs.vezel.dev/cathode
BSD Zero Clause License
91 stars 7 forks source link

Ability to use Arguments on ChildProcess rather than ArgumentList #159

Closed scottbilas closed 6 days ago

scottbilas commented 3 months ago

I'd like to be able to ask ChildProcessBuilder to set ProcessStartInfo.Arguments, rather than ArgumentList. The problem with ArgumentList is that it ends up escaping its arguments before composing them into a single string.

My specific case is using ChildProcess to run (for example) cmd with /c dir \, but ArgumentList causes this to become "/c dir \\"``, which then fails with an error fromcmd`.

This little hack works around it:

diff --git i/src/core/Processes/ChildProcess.cs w/src/core/Processes/ChildProcess.cs
index 1f7553f..dcfaed4 100644
--- i/src/core/Processes/ChildProcess.cs
+++ w/src/core/Processes/ChildProcess.cs
@@ -46,8 +46,13 @@ public sealed class ChildProcess
             RedirectStandardError = redirectError,
         };

-        foreach (var arg in builder.Arguments)
-            info.ArgumentList.Add(arg);
+        if (builder.Arguments.Length == 1)
+            info.Arguments = builder.Arguments[0];
+        else
+        {
+            foreach (var arg in builder.Arguments)
+                info.ArgumentList.Add(arg);
+        }

         foreach (var (name, value) in builder.Variables)
             info.Environment.Add(name, value);

Obviously overloading the meaning of single-element array is bad. Though I've never really liked PSI's two ways of doing arguments either! Perhaps ChildProcessBuilder can do something a little nicer than those ideas. :)

alexrp commented 3 months ago

Hm, my understanding was that ArgumentList made Arguments basically obsolete. Is there really no way to do this with ArgumentList? :thinking:

I'm okay with just exposing Arguments if it's really necessary; I just didn't expect it to be...

Some potentially useful context that I'd need to read through: https://github.com/dotnet/runtime/issues/23347

scottbilas commented 3 months ago

Hmm, a helpful link from that issue: https://learn.microsoft.com/en-us/cpp/cpp/main-function-command-line-args?view=msvc-170#parsing-c-command-line-arguments

My tool has a cli format that supports -- COMMAND <remaining-args>... where I pass through verbatim remaining-args to COMMAND. To do this, I use Environment.CommandLine rather than the incoming string[] args. My code is therefore very Windowsy in its command line processing. When it passes this as the only arg (via ChildProcessBuilder) to ArgumentList, it gets escaped according to the inverse of those MSVC rules so that it will roundtrip back to a single arg. Definitely not the behavior I want.

So: anyone doing Windowsy cli processing like this will need access to Arguments.

But - I'm realizing that I should (since I do want this tool to be xplat) not use Environment.CommandLine and instead use args, letting the machinery underneath do its job. I changed my code to work this way and I'm now getting the behavior I want and have removed my local hack to Cathode.

alexrp commented 3 months ago

Hmm, I see. Perhaps Environment.CommandLine ought to be considered soft-obsolete in the same way that ProcessStartInfo.Arguments is. I wonder if it would make sense to propose an Environment.CommandLineList property for cases where you don't have access to args for whatever reason (e.g. plugins). :thinking: The lack of such a property for that particular use case might still be a good reason to do this feature request. I'll leave this open for now while I think on it.

alexrp commented 2 months ago

It's a bit unfortunate that ChildProcessBuilder.Arguments already exists as an array. I might have to do a breaking change and rename that to ArgumentList, so that I can add an Arguments property. :thinking:

alexrp commented 2 weeks ago

Just going to do the breaking change here; the current Arguments property will become ArgumentList, and a new Arguments property will be added.

If an API user fills out both, what should the behavior be? For ProcessStartInfo, .NET will prefer ArgumentList and ignore Arguments in that case. So there's a fair, uh, argument that we should match that behavior. OTOH, this could indicate a programmer mistake, so perhaps throwing an exception could also be justified. :thinking:

alexrp commented 6 days ago

Went in a slightly different direction. You can now use .WithJoinArguments(true) on the builder. This will make the argument list get concatenated with a ` separator and passed toProcessStartInfo.Arguments. If you just want the latter part without the concatenation, just add all your arguments as a single string to the list and enable that option - that single string then gets passed verbatim toProcessStartInfo.Arguments`.

@scottbilas this should be fine for your use case, right?