bpo-35537: Change subprocess to use posix_spawnp instead of posix_spawn #11917
Conversation
nanjekyejoannah
changed the title
Lib/subprocess.py
Outdated
| @@ -1525,7 +1523,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, | |||
| if executable is None: | |||
| executable = args[0] | |||
|
|
|||
| if (_USE_POSIX_SPAWN | |||
| if (_USE_POSIX_SPAWNP | |||
| and os.path.dirname(executable) | |||
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!
| @@ -0,0 +1 @@ | |||
| subprocess.py now uses posix_spawnp instead of posix_spawn. | |||
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`. |
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
| @@ -317,14 +317,13 @@ xml | |||
| Optimizations | |||
| ============= | |||
|
|
|||
| * The :mod:`subprocess` module can now use the :func:`os.posix_spawn` function | |||
| * The :mod:`subprocess` module can now use the :func:`os.posix_spawnp` function | |||
| in some cases for better performance. Currently, it is only used on macOS | |||
| and Linux (using glibc 2.24 or newer) if all these conditions are met: | |||
|
|
|||
| * *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
| @@ -1535,7 +1532,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, | |||
| and (c2pwrite == -1 or c2pwrite > 2) | |||
| and (errwrite == -1 or errwrite > 2) | |||
| 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!! |
I have added a change to make subprocess.py to use posix_spawnp instead of posix_spawn.
cc @vstinner
https://bugs.python.org/issue35537