@asvetlov @ethanfurman this follows #23536.

TODO: update documentation for the tty module; maybe also add tests for it.

Note: I have put my name in one of the files; let me know if that is not customary :D

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.

@asvetlov Thank you very much for the reviews. As per your suggestion, the updated commit will include documentation, etc.

Edit: you mentioned "I suggest esablish public function names first, update the tty module documentation, and make the final review". I am inexperienced when it comes to the cpython development process. How do I "establish public function names"? Thank you in advance.

@asvetlov : Thanks!

A few thoughts:

1. For consistency, don't we need an mkcbreak as well?

2. The setraw implementation in prompt_toolkit is slightly different. But I'm certainly not an expert on this.

• See: https://github.com/prompt-toolkit/python-prompt-toolkit/blob/master/prompt_toolkit/input/vt100.py#L204
• It's a context manager.
• It's clearing IXOFF as well (not sure if needed).
• We clear INLCR and IGNCR to avoid translation of carriage return to newlines on input. This patch only clears ICRNL.
• There are a few other flags we don't clear in prompt_toolkit, but probably should: BRKINT, INPCK, ISTRIP, OPOST, CSIZE, PARENB.

3. I'm not familiar with the login_tty functionality.

4. setwinsize and getwinsize look fine. I've never used the pixel values though, and I've never done a "set size".

Overall I think this looks good.

One thing slightly related, but out of scope for this PR, that often missed in Python is that sys.stdin/sys.stdout are not context variables. So it's impossible to write an asyncssh server that exposes a REPL where print/input is working properly. (I guess I should propose that on python-ideas or bpo, but no energy/time to drive the discussion now.)

@asvetlov Thank you for your patience. I have made the requested changes; please review again. I restructured the files a little bit. As a result, your reviews are showing up as outdated; they are not. Here are some notes to make reviewing easier for you:

1. Instead of using hasattr(termios, "TIOCGWINSZ"), I used

try:
    from termios import TIOCGWINSZ
    ...
except ImportError:
    ...

I did this to avoid import termios, since we already have a from termios import * which cannot be removed because of backward compatibility reasons. I understand that from termios import * and import termios can coexist; it just felt a little strange doing that :D If you think that using import termios + hasattr() is the better option, then let me know; I will make the changes.

2. Did I add the stubs for getwinsize() and setwinsize() properly?

3. I have renamed the functions. One of the comments that I left on one of your reviews was related to the naming of login_tty(); please let me know if that is acceptable.

4. getwinsize() and setwinsize() now handle the packing/unpacking of structs. As the result, their input/output are tuples of integers instead of bytestrings. That makes them more user-friendly; this helped me remove the pack("HHHH", ...) from test_pty.py and abstract out the important parts.

5. I have written some documentation. I welcome suggestions!

I too would be honored if this work was reviewed by @jonathanslenders.

Edit: after posting this, I noticed that @jonathanslenders has already posted a review! Thank you very much for this!

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

Could we return a namedtuple from getwinsize?

@asvetlov @jonathanslenders I have made the requested changes; please review again.

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

These are some of my old comments and notes; I used them to track progress and make decisions; they may be ignored. Since the comment section was a total mess with mostly me talking to myself, I had to put them together to make it look cleaner. I had also made some "reviews" as notes for myself; they are now gone since I removed a lot of code (the entire class winsize, tty.login_tty() etc) by making a force-push.

old comment 1

@asvetlov @jonathanslenders

1. Yes! Named tuples sound like a good idea. Working on it!
2. mkcbreak will be added.
3. Thank you for pointing us toward the setraw() implementation in prompt_toolkit. I will look into it after taking a nap :D My original idea was to make a clone of cfmakeraw(3): https://sourceware.org/git/?p=glibc.git;a=blob;f=termios/cfmakeraw.c, which does what you mentioned except for clearing INPCK. Should I make it clear all the flags that glibc's cfmakeraw() clears + maybe INPCK as well?

old comment 2

@asvetlov @jonathanslenders I have made the requested changes; please review again. Notes follow.

