Fix NetBSD executable path to not use procfs #9085
Conversation
|
@faho is there some magical reason this was closed without explanation? Kind of rude, i guess you don't like attracting new contributors... |
|
I didn't close it? This just says "This pull request was closed" to me? Let me reopen, no idea what happened there. |
|
interesting, sorry about that, i don't know what is going on. |
|
It still shows that message below the last comment at the time? WTF is going on here, is github broken right now? |
|
As a different pr, I'd like to add a solution for getting the OpenBSD executable path. :) again, my bad, I've come across a lot of wild people on the internet and it kind of keeps me on edge. |
I'm getting that too, perhaps a site glitch? |
|
its finally gone woohoo |
|
Okay, so let's go back to the problem at hand. You don't want to go via procfs, because that's a foreign thing (a plan9-ism, originally, if I'm not mistaken) and might not be available? And so you use the sysctl path we also use elsewhere instead. However, this can sometimes print a path that is aesthetically not ideal. It's still a valid, absolute path to the fish executable, but includes superfluous useless components or duplicate (not triplicate) slashes? If that's the case: Honestly I don't mind. The path works for its purpose - which is to figure out where fish is. You might want to look at Tbh I'd ask the NetBSD people if this could be fixed, but as far as we're concerned it's a minor cosmetic issue that can just stay. It's certainly not something to write our own realpath over. |
Agreed, rewriting realpath is a bit overkill, if its fine as it stands, then we're on the same page. I just wasn't sure if there was a technical reason this could be problematic. It doesn't always happen, the only time it seems to be reproducible is when you run the exe from the command line using a relative path. I'll file a bug report with the NetBSD team. |
Just to be careful: Do you get a relative path from this? Like if you run Actual relative paths would be problematic, unnormalized absolute paths are okay. |
|
It wasn't a relative path, i think NetBSD under the hood returns argv[0] and if the path is relative prepend the posix working directory $PWD. That would explain why it has an absolute path but not normalized, but it's just speculation. I do something similar for my OpenBSD solution. The POSIX working directory is the working directory on application launch, meaning it doesn't change at runtime if the working directory is changed with chdir(). TL;DR yes I am sure it is absolute. |
|
You got my curiosity up, im going to run a few tests and see if i can get it to spit out a relative path in any obscure case. Although if i don't find anything within 15 minutes of getting unnormalized absolute paths i think it should be safe to assume we're good here. I'll share screenshots of my results if it helps. |
|
Very interesting... (ignore the GTK+ garbage xfce4-screenshooter output)
This is what I tested: https://github.com/time-killer-games/xproc I removed the call to realpath() xproc uses internally to make the path normalized to get the results in the screenshot. I removed the realpath call on this line: ... and made it return the variable exe1 instead of exe2. |
|
Those are still absolute paths - they don't depend on the current working directory but start from Which makes them usable to us, and I'm okay with leaving the rest up to NetBSD. Alternatively: > path normalize -- /root/../root/../root/xproc/../xproc/./xproc
/root/xproc/xprocThat seems to be our |
|
in the first case in the screenshot as you pointed out: /root/../root/../root/xproc/../xproc/./xproc ... is an absolute path and not the current working directory, because the current working directory is always normalized. in which case the underlying code is likely using the argv[0] as-is for the executable path, because it is not relative. However in the second case: ./xproc is argv[0] but it still knew where the exe was despite being relative it still resolved to /root/./xproc as you might notice /root is the part likely taken from $PWD and that path is then joined with the argv[0] which is ./xproc thus, "/root" + "/" + "./xproc" is basically the same as $PWD + "/" + argv[0] underneath. Still speculation, but I'm giving more context to what i meant. I'll add that function now. |
|
i know why this pr was closed now, i accidentally deleted my fork. i'll open a new pr with everything in squash commit |
|
Actually i take that back, this can be merged as-is. I don't know how to use wcstring, it appears to be a custom string type you wrote? That's pretty cool, but I'm ready to move on to my OpenBSD pr. Sorry for starting the CI again needlessly. |
Nah, it's just an alias for std::wstring, which is std::string using wchar_t. |
|
I think the more correct solution would be to let the NetBSD team handle this. We shouldn't have to compensate for an OS-level bug if it doesn't break your code at all. The code logic will just end up getting removed when NetBSD rolls out a fix, anyway. |
|
Alright, merged, thanks! |
get_executable_path says: "This needs to be realpath'd" So how about we do that? The only other place we use it is fish.cpp, and we realpath it there already. See #9085
Description
NetBSD can sometimes give an absolute path but with an odd period and extra slash, for example:
"/path/to/./executable"
to fix this, i would recommend using realpath, or writing your own realpath implementation based on the one NetBSD uses in its source code, but without the PATH_MAX limitation. Let me know what your thoughts are on this.