◐ Shell
reader mode source ↗
Skip to content

bpo-41818: Add termios.tcgetwinsize(), termios.tcsetwinsize(). Update docs.#23686

Merged
gpshead merged 9 commits into
python:mainfrom
8vasu:tty_winsize
Aug 27, 2021
Merged

bpo-41818: Add termios.tcgetwinsize(), termios.tcsetwinsize(). Update docs.#23686
gpshead merged 9 commits into
python:mainfrom
8vasu:tty_winsize

Conversation

@8vasu

@8vasu 8vasu commented Dec 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

This follows #23536. Also, see #23546, #23740.

tcgetwinsize() and tcsetwinsize() are expected to appear in IEEE Std 1003.1 ("POSIX.1") issue 8 [ the upcoming version of POSIX ] as declarations in <termios.h>; see https://www.austingroupbugs.net/view.php?id=1151#c3856.

NetBSD already has them (thanks to user "kre"):

  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

Update: musl libc also has these functions.
Update2: The patches that I had submitted to add tcgetwinsize() and tcsetwinsize() to the FreeBSD libc have been merged. I am still working on getting them included in glibc.

When POSIX.1 issue 8 is released, termios.tcgetwinsize(), termios.tcsetwinsize() can be updated to utilize the native versions.

Post #23686, #23546, #23740 goals:

  1. add test_winsize() to "Lib/test/test_pty.py";
  2. major revision of "Lib/pty.py", which has not happened since the beginning of the millennium.

Signed-off-by: Soumendra Ganguly soumendraganguly@gmail.com

https://bugs.python.org/issue41818

@8vasu
Add termios.tcgetwinsize(), termios.tcsetwinsize(). Update docs. …
78aaac7 
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@8vasu
Add news, clinic generated code. …
0553063 
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@8vasu 8vasu mentioned this pull request Dec 8, 2020
Closed
@8vasu

8vasu commented Dec 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@ethanfurman @aeros @gpshead @asvetlov

Update1: These functions perform ioctl()+TIOCGWINSZ/TIOCSWINSZ when the constants are available; they throw NotImplementedError when they are not available. I have implemented os.login_tty(); please see #23740.

Update2: Added ioctl()+TIOCGSIZE to termios.tcgetwinsize().

Update3: Added ioctl()+TIOCSSIZE to termios.tcsetwinsize().

Update4: minor change: unlike struct winsize, the SunOS struct ttysize has members of type int; please refer to https://sourceware.org/git/?p=glibc.git;a=blob;f=bits/ioctl-types.h;h=474a52b4358bc1b4fd211d43ac96f03b168833b7;hb=HEAD#l107

@8vasu
Improve termios.tcsetwinsize(). …
0d2d35b 
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@8vasu 8vasu mentioned this pull request Dec 11, 2020
Closed
@8vasu

8vasu commented Dec 14, 2020

Copy link
Copy Markdown
Contributor Author

@zoulasc Sir, can you please take a look at the changes that I made to termios.c? termios_tcgetwinsize_impl() and termios_tcsetwinsize_impl() are the functions that I mentioned earlier.

8vasu added 2 commits December 13, 2020 22:15
@8vasu
Add TIOCGSIZE support to termios.tcgetwinsize() …
628b7dc 
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@8vasu
Add TIOCSSIZE support to termios.tcsetwinsize() …
6ef8a29 
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
zoulasc
zoulasc approved these changes Dec 14, 2020
View reviewed changes

@zoulasc zoulasc left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hide comment

LGTM

@8vasu

8vasu commented Dec 14, 2020

Copy link
Copy Markdown
Contributor Author

@zoulasc Thank you for your time.

@8vasu
The SunOS struct ttysize had fields of type "int"; update termios.tcg… …
91374ea 
…etwinsize().

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@github-actions

github-actions Bot commented Jan 26, 2021

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 Jan 26, 2021
@8vasu

8vasu commented Jan 27, 2021

Copy link
Copy Markdown
Contributor Author

Still waiting for core review.

@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jan 28, 2021
@github-actions

github-actions Bot commented Feb 28, 2021

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 label Feb 28, 2021
gpshead
gpshead requested changes Mar 15, 2021
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

I'd ordinarily also ask for unittest coverage... but this is termios which doesn't appear to have any and might be challenging to automate given its purpose is more for interactive things (?). if you have ideas on that front, feel free to add the beginnings of a Lib/test/test_termios.py - but none of us are going to block this PR on that happening. :)

some comments to address below. primarily: use two item tuples instead of lists.

@bedevere-bot

bedevere-bot commented Mar 15, 2021

Copy link
Copy Markdown

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.

@gpshead gpshead self-assigned this Mar 15, 2021
@gpshead gpshead added the type-feature A feature request or enhancement label Mar 15, 2021
11 hidden items Load more…
@8vasu

8vasu commented Aug 12, 2021

Copy link
Copy Markdown
Contributor Author

@gpshead Unfortunately, I am very busy with my PhD math research currently, and I apologize in advance for not having the time to write unittests for termios :(

Fortunately, I had some time to spare during last December, when I had already written unittests for the pty module, and they were merged (#22962, #23536, #23526). The final goal of this PR (and of #23740 , #23546) is to enhance the pty module, the code for which I already have here https://github.com/8vasu/pypty2, and will create a pull request if/when all of the currently open ones are merged so that all the problems in pty are fixed in a systematic fashion :)

@ambv please take a look at https://bugs.python.org/issue41818 if you have time :)

@8vasu

8vasu commented Aug 14, 2021

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again. Thank you for your patience.

@bedevere-bot

bedevere-bot commented Aug 14, 2021

Copy link
Copy Markdown

Thanks for making the requested changes!

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

@bedevere-bot bedevere-bot requested a review from gpshead August 14, 2021 06:38
@gpshead gpshead removed the stale Stale PR or inactive for long period of time. label Aug 27, 2021
@gpshead gpshead merged commit ae224bb into python:main Aug 27, 2021
gpshead added a commit that referenced this pull request Aug 27, 2021
@gpshead
bpo-41818: ++ termios versionadded markers. (GH-27987) …
245f1f2 
overlooked in #23686
@8vasu 8vasu mentioned this pull request Aug 27, 2021
Merged
@ambv

ambv commented Aug 27, 2021

Copy link
Copy Markdown
Contributor

Awesome to see this landed 👏🏻👏🏻👏🏻

@8vasu 8vasu mentioned this pull request Nov 20, 2021
Merged
@8vasu 8vasu mannequin mentioned this pull request May 5, 2022
Open
@mattip mattip mentioned this pull request Nov 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead approved these changes

+1 more reviewer

@zoulasc zoulasc zoulasc approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gpshead gpshead

Labels

type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@8vasu @bedevere-bot @ambv @gpshead @zoulasc @the-knights-who-say-ni