Lib/pty.py major revision

[8vasu]

BPO	41818
Nosy	@gpshead, @pfmoore, @ronaldoussoren, @tjguk, @ned-deily, @encukou, @asvetlov, @zware, @koobs, @zooba, @kulikjak, @aeros, @8vasu
PRs	bpo-41818: Updated tests for the standard pty library #22962 bpo-41818: test_openpty succeed on Gentoo, don't expect to fail on this platform #23514 bpo-41818: Make test_openpty() avoid unexpected success due to number of rows and/or number of columns being == 0. #23526 bpo-41818: test_master_read() in test_pty.py should be expected to fail on every platform that is not Linux; that includes Solaris. #23533 bpo-41818: Fix test_master_read() so that it succeeds on all platforms that either raise OSError or return b"" upon reading from master #23536 bpo-41818: Make additions to the tty module #23546 bpo-41818: Add termios.tcgetwinsize(), termios.tcsetwinsize(). Update docs. #23686 bpo-41818: Add os.login_tty() for *nix. #23740 bpo-41818: Close file descriptors in test_openpty #24119 bpo-41818: ++ termios versionadded markers. #27987 bpo-41818: Add os.login_tty() for *nix. #29658

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2020-09-19.23:19:34.173>
labels = ['OS-mac', '3.8', '3.9', '3.10', 'library', 'tests', 'OS-windows']
title = 'Lib/pty.py major revision'
updated_at = <Date 2021-11-20.10:06:35.452>
user = 'https://github.com/8vasu'

bugs.python.org fields:

activity = <Date 2021-11-20.10:06:35.452>
actor = 'soumendra'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)', 'macOS', 'Tests', 'Windows', 'FreeBSD']
creation = <Date 2020-09-19.23:19:34.173>
creator = 'soumendra'
dependencies = []
files = []
hgrepos = []
issue_num = 41818
keywords = ['patch']
message_count = 22.0
messages = ['377195', '377198', '377202', '381830', '381839', '381843', '381845', '381847', '381852', '381870', '381922', '381930', '381941', '381965', '381968', '381980', '382016', '384408', '384413', '385257', '400389', '400393']
nosy_count = 13.0
nosy_names = ['gregory.p.smith', 'paul.moore', 'ronaldoussoren', 'tim.golden', 'ned.deily', 'petr.viktorin', 'asvetlov', 'zach.ware', 'koobs', 'steve.dower', 'kulikjak', 'aeros', 'soumendra']
pr_nums = ['22962', '23514', '23526', '23533', '23536', '23546', '23686', '23740', '24119', '27987', '29658']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue41818'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

Linked PRs

• gh-85984: Remove legacy Lib/pty.py code. #92365
• gh-85984: Utilize new "winsize" functions from termios in pty tests. #101831
• gh-85984: New additions and improvements to the tty library. #101832
• gh-85984: Major revision of the pty library. #101833
• gh-85984: Add POSIX pseudo-terminal functions. #102413
• [3.11] gh-85984: Utilize new "winsize" functions from termios in pty tests. (GH-101831) #104652
• gh-85984: Document change in return type of tty functions #110028
• [3.12] gh-85984: Document change in return type of tty functions (GH-110028) #110324
• gh-85984: Add _POSIX_VDISABLE from unistd.h to termios module. #114985

[8vasu]

The current pty library has the following issues:

1. Does not set slave termios. Documented in the source.

2. Does not set initial slave window size. Documented in the source. Does not handle SIGWINCH. See bpo-41494, bpo-41541. This is essential in the following practical scenarios: i. creating split windows/panes while using a terminal multiplexer; ii. when resizing GUI terminal emulator window, especially relevant when using tiling window managers; iii. resizing an ansi-term window created inside a GNU Emacs frame.

3. Does not perform signal handling. Signals must be blocked during sensitive portions of code.

4. Hangs on FreeBSD. See bpo-26228.

5. Includes deprecated functions pty.master_open(), pty.slave_open().

6. In pty.fork(), the fallback code should try using TIOCSCTTY first. It is still using the old method of opening a tty to make it the controlling tty. Currently even SysV based systems provide TIOCSCTTY. See https://stackoverflow.com/questions/51593530/code-explanation-for-glibc-login-tty-function-openttyname-immediately-f

The current version of pty.spawn() uses pty.fork() internally. However, pty.fork() closes slave and only returns (pid, master_fd). To update winsize, access to slave is necessary. Further, slave termios must be properly set. The proposed modifications do this by implementing a login_tty(3) based function ( tty.login() ), and using that in pty.spawn() instead of pty.fork(). tty.login() tries TIOCSCTTY before falling back to the old SysV method because Python currently does not provide an interface to the native login_tty(3).

7. tty.setraw() is called right after tty.tcgetattr(). This increases redundancy of code because tty.setraw() itself makes an identical tty.tcgetattr() call.

8. Requires testing/porting to more platforms. Solaris, Illumos, macOS, Cygwin, etc. Windows ConPTY?

9. There should be an option in pty.spawn() to turn off slave's ECHO flag. For example, when it is being used in a pipe. See util-linux/util-linux@1eee1ac and util-linux/util-linux@75ccd75#diff-3834a3d25eeaf20d9d0dcb05a46995f6

10. Tests are incomplete. Tests consider OSes such as Tru64 but not {Free/Net/Open/...}BSD.

Find ongoing work here: https://github.com/8vasu/pypty2

[aeros]

In addition to the above, if a major revision is made to pty, I'd suggest also addressing the issue of "master/slave" terminology, and replace it with something comparable like "parent/child". There's an open devguide issue (python/devguide#605) to more explicitly state terms to avoid, and support for avoiding usage of "slave/master" seems uncontroversial (especially in any new code).

[8vasu]

Makes sense. I will happily make a change of terminology in the pypty2 repository after the most desirable alternative is determined based on the choice of the majority. I think 'mother/son' sounds cute while still retaining the same initials as before; people used to the older
terminology will find this easy to remember. Terminology such as parent/child and server/client might make it a little confusing.

[asvetlov]

New changeset c13d899 by Soumendra Ganguly in branch 'master':
bpo-41818: Updated tests for the standard pty library (GH-22962)
c13d899

[vstinner]

This change broke x86 Gentoo buildbots: bpo-42463.

[asvetlov]

#23514 has the fix, waiting for all buildbots finish before pressing "Merge" button.
Gentoo bots are green.

[vstinner]

#23514 has the fix, waiting for all buildbots finish before pressing "Merge" button.

Thanks for working on a fix :-)

[vstinner]

In addition to the above, if a major revision is made to pty, I'd suggest also addressing the issue of "master/slave" terminology

In bpo-34605, I chose to leave the pty module unchange since it *seems* like "master_fd" and "slave_fd" is part of the API. See my rejected PR 9100.

More recently, I discussed with a glibc maintainer who is open to change the openpty() manual page to avoid "master" and "slave" terms.

In fact, these terms are not part of the API. They are just variable names in a manual page.

"parent" and "child" terms would work here. I'm not sure of the exact relationship between the two file descriptors.

[asvetlov]

New changeset 87f7ab5 by Andrew Svetlov in branch 'master':
bpo-41818: test_openpty succeed on Gentoo, don't expect to fail on this platform (GH-23514)
87f7ab5

[zware]

Moving my notes from PR23514 to here, the issue that that PR addressed is not Gentoo-specific; here's a simple reproducer on Ubuntu:

./python -c "import pty;pty.spawn('./python -m test -v test_pty'.split())"

[8vasu]

The reproducer was helpful. #23526 should fix this issue.

[kulikjak]

This change also broke Solaris (SunOS), where (similarly to BSDs and Darwin) OSError is not raised in the new test_master_read() test.

Adding or PLATFORM == "SunOS" into the expectedFailureOnBSD function fixes this issue, but it's no longer just BSDs and it's derivates.

[asvetlov]

New changeset f5a19ea by Soumendra Ganguly in branch 'master':
bpo-41818: Make test_openpty() avoid unexpected success due to number of rows and/or number of columns being == 0. (GH-23526)
f5a19ea

[8vasu]

