-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
bpo-35674: Expose posix_spawnp #11554
Conversation
Lib/test/test_posix.py
Outdated
if pid2 != pid: | ||
raise Exception(f"pid {pid2} != {pid}") | ||
if status != 0: | ||
raise Exception(f"status = {status}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe write: f"status {status} != 0"
Misc/NEWS.d/next/Library/2019-01-14-14-13-08.bpo-35674.kamWqz.rst
Outdated
Show resolved
Hide resolved
Modules/posixmodule.c
Outdated
@@ -5489,8 +5461,15 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, | |||
attrp = &attr; | |||
|
|||
_Py_BEGIN_SUPPRESS_IPH | |||
err_code = posix_spawn(&pid, path->narrow, | |||
if (whichspawn == 0){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I suggest use_posix_spawnp, I also suggest:
if (use_posix_spawnp) {
... posix_spawnp ...
}
else {
... posix_spawn ...
}
Modules/posixmodule.c
Outdated
[clinic start generated code]*/ | ||
|
||
|
||
#ifdef HAVE_POSIX_SPAWNP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The #ifdef must be outside [clinic start generated code].
I propose to reuse the existring #ifdef HAVE_POSIX_SPAWN, so don't add a new #ifdef HAVE_POSIX_SPAWN but just move the closing #endif /* HAVE_POSIX_SPAWN */ to include your new code.
Note: Currently, "make clinic" fails. Make sure that "make clinic" works as expected.
I bet that posix_spawnp() is always available when posix_spawn() is available. If it's not the case, we can fix that later.
That's because of the #ifdef issue, see my comment. |
Modules/posixmodule.c
Outdated
PyObject *env, PyObject *file_actions, | ||
PyObject *setpgroup, int resetids, PyObject *setsigmask, | ||
PyObject *setsigdef, PyObject *scheduler) | ||
PyObject *setsigdef, PyObject *scheduler, int which) | ||
/*[clinic end generated code: output=45dfa4c515d09f2c input=2891c2f1d457e39b]*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line needs to be deleted to avoid confusing the argument clinic
Very good job Joannah! :) I left some comments together with Victor's review. Ping me when you have addressed them. |
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
87d6fed
to
e2a9603
Compare
@@ -0,0 +1,2 @@ | |||
:func:os.posix_spawnp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, you need to write a sentence. Something like:
:func:os.posix_spawnp | |
Add a new :func:`os.posix_spawnp` function. Patch by Joannah Nanjekye. |
e2a9603
to
fddc45e
Compare
* Fix also indentation in posixmodule.c. * Remove "if the *path* argument contains no directory" from os.posix_spawnp() doc * Remove outdated comment from py_posix_spawn(): /*[clinic end generated code: ...]*/
Serhiy's comments have been addressed.
I checked Travis CI jobs. posix_spawnp is available on Linux and macOS jobs, but glibc is too old on the Linux for subprocess. Linux:
macOS:
|
Well done @nanjekyejoannah :-) Thanks to Joannah for this nice enhancement and to all reviewers. |
I have exposed posix_spawnp.
Co-Author: Victor Stinner vstinner@redhat.com
https://bugs.python.org/issue35674