bpo-41541: Make pty.spawn set window size

[8vasu]

• Test environment: Linux 4.19.0-9-amd64 Debian 10 GNU/Linux; xterm; bash; Docker python:latest/3.8.5-buster.
• Bug addressed: window size is not set by pty.spawn().
• How to reproduce issue: Use "stty -F rows x cols y" to change window size.
• Issue description: Without the patch, when the number of columns is very small, the output of "ls" is hard to visually parse. After the patch is applied, the output of "ls" is rendered as expected.
• Patch summary: make pty.spawn set window size ( TIOCSWINSZ ).

https://bugs.python.org/issue41541

[ethanfurman]

I don't see any new tests. There should be at least one that fails before the new code is applied which then passes after it is applied.

The current tests are in Lib/test/test_pty.py.

[8vasu]

Sir, thank you for taking the time to review this. I understand. While this was originally meant to be applied, now it is merely a sample. I have made several other improvements, and the final result is here: https://github.com/8vasu/pypty2

The final result has conflicts with this patch. While there is nothing wrong with this patch, it should not be applied. However, I still suggest that this tiny change be used as a starting point to get a sense of what can happen if window resizing is not present. I will add a test now. I will also reupload the png images to bpo.

[8vasu]

I have added the before and after screenshots and the short example that I had used to test this to the BPO issue. This example is from the docs ( see https://docs.python.org/3/library/pty.html ). I took the screenshots to make it easy for the reviewer: this is a visual change. Please follow these steps in indicated order to reproduce the issue:

1. Resize your xterm window to be very narrow [ see before.png ]
2. Start script.py
3. Type ls

The output is expected to be wrapped around the edges of the xterm window, making it hard to visually parse.
Applying this patch should fix this issue. Alternatives to resizing terminal emulator X window: use tmux to split pane; use ansi-term in GNU Emacs and resize the ansi-term window.

Note that this patch does NOT handle SIGWINCH. It merely sets initial window size for slave. Therefore, if the xterm window is resized AFTER script.py is started, this patch will not resize slave accordingly. I have added that functionality to pypty2. More rigorous stty-based tests will be added to pypty2 if you find this smaller change useful. These changes are very useful if one uses a tiling window manager.

[8vasu]

@ethanfurman

May I please have a short chat session with you on freenode #python-dev sometime?

[ethanfurman]

Unfortunately I have never learned IRC.

If I understand what you would like to do, which is to simply replace the current pty with your pypty2 package, I think the first step is going to be to add a bunch of tests to the current pty module. There should be a test for everything that the current pty does correctly. Once you have that, it will be more reasonable to substitute yours in because we can verify that everything that used to work, still does.

You should also add tests for the known problems that the current pty has, and guard them with @expectedFailure.

I know it's a big task, but doing it would prove a couple things:

• you know what you're doing ;-)
• you are committed to being the maintainer

Okay, apparently I am now registered on freenode. Feel free to look me up!

[aeros]

From my understanding, writing a test for this PR seems to mostly be a matter of creating a terminal from STDIN with a specified window size, and then reading the FD's (slave_fd) window size to ensure it's the same after it's spawned. Since pty.spawn() doesn't appear to expose the FD for the terminal window, you would write the tests against login_tty(). Alternatively, you could create a mock version of pty.spawn() that exposes the FD or allows you to pass a pair created from os.opentty() rather than within pty.spawn() (purely for testing purposes, within test_pty -- this could be potentially written as a nested function within something like test_spawn_winsize()).

TL;DR the test(s) should somehow address the change that pty.spawn() now sets window size based on STDIN.

(I'm very far from an expert with pty, but hopefully the above gives you a general idea of what the appropriate tests might involve. If/when Thomas (@Yhg1s) gets around to reviewing the PR, he could have more detailed feedback.)

Edit: For additional context, I left this review comment based on what the OP wrote in core-mentorship, they seemed to be seeking some guidance on how the unit test(s) should be written for this change.

[8vasu]

@ethanfurman @aeros This was very helpful. I (we :D ?) can work on this during weekends.

Note: This pull request adds login_tty() to pty because I wanted to avoid editing multiple files. It does not belong there. In pypty2, the pty module appropriately imports login() from tty; I added 3 new functions to the latter.

[8vasu]

Here you go: https://bugs.python.org/issue41818

I have started doing my homework on unittest. I am tempted to go the mock function route for all pty.spawn() tests. Is there any particular time during next weekend when you will be available on IRC #python-dev? Thank you for your patience.

[aeros]

@ethanfurman Would the mock function route (as I suggested earlier) be reasonable in the unit test(s) for this specific change? Just wanted to double check before the author invests too much time into it, since I have not worked with the internals of tty previously.

[ethanfurman]

The purpose of mock is to be a stand-in for supporting parts of a test that aren't actually the target of the tests.

For example, if you have a function that returns data from a database using a complex query then your test should be verifying that the created query is correct, so you would use mock to simulate the database connection.

In this case the target of the test is the window itself, and its size -- so a test needs to be able to create the window, and then retrieve information about it, and mock won't help us with that.

[8vasu]

@ethanfurman I get it. Upon request of @aeros, I will use modified terminology for the pty ends. I will use mother/son. I have to move the son window size setting/SIGWINCH functionality to pty2.openpty() itself. It would also be a good idea to add that functionality to pty2.fork(). Then pty2.spawn() can simply call pty2.fork(). This should actually increase functionality and code reuse. Also, that way testing should become simpler: termios and winsize tests for son will be added to the existing pty.openpty() test. The only additional tests that need to be added to pty2.spawn() are the ones that deal with signal blocking and the hang issue on non-Linux platforms. Expect some of this progress to happen on next Saturday. If you have time to spare on next Saturday, then we can have a short chat on IRC :) Let me know the particular time.

[ethanfurman]

Cool. Instead of mother/son, though, use parent/child.

If it looks like I'll time on Saturday I'll post a comment here.

[8vasu]

Haha. Got it! The reason why I used mother/son was that the initials still stay the same. If it has to be gender neutral, then we should try to come up with some other terminology that does not involve a human relationship at all. We should avoid using parent/child ( or server/client ) at all costs because that will cause massive amount of confusion in a project that already uses these terms in some other context. For example, pty uses parent/child to refer to processes. Using parent/child to refer to the pty fds will make search and replace much harder. Also, instead of just writing "master", we will complicate our lives by having to write "parent end of pty" each time. We cannot use source/sink because that does not suggest a correct relationship; both the ends can serve as data sources/sinks depending on situation. The original terminology of master/slave is very similar to controller/robot; in fact, "robot" is from Czech "robota", which means "forced labor". As a result, controller/robot is not an option. I was actually thinking about using remote/tdev or remote/pterm or remote/ptty or something along those lines. This is appropriate because the slave end is always supposed to be a tty device [ isatty(slave_fd) ] and the master end is something that can be used by the parent process to control said device.

[aeros]

We should avoid using parent/child ( or server/client ) at all costs because that will cause massive amount of confusion in a project that already uses these terms in some other context. For example, pty uses parent/child to refer to processes. Using parent/child to refer to the pty fds will make search and replace much harder.

Why not call it something like parent_fd/child_term? The other could be changed to parent_proc and child_proc as well, if needed (assuming it's not backwards incompatible). I think the main issue is that "parent" and "child" are used to describe too many different things, but that can be addressed by making the variables more specific to the use case.

That being said, I think either remote/pterm (as suggested above) or controller/pterm would also work. I'll leave it up to @ethanfurman since my experience with pty in general is likely far less. :-)

The original terminology of master/slave is very similar to controller/robot; in fact, "robot" is from Czech "robota", which means "forced labor". As a result, controller/robot is not an option.

Not to go too deep into the terminology rabbit hole, but I think it's perfectly reasonable to base the connotation of a term in a project from the primary language it uses (English in this case) rather than pursuing every culture's. Otherwise, the number of useful analogy terms starts to become very limited to an extent that I would consider to be impractical for ease of communication. So, I'm not personally opposed to controller/robot, when it fits.

[8vasu]

Thank you for the feedback @aeros. As you said, not to go too deep into the terminology rabbit hole, but here is a final suggestion toward which I am inclined: tcntl/tdev. They are short and suggestive.

tcntl = "terminal controller" is appropriate because

1. Inspired by fcntl, which is also a member of the Unix family
2. Should be shorter than controller and control. I am following https://www.kernel.org/doc/html/v4.10/process/coding-style.html#naming
3. ctl and cntl will make search and replace slightly harder ( ioctl and fcntl )
4. ctrl could work, but then 1. is not satisfied anymore
5. Often the term "master end of pty" is used; however, "remote end" wrongly suggests large physical separation

tdev = "terminal device" is appropriate because

1. Suggests that it is an actual text terminal device [ isatty(any_slave_fd) ]
2. pterm and ptty are a little confusing since they sound almost like pty

If @ethanfurman @aeros like these terms, then I will start to use them in pypty2 :)

[8vasu]

@ethanfurman @aeros

Updates: now pty2.openpty() and pty2.fork() accept a termios and a winsize as optional arguments, just like openpty(3) and forkpty(3). As mentioned earlier, this will enable us to write window resizing tests against pty2.openpty() instead of having to deal with the more complicated pty2.spawn(). Also, a very convenient tty.getwinsz() has been added! The unittests have not been written yet; that is the goal of next Saturday. While I feel like writing the tests will not be very difficult, I can benefit immensely from your opinion. For example, what would be an acceptable way to write the *BSD hang test? If either of you can find time for a short chat during next weekend, please let me know. In any case, I will work on the project again on next Saturday.

Edit: the terminology has not been changed yet.

[8vasu]

@ethanfurman @aeros

Updates: thank you for being patient. I have added the main tests to pypty2: slave termios, slave winsize, and FreeBSD hang. Do you prefer a pull request on GitHub or do you want me to upload pypty2 to bpo first?

Edit: I can make a pull request/upload the files on next Saturday.

[8vasu]

@ethanfurman @aeros

Sir, I have created the first pull request [ #22962 ]. Thank you very much for making me do this :) I now have some experience with python's unittest library! To make it easier for you, this pull request only includes the tests from pypty2; I will make subsequent pull requests based on your suggestions. The new tests are

1. convert the test for pty.master_open() to a test for pty.openpty()
2. checking slave termios
3. checking slave winsize
4. checking FreeBSD hang

As you suggested, I have added @expectedfailure for no. 2 and no. 3 and a custom @expectedFailureOnBSD for no. 4.

Edit: @expectedfailure has been changed to @expectedFailureIfStdinIsTTY since the automated tests on GitHub were failing otherwise: their stdin is not a tty. Please run the tests on a tty to achieve desired results :)

[plaffitt]

I need this fix in a project that I'm working on. What is missing to merge the branch?

[8vasu]

This is being done in a series of PRs. I do not want to make a hasty fix for this. To expedite the process can you please help review some of my PRs? Please see bpo-41818. bpo-41541 is abandoned. Thank you.

…

On Wed, Apr 27, 2022, 3:10 AM Paul Laffitte ***@***.***> wrote: I need this fix in a project that I'm working on. What is missing to merge the branch? — Reply to this email directly, view it on GitHub <#21861 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQDGGD5JILUV6SJKUD25DPLVHDY7RANCNFSM4P6V42AQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[plaffitt]

Ok, I understand. Thanks for the quick answer. I took a look at your PRs, but I don't really know what could I do to help. I never contributed to python before and it looks like some people are already reviewing them, so I feel I will not be very useful in this context 😅

[8vasu]

Thank you for understanding :) I am trying my best to get this done. The python core devs (I am not one of them) have limited time; they are also doing their best to help us.

…

On Thu, Apr 28, 2022, 12:28 PM Paul Laffitte ***@***.***> wrote: Ok, I understand. Thanks for the quick answer. I took a look at your PRs, but I don't really know what could I do to help. I never contributed to python before and it looks like some people are already reviewing them, so I feel I will not be very useful in this context 😅 — Reply to this email directly, view it on GitHub <#21861 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQDGGD6JCZS76DGP2WOECEDVHLDCHANCNFSM4P6V42AQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[encukou]

While this was originally meant to be applied, now it is merely a sample.

I will close the PR and issue. If the situation changed, please reopen (or comment, if you don't have permission).

---

In case it is reopened:

This will need docs and tests.

Let's keep inclusive terminology for another PR -- it's important, but also complicated, orthogonal to the functionality, and best discussed with another group of people (see PEP-732). For now, be consistent with terminology that's already in the docs, and in the man pages of the POSIX API we're wrapping: master/slave.

[8vasu]

@encukou Thank you for closing this.