Skip to content

bpo-42388: Fix subprocess.check_output input=None when text=True#23467

Merged
gpshead merged 4 commits into
python:masterfrom
gpshead:subprocess-issue42388
Dec 25, 2020
Merged

bpo-42388: Fix subprocess.check_output input=None when text=True#23467
gpshead merged 4 commits into
python:masterfrom
gpshead:subprocess-issue42388

Conversation

@gpshead

@gpshead gpshead commented Nov 22, 2020

Copy link
Copy Markdown
Member

When the modern text= spelling of the universal_newlines= parameter was added
for Python 3.7, check_output's special case around input=None was overlooked.
So it behaved differently with universal_newlines=True vs text=True. This
reconciles the behavior to be consistent and adds a test to guarantee it.

The documentation already described this check_output input=None behavior
difference vs the other subprocess functions, but it wasn't very clear to readers.
The documentation has been clarified as part of this.

https://bugs.python.org/issue42388

When the modern text= spelling of the universal_newlines= parameter was added
for Python 3.7, check_output's special case around input=None was overlooked.
So it behaved differently with universal_newlines=True vs text=True.  This
reconciles the behavior to be consistent and adds a test to guarantee it.

The documentation already describes this check_output input=None behavior
difference vs the other subprocess functions.  This ensures that documented
behavior remains accurate in all cases.
@izbyshev

Copy link
Copy Markdown
Contributor

Would it also make sense to clarify the following sentence in the docs?

However, explicitly passing input=None to inherit the parent’s standard input file handle is not supported.

It's not clear to me whether I must not pass input=None at all, or, if I may do that, what the effect is (other than that it's not the inheritance of the parent's stdin). May be it's just me though, English is not my first language.

@github-actions

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Dec 23, 2020
@gpshead

gpshead commented Dec 23, 2020

Copy link
Copy Markdown
Member Author

Thanks @izbyshev, I've clarified the docs.

@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Dec 24, 2020

@izbyshev izbyshev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! If I may nitpick, I suggest an even more clarified change :)

Comment thread Doc/library/subprocess.rst Outdated
Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
@gpshead gpshead closed this Dec 25, 2020
@gpshead gpshead reopened this Dec 25, 2020
@gpshead
gpshead merged commit 64abf37 into python:master Dec 25, 2020
@miss-islington

Copy link
Copy Markdown
Contributor

Thanks @gpshead for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 25, 2020
…honGH-23467)

When the modern text= spelling of the universal_newlines= parameter was added
for Python 3.7, check_output's special case around input=None was overlooked.
So it behaved differently with universal_newlines=True vs text=True.  This
reconciles the behavior to be consistent and adds a test to guarantee it.

Also clarifies the existing check_output documentation.

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
(cherry picked from commit 64abf37)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
@gpshead
gpshead deleted the subprocess-issue42388 branch December 25, 2020 04:57
@bedevere-bot

Copy link
Copy Markdown

GH-23934 is a backport of this pull request to the 3.9 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 25, 2020
…honGH-23467)

When the modern text= spelling of the universal_newlines= parameter was added
for Python 3.7, check_output's special case around input=None was overlooked.
So it behaved differently with universal_newlines=True vs text=True.  This
reconciles the behavior to be consistent and adds a test to guarantee it.

Also clarifies the existing check_output documentation.

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
(cherry picked from commit 64abf37)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
@bedevere-bot

Copy link
Copy Markdown

GH-23935 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit that referenced this pull request Dec 25, 2020
…23467)

When the modern text= spelling of the universal_newlines= parameter was added
for Python 3.7, check_output's special case around input=None was overlooked.
So it behaved differently with universal_newlines=True vs text=True.  This
reconciles the behavior to be consistent and adds a test to guarantee it.

Also clarifies the existing check_output documentation.

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
(cherry picked from commit 64abf37)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
miss-islington added a commit that referenced this pull request Dec 25, 2020
…23467)

When the modern text= spelling of the universal_newlines= parameter was added
for Python 3.7, check_output's special case around input=None was overlooked.
So it behaved differently with universal_newlines=True vs text=True.  This
reconciles the behavior to be consistent and adds a test to guarantee it.

Also clarifies the existing check_output documentation.

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
(cherry picked from commit 64abf37)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
…honGH-23467)

When the modern text= spelling of the universal_newlines= parameter was added
for Python 3.7, check_output's special case around input=None was overlooked.
So it behaved differently with universal_newlines=True vs text=True.  This
reconciles the behavior to be consistent and adds a test to guarantee it.

Also clarifies the existing check_output documentation.

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants