GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-35537: Change subprocess to use posix_spawnp instead of posix_spawn by nanjekyejoannah · Pull Request #11917 · python/cpython

Conversation

nanjekyejoannah commented Feb 18, 2019 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          change subprocess to use posix_spawnp instead of posix_spawn

1013d06

nanjekyejoannah requested a review from gpshead as a code owner

February 18, 2019 12:42

the-knights-who-say-ni added the CLA signed label

Feb 18, 2019

bedevere-bot added the awaiting review label

Feb 18, 2019

nanjekyejoannah changed the title ~~change subprocess to use posix_spawnp instead of posix_spawn~~ bpo-35537: Change subprocess to use posix_spawnp instead of posix_spawn

Feb 18, 2019

vstinner reviewed

Feb 18, 2019

View reviewed changes

Comment thread

Lib/subprocess.py Outdated

vstinner Feb 18, 2019

Copy link

Copy Markdown

Member

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 whole purpose of the change is to remove "and os.path.dirname(executable)" but you kept the test... Please remove it!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner reviewed

Feb 18, 2019

View reviewed changes

Comment thread

Misc/NEWS.d/next/Library/2019-02-18-12-45-33.bpo-35537.xshtGJ.rst Outdated

vstinner Feb 18, 2019

Copy link

Copy Markdown

Member

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.

You can enhance the entry using better formatting, and please avoid ".py" when you mention a module. User use "import subprocess", they don't access directly to subprocess.py. NEWS entries are for end users.

Suggested change

	subprocess.py now uses posix_spawnp instead of posix_spawn.
	The :mod:`subprocess` now uses :func:`os.posix_spawnp` instead of :func:`os.posix_spawn`.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner requested changes

Feb 18, 2019

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

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.

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 :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting changes and removed awaiting review labels

Feb 18, 2019

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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?

gpshead reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          change subprocess to use posix_spawnp instead of posix_spawn

nanjekyejoannah force-pushed the sub_use_spawnp branch from 68d7580 to 6309240 Compare

February 18, 2019 13:43


          update docs

ed39610

vstinner reviewed

Feb 18, 2019

View reviewed changes

Comment thread

Doc/whatsnew/3.8.rst Outdated


		* close_fds is false;
		* preexec_fn, pass_fds, cwd and start_new_session parameters
		are not set;

vstinner Feb 18, 2019

Copy link

Copy Markdown

Member

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.

typo: "are not set;" should become "are not set."

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner reviewed

Feb 18, 2019

View reviewed changes

Comment thread

Lib/subprocess.py Outdated

		and not start_new_session):
		self._posix_spawn(args, executable, env, restore_signals,
		self._posix_spawnp(args, executable, env, restore_signals,
		p2cread, p2cwrite,

vstinner Feb 18, 2019

Copy link

Copy Markdown

Member

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.

Please fix the indentation: p2cread must be indentation with args.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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 :-(

gpshead reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nanjekyejoannah and others added 3 commits

February 19, 2019 09:21


          fix typo and indetation

001699d


          Merge branch 'sub_use_spawnp' of https://github.com/nanjekyejoannah/c…

34cef7a

…python into sub_use_spawnp


          Fix typo.

9af6006

gpshead added the DO-NOT-MERGE label

Feb 19, 2019

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner closed this

Feb 19, 2019

nanjekyejoannah commented Feb 19, 2019

Copy link

Copy Markdown

Contributor Author

cool noted!!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Reviewers

vstinner

vstinner requested changes

gpshead

Awaiting requested review from gpshead

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

nanjekyejoannah commented Feb 18, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner Feb 18, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner Feb 18, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Feb 18, 2019

Uh oh!

izbyshev commented Feb 18, 2019

Uh oh!

vstinner Feb 18, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner Feb 18, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner commented Feb 18, 2019

Uh oh!

vstinner commented Feb 19, 2019

Uh oh!

nanjekyejoannah commented Feb 19, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nanjekyejoannah commented Feb 18, 2019 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.