bpo-35674: Expose posix_spawnp #11554

nanjekyejoannah · 2019-01-14T14:10:25Z

I have exposed posix_spawnp.

Co-Author: Victor Stinner vstinner@redhat.com

https://bugs.python.org/issue35674

Doc/library/os.rst

Lib/test/test_posix.py

vstinner · 2019-01-14T14:43:40Z

Lib/test/test_posix.py

+            if pid2 != pid:
+                raise Exception(f"pid {pid2} != {pid}")
+            if status != 0:
+                raise Exception(f"status = {status}")


maybe write: f"status {status} != 0"

Misc/NEWS.d/next/Library/2019-01-14-14-13-08.bpo-35674.kamWqz.rst

Modules/posixmodule.c

vstinner · 2019-01-14T14:47:52Z

Modules/posixmodule.c

@@ -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){


Since I suggest use_posix_spawnp, I also suggest:

if (use_posix_spawnp) { ... posix_spawnp ... } else { ... posix_spawn ... }

Lib/test/test_posix.py

vstinner · 2019-01-14T15:21:43Z

Modules/posixmodule.c

+[clinic start generated code]*/
+
+
+#ifdef HAVE_POSIX_SPAWNP


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.

vstinner · 2019-01-14T15:22:14Z

AppVeyor build failed

That's because of the #ifdef issue, see my comment.

pablogsal · 2019-01-14T15:27:46Z

Modules/posixmodule.c

                    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]*/


I think this line needs to be deleted to avoid confusing the argument clinic

Modules/posixmodule.c

pablogsal · 2019-01-14T16:22:49Z

Very good job Joannah! :)

I left some comments together with Victor's review. Ping me when you have addressed them.

Modules/posixmodule.c

Doc/library/os.rst

Lib/test/test_posix.py

bedevere-bot · 2019-01-14T16:50:54Z

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner · 2019-01-15T22:11:27Z

Misc/NEWS.d/next/Library/2019-01-14-14-13-08.bpo-35674.kamWqz.rst

@@ -0,0 +1,2 @@
+:func:os.posix_spawnp


Hum, you need to write a sentence. Something like:

Suggested change

:func:os.posix_spawnp

Add a new :func:`os.posix_spawnp` function. Patch by Joannah Nanjekye.

Modules/posixmodule.c

* 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.

vstinner · 2019-01-16T13:26:59Z

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:

# configure
checking for posix_spawn... yes
checking for posix_spawnp... yes

# pythoninfo
platform.libc_ver: glibc 2.19
subprocess._USE_POSIX_SPAWN: False

macOS:

# configure
checking for posix_spawn... yes
checking for posix_spawnp... yes

# pythoninfo
platform.platform: macOS-10.13.3-x86_64-i386-64bit
subprocess._USE_POSIX_SPAWN: True

vstinner · 2019-01-16T14:33:23Z

Well done @nanjekyejoannah :-) Thanks to Joannah for this nice enhancement and to all reviewers.

the-knights-who-say-ni added the CLA signed label Jan 14, 2019

bedevere-bot added the awaiting review label Jan 14, 2019

pablogsal self-assigned this Jan 14, 2019

pablogsal self-requested a review January 14, 2019 14:23

vstinner reviewed Jan 14, 2019

View reviewed changes

vstinner mentioned this pull request Jan 14, 2019

bpo-35674: Expose posix_spawnp #11545

Closed

pablogsal reviewed Jan 14, 2019

View reviewed changes

Lib/test/test_posix.py Show resolved Hide resolved

vstinner reviewed Jan 14, 2019

View reviewed changes

pablogsal reviewed Jan 14, 2019

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

serhiy-storchaka previously requested changes Jan 14, 2019

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

Modules/posixmodule.c Outdated Show resolved Hide resolved

Doc/library/os.rst Show resolved Hide resolved

Lib/test/test_posix.py Show resolved Hide resolved

bedevere-bot removed the awaiting review label Jan 14, 2019

bedevere-bot added the awaiting changes label Jan 14, 2019

nanjekyejoannah force-pushed the spawnp branch 3 times, most recently from 87d6fed to e2a9603 Compare January 15, 2019 21:51

vstinner requested changes Jan 15, 2019

View reviewed changes

Expose posix_spawnp

fddc45e

nanjekyejoannah force-pushed the spawnp branch from e2a9603 to fddc45e Compare January 16, 2019 08:13

deleted unwanted patch file and uncomment test

37902ca

vstinner reviewed Jan 16, 2019

View reviewed changes

Modules/posixmodule.c Show resolved Hide resolved

vstinner reviewed Jan 16, 2019

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

Fix py_posix_spawn() if HAVE_POSIX_SPAWNP is not defined

1ad7ad8

* 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: ...]*/

vstinner approved these changes Jan 16, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Jan 16, 2019

vstinner merged commit 92b8322 into python:master Jan 16, 2019

bedevere-bot removed the awaiting merge label Jan 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-35674: Expose posix_spawnp #11554

bpo-35674: Expose posix_spawnp #11554

nanjekyejoannah commented Jan 14, 2019 •

edited

Loading

vstinner Jan 14, 2019

vstinner Jan 14, 2019

vstinner Jan 14, 2019

vstinner commented Jan 14, 2019

pablogsal Jan 14, 2019

pablogsal commented Jan 14, 2019

bedevere-bot commented Jan 14, 2019

vstinner Jan 15, 2019

vstinner commented Jan 16, 2019

vstinner commented Jan 16, 2019

	:func:os.posix_spawnp
	Add a new :func:`os.posix_spawnp` function. Patch by Joannah Nanjekye.

bpo-35674: Expose posix_spawnp #11554

bpo-35674: Expose posix_spawnp #11554

Conversation

nanjekyejoannah commented Jan 14, 2019 • edited Loading

vstinner Jan 14, 2019

Choose a reason for hiding this comment

vstinner Jan 14, 2019

Choose a reason for hiding this comment

vstinner Jan 14, 2019

Choose a reason for hiding this comment

vstinner commented Jan 14, 2019

pablogsal Jan 14, 2019

Choose a reason for hiding this comment

pablogsal commented Jan 14, 2019

bedevere-bot commented Jan 14, 2019

vstinner Jan 15, 2019

Choose a reason for hiding this comment

vstinner commented Jan 16, 2019

vstinner commented Jan 16, 2019

nanjekyejoannah commented Jan 14, 2019 •

edited

Loading