Try to make exename portable. Refactor searchForIncludePath.

[fruffy]

Fixes #3812.

Ran into this while working on a PR. Turns out exename doesn't seem to properly work. Maybe this approach works better? Still trying to figure out the right approach for getExecutablePath.

Also use std::filesystem::path to set the include path instead of char manipulation.

This change preserves all APIs, still marking as breaking in case there are some differences in behavior.

[asl]

See comments

[asl]

Can exename be converted to std::filesystem::path while here?

[fruffy]

This would break downstream use of that function, figured I should do that in a separate PR. Ditto for all the cstring typing of variables that should be a path.

[asl]

Change to vector of std::filesystem::path?

[asl]

Maybe adopt https://github.com/gpakosz/whereami? There is also https://original.boost.org/doc/libs/1_84_0/doc/html/boost/dll/program_location_idm4410.html

[fruffy]

Figured we shouldn't depend on external dependencies for a utility like this. Particularly boost which we want to get rid of. Let me take a look at whereami

[asl]

Yes. Though there is bunch of domain knowledge here :(

[fruffy]

The Ubuntu failure is unrelated, should be fixed with #5177.

[asl]

I just realized that exename is horrible broken. In few places exename is called without any arguments. Then it could result in some bogus exename determination. What is terrible is that it caches the result. So any subsequence calls of exename(argv[0]) produces same bogus result (!).

Now let's take a look into crash.cpp. This unwanted side effect is always there as exename() is called in the early beginning when we initialize crash logger.

How it could be wrong without argv[0]? Well, one of the case is:

    } else if (getenv("_")) {
        strncpy(buffer, getenv("_"), sizeof(buffer));
        buffer[sizeof(buffer) - 1] = 0;
    } else {

This means if we'd execute p4c via some wrapper script, then it will place the full path of that wrapper script there and not p4c itself(!)

This is exactly what was in described in #3812

And the problem is that include paths could be silently dependent on the enclosing shell, etc.

[fruffy]

I just realized that exename is horrible broken. In few places exename is called without any arguments. Then it could result in some bogus exename determination. What is terrible is that it caches the result. So any subsequence calls of exename(argv[0]) produces same bogus result (!).

Now let's take a look into crash.cpp. This unwanted side effect is always there as exename() is called in the early beginning when we initialize crash logger.

How it could be wrong without argv[0]? Well, one of the case is:

    } else if (getenv("_")) {
        strncpy(buffer, getenv("_"), sizeof(buffer));
        buffer[sizeof(buffer) - 1] = 0;
    } else {

This means if we'd execute p4c via some wrapper script, then it will place the full path of that wrapper script there and not p4c itself(!)

This is exactly what was in described in #3812

And the problem is that include paths could be silently dependent on the enclosing shell, etc.

The new version should address most of these. I believe the getenv("_") use case is intentional but should only be triggered in special cases. It's tricky to write tests for this...

[asl]

I believe the getenv("_") use case is intentional but should only be triggered in special cases.

I'd remove it. Together with caching it could produce wrong results. Caching should be removed as well, this way we always stuck to incorrect answer if it happen to appear first.

[fruffy]

I believe the getenv("_") use case is intentional but should only be triggered in special cases.

I'd remove it. Together with caching it could produce wrong results. Caching should be removed as well, this way we always stuck to incorrect answer if it happen to appear first.

Some context: #5185 (comment)

At least cashing was removed in the new implementation? Unless you see caching somewhere else.

[asl]

I used to see problems with "old" version. Yes, indeed caching was removed which is good.

[vlstill]

This functions should really get a better interface overall, the output const char * argument that is heap-allocated is quite ugly. But I agree with doing it in a separate PR.

[vlstill]

nitpick: I would also explicitly separate the no-argument overload (let it call the normal one with empty path in the .cpp, but let this ugly detail be hidden).

[fruffy]

So you want two functions or require always a suggested path?

[vlstill]

I suggest to have two overloads.

std::filesystem::path getExecutablePath(const std::filesystem::path &suggestedPath);
std::filesystem::path getExecutablePath();

The reason is that it is not obvious what the empty value for suggested path means (I'd say it is not natural "none" value of paths as it is not even a valid path).

[ChrisDodd]

I believe the getenv("_") use case is intentional but should only be triggered in special cases.

I'd remove it. Together with caching it could produce wrong results. Caching should be removed as well, this way we always stuck to incorrect answer if it happen to appear first.

If nothing else works, it is better than nothing, but I agree it should not be cached -- only a result that is definitively correct (eg, comes from /proc filesystem or syscall) should be cached.