The automatic windows parameter doesn't work for powershell scripts about simple-exec HOT 9 CLOSED

@mockjv after more thought, I'm leaning towards not providing any special in SimpleExec here. After all, the caller can make the command name and arguments conditional on the OS or any other variable.

For your case, that would mean doing something like this:

await Command.RunAsync(
    RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "powershell" : Paths.DotnetInstallScriptPath,
    $"{(RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? Paths.DotnetInstallScriptPath : "")} -c {version}");

from simple-exec.

it looks like your considering re-introducing that argument

@mockjv as I said in my previous comment (to which you reacted with 👍), I'm leaning towards not providing any special in SimpleExec here, and leaving it to the caller to make the command name and args conditional on the OS, etc.

Note that ps1 is not treated as "executable" in Windows. The only way to "execute" a ps1 file in Windows is to pass it as an argument to powershell.exe (or rely on file extension association to do that for you). So when using SimpleExec to execute a ps1 file, the correct way to do that is:

await RunAsync("powershell", "foo.ps1 bar baz");

If you are running something else on Linux or macOS then you need make your code conditional on the OS. E.g.

await Command.RunAsync(
    RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "powershell" : "foo",
    $"{(RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "foo.ps1" : "")} bar baz");

I acknowledge that you were using the windowsName and windowsArgs parameters for this purpose before:

await Command.RunAsync("foo", "bar baz",
    windowsName: "powershell",
    windowsArgs: "foo.ps1 bar baz");

But I'm not convinced that code is much nicer, not least because you have to pass the "non-script" arguments "bar baz" to both the args and windowsArgs parameters, and therefore I'm not convinced it's worth maintaining that more complex API in SimpleExec for this purpose.

from simple-exec.

@adamralph Ok, sorry for the confusion. I could have sworn I'd seen a commit where changes were reverted but I must be losing my mind or I was looking at the wrong thing. I completely understand your concerns regarding the added complexity in your library, so no worries here!

from simple-exec.

@mockjv thanks for reporting this. How did you execute the script when windowsName was available?

from simple-exec.

@adamralph Glad to help out! This project is really helpful...

The more I think of it, I could see this possibly snowballing a bit on the windows side depending upon exactly how many scripting extensions you would want to support because it would require pre-empting with the appropriate runtime executable:

await Command.RunAsync(Paths.DotnetInstallScriptPath, $"-c {version}",
    windowsName: "powershell",
    windowsArgs: $"{Paths.DotnetInstallScriptPath} -c {version}"
);

I could be wrong but I think it's cscript that's used for vbs, but I'm not sure when it comes to .js (the windows js scripting engine rather than node.js).

Unfortunate that Windows doesn't have the concept of a #! like *nix.

from simple-exec.

This project is really helpful...

Good to know!

I see, so you are executing foo.ps1 on Linux/macOS, and powershell foo.ps1 on Windows. Although this isn't the use case windowsName and windowsArgs were designed for, I can see how they are an obvious feature to use in this scenario.

For some background—windowsName and windowsArgs were added for the scenario where, in a terminal, you would execute just foo on all platforms, where foo really is named foo on Linux/macOS, and is named foo.cmd or foo.bat on Windows (if it was called foo.exe then it would already work). The surprise for users was that you couldn't then just execute SimpleExec.Command.Run("foo") on all platforms due to the behaviour of System.Diagnostics.Process. At the time, I didn't want to get into the weeds of how to make that Just Work on Windows so I added windowsName and windowsArgs instead to allow the user to work out the correct command themselves (it was usually just Run("cmd", "/c foo")). Eventually, I decided to take the plunge in version 10, and now Run("foo") does Just Work on all platforms, and windowsName and windowsArgs are gone.

Spelling things out to ensure I understand correctly—your use case is a bit different. foo.ps1 is not a Windows executable (.exe., .cmd, or .bat) but it has an extension which the system shell associates with a specific executable. In this case, powershell.exe. As you say, on Linux/macOS, we have #!, which allows it to work, but without that, it wouldn't work on Linux/macOS either. If you try and "run" a file which doesn't have #!, and instead has a file extension association in the system shell (for example, a LibreOffice .ods file), it fails in a similar way.

So how do we resolve this? I guess there are a few options:

1. Keep using a conditional in the calling code. A bit ugly and verbose, but I guess it does work.
2. Restore windowsName and windowsArgs—I don't like this option since it's really just "run one thing everywhere apart from Windows" and "run this other special thing just on Windows", which is not really what we want to do. We want to run the same thing everywhere, but Windows needs some help doing that. It's also a bit clunky because you have to shift the actual thing you want to run into the "args".
3. Add overloads which accept something like Func<OS, string> for name and args. This is really just a variation on the previous option, but gives more flexibility by making name and args completely dependent on the OS, rather than just special-casing Windows.
4. Add a new parameter named something like executable, allowing you to specify the name of the executable. E.g. Run("foo.ps1", executable: "powershell"). On Linux/macOS, this would effectively bypass #!.
5. Add a new parameter named something like windowsExecutable. This is a variation of the previous option, but the argument is only used on Windows, allowing #! to be used instead on Linux/macOS (or any OS which supports something similar). Again, we are special-casing Windows, on the assumption that it is the only OS which doesn't support #! or similar.

I think we can discount option 1—we should provide some feature here. Even the simplest single statement feels too clunky:

Run(
    RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "powershell" : "foo.ps1",
    RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "foo.ps1" : "");

Options 2 and 3 seem to provide a too general solution for a very specific problem, so I'm inclined to discount them.

Options 4 and 5 seem to make more sense. I'm not so keen on option 4, since bypassing #! on Linux/macOS feels wrong. E.g. if I want to change the executable used to run the file, I should be able to just change the #! line. I should not have to change the code which calls SimpleExec. Option 5 is effectively a surrogate for #! on Windows, which does somehow feel like the "right" way to do this, although it does assume that only Windows requires an explicit executable in lieu of #! or similar. I don't like making assumptions like that, but I guess it's probably a reasonable assumption.

So while I don't feel completely comfortable with it, I'm inclined to go with option 5.

@mockjv what do you think? Are there more options I haven't thought of?

from simple-exec.

@mockjv looking at your specific scenario more closely, it looks like you are running the dotnet-install scripts, which have different filenames depending on the OS (.ps1 for Windows and .sh for everything else), so I guess some of my last comment isn't relevant. In that case, a conditional in the consuming code probably is the best solution, and it seems to be what you are already doing with your Paths.DotnetInstallScriptPath property.

It also struck me that the executable parameter mentioned in option 4 could also be set conditionally. For example, you could do:

Paths.DotnetInstallScriptPath = RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
    ? "dotnet-install.ps1" : "dotnet-install.sh";

Paths.DotnetInstallScriptExecutable = RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
    ? "powershell" : "";

await Command.RunAsync(
    Paths.DotnetInstallScriptPath, $"-c {version}", executable: Paths.DotnetInstallScriptExecutable);

And I guess you could even do this now, in SimpleExec 10.0.0:

Paths.DotnetInstallScriptPath = RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
    ? "dotnet-install.ps1" : "dotnet-install.sh";

Paths.DotnetInstallScriptExecutable = RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
    ? "powershell" : "bash";

await Command.RunAsync(
    Paths.DotnetInstallScriptExecutable, $"{Paths.DotnetInstallScriptPath} -c {version}");

But again, it feels weird to be bypassing #! in the .sh file, because that could be set to zsh or any other shell, so I think it probably is better to introduce the executable parameter, purely to act as a surrogate for #! in Windows. And although having string executable instead of string windowsExecutable could be viewed as too general a solution, it avoids making another assumption about Windows in SimpleExec, so I think I prefer it.

from simple-exec.

So I had a go at adding the executable parameter, but it turns out to be non-trivial. The problem is that, internally, SimpleExec assigns name to ProcessStartInfo.FileName and args to ProcessStartInfo.Arguments. By introducing executable, SimpleExec would have to conditionally assign executable to ProcessStartInfo.FileName, and it would have to concatenate name and args to assign to ProcessStartInfo.Arguments. But then it would also have to escape name appropriately. For example it may contains spaces, so it would have to be wrapped in ". But what if it's already wrapped in "? It's a can of worms I don't want to get into.

So, I am now thinking to backtrack all the way back and just restore windowsName and windowsArgs. 😆 🤦 Although their purpose would be different to their original one. They would no longer be required when foo implicitly refers to foo.cmd or foo.bat, but they would be required for any other filename (other than foo.exe).

from simple-exec.

Sorry, it's been a hectic month and I just haven't had a lot of time to spend beyond day-to-day work. I completely respect that essentially having to account for the lack of #! support in Windows can snowball pretty quickly. My original thought was just highlighting a use case that regressed with the removal of the windowsName argument, but it looks like your considering re-introducing that argument.

from simple-exec.