bpo-35537: Change subprocess to use posix_spawnp instead of posix_spawn by nanjekyejoannah · Pull Request #11917 · python/cpython
change subprocess to use posix_spawnp instead of posix_spawn
bpo-35537: Change subprocess to use posix_spawnp instead of posix_spawn
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!
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.
| 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`. |
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 :-)
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.
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?
|
|
||
| * *close_fds* is false; | ||
| * *preexec_fn*, *pass_fds*, *cwd* and *start_new_session* parameters | ||
| are not set; |
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."
| and not start_new_session): | ||
| self._posix_spawn(args, executable, env, restore_signals, | ||
| self._posix_spawnp(args, executable, env, restore_signals, | ||
| p2cread, p2cwrite, |
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.
The previous attempt was reverted because of semantic differences between
posix_spawnpand 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 :-(
@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.
