◐ Shell
reader mode source ↗
Skip to content

bpo-35537: Change subprocess to use posix_spawnp instead of posix_spawn#11917

Closed
nanjekyejoannah wants to merge 6 commits into
python:masterfrom
nanjekyejoannah:sub_use_spawnp
Closed

bpo-35537: Change subprocess to use posix_spawnp instead of posix_spawn#11917
nanjekyejoannah wants to merge 6 commits into
python:masterfrom
nanjekyejoannah:sub_use_spawnp

Conversation

@nanjekyejoannah

@nanjekyejoannah nanjekyejoannah commented Feb 18, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

I have added a change to make subprocess.py to use posix_spawnp instead of posix_spawn.

cc @vstinner

https://bugs.python.org/issue35537

@nanjekyejoannah nanjekyejoannah changed the title change subprocess to use posix_spawnp instead of posix_spawn Feb 18, 2019
vstinner
vstinner requested changes Feb 18, 2019
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Ah, one last thing: your PR makes the following doc outdated:
https://docs.python.org/dev/whatsnew/3.8.html#optimizations

Please update: "The subprocess module can now use the os.posix_spawn() function...": it's now posix_spawnp. Moreover, "the executable path contains a directory." condition can now be removed. Well, update the doc :-)

@bedevere-bot

bedevere-bot commented Feb 18, 2019

Copy link
Copy Markdown

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.

@izbyshev

izbyshev commented Feb 18, 2019

Copy link
Copy Markdown
Contributor

The previous attempt was reverted because of semantic differences between posix_spawnp and the existing behavior of subprocess. What has changed since that attempt? Is there any discussion I could follow to understand that?

@vstinner

vstinner commented Feb 18, 2019

Copy link
Copy Markdown
Member

The previous attempt was reverted because of semantic differences between posix_spawnp and the existing behavior of subprocess. What has changed since that attempt? Is there any discussion I could follow to understand that?

Oh.... It was last month but I already completely forgot my own previous attempt :-(

@vstinner

vstinner commented Feb 19, 2019

Copy link
Copy Markdown
Member

@nanjekyejoannah: Sorry, but I forgot that I made exactly the same change last month but we had to revert it... See @izbyshev's comment:
"The previous attempt was reverted because of semantic differences between posix_spawnp and the existing behavior of subprocess. What has changed since that attempt? Is there any discussion I could follow to understand that?"

I close your PR.

@vstinner vstinner closed this Feb 19, 2019
@nanjekyejoannah

nanjekyejoannah commented Feb 19, 2019

Copy link
Copy Markdown
Contributor Author

cool noted!!

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

Reviewers

@vstinner vstinner vstinner requested changes

@gpshead gpshead Awaiting requested review from gpshead

Assignees

No one assigned

Labels

awaiting changes DO-NOT-MERGE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@nanjekyejoannah @bedevere-bot @izbyshev @vstinner @gpshead @the-knights-who-say-ni