Closed SpartanJ closed 2 years ago
Any chance you've got a test case for this?
Sure, no problem:
#include <string>
#include "subprocess.h"
int main() {
const char *command_line[] = {"cat", "--version", NULL};
struct subprocess_s subprocess;
int ret = EXIT_SUCCESS;
int result = subprocess_create( command_line,
subprocess_option_inherit_environment,
&subprocess );
if ( 0 == result ) {
std::string buffer( 1024, '\0' );
std::string data;
unsigned bytesRead = 0;
do {
bytesRead = subprocess_read_stdout( &subprocess, &buffer[0], buffer.size() );
data += buffer.substr( 0, bytesRead );
} while ( bytesRead != 0 );
subprocess_join( &subprocess, &ret );
subprocess_destroy( &subprocess );
printf("%s", data.c_str());
}
return ret;
}
See the command_line, I'm not passing the binary path, only the file name. That will not work with execv
(line 767 from the latest commit). That was working before because it was using execvp
. It will only fail when using the subprocess_option_inherit_environment
flag, otherwise, it will use execve
that also expects a file name or path.
Had the same issue and also solved it by calling execvp
. Just wanted to add a small note that for the variants that specify the environment I don't think that execve
will search in PATH, only execvpe
would, but it's a GNU extension. The easiest portable way seems to set environ and always call execvp, as proposed here https://stackoverflow.com/questions/67633176/portable-alternative-to-execvpe .
This would make the behavior consistent with the behavior of CreateProcessA on Windows, which if I understood correctly will search in the %PATH% variable if the first lpApplicationName parameter is null as it's currently the case.
Anyway, pushed a commit doing that environ
change here: https://github.com/nburrus/subprocess.h/commit/64bc953376a0a51f114d9d84db82606f20552aad . I can do a PR if you prefer, but I only briefly tested on Linux and macOS.
If you had some way to do that PR with a test case that would fail without the change I'd be very grateful! Even if you can't make a failing test, please do the PR 😄
Sure, just made a PR :) I actually ended up adding an explicit option because it would otherwise break the no slash unit tests, since execvp will not search for a program name as a relative path anymore, but only in the PATH folders.
Note that on Windows that option is not required as it will always search for both. I think it could made more consistent by specifying lpApplicationName to CreateProcessA when subprocess_option_search_user_path is unset. This would disable the user path search. I don't have a Windows setup to test right now though, but didn't try to address that.
Thanks to @nburrus subprocess_option_search_user_path
enables this behaviour.
Hi! I recently updated the library with the latest version and it wasn't working for me. I analyzed the changes of the new version and found out that the call that was using
execvp
now is replaced forexecv
(insubprocess_create_ex
). The problem is that breaks the behavior sinceexecv
expects a path (the path to the binary file) andexecvp
expects a file (a binary file name or a path, if you provide the binary file name it will search in PATH the binary name provided). It's also inconsistent with the other exec calls sinceexecve
is being used (which expects also a file). For the moment it just made that change locally but I just wanted to let you know this. Thanks for the library.