Skip to content

Windows command files support#95

Open
vsrs wants to merge 2 commits into
matklad:masterfrom
vsrs:win_cmd_files
Open

Windows command files support#95
vsrs wants to merge 2 commits into
matklad:masterfrom
vsrs:win_cmd_files

Conversation

@vsrs

@vsrs vsrs commented Jul 19, 2024

Copy link
Copy Markdown

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

@vsrs vsrs force-pushed the win_cmd_files branch 2 times, most recently from 6d3dae9 to 79faeaf Compare July 19, 2024 19:34
@matklad

matklad commented Jul 23, 2024

Copy link
Copy Markdown
Owner

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

vsrs commented Jul 23, 2024

Copy link
Copy Markdown
Author

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

matklad commented Jul 23, 2024

Copy link
Copy Markdown
Owner

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

vsrs commented Jul 23, 2024

Copy link
Copy Markdown
Author

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 vsrs marked this pull request as draft July 23, 2024 15:01
@vsrs vsrs force-pushed the win_cmd_files branch 2 times, most recently from 6e0a5a2 to 59d9122 Compare July 23, 2024 16:27
@vsrs

vsrs commented Jul 23, 2024

Copy link
Copy Markdown
Author

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.

@vsrs vsrs marked this pull request as ready for review July 23, 2024 16:37
Find .cmd or .bat in paths.
@vsrs

vsrs commented Feb 13, 2025

Copy link
Copy Markdown
Author

@matklad, I've rebased the PR on the latest master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failure on Windows when running npm ...

2 participants