facebook
Only absolutize exe if path is a file and executable
Hi.
I took a stab at fixing issue #670 where a path to an exe is constructed which might refer to a directory, so trying to spawn a process using that path fails with an obscure error.
Fixes #670
@JakobDegen has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
I think the fix is incorrect. I mean, it fixes the problem reported by a user, but instead this should should just avoid "absolutizing" if given path does not contain slashes (so it is searched in $PATH).
Current fix has the same problem as before: if there's nix executable file in the current directory, it will be executed instead of nix in $PATH.
-- @stepancheg
This seems like a reasonable alternative to me, thoughts?
This seems like a reasonable alternative to me, thoughts?
Yes, maybe it's a good idea to take a step back. The problem resolved by https://github.com/facebook/buck2/commit/37f3d258887d0c4565a23eafb2520057d923c6ac was described like this:
If you do
Command::new("a").current_dir("b")with your cwd beingc, on UNIX you runc/b/a, on Windows you runc/a.
As noted by @stepancheg, that is not really true, since on Unix, you run "a" found in the PATH , the binary relative to c/b is not picked up (except if . is in the PATH).
On Windows, however, a binary is first picked up from the current directory, before doing a PATH lookup, AFAIK.
Since this is solving a problem specific to Windows, the fix should actually only be applied on Windows. How about never absolutize on unix, but check if the executable exists in the spawned processes' cwd and return the absolute path for that on Windows.
I am just wondering though, you would have to include the file extension in the command currently, otherwise this fails right? I mean, Windows looks for various file extensions it denotes as executable (like .exe, .com, .cmd, .bat et cetera) and this would fall short if you mean to run "c/b/nix.exe" but only specify nix as the command...
How about never absolutize on unix
This part seems right to me
but check if the executable exists in the spawned processes' cwd and return the absolute path for that on Windows
This depends - does window generally behave as if . is a part of the path? If not, we should consider restricting this behavior to the case where the path has at least one /
I am just wondering though, you would have to include the file extension in the command currently, otherwise this fails right? I mean, Windows looks for various file extensions it denotes as executable (like .exe, .com, .cmd, .bat et cetera) and this would fall short if you mean to run "c/b/nix.exe" but only specify nix as the command
This seems correct. It's possible that this means we have to re-implement a significant part of Window's command resolution.
I guess it would probably be a good start if someone could properly document what it even is that Windows does here, and then we can go from there