This is actually good news. I had not tested the code on Solaris; this confirms my suspicion that Linux is the only platform that "behaves differently". Sadly, the current Lib/pty.py code depends on such "different behavior" to exit from pty.spawn()'s copy loop, which is why it hangs on the BSDs, macOS, and now we know that it (probably) also hangs on Solaris. Adding 'or PLATFORM == "SunOS"' is the correct thing to do. I am working on this now.

[8vasu]

PR-23533 should fix the test_master_read() issue on Solaris. Actually, instead of adding 'or PLATFORM == "SunOS"', I ended up removing the whole line and replaced it with 'if platform.system() != "Linux"' because I expect that test to fail on every platform that is not Linux.

[8vasu]

Update: I closed PR 23533. PR 23536 is much better. It will help us detect exact behavior on each platform, which is necessary for making pty.spawn() successfully exit its copy loop.

[asvetlov]

New changeset 74311ae by Soumendra Ganguly in branch 'master':
bpo-41818: Fix test_master_read() so that it succeeds on all platforms that either raise OSError or return b"" upon reading from master (GH-23536)
74311ae

[encukou]

I noticed an issue in one of the newly added tests; see #68307

[8vasu]

Thank you for the fix. That test was created by modifying an existing test which already had that issue; it is documented in a comment by user nnorwitz in the file. If your solution resolves the problem for all the tests in "class PtyTest", then can you please also remove that comment?

[encukou]

