[llvm-commits] [llvm] r117596 - /llvm/trunk/lib/System/Unix/Program.inc

[Dan Gohman]

On Oct 29, 2010, at 3:24 AM, Mikhail Glushenkov wrote:

> Hi,
> 
> On Thu, Oct 28, 2010 at 05:28:12PM -0700, Dan Gohman wrote:
>> 
>>> 
>>> Thanks, I've missed this. What about changing
>>> 
>>> if (progName.find('/') != std::string::npos && temp.canExecute())
>>>   return temp;
>>> 
>>> to
>>> 
>>> if (progName.find('/') != std::string::npos) {
>>>   if (temp.canExecute())
>>>     return temp;
>>>   else
>>>     return Path();
>>> }
>>> 
>>> This would be consistent with the Win32 version.
>> 
>> This would prevent clients from being able to distinguish
>> between "not found" and "not executable".
> 
> Do they really need to? I think that those two cases are synonymous for the
> purposes of FindProgramByName - "an executable called X was not found".

It is nice to be a little more specific here.

> Besides, if clients want to distinguish between executable and non-executable
> absolute paths they can use functions from the Path class. It is not possible to
> do this *from client code* by just using FindProgramByName, because for absolute
> paths it is the identity operation.

The idea is that if FindProgramByName returns a non-empty string, the client
will just execute it, and check for errors (which it's expected to do anyway).

> If FindProgramByName is given a path without slashes, it doesn't return a
> non-executable path even if it is found in the PATH. Why should the behaviour
> for absolute paths be different?

The convention is that there are two different kinds of queries that can be made.

If the requested path contains a slash, it's a request for a specific path,
regardless of whether it appears executable or not.  If it's not executable
or not present, executing it will fail with an appropriate error.

If the requested path does not contain a slash, it's a request to "go find
a program with this name".  I don't know all the reasons why this checks
for executability, but the main point is that the request isn't for a specific
path, so it's less important.  If the user cares which specific file is being
found, they can examine the contents of their PATH.

> 
> If we just care about the error message shown to the user, why not add an
> optional output parameter ErrorMessage to FindProgramByName/FindExecutable?

We don't need to; the OS does this for us :-).  (Or, it now does so with our
help, in the case of posix_spawn systems.)

>>> Alternatively, we can change FindExecutable to take into account that canExecute
>>> can be false for the path returned by FindProgramByName.
>> 
>> 
>> My assumption is that it's perfectly acceptable for FindExecutable
>> to return a non-executable path here; its caller will attempt to
>> execute the path, get an error, and report the error to the user,
> 
> I think it's not. Currently, FindExecutable always succeeds (on unix-likes),
> even if given a non-absolute path. FindExecutable("something") =
> Path("$EXE_DIR").appendComponent("something"), which was not the original
> intent.

The code to call FindProgramByName there was only added as a
hack for Windows to add a ".exe" suffix.  FindExecutable ideally should
always succeed, because it is primarily just a path manipulation.

> 
>> What problem are you trying to solve here?
> 
> I find the current behaviour confusing and I want it to be consistent across
> platforms. If we decide to standardise on the Unix behaviour, then we should
> also make the Win32 version of FindProgramByName return absolute paths
> untouched, and mention this issue in the comments.


Currently my understanding is that it modifies absolute paths by appending
a ".exe" suffix.  If there's another way to achieve this, that would probably be
better.

Make