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

Allow setting the cwd when spawning a process #694

Merged
merged 2 commits into from Aug 3, 2019

Conversation

talex5
Copy link
Contributor

@talex5 talex5 commented Jun 13, 2019

Fixes #163.

This patch adds a new ?cwd argument after every ?env argument in Lwt_process.

Note: Windows changes have not been tested. Someone with access to a Windows machine should try it :-)

Note: Windows changes have not been tested.
try Sys.chdir dir
with ex ->
let err = Printf.sprintf "chdir(%S) failed: %s\n" dir (Printexc.to_string ex) in
let _ : int = Unix.write_substring Unix.stderr err 0 (String.length err) in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think Lwt should have the child process write to STDERR. I suggest to let the surrounding try handler handle this error the same as it handles the rest of the potential errors in this part of the code. If that's not sufficient, we can improve all the error handling together in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having Lwt do the chdir is an alternative for processes that don't support the -C option. Processes that are able to chdir themselves usually respond in this way (writing a message to stderr and exiting). e.g.

~ $ make -C /missing >/dev/null
make: *** /missing: No such file or directory.  Stop.
~ $ git -C /missing show  >/dev/null
fatal: cannot change to '/missing': No such file or directory

Without this, it is very difficult to debug errors (e.g. permission denied) because it will be reported instead as the process failing to start.

If the parent process is behaving correctly then it won't be passing an invalid directory here and so this won't make any difference. If it is passing an invalid directory, then getting a message about that is surely an improvement?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the existing error handling in child processes is bad. What I am saying is that I prefer not to:

  1. Special-case it for chdir. For comparison, if the parent process passed a bad program path, or the child process can't be started due to a similar error (e.g. another permission denied), we don't print to STDERR in the surrounding code.

  2. Commit to printing to STDERR from a library (Lwt). In particular, we shouldn't choose "for make" and "for git" which message to print, whether to print a message (or respond otherwise), etc. make and git aren't even printing from a fork here, but if they were, and were using Lwt, they would have no control over what is shown (or whether it is shown).

We need a better solution for all these errors, not just chdir.

I'm just proposing to push that to a later PR, so we can do it as necessary, and not conflate chdir with fixing the error handling. Unless you'd like to do that, of course.

@aantron aantron merged commit 357414d into ocsigen:master Aug 3, 2019
@aantron
Copy link
Collaborator

aantron commented Aug 3, 2019

@talex5, I merged this with the error message removed.

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.

Lwt_process.exec should support a chdir option
2 participants