New changeset 65cf1ad by Petr Viktorin in branch 'master':
bpo-41818: Close file descriptors in test_openpty (#GH-24119)
65cf1ad

[gpshead]

New changeset ae224bb by Soumendra Ganguly in branch 'main':
bpo-41818: Add termios.tcgetwinsize(), termios.tcsetwinsize(). (GH-23686)
ae224bb

[serhiy-storchaka]

Why not make cfmakeraw() and cfmakecbreak() returning a new list instead of modifying the input in-place? In any case you need to make a copy, but it is errorprone, because the list contains a nested list. See #110392.

[encukou]

Regarding "master/slave" terminology:
I plan to use a strategy I learned at @evildmp's workshop. First, merge #102413, which extends os with a bunch of functions that handle PTY-pairs. There's no simple way to avoid using the inappropriate terms in these functions' docs.
The merge will make the os docs worse in this respect -- but it'll also make the issue more obvious, and hopefully easier to solve.
After that I'll propose a PR. Still don't know how that will look, but there's lively brainstorming and style guide recommendations over at the Docs community discord.

Anyway: @8vasu, feel free to focus on the techincal side, and leave terminology to me.

[vstinner]

Regarding "master/slave" terminology

See also: python/devguide#605 (which was already mentioned previously).

[8vasu]

I have also commented a variation of this on: #102413

@vstinner While I am not a CPython maintainer and hence do not have much say in deciding which directions such a prestigious project should take, I am an end user who does pty programming frequently (Python or otherwise) and I will have to deal with the consequences (good or bad) of any decisions taken here.

I have severe clinical OCD/OCPD and would like to issue a warning here: using generic names like main_fd, second_fd, or parent_fd, child_fd, or server_fd, client_fd will lead to havoc very easily. Just take a look at the source of your very own https://github.com/python/cpython/blob/3.12/Lib/pty.py and try a search/replace involving terms parent/child (already used by processes) or a program like ssh(1) which uses the terms server/client in networking context and also sets up a pty pair.

On the other hand one can easily have first_fd, second_fd, third_fd or even main_fd in their existing programs. The pair main, subordinate comes to mind, but again, main_fd might be taken and main, subordinate carry the same overtones as master, slave, which is what you are trying to avoid in the first place.

For a change that is easy to maintain for current and future Python maintainers and eliminates all possibility of confusion for end users when they are referring to documentation, we must use something that is:

• not already taken to almost absolutely eliminate any chances of variable name collision while
• maintaining that the initials m and s stay the same.

---

Edit:

Some data: about naming pty-pairs; apart from the file/project/module/documentation-local confusion that inevitably will arise due to using process-specific terminology parent/child; networking-specific terminology server/client, and generic terms main, primary/seconday, here is some global data directly generated using script at the bottom of this post:

Current timezone and date: Sun Jan 21 06:17:13 AM CET 2024
Current directory: /tmp/tmp.21-Jan-2024+05:14:25.JZlA6tqeCz
Temporarily moving into directory: /tmp/tmp.uZECXBwUzE

Cloning CPython repository...

Cloning into 'cpython'...
remote: Enumerating objects: 1004157, done.
remote: Counting objects: 100% (690/690), done.
remote: Compressing objects: 100% (425/425), done.
remote: Total 1004157 (delta 428), reused 448 (delta 265), pack-reused 1003467
Receiving objects: 100% (1004157/1004157), 549.31 MiB | 5.16 MiB/s, done.
Resolving deltas: 100% (804152/804152), done.

Moving into CPython repository...
/tmp/tmp.uZECXBwUzE/cpython

Running grep to count number of occurences of concerned terms:

master: 757 occurences ignoring cases and 727 occurences not ignoring cases
master_fd: 80 occurences ignoring cases and 80 occurences not ignoring cases
slave: 155 occurences ignoring cases and 154 occurences not ignoring cases
slave_fd: 43 occurences ignoring cases and 43 occurences not ignoring cases
parent: 3760 occurences ignoring cases and 3255 occurences not ignoring cases
parent_fd: 0 occurences ignoring cases and 0 occurences not ignoring cases
child: 5703 occurences ignoring cases and 5067 occurences not ignoring cases
child_fd: 3 occurences ignoring cases and 3 occurences not ignoring cases
server: 6237 occurences ignoring cases and 5045 occurences not ignoring cases
server_fd: 0 occurences ignoring cases and 0 occurences not ignoring cases
client: 3036 occurences ignoring cases and 2682 occurences not ignoring cases
client_fd: 0 occurences ignoring cases and 0 occurences not ignoring cases
primary: 378 occurences ignoring cases and 346 occurences not ignoring cases
primary_fd: 0 occurences ignoring cases and 0 occurences not ignoring cases
secondary: 38 occurences ignoring cases and 35 occurences not ignoring cases
secondary_fd: 0 occurences ignoring cases and 0 occurences not ignoring cases
main: 25612 occurences ignoring cases and 24847 occurences not ignoring cases
main_fd: 0 occurences ignoring cases and 0 occurences not ignoring cases
second: 4361 occurences ignoring cases and 3987 occurences not ignoring cases
second_fd: 0 occurences ignoring cases and 0 occurences not ignoring cases
mother: 1 occurences ignoring cases and 1 occurences not ignoring cases
mother_fd: 0 occurences ignoring cases and 0 occurences not ignoring cases
(^|[^j])son: 0 occurences ignoring cases and 0 occurences not ignoring cases
(^|[^j])son_fd: 0 occurences ignoring cases and 0 occurences not ignoring cases

Removing CPython repository to free up space in /tmp...

---

#!/bin/sh -e

l="master master_fd slave slave_fd parent parent_fd \
child child_fd server server_fd client client_fd primary \
primary_fd secondary secondary_fd main main_fd second \
second_fd mother mother_fd (^|[^j])son (^|[^j])son_fd"
TMP_DIR_PARENT="${1:-/tmp}"

echo "*Current timezone and date:* $(date)"
echo "*Current directory:* $(pwd)"

# mktemp(1) is not POSIX
TMP_DIR="$(mktemp -d --tmpdir="$TMP_DIR_PARENT")"
echo "*Temporarily moving into directory:* $TMP_DIR"
cd "$TMP_DIR"
echo

echo "*Cloning CPython repository...*"
echo
git clone https://github.com/python/cpython.git
echo
echo "*Moving into CPython repository...*"
cd cpython
echo

echo "*Running grep to count number of occurences of concerned terms:*"
echo
for i in $l
do
# "grep -r" is not POSIX
n="$(grep -r -e $i 2>/dev/null | wc -l)"
m="$(grep -ri -e $i 2>/dev/null | wc -l)"

echo "\`$i\`: \`$m\` occurences ignoring cases and \`$n\` occurences not ignoring cases"
done

echo
echo "*Removing CPython repository to free up space in ${TMP_DIR_PARENT}...*"
cd ..
rm -rf ./cpython