Better support for windows

[Lej77]

I tried to use this on windows and got some compile errors so I fixed those. Then I noticed that the started tasks failed to start (they were started using sh instead of cmd). The output from cmd had lots of unicode replacement characters in them so I added a flag to cmd that made it output unicode instead and then wrote some code to parse the output as utf16 instead of utf8. Now it seems to work for simple tasks so I thought I'd create this pull request with my changes.

You will notice that the daemon/task_handler.rs file just returns errors when signals are used. So something better should probably be done there but otherwise it should hopefully work. Also I'm not 100% sure about the config paths but they seem to be okay.

[Nukesor]

Wow! Thanks a lot!

I'll take a proper look at it as soon as I get some spare time!

One thing you can already do:
Add this back to the .github/workflows/{test.yml,package-binary.yml} matrix section:

matrix:
...
    os: [ubuntu-latest, windows-latest, macos-latest]
    include:
            - os: windows-latest
              client_name: pueue.exe
              client_release_name: pueue-windows-amd64.exe
              daemon_name: pueued.exe
              daemon_release_name: pueued-windows-amd64.exe

[Nukesor]

I just saw, that the whole include section is not longer necessary in test.yml.
If you want you can remove the include: section from test.yml while you're at it.

[Nukesor]

Thanks again for contributing!

Besides the two small code style issues, there is one more thing that needs to be done.
Pueue allows streaming of stdout/stderr, which continuously reads from the output files over here:
https://github.com/Nukesor/pueue/blob/master/daemon/streaming.rs#L57

You would need to use the command_data_to_text function over there as well.

Besides these small issues, it looks good to me. I'll merge it as soon as those issues are addressed :)

[Lej77]

I agree that the command_data_to_text functions name wasn't that great and I change it to the one you suggested instead. The switch from Cow<str> to String definitely simplifies the code a bit as well. For the streaming I used use the process_output_to_text function to convert the data to text, from what I understand that should be all that was needed.

cmd argument escaping

Earlier I also added a Escape task command correctly on windows using powershell commit since cmd has some really weird argument escaping. The current code should work but there is other ways that the issue could have been solved. The issue is that cmd expects all quotes to not be escaped (don't convert " to \") but Rust std::process::Command API doesn't provide any way to skip that conversion. So to fix that I pass the arguments to powershell instead since that expects the standard string escaping. Then I use powershell to start cmd since its API doesn't auto escape arguments.

Alternatives

One alternative could be to just use powershell for tasks instead of cmd, I choose not to do that since I think more people are used to cmd than powershell but its an option. Another alternative would be to start cmd some other way, for example via a temporary VB Script file, via a crate that provides a raw process start API or by writing the cmd command to a temporary batch file that is then started. I think the current solution is good enough but there is certainly alternatives.

Issues with starting cmd from powershell

Using powershell to start cmd does have some disadvantages:

1. It might have a performance overhead (though this should hopefully be negligible)
2. From what I have observed powershell have issues with current directory paths that contain [ or ] characters (this has caused me issues in the past). When it is started with a current directory that contains those characters its current directory will instead default to "C:\WINDOWS\System32\WindowsPowerShell\v1.0".
3. Any errors in the powershell command will write to stderr in another encoding than cmd making these errors show up as gibberish.
   • Hopefully the powershell command should never fail, then we could ignore this issue.
     • If the command only starts cmd then I can think of only two issues. Either cmd couldn't be started or the task command wasn't escaped correctly.
4. The user must have powershell on their machine. I think this is always included but I could be wrong.

powershell current directory path issue workarounds

We could work around the second issue by setting the current directory with a cmd command like cmd /U /S /C start /D "C:\Users\Henrik\Downloads\New folder [hello] (5)" /Wait /B cmd /U /S /C "INSERT_REAL_TASK" but the task command would need to be escaped in some way and it seems like some things probably couldn't be escaped correctly (such as %CD%).

Another workaround would be to use the powershell start command when starting cmd and specifying the current directory. That seems to work but the command will error out if the current directory path doesn't exist. Here is some code for generating such a powershell command:

// Set working directory explicitly for `cmd` since `powershell` will fallback to a default current directory if the path contains some characters such as `[` or `]`.
.arg(format!(
    "start -NoNewWindow -Wait -WorkingDirectory '{}' cmd /U,/S,/C,'{}'",
    task.path.replace('\'', "''").replace('[', "`[").replace(']', "`]"),
    task.command.replace('\'', "''")
));

Yet another workaround would be to detect if pwsh (PowerShell Core) is installed and if it is then use that instead of powershell to execute the command. Since pwsh doesn't have the current directory path issues this would solve the problem for users that have it installed (and offer a simple solution for people that need a workaround).

Anyway I don't think its worth bothering about, if powershell can ignore these path issues then we probably can as well.

[Nukesor]

Ok. This seems to be way more complicated than I expected.
First of all:
Is the syntax used in Powershell and cmd very different? If that's not the case, just using Powershell would probably be the easiest mode of operation.

Regarding the disadvantages:

1. Performance overhead should really be negligible
2. I guess there exists some kind of library for proper escaping for Powershell somewhere. If we want to be safe, we should include this. Anyway, I think this is something for a different PR. Let's get the basics up and running for now.
3. This could be annoying, but hopefully nothing like this should happen. If we decide to not use cmd this doesn't even become a problem.

I guess just using PowerShell is probably the easiest solution for all problems?

[Lej77]

I think it works well enough currently. Still your right that just using powershell could be easier, I just started with cmd since that seemed the go to solution. Its also slightly harder to run .bat files with powershell, you can do it with cmd /c "bat file name" or & ".\bat file name" while in cmd you can do it with "bat file name". Starting programs should be about the same though (there is slightly different syntax for strings).

From what I read about powershell strings there should hopefully not be any characters that are unescaped but powershell seems to be inconsistent with what characters need to be escaped depending on where the string is going to be used. You can see that in the example code in my previous comment I replaced more characters when the string was going to be used as a path. Those replacements should not be done for the command string.

Since the powershell command should never fail the third issue shouldn't be a problem. I could change the code to use powershell directly but then I would need to figure out how to get it to output a sensible encoding (does it do this by default?) so that would be some extra work. Still it could simplify the code slightly since the utf-16 string decoding would probably not be needed. I think it is pretty good as it is but if you want to I can change it to only use powershell. Maybe there could be a config for which shell should be used? But that is probably not something we should do in this PR, we could leave that for future improvements.

[Lej77]

I noticed that the cmd encoding didn't affect external programs so running a task like cargo --help would produce gibberish. For that reason and the ones you discussed above I switched to using powershell instead of cmd for commands.

It would be nice if we could detect if pwsh is installed and use that instead of powershell when it is to fix those pesky current directory issues but otherwise I think that this solves all the issues I brought up.

[Nukesor]

Well, let's merge this first. We will see if any problems arise.
I'll add a notice to Readme, that Windows support is still very experimental :)

[Lej77]

Sounds good! There isn't really that much windows specific code (this PR is overall quite small) so I think everything should mostly work. I'm quite happy with the powershell solution and it shouldn't really be an issue for most users. So you probably don't need to specify that it is very experimental but you could mention that it is less tested and there might be some bugs.