Feature Request: ExecShell

[brthor]

Throwing this up for discussion based on the conversations today around extension inference in exec.

Let's flesh out the requirements, and features this task should implement.

Extension Inference

From the migration perspective, extension inference is a must, and which extensions are inferred should be configurable via a semicolon delimited list passed as a task parameter (InferredExtensions?)

Determining the first token in the passed in Command for this task, is a difficult but solved problem. We can take advantage of existing implementations of these algorithms.

For breaking apart the input string array into tokens we can use the implementation in CoreFX that does this:
https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs#L437

After mutating the tokens, to recombine them into a single string for use by Process.Start, we can use the analog implementation of the above algorithm present in the cli:
https://github.com/dotnet/cli/blob/rel/1.0.0/src/Microsoft.DotNet.Cli.Utils/ArgumentEscaper.cs#L23

@rainersigwald @livarcocc @eerhardt @cdmihai @AndyGerlicher @Sarabeth-Jaffe-Microsoft

[rainersigwald]

To take a step back: the motivation for this desire is that it's common practice (look even in our own repo!) to have two copies of a script, for example build.sh for *nix and build.cmd for Windows. But then it's a hassle to refer to the script in an Exec task, because you have to splice in a different script depending on the current environment.

The dotnet CLI solved this in project.json by being smart: given a tool definition like build argument1 argument2, there was logical fallback behavior to determine that build might be a script, and then to order candidates to find the best one.

We thought about adding similar behavior to Exec. But that's concerning, because Exec has always behaved as though its contents were themselves in a script, and changing that seems risky.

Creating a new built-in task ExecScript task would have no compat impact, and the behavior could be clear from the arguments and call location.

[jeffkl]

Would it make sense to add a property to Exec like IsScript=true which could enable this logic? I'm worried maintaining to tasks might be a problem. Unless this ExecScript just inherits from Exec...

[rainersigwald]

❓ Open design question:

What should the arguments to this task be? I see a decision point:

• "Command Line" as an argument.
  • ➕: Directly analagous to Exec
  • ➕: Requires minimal logic at conversion-from-project.json time
  • ➖: Requires parsing the command line for every task execution--and that's not trivial
• Separate "script" and "arguments" arguments
  • ➕: Makes it very clear at the call site what will be substituted
  • ➕: Simpler to implement
  • ➕: More analagous to Process.Start
  • ➖: Requires conversion logic to handle the tokenization + splitting
  • ➖: More verbose, somewhat harder to call correctly

I lean toward the latter: a simpler implementation is more likely to be correct, and conversion is a point-in-time problem; we should provide a good way to deal with the long future.

[rainersigwald]

❓ Open design question:

Is there an opportunity to combine this with #399 (direct Process.Start with no shell layer)? Some of the design considerations are similar.

[cdmihai]

Maybe ExecScript could have an InvocationMode parameter which defaults to using Process.Start, but the user could also pick a shell exec?

[jonpryor]

Aside: I do not like the name ExecShell, as it implies invokes a shell, which sounds crazy.

Extension inference also seems unnecessary.

Here's why: Process.Start() will automatically check some filename extensions including .exe and .cmd to determine file extensions to use. For example, if you have a foo.cmd file, and you Process.Start("foo", ""), foo.cmd will be executed.

Similarly, in the same way that the Run dialog box can accept an executable file name with or without the .exe extension, the .exe extension is optional in the fileName parameter.

This thus provides a portable cross-platform pattern: instead of platform-specific file-extensions, embrace convention: provide a build script for Unix, and a build.cmd script for Windows. <ExecProcess FileName="build" .../> will be Process.Start ("build", ...), and on Unix that will invoke build, while on Windows that will build build.cmd, and everybody is happy. :-)

I thus think that this issue is actually a duplicate of Issue #399.

[brthor]

Thanks for taking this a step back and looking at the high level @rainersigwald

ExecShell was entirely something I made up and I like ExecScript as the name quite a bit better, it fits with the intentions.

Is there an opportunity to combine this with #399 (direct Process.Start with no shell layer)? Some of the design considerations are similar.

From my experience dealing with splitting, recombining command line arguments in the cli, avoiding a shell layer will make this problem simpler. 👍 for this

What should the arguments to this task be? I see a decision point:

I would like to propose a 3rd option (very similar to option #2) which will create less indirection from a user perspective.

• Separate "Executable" and "Arguments" (Arguments takes in an item group or semicolon delimited property)

➕: Makes it very clear at the call site what will end up in my string[] args in main of my next program, Users don't need to worry about escaping, string[] args will be the exact strings put into the items
➕: Makes it very clear at the call site what will be substituted
➕: Simpler to implement, can leverage CLI code for escaping
➕: More analagous to Process.Start, except the bad parts where you need to escape things
➖: Requires conversion logic to handle the tokenization + splitting
➖: More verbose, somewhat harder to call correctly

I think the primary difference here is that Arguments is an item group. The intention is that whatever strings are put in there will be the exact argument array passed to the Executable. This puts the onus of escaping properly on us, and not on the users. This is possible if we do as Rainer suggested and use Process.Start directly rather than use a shell layer. Using a shell layer will be much more difficult to implement this correctly on all platforms. CLI implemented this with robust tests surrounding it, so potential for reuse is there.

@cdmihai

Maybe ExecScript could have an InvocationMode parameter which defaults to using Process.Start, but the user could also pick a shell exec?

This is very similar to Process.Start but would require that we develop escaping logic for every supported shell or put that onus on the user.

@jonpryor

Extension inference also seems unnecessary.

The extension inference you're referring to is only present on Windows AFAIK and IIRC is based on the PATHEXT environment variable. The Windows only piece is enough reason to need this feature, but I think the fact that the inferred extensions are based on an environment variable is very undesirable for creating reproducible, deterministic builds.

The example you've given supposes that build has no extension, but what if build has the sh extension, or I have a build.sh and build.py that I want to use on different platforms, where bash isn't available. That's somewhat contrived but I think shows a basic use case for script extension inference on all platforms.

[livarcocc]

Do we have enough to make a choice here? If so, can we pick one and get an ETA to when we can get this in?

[blackdwarf]

To weigh somewhat on the "extensionless" scripts mentioned above, I don't think we should force our users down a specific convention in this case given the prevalent amount of evidence that people do, in fact, use the .sh extension for their build scripts (and, as @brthor mentioned, there are other languages you may opt for in this use case). Forcing people to mess with their scripts in this way sounds like a non-optimal UX to me.