bpo-35537: Change subprocess to use posix_spawnp instead of posix_spawn#11917
bpo-35537: Change subprocess to use posix_spawnp instead of posix_spawn#11917nanjekyejoannah wants to merge 6 commits intopython:masterfrom
Conversation
nanjekyejoannah
changed the title
Lib/subprocess.py
Outdated
There was a problem hiding this comment.
The whole purpose of the change is to remove "and os.path.dirname(executable)" but you kept the test... Please remove it!
There was a problem hiding this comment.
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`. |
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this 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 :-)
|
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 |
|
The previous attempt was reverted because of semantic differences between |
nanjekyejoannah
force-pushed
the
sub_use_spawnp
branch
from
68d7580 to
6309240
Compare
Doc/whatsnew/3.8.rst
Outdated
|
|
||
| * *close_fds* is false; | ||
| * *preexec_fn*, *pass_fds*, *cwd* and *start_new_session* parameters | ||
| are not set; |
There was a problem hiding this comment.
typo: "are not set;" should become "are not set."
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, |
There was a problem hiding this comment.
Please fix the indentation: p2cread must be indentation with args.
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: I close your PR. |
|
cool noted!! |
6 participants
I have added a change to make subprocess.py to use posix_spawnp instead of posix_spawn.
cc @vstinner
https://bugs.python.org/issue35537