1. Updated docs.
2. mkcbreak() implemented.
3. Our current mkraw now sets/unsets all flags mentioned by @jonathanslenders except that IXOFF is not being cleared. I think that it would not hurt to clear IXOFF; it is just that cfmakeraw(3) does not do it. Should I go ahead and also clear IXOFF? Since IXOFF is POSIX compliant, it will be present on every system that is immediately relevant.
4. getwinsize() now returns a namedtuple; the field names are same as that of the traditional struct winsize. The documentation is also advertising the winsz argument of setwinsize() as being a namedtuple of the same type.
5. A comment has been added to the except (NameError, OSError): clause in tty.login_tty().
6. Docstring of tty.login_tty() has been modified to reflect the fact that fd must be a file descriptor of a tty.

old comment 3

https://pubs.opengroup.org/onlinepubs/7908799/xcurses/intov.html#tag_001_005_002 mentions that cbreak Mode must additionally have ICRNL cleared. The document is about "xcurses"; however, I checked using stty that even the python curses.cbreak() clears ICRNL. I am going to make another updated commit with very detailed comments!

old comment 4

I compared the following definitions/implementations of "raw mode tty" and "cbreak mode tty":

raw

1. BSD cfmakeraw()
2. Python curses.raw()
3. stty raw on POSIX, Linux, BSD, and Solaris [ from manpages ]. The one on my Debian Linux system is the GNU coreutils stty(1), which is the most general: it clears all the input flags that are there on the POSIX manpage of stty, which includes IXOFF.
4. prompt_toolkit's setraw!
5. the current python tty.setraw()

cbreak

1. Python curses.cbreak(); based on what this does, and https://pubs.opengroup.org/onlinepubs/7908799/xcurses/intov.html#tag_001_005_002, our mkcbreak() function should also clear ICRNL; however, it should not modify ISIG and IXON.
2. stty cbreak
3. the current Python tty.setcbreak()

I would like to put down what I learned about IXOFF today: IXON enables Xon/Xoff "software flow control" on output, and IXOFF controls that on the input. Therefore, enabling or disabling IXOFF has no effect on xterm(1) or the ttys provided by the linux kernel (/dev/tty1, /dev/tty2,...); it is probably more relevant when working with a hardware terminal; with a terminal emulator on a fast pc, that does not matter. See https://tldp.org/HOWTO/Text-Terminal-HOWTO-11.html, https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap11.html#tag_11_01_09. Both C-s and C-q are only affected when IXON are turned on or off; if IXANY is also enabled, then any character can be used to resume the flow control as opposed to just C-q (or whatever is the START character).

Edit: @asvetlov Sir, I just realized after reading the devguide that "non-trivial contributions are credited in the Misc/ACKS file" and in the "news entry". Am I eligible for this? Would it be better if I removed my name/email from the main files? I have absolutely no problem in doing that; I just want to understand the correct protocol :)

More info: The "Scroll Lock" key on this 1991 IBM Model M can also be used for "software flow control"; this key is unaffected by either IXON or IXOFF.

old comment 5

New commit: winsize is now a separate class to enforce nonnegative integer entries.

old comment 6

@jonathanslenders I have made the changes that you requested. I request you to please review this again when you have time.

Notes: winsize is now a class so that we can force the fields to be nonnegative. As you had hinted before, the ws_xpixel and ws_ypixel might be less useful now. I am not even sure if there exists an OS that still actually uses them. Should we remove them? They are certainly not hurting other functionality at the moment. The "get size" functionality already exists as os.get_terminal_size(): please see os_get_terminal_size_impl() in https://github.com/python/cpython/blob/master/Modules/posixmodule.c. However, that function also supports windows CONIO; further, there is no function for setting window size. I need both "get" and "set" window size functions (only on *nix) to be able to fix some problems with pty: see https://bugs.python.org/issue41818. Further, note that shutil.get_terminal_size() is not appropriate for this since it simply queries the variables LINES and COLUMNS instead of using TIOCGWINSZ. Thank you for you time.

Edit: to ease the review process, here are the various implementations of login_tty(3):

1. FreeBSD: https://github.com/freebsd/freebsd/blob/master/lib/libutil/login_tty.c
2. OpenBSD: https://github.com/openbsd/src/blob/master/lib/libutil/login_tty.c
3. NetBSD: https://github.com/NetBSD/src/blob/trunk/lib/libutil/login_tty.c
4. glibc: https://github.com/lattera/glibc/blob/master/login/login_tty.c

Edit2: Here is where I am using tty.login_tty() [ it is still named tty.login() there ]: https://github.com/8vasu/pypty2/blob/master/pty2.py. This will be a part of the next (which is possibly the final) PR in this sequence.

Edit3: List of all POSIX.1-2017 input mode flags - https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/termios.h.html#tag_13_72_03_02

old comment 7

TIOCGWINSZ and TIOCSWINSZ are widely available, but they are not POSIX compliant since POSIX generally does not standardize the ioctl() except for the STREAMS interface: see https://www.austingroupbugs.net/view.php?id=1151

They have decided to include functions tcgetwinsize() and tcsetwinsize() in IEEE Std 1003.1 ("POSIX.1") issue 8 [ the upcoming version of POSIX ]; see https://www.austingroupbugs.net/view.php?id=1151#c3856. NetBSD has already implemented them:

1. http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/termios/tcgetwinsize.c?only_with_tag=MAIN
2. http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/termios/tcsetwinsize.c?only_with_tag=MAIN

Therefore, now I am highly tempted to change the winsize class back to a couple of functions named tcsetwinsize(), tcgetwinsize() OR at least rename the methods to tcsetwinsize(), tcgetwinsize(). I need some help in making this decision. The benefit of having a class is that we can then have named attributes AND enforce nonnegative integer values for them.

Additional notes:

1. In issue 8, POSIX will also standardize SIGWINCH, which we need for the pty module.
2. They were originally going to name the functions tcsetsize(), tcgetsize(); see

• https://www.opengroup.org/austin/docs/austin_839.txt
• https://www.opengroup.org/austin/docs/austin_838.txt
• https://www.opengroup.org/austin/docs/austin_833.txt
• https://www.opengroup.org/austin/docs/austin_837.txt
• https://www.opengroup.org/austin/docs/austin_834.txt
• https://www.opengroup.org/austin/docs/austin_823.txt
• https://www.opengroup.org/austin/docs/austin_836.txt
• https://www.opengroup.org/austin/docs/austin_841.txt
• https://www.opengroup.org/austin/docs/austin_913.txt
• https://www.opengroup.org/austin/docs/austin_841.txt

Edit: renamed the methods; I am keeping the class definition.

old comment 8

@asvetlov @jonathanslenders Thank you for your patience. I have added the final test for pty to this pull request; please look for test_winch(). This test fails when current stdin is a tty and when SIGWINCH is available; it pinpoints the reason why pty.spawn() is unable to dynamically resize the slave window, which leads to gibberish output after, say, the current stdin is resized because of the creation of a tmux pane, or an emacs window in a frame while using ansi-term, or when resizing the GUI [X/Wayland] terminal window. See before and after pictures here: https://bugs.python.org/file49455/before.png, https://bugs.python.org/file49456/after.png

There should only be one more pull request after this :D

Rebased to have only one clean commit. Removed login_tty(); it will be a part of the final pull request. All functions/methods now have standard POSIX/BSD names or are inspired by such names.

@jonathanslenders Sir, can you please review this again? I have removed a lot of the code [ winsize and login_tty() ]. Thank you in advance.

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

Still waiting for change review.

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

@asvetlov @gpshead Please take a look at this if time permits. You can ignore all comments made before since most of that is now covered by termios.tcgetwinsize(), termios.tcsetwinsize(), and os.login_tty().

New summary for this PR: add cfmakeraw(), cfmakeecho(), and cfmakecbreak(); make setraw() and setcbreak() return saved termios attributes to avoid extra tcgetattr() calls.