sheredom / subprocess.h

🐜 single header process launching solution for C and C++
The Unlicense
1.1k stars 97 forks source link

documentation unclear: subprocess_create needs full path ? #39

Closed franko closed 3 years ago

franko commented 3 years ago

Thank you for sharing this nice library.

I was using it in one of my project but I stumbled in a problem on linux about PATH resolution not done.

Code snippet is:

  const char *command_line[] = {"xrdb", "-query", NULL};
  struct subprocess_s process;
  int result = subprocess_create(command_line, 0, &process);

but it fails, it seems the full path is needed. The only way I found to make it work was to provide full path and using the inherit environment option.

Now this is confusing to me. Looking at the documentation my understanding was that the full path is not needed. I addition the documentation says absolutely nothing about that.

There is an easy way to have PATH resolution for the binary out-of-the-box ? Shouldn't the documentation says something about the PATH ?

PS: I found a related issue that lead to the PR that introduced create_subprocess_ex but I failed to understand the relation with the PATH issue I had.

sheredom commented 3 years ago

I test both fully relative and not paths in the unit tests (like here https://github.com/sheredom/subprocess.h/blob/master/test/main.c#L780). So it should work.

francesco-st commented 3 years ago

Thank you for your feedback, I appreciate.

Now that you clarified that it should work without the full path I will test again. Maybe I made some error with my previous attempts.

In the documentation you mention that the option "inherit environment" is needed for internet access so I guess it shouldn't be needed for "xrdb" but I am not very positive about that.

franko commented 3 years ago

I confirm on linux, Linux Mint 19.3, to make a call to "xrdb" I need to use the full path and subprocess_option_inherit_environment. It works only in this case.

I went further and tested the "echo" command instead of "xrdb" and it also doesn't works unless the full path is given.

Here the snippet of code I tested:

static void my_linux_call() {
  const char *command_line[] = {"xrdb", "-query", NULL};
  struct subprocess_s process;
  int result = subprocess_create(command_line, subprocess_option_inherit_environment, &process);
  if (result == 0) {
    ...
sheredom commented 3 years ago

No idea why this doesn't work tbh - I'm using execve which shouldn't require it from my reading :(

francesco-st commented 3 years ago

Hi @sheredom, I finally coded the thing myself without using subprocess.

What I found is the following: if I use something like execlp, it actually search in the PATH directory but doesn't pass the environment variables so xrdb complains about missing DISPLAY variable. I got it to work using "execve" but it only works by providing the full path. I ended up coding myself the search with the PATH directories calling execve until it succeed.

This is expected since execve is documented to require the full pathname.

If you want my two cents subprocess should hide these details and do what is needed behind the curtains. So when calling with environment you should code your own function to search into the PATH.

sheredom commented 3 years ago

I think path searching is out of scope for what I want to do with subprocess, that's a lot of extra work for the tool. If you or someone else wants to add the code I'll accept the PR, but its not something I'm going to take on my shoulders!

francesco-st commented 3 years ago

Hi, I actually already have the code in the file there for my project:

https://github.com/franko/lite-xl/blob/3b040aabc74937b7bc6f2230f7027f86d6284903/src/xrdb_parse.c

The code should be adapted but it is very simple:

From xrdb_open:

A simple helper function join_path is used to join the path string with the filename.

I may contribute a PR if you are interested.