-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement current_exe for AIX #104521
Implement current_exe for AIX #104521
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
This comment has been minimized.
This comment has been minimized.
if let Some(pstr) = path.to_str() && pstr.contains("/") { | ||
return getcwd().map(|cwd| cwd.join(path))?.canonicalize(); | ||
} | ||
// Search PATH to infer current_exe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we searching PATH and PWD on other platforms today? I'm not sure we should do that here -- it increases the amount of work done and if it's not done on other platforms is somewhat inconsistent (and could have false positives much more easily).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we searching PATH and PWD on other platforms today?
I don't know much about other rare seen platforms. On AIX, we don't have information like /proc/self/exe
in /proc
filesystem. We are using similar method in LLVM on AIX right now, see https://github.com/llvm/llvm-project/blob/42b21ddaadd3545945f29a8ccdcc89779542c30e/llvm/lib/Support/Unix/Path.inc#L251. (Actually, we don't have /proc/curproc/file
on AIX).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but you're using argv[0] just above which is what we do on other platforms (AFAICT, looking in this file, for example).
I am inclined to leave it there unless there's strong motivation for doing this cwd/path traversal. current_exe is documented as fallible for a reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extended the one for fuchsia
. Correct me if I still failed to get your point. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies if I wasn't clear. I think we should remove the PATH searching from this function, just returning the passed argument. IOW, I think if just adding aix to the fuchsia function works, then that seems like the right initial implementation here. (Not adding any other code to it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove the PATH searching from this function, just returning the passed argument.
Could you please elaborate more about your concerns of searching in PATH
here? Cuz on AIX, we don't have other methods to find the executable file of current process if argv0
is not an absolute path.
The job Click to see the possible cause of the failure (guessed by this bot)
|
Hi @Mark-Simulacrum , this PR is subsumed by #109882, could you please help review that PR? |
Added
current_exe
implementation for AIX. This implementation doesn't rely on system specific syscalls, I guess some other targets can also use it.