matklad / xshell

Apache License 2.0
696 stars 30 forks source link

Windows command files support #95

Open vsrs opened 4 months ago

vsrs commented 4 months ago

Fixes #82,

BTW, there are some test failures on Windows (at least on my machine):

failures:
    env::test_env_clear
    ignore_status_no_such_command
    unknown_command

this MR does not fix them

matklad commented 3 months ago

I don't think we should use cmd for spawning any command, only the bat scripts. So, what needs to happen here I think is a lookup in path, and, if that returns a bat, spawning a cmd.

vsrs commented 3 months ago

There are two problems:

1) It is quite difficult to determine that a command is a bat file. For example, a typical node\npm setup:

    >where npm
    C:\Program Files\nodejs\npm
    C:\Program Files\nodejs\npm.cmd
    C:\Users\Vit\AppData\Roaming\npm\npm
    C:\Users\Vit\AppData\Roaming\npm\npm.cmd
That is, there are 4 options for what "npm" could be in the PATH. Which one should be used?

2) Windows internal commands like echo. There is no echo.exe or similar; the command is implemented directly in the command interpreter (cmd), so cmd!(sh, "echo text").run()?; fails as well at the moment.

That is why I think it's better to always use cmd on Windows.

matklad commented 3 months ago

It is quite difficult to determine that a command is a bat file. For example, a typical node\npm setup:

We should repeat the exact logic that rust stdlib does here

Windows internal commands like echo

This shouldn't work, like something like cd won't work on Linux. cmd! is an interface for runing programs, not for talking to your shell.

vsrs commented 3 months ago

or, as an alternative, we can invert the logic: look up the command, if it is an exe, run it, otherwise use cmd /c.

vsrs commented 3 months ago

It turned out that the rust stdlib can already run command files correctly via cmd /c, but you must specify the extension. Therefore, I just search for suitable files using the same rules that the stdlib uses to search for executables.