Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows command files support #95

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

vsrs
Copy link

@vsrs vsrs commented Jul 19, 2024

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
Copy link
Owner

matklad commented Jul 23, 2024

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
Copy link
Author

vsrs commented Jul 23, 2024

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
Copy link
Owner

matklad commented Jul 23, 2024

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
Copy link
Author

vsrs commented Jul 23, 2024

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
Copy link
Author

vsrs commented Jul 23, 2024

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
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