Skip to content

bpo-35674: Expose posix_spawnp #11554

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.

Sign up for GitHub

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

Jump to bottom
Merged
vstinner merged 3 commits into from
Jan 16, 2019
Merged

bpo-35674: Expose posix_spawnp #11554

vstinner merged 3 commits into from
Jan 16, 2019

Conversation

@nanjekyejoannah
Copy link
Contributor

@nanjekyejoannah nanjekyejoannah commented Jan 14, 2019 •
edited
Loading

I have exposed posix_spawnp.

Co-Author: Victor Stinner [email protected]

https://bugs.python.org/issue35674

@pablogsal pablogsal self-assigned this Jan 14, 2019
@pablogsal pablogsal self-requested a review January 14, 2019 14:23
vstinner
vstinner reviewed Jan 14, 2019
View reviewed changes
Lib/test/test_posix.py Outdated
Copy link
Member

@vstinner vstinner Jan 14, 2019

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"

Modules/posixmodule.c Outdated
Copy link
Member

@vstinner vstinner Jan 14, 2019

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

@vstinner vstinner mentioned this pull request Jan 14, 2019
Closed
pablogsal
pablogsal reviewed Jan 14, 2019
View reviewed changes
vstinner
vstinner reviewed Jan 14, 2019
View reviewed changes
Modules/posixmodule.c Outdated
Copy link
Member

@vstinner vstinner Jan 14, 2019

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.

@vstinner
Copy link
Member

vstinner commented Jan 14, 2019

AppVeyor build failed

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

pablogsal
pablogsal reviewed Jan 14, 2019
View reviewed changes
Modules/posixmodule.c Outdated
Copy link
Member

@pablogsal pablogsal Jan 14, 2019

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

pablogsal
pablogsal reviewed Jan 14, 2019
View reviewed changes
@pablogsal
Copy link
Member

pablogsal commented Jan 14, 2019

Very good job Joannah! :)

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

serhiy-storchaka
serhiy-storchaka previously requested changes Jan 14, 2019
View reviewed changes
@bedevere-bot
Copy link

bedevere-bot commented Jan 14, 2019

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.

@nanjekyejoannah nanjekyejoannah force-pushed the spawnp branch 3 times, most recently from 87d6fed to e2a9603 Compare January 15, 2019 21:51
vstinner
vstinner requested changes Jan 15, 2019
View reviewed changes
Misc/NEWS.d/next/Library/2019-01-14-14-13-08.bpo-35674.kamWqz.rst Outdated
Copy link
Member

@vstinner vstinner Jan 15, 2019

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:

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

vstinner
vstinner reviewed Jan 16, 2019
View reviewed changes
vstinner
vstinner reviewed Jan 16, 2019
View reviewed changes
@vstinner
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 vstinner dismissed serhiy-storchaka’s stale review January 16, 2019 13:19

Serhiy's comments have been addressed.

@vstinner
Copy link
Member

vstinner commented Jan 16, 2019

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 vstinner merged commit 92b8322 into python:master Jan 16, 2019
@vstinner
Copy link
Member

vstinner commented Jan 16, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsal pablogsal pablogsal left review comments

@vstinner vstinner vstinner approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

Assignees

@pablogsal pablogsal

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@nanjekyejoannah @vstinner @pablogsal @bedevere-bot @serhiy-storchaka @the-knights-who-say-ni