Only absolutize exe if path is a file and executable

[avdv]

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]

Sorry this took me so long to get to. Approach generally looks correct

[JakobDegen]

This looks like a bit of a race condition to me. Can you add fs_util::metadata_if_exists instead and then use that?

[avdv]

Good catch! Done.

[facebook-github-bot]

@JakobDegen has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[JakobDegen]

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?

[avdv]

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 37f3d25 was described like this:

If you do Command::new("a").current_dir("b") with your cwd being c, on UNIX you run c/b/a, on Windows you run c/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...

[JakobDegen]

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