Skip to content
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

bpo-28647: Update -u documentation after bpo-30404 #3961

Merged
merged 7 commits into from
Oct 13, 2017

Conversation

berkerpeksag
Copy link
Member

@berkerpeksag berkerpeksag commented Oct 12, 2017

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Thank you. Please update also the man page.

@berkerpeksag
Copy link
Member Author

@serhiy-storchaka should I add the "stdin is always buffered" part to 77732be#diff-1dc031ac9a0d3915ff7805109a005799R306 too?

@serhiy-storchaka
Copy link
Member

I don't know. This option doesn't affect stdin. Does it add clarity or noise if document this explicitly?

Maybe add this to the RST documentation, but remove from the help and manpage? What other core developers think?

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@vstinner
Copy link
Member

@serhiy-storchaka should I add the "stdin is always buffered" part to 77732be#diff-1dc031ac9a0d3915ff7805109a005799R306 too?

I like the idea of mentionning that stdin is always buffered in documentation of the "-u" command line option.

You may also update the man page in this PR, or it can be done in a different PR.

@vstinner
Copy link
Member

Strange. The docs job of Travis CI failed on "pip --version":

$ pip --version

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

The build has been terminated

First time that I see this error.

@berkerpeksag
Copy link
Member Author

I've addressed review comments.

Modules/main.c Outdated
-u : unbuffered binary stdout and stderr, stdin always buffered;\n\
also PYTHONUNBUFFERED=x\n\
-u : force the stdout and stderr streams to be unbuffered;\n\
stdin is always buffered; also PYTHONUNBUFFERED=x\n\
see man page for details on internal buffering relating to '-u'\n\
Copy link
Member

Choose a reason for hiding this comment

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

The man page doesn't contain additional information in Python 3.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, removed outdated sentence.

@berkerpeksag
Copy link
Member Author

@serhiy-storchaka documented stdin as unbuffered.

@serhiy-storchaka
Copy link
Member

LGTM, but wait if there are other comments.

@vstinner
Copy link
Member

"stdin is always unbuffered"

Wait, what? It's always buffered, no?

Reading one byte in Python reads 1024 bytes at the C level:

haypo@selma$ strace -o trace ./python -u -c 'import sys; sys.stdin.buffer.read(1)'
abc
haypo@selma$ grep -F 'read(0,' trace
read(0, "abc\n", 1024)                  = 4

If stdin is a not a TTY, we read even more at the C level: 4096 bytes.

haypo@selma$ echo abc|strace -o trace ./python -u -c 'import sys; sys.stdin.buffer.read(1)'
haypo@selma$ grep -F 'read(0,' trace
read(0, "abc\n", 4096)                  = 4

The -u option has an effect on sys.stdin: line_buffering. I don't think that it matters in practice, since line buffering only impacts write() whereas sys.stdin is open for read-only.

haypo@selma$ ./python -c 'import sys; print(sys.stdin.line_buffering)'
True
haypo@selma$ ./python -u -c 'import sys; print(sys.stdin.line_buffering)'
False

@vstinner
Copy link
Member

I checked with gdb just to be sure: I confirm that sys.stdin.buffer.read(1) calls read(1024), so reads more than 1 byte, store the following bytes for the following buffer.read() and so is buffered.

@@ -303,7 +303,8 @@ Miscellaneous options

.. cmdoption:: -u

Force the stdout and stderr streams to be unbuffered.
Force the stdout and stderr streams to be unbuffered. The stdin stream is
always unbuffered.
Copy link
Member

Choose a reason for hiding this comment

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

always buffered

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

@serhiy-storchaka
Copy link
Member

It is buffered in other meaning than in Python 2. This buffering doesn't affect behavior (unlike to Python 2). The note about buffering was related to Python 2 buffering. Try the following code in Python 2 and Python 3:

./python -u -c $'import sys\nfor x in sys.stdin: print(repr(x))'

@vstinner
Copy link
Member

It is buffered in other meaning than in Python 2. This buffering doesn't affect behavior (unlike to Python 2).

In that case, we should elaborate that in the documentation. I disagree to simply say "unbuffered".

I suggest to not compare Python 3 to Python 2. Python 2 behaviour is weird and cannot be defined :-D

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Oct 12, 2017

We are fixing the outdated documentation inherited from Python 2. First than keep some statement we should consider what it means in the context of Python 2 and what it means in the context of Python 3.

I suggest to continue the discussion on the tracker.

@vstinner
Copy link
Member

For comparison, this is fully unbuffered read:

haypo@selma$ strace -o trace python3 -c 'f = open("/etc/issue", "rb", buffering=0); f.read(1)'
haypo@selma$ cat trace
...
open("/etc/issue", O_RDONLY|O_CLOEXEC)  = 3
...
read(3, "\\", 1)                        = 1
...
close(3)                                = 0
...

Reading a single byte does... read exactly one byte, and not more.

@berkerpeksag
Copy link
Member Author

I've now updated the PR.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

What do you think Serhiy?

@@ -1276,8 +1276,10 @@ always available.
:envvar:`PYTHONIOENCODING` environment variable before starting Python.

* When interactive, standard streams are line-buffered. Otherwise, they
Copy link
Member

Choose a reason for hiding this comment

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

I think all this paragraph is related to stdout and stderr. Just replace "standard streams" with "stdout and stdin". No other changes are needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just replace "standard streams" with "stdout and stdin".

Did you mean "stdout and stderr" instead of "stdout and stdin"?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Sorry.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can change that. Another question is that shouldn't we still document stdin buffering?

@berkerpeksag berkerpeksag merged commit 7f58097 into python:master Oct 13, 2017
@berkerpeksag berkerpeksag deleted the 28647-master branch October 13, 2017 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants