`Process.find_executable` considers all files as executable on Windows
straight-shoota opened this issue · 4 comments
On Windows files have no executable status bit like on Unix platforms. The current implementation of Process.file_executable? (private API) always returns true on Windows. But this does not appear very useful.
For example, git considers a file executable if its name ends with .exe or the content starts with #! a she-bang. It apparently handles she-bang scripts in a custom way on Windows.
https://github.com/git/git/blob/23d289d273d861f0a6244480e89ff937f66efa77/run-command.c#L131-L171
Batch files (.bat, .cmd) are not intended to be directly executable without explicitly spawning a shell (#14536).
I'm not sure what's about other extensions for commonly executable files on Windows such as .com, .msi, .vbs or .ps1 (and basically anything else in %PATHEXT%?).
There's also the Win32 API function GetBinaryTypeW which can tell whether a file is executable.
Related:
Checking the file extension is the most appropriate method. Shebangs don't work on Windows and, more importantly, they are in no way an indicator that the file is executable.
PATHEXT should not be used as a reference for "executable files" as that is subject to interpretation (literally). You can see what actually counts as "executable files" by running ASSOC in CMD, which clearly does not match up with PATHEXT. Really, these values represent what can be executed and not what is executable.
Could you give a practical example of a case where the behavior of Process.find_executable doesn't match your ideal expectation?
The reason that I ask this is: there is a factor that proves that the behavior of the function is objectively correct.
find_executable(foo) answers the question: if I run Process.run(foo), which file will that end up running?
And I wrote a test script that verifies this for all cases
So we can't argue that the function does something wrong, assuming that we define it in this way.
But if we find significant practical reasons that this function could be a lot more useful if it didn't follow this definition, then we can actually argue something
I wasn't aware of the test script, thanks for pointing that out 🙇
Seems like it sufficiently ensures congruency with Process.run so everything should be fine here👍