-
-
Notifications
You must be signed in to change notification settings - Fork 34k
gh-42127 Expand explanation of child process console opening in subprocess doc #97614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Thank you, change made! Is it better to use, or not use, the file edited in the title? |
|
Not, because that makes for very long titles which then become very long commit messages. Another minor point: imperative mode is better than indicative present or past, and make the message describe what the change does, not what the author did. This may be of interest: https://initialcommit.com/blog/git-commit-messages-best-practices |
|
@eryksun You gave by far the most information on the issue. Can you verify that the patch is correct? I will can merge if you say it is ready. If anyone else merges, delete me as co-author if I get listed as such due to resolving the merge conflict. |
|
@LeamHall Line numbers changing due to addition/deletion elsewhere are not necessarily a problem. The merge conflict was due to someone moving None from the end of the list to the beginning, to be consistent with the order of explanations. Your patch made the order consistent by moving the explanation of None last (and expanding it). I decided that the long explanation was better at the end where you moved it, so I kept your text in favor of revising the slightly altered old text. The only change to your patch was marking the first occurrence of 'file object', which was present already in the second and added to the first in the older patch. See 819ce88. For the second conflict, I just kept your version. Please reread the cumulative 'file changed' diff to make sure it reads properly after the merge conflict. |
@terryjreedy, it all looks good to me. I'm new to this, and not an expert. |
| longer works. | ||
|
|
||
| *stdin*, *stdout* and *stderr* specify the executed program's standard input, | ||
| standard output and standard error file handles, respectively. Valid values |
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.
Rather than repeating the detailed description in the Popen constructor docs for these handles, end on "respectively. " and just add one additional sentence linking to the frequently-used-arguments section that already documents these semantics. (deleting the bulk of the original paragraph)
| ``None``. :data:`PIPE` indicates that a new pipe to the child should be created. | ||
| :data:`DEVNULL` indicates that the special file :data:`os.devnull` will | ||
| be used. With the default settings of ``None``, no redirection will occur; | ||
| the child's file handles will be inherited from the parent for console |
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.
This wording seems to be Windows specific. There is no such thing as a "console" or "console application" or "window manager" or "graphical shell" having anything to do with file descriptors on POSIX systems.
But I think all this needs is some disambiguation wording:
Turn the ; into a . and start a new Windows specific paragraph:
"On Windows, the child's file handles will be inherited from the parent if the child can connect to the console. Otherwise ..."
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 description for Windows needs to be corrected and fleshed out.
If the value is None, then no redirection occurs. On POSIX, the child inherits a standard file that is not redirected only if its file descriptor (i.e. 0, 1, or 2) is inheritable. The latter is irrespective of the value of the close_fds parameter.
On Windows, if one or more of the standard files is redirected, then, for each valid standard file that is not redirected, the child inherits a duplicate of the current handle. If the current handle value is None, a handle for a new pipe is substituted. If none of the standard files is redirected and close_fds is false, then the child inherits each standard handle only if it is inheritable. If none of the standard files is redirected, and close_fds is true, the startupinfo handle list is not used explicitly, and the child is a console application that inherits the current console session, then Windows implicitly duplicates the standard handles to the child. Otherwise, the initial standard handle values in the child will be default values. If the child is a console application that is spawned with a new console session, the default standard handles reference console I/O. Else the default values are None.
Beyond just documenting the current behavior in regard to redirecting the standard files, what I'd like to see is new behavior that's more consistent between POSIX and Windows, and behavior that's easier to explain and understand on Windows.
To better match POSIX, the Windows implementation could directly use the current value of a standard handle when it isn't redirected, instead of duplicating an inheritable handle. The pipe fallback for when the current value is None is completely unnecessary behavior. For example, for stdin:
if stdin is None:
p2cread = _winapi.GetStdHandle(_winapi.STD_INPUT_HANDLE)
if p2cread is not None:
try:
if not os.get_handle_inheritable(p2cread):
p2cread = None
except OSError:
p2cread = None
else:
if stdin == PIPE:
# FIXME: _winapi.CreatePipe(None, max(self.pipesize, 0))
p2cread, p2cwrite = _winapi.CreatePipe(None, 0)
p2cread, p2cwrite = Handle(p2cread), Handle(p2cwrite)
elif stdin == DEVNULL:
p2cread = msvcrt.get_osfhandle(self._get_devnull())
elif isinstance(stdin, int):
p2cread = msvcrt.get_osfhandle(stdin)
else:
p2cread = msvcrt.get_osfhandle(stdin.fileno())
p2cread = self._make_inheritable(p2cread)If the handle list is used in _exec_child(), then 0 and None values would have to be filtered out via handle_list[:] = [int(h) for h in handle_list if h].
There's also a behavior difference if none of the standard files is redirected when close_fds is true. It could be implemented to always inherit each standard file if the handle is inheritable, as is implemented on POSIX. This entails removing the check in _get_handles() for when none of the standard files is redirected. Instead, when none of the standard files is redirected, it should return the current values in p2cread, c2pwrite, and errwrite. With this change, Popen() would always use STARTF_USESTDHANDLES.
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.
Instead of all this, I would have added a separate new paragraph, after the text "into the same file handle as for stdout" in the original, which explicitly noted that it was Windows specific. Something like:
On Windows, the default settings result in the subprocess output going to your code's standard output. If your code is running as a GUI application (which has no console, and hence no standard output) then the OS will automatically open a console window to display the subprocess output.
This is deliberately a little vague, and only approximates the truth. That's because the actual underlying behaviour is subtle, but it's (IMO) not the job of the Python documentation to explain the nuances of Windows (and the whole console vs GUI application thing).
Honestly, though, I'm not sure this warrants a change in any case. The original issue seems to be arguing for completeness and precision, but doesn't offer any motivating real-world situation where knowing this level of detail is important. I suspect that's either because there isn't one, or there is, but anyone who encounters it has access to far better (more detailed and complete) sources of information1 than we can offer here.
Footnotes
-
Such as MSDN. ↩
|
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 |
|
Closing per @LeamHall's request. |
This expands the child process console opening in Doc/library/subprocess.rst. This PR is compatible with branch 3.11, but not with branch 3.10.