rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.03k stars 12.68k forks source link

Command::spawn has weird rules for finding binaries on Windows #37519

Open ollie27 opened 7 years ago

ollie27 commented 7 years ago

The offending code is here which is apparently to have it search the child's %Path% for the binary to fix #15149. It has a few issues though.

  1. It is only triggered when the child's environment has been customised. This means Command::new("foo.exe").spawn() and Command::new("foo.exe").env("Thing", "").spawn() uses different rules for finding the binary which seems unexpected.
  2. It will replace any extension on the program name with .exe. This means Command::new("foo.bar").env("Thing", "").spawn() attempts to launch foo.exe first. Command::new("foo.bar").spawn() correctly tries to launch foo.bar as CreateProcess will not replace the extension.
  3. If the program name is an absolute path then this is still triggered but will just try that path for every item in the child's %Path%. For example Command::new(r"C:\foo.bar").env("Thing", "").spawn() looks for C:\foo.exe several times.
  4. The use of .to_str().unwrap() means it panic!s if the program name is not valid Unicode.
  5. Prehaps the biggest issue, but maybe it's itentional, is that none of the logic for finding binaries seems to be documented (std::process::Command).

An easy way to fix this is to just remove the hack so we just rely on the behaviour of CreateProcess on Windows which is a least documented. The behaviour is already very different between Windows and Linux and we should probably just accept that in order to get reliable results cross-platform it's best to use absolute paths.

retep998 commented 7 years ago

Personally I'd much rather we didn't have this hack and just accepted that Windows and Unix are different and document their differences.

alexcrichton commented 7 years ago

@ollie27 these all sound like bugs to me rather than weird behavior. Removing this behavior is not an option as the are plenty of programs currently relying on this behavior. Would be great to fix the issues, however!

luser commented 6 years ago

It would definitely be nicer if Command didn't try to be too smart here, but breaking existing usage would also suck. For sccache I contributed to the which crate, which will append an exe extension on Windows if necessary (but not replace an existing extension). One thing that the which crate doesn't implement is support for using the extensions from the PATHEXT environment variable, which on my Windows 10 machine does include .COM. (The Python which package does support this.)

jokeyrhyme commented 6 years ago

Another issue is that .spawn() assumes that ".exe" is the only extension that Windows' executables have: https://github.com/rust-lang/rust/blob/1.24.0/src/libstd/sys/windows/process.rs#L154 This ignores ".cmd", ".com", etc

cuviper commented 6 years ago

Another issue is that .spawn() assumes that ".exe" is the only extension that Windows' executables have

I just found in https://github.com/rust-lang/rust/issues/50870#issuecomment-390309938 that it's consistent with CreateProcess to only try ".exe".

BasixKOR commented 4 years ago

I just found a documentation about how file extensions work in Windows, and it might help since CMD finds the application to open a file with %PATHEXT% using this information (I found this on Stack Overflow answer, but it may be wrong.).

anp commented 4 years ago

One thing that the which crate doesn't implement is support for using the extensions from the PATHEXT environment variable, which on my Windows 10 machine does include .COM.

By the way @luser, after reading through this thread I found that in the interim the which crate has gained support for other extensions through PATHEXT.

ChrisDenton commented 2 years ago

The particular code in question has been replaced as a side effect of #87704. Most notably the behaviour is now much more consistent.

So I think this can be closed.

kryptan commented 1 year ago

Command::spawn cannot launch *.cmd file located in PATH if you don't specify extension.

E.g. launching VSCode doesn't work:

Command::new("code").spawn().unwrap(); // Error { kind: NotFound, message: "program not found" }

CMD and PowerShell launch code without extension just fine. I would expect Rust's Command behaviour to match CMD and PowerShell.

In Rust you need to specify extension Command::new("code.cmd") or launch with CMD Command::new("cmd").arg("/C").arg("code").

blaenk commented 1 year ago

If some of you weren't aware, it might help to know that the which crate has logic for handling this without requiring you to append .cmd, .com, .bat, .exe, etc. This is based on the PATHEXT environment variable on Windows, which you don't have to set it yourself.

See https://github.com/harryfei/which-rs/blob/e5ec71143965a7c7751c853fb229cb973bfcaa13/src/finder.rs#L158-L162

Hope that helps others like it helped me!

ChrisDenton commented 1 year ago

This thread was about buggy behaviour that has since been fixed (aside from improving the documentation).

The fact that Command is a thin wrapper around CreateProcessW is intentional. If you would like more shell-like behaviour then using a crate is definitely the preferred method.

I opened #94743 to discuss running scripts, etc, from Command but there was a distinct lack of enthusiasm for it. I'd suggest moving conversation there if there's a strong argument for it. Or open an API Change Proposal (ACP) on the library team's repo.

tisonkun commented 7 months ago

Perhaps helpful - https://github.com/cli/safeexec

sourcefrog commented 7 months ago

This thread was about buggy behaviour that has since been fixed (aside from improving the documentation).

The fact that Command is a thin wrapper around CreateProcessW is intentional. If you would like more shell-like behaviour then using a crate is definitely the preferred method.

I opened #94743 to discuss running scripts, etc, from Command but there was a distinct lack of enthusiasm for it. I'd suggest moving conversation there if there's a strong argument for it. Or open an API Change Proposal (ACP) on the library team's repo.

Adding more rustdocs to clarify this sounds like a good idea. Maybe someone who regularly works with this on Windows can put up a PR?