Skip to content
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

Add CWD option to subprocess_create_ex #83

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

caesay
Copy link

@caesay caesay commented Mar 6, 2024

Closes #52.

Disclaimer: I am not a posix developer. I verified the Windows code works, but did not test the UNIX stuff. I just copied this code from that issue without testing:

  // Set working directory
  if (0 != posix_spawn_file_actions_addchdir_np(&actions, process_cwd)) {
    posix_spawn_file_actions_destroy(&actions);
    return -1;
  }

I did notice that there was a posix_spawn_file_actions_addchdir_np function (from the screenshot) and a posix_spawn_file_actions_addchdir function, the latter of which is "portable" and the former is "non-portable". I don't really understand what that means in this context, however when searching I did notice that Rust uses the "non-portable" variation, so I'm guessing it's the preferred one.

@sheredom
Copy link
Owner

sheredom commented Mar 8, 2024

As far as I know (not an expert either) the _np ones are just not compliant with POSIX, but if enough systems support them you can just YOLO use them. For instance macOS has a bunch of them that aren't standard, but on macOS they effectively are!

subprocess.h Outdated Show resolved Hide resolved
subprocess.h Outdated Show resolved Hide resolved
@shakfu
Copy link

shakfu commented Mar 21, 2024

I was a getting a segfault with this PR version and I figured out how to fix it by making the following change to subprocess_create (i.e. changing SUBPROCESS_NULL to "."):

int subprocess_create(const char *const commandLine[], int options,
                      struct subprocess_s *const out_process) {
  return subprocess_create_ex(commandLine, options, SUBPROCESS_NULL,
                              // out_process, SUBPROCESS_NULL);
                              out_process, ".")

@caesay
Copy link
Author

caesay commented Mar 21, 2024

It will need to be SUBPROCESS_NULL on windows, I suspect a better fix will be to check if cwd is null, and if so, skip the call to posix_spawn_file_actions_addchdir_np. I'll make this change along with the other comments on the PR.

@caesay
Copy link
Author

caesay commented Mar 21, 2024

I have implemented the requested changes. Before doing so, I noticed some tests on ubuntu had failed with:

error: implicit declaration of function ‘posix_spawn_file_actions_addchdir_np’;

I assume this is because the function is missing, but this function is imported via the spawn.h header, so I'm guessing this function is just missing on that OS/compiler? Not sure how to resolve this.

@sheredom
Copy link
Owner

Yeah we're gonna have to find another function we can use on Linux it seems. I'll have a google about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: set the cwd for created processes
3 participants