Windows compatibility

[ttencate]

Godot's process running API does not support changing the working directory, which we have to do before invoking cargo build.

Because cargo -C is still unstable, the addon runs a shell to change directory before executing the build. This shell is currently hardcoded to be /bin/sh, which does not work on Windows.

I don't have Windows anymore, but support for cmd.exe should be easy to add for someone actually able to test this.

[sylbeth]

As I explained on Discord, it can be done with powershell.exe -Command or cmd.exe /c. We could add a configuration to change between them, and I can check myself whether they work or not. Would you like for me to add a pr on this? I already started, the plan was to add two variables:

# The shell command to be used depending on the OS.
var shell: String
# The first argument to provide the shell command.
var argument: String

And then to change their values in the match before running the command, as the command is the same. I would also suggest we add a warn message to both MacOS and Android saying that the support is experimental and should be tested, and to not support iOS, as I believe it's not possible, and there is no Godot Editor for iOS-

[ttencate]

I think cmd.exe should be sufficient. It's always available. No need to bother with a configuration.

The question is, what command do we feed to the shell? In the Unix sh shell, cd some/dir && /path/to/cargo build works, but what's the equivalent syntax for cmd.exe? Or does && also work there?

Edit: OK, I've got WINE, it's got cmd.exe, and the && syntax also works there. So this should be very easy to do!

(It gets trickier if we ever need to escape special characters like spaces in the arguments. But right now it's just build, --color=always, --profile=dev, which need no escaping.)

[ttencate]

Yeah, let's officially not even try on Android and iOS right now. Almost nobody runs Rust compilers there anyway.

macOS is "just Unix" so it should work, right?

[sylbeth]

Android should work if jailbroken, and it should simply be bin/sh. MacOS should work if the app is open in a terminal but not if it's a standalone GUI app. As for iOS, I don't even think it's possible.

[sylbeth]

I think it would be better for RustToolsSubprocess to access the configs directly. cargo.gd makes an indirection that just makes it harder to maintain if more configurations are added. The configs could be accessed on init (as they are now) or on config change and keep RustToolsSubprocess as a singleton.

[ttencate]

I think RustToolsSubprocess is a sensible abstraction: a command runner that papers over the limitations of the built-in OS.execute and OS.execute_with_pipe. It can't be a singleton because it sends a signal upon process completion, which we can await in the caller – so each instance needs to represent a separate process. It was inspired by Python's subprocess module. I don't see a need to conflate it with a runner for just cargo commands.

Arguably the builder and the representation of the running process should be two separate classes, similar to Rust's Command vs. Child, but since it's only used in one place right now, this is not causing any problems.

That said, cargo.gd can absolutely use some cleanup. It contains too much code duplication and functions that do more than one thing. But it's easy to do independently of this issue of Windows compatibility, right?

[ttencate]

Did that refactoring in 1acabb3, so cargo.gd is now more decoupled and more easily extensible.

[sylbeth]

I think RustToolsSubprocess is a sensible abstraction.

I agree but then you'd have to maintain the config accessing outside and inside it (to get them from config, and to give them to the object)

It can't be a singleton because it sends a signal upon process completion, which we can await in the caller

That could be solved by giving each subprocess a unique id, but yes, no need

I don't see a need to conflate it with a runner for just cargo commands.

That's a fair argument

this is not causing any problems.

I was facing that when adding one setting (whether to use cmd or powershell), I had to add it everywhere, and either add more init parameters to init or add a new method.

That said, cargo.gd can absolutely use some cleanup.

Yeah, that's a good concession, it's easier to update.

But it's easy to do independently of this issue of Windows compatibility, right?

It is, but it happened to come up here.

Did that refactoring in 1acabb3, so cargo.gd is now more decoupled and more easily extensible.

Yeah, decoupling is nice, that solves the too many calls to the same methods.

Also, I'm working on this, but want to do more thorough testing than not pr when I'm not confident all works as intended.

[ttencate]

I was facing that when adding one setting (whether to use cmd or powershell), I had to add it everywhere, and either add more init parameters to init or add a new method.

Why would we want to use Powershell though?

[sylbeth]

In case someone prefers not using cmd. Although given that is not uninstallable and that it's just used for cd to place, maybe it's just best only to use CMD?

[ttencate]

That's what I think, yes, just cmd.exe should be fine. Adding options "just in case", without a compelling use case, only makes the code harder to maintain and the plugin harder to use.

[sylbeth]

Made it work, but for compatibility, we have to make one of the following choices, since enclosing in single quotes is apparently not valid in cmd.exe.

• We enclose it in double quotes only for Windows, which may break some things.
• We use powershell instead, breaking compatibility with earlier Windows
• We enclose all systems in double quotes.

For now, I've enclosed only Windows in double quotes and run a check for non escaped double quotes.

[ttencate]

Sounds good to me. If anyone is actually crazy enough to use quotes in their file names, we can always add code to escape them properly instead of erroring out.