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

Unix: add open_process_args{,_in,_out,_full} #1792

Merged
merged 5 commits into from May 24, 2018

Conversation

Projects
None yet
3 participants
@nojb
Copy link
Contributor

commented May 21, 2018

See MPR#7794.

  • The idea is to add a variant of the Unix.open_process{,_in,_out,_full} functions that work with a pair (program, args) directly, without going through the system shell, which introduces extra complexity (in particular, with respect to quoting).

  • I used the name Unix.open_process_args which is the one suggested in ocaml-batteries-team/batteries-included#858, but I am not completely sold on it.

  • One uses the usual "close" functions Unix.close_process{,_in,_out,_full} with the new functions as well.

  • The "old" functions Unix.open_process{,_in,_out,_full} are re-implemented in terms of the "new" functions, so I did not feel pressed to add new tests.

@nojb nojb force-pushed the nojb:open_process_args branch from f7b9f4c to 39200c1 May 22, 2018

@diml

This comment has been minimized.

Copy link
Member

commented May 22, 2018

The implementation looks correct to me. I don't have better names to suggest though.

When I implemented similar functions in Lwt, I used the following API to avoid duplication:

type command = string * string array
val shell : string -> command
val open_process_in : command -> ...

But I guess it's too big of a change for the Unix module.

@diml

diml approved these changes May 22, 2018

nojb added some commits May 22, 2018

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented May 23, 2018

Looks good to me. I'm giving it a round of "CI precheck" to re-test all configs.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented May 24, 2018

CI is positive. Merging.

@xavierleroy xavierleroy merged commit 76f30d0 into ocaml:trunk May 24, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nojb

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2018

Thanks for the quick review!

@nojb nojb deleted the nojb:open_process_args branch May 24, 2018

@nojb nojb referenced this pull request Jun 3, 2018

Closed

factor out Unix.shell #1813

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.