Skip to content

Conversation

@LeamHall
Copy link

@LeamHall LeamHall commented Sep 28, 2022

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.

@merwok

This comment was marked as resolved.

@LeamHall LeamHall changed the title Edited Doc/library/subprocess.rst per gh-42127 gh-42127 Expanded the explanation of child process console opening in Doc/library/subprocess.rst Sep 28, 2022
@LeamHall
Copy link
Author

Minor point: could you edit the PR title to follow the conventions? gh-xxxx: Change something specific

We don’t want hundreds of PR saying Edited this/and/that/file slightly_smiling_face

Thank you, change made! Is it better to use, or not use, the file edited in the title?

@merwok
Copy link
Member

merwok commented Sep 28, 2022

Not, because that makes for very long titles which then become very long commit messages.
We also have shorter ways to reference things: Support X in zipfile or Fix table in audioop doc don’t need to spell out Lib/zipfile.py or Doc/library/audioop.rst. The exact files are not important, the module/topic/etc is.

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

@merwok merwok changed the title gh-42127 Expanded the explanation of child process console opening in Doc/library/subprocess.rst gh-42127 Expand explanation of child process console opening in subprocess doc Sep 28, 2022
@terryjreedy
Copy link
Member

@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.

@terryjreedy
Copy link
Member

@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.

@LeamHall
Copy link
Author

@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
Copy link
Member

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
Copy link
Member

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 ..."

Copy link
Contributor

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.

Copy link
Member

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

  1. Such as MSDN.

@bedevere-bot
Copy link

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.

@ethanfurman
Copy link
Member

Closing per @LeamHall's request.

@LeamHall LeamHall deleted the gh_42127 branch July 22, 2023 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting changes docs Documentation in the Doc dir needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes skip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants