GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-41818: Add termios.tcgetwinsize(), termios.tcsetwinsize(). Update docs. by 8vasu · Pull Request #23686 · python/cpython

Conversation

@8vasu

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

Uh oh!

There was an error while loading. Please reload this page.

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 [email protected]

https://bugs.python.org/issue41818

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
nmeum and ambv reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@8vasu
Add termios.tcgetwinsize(), termios.tcsetwinsize(). Update docs.
78aaac7 
Signed-off-by: Soumendra Ganguly <[email protected]>
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Dec 8, 2020
@bedevere-bot bedevere-bot added the awaiting review label Dec 8, 2020
@8vasu
Add news, clinic generated code.
0553063 
Signed-off-by: Soumendra Ganguly <[email protected]>
@8vasu
8vasu force-pushed the tty_winsize branch from a2824f9 to 0553063 Compare December 8, 2020 01:15
@8vasu 8vasu mentioned this pull request Dec 8, 2020
bpo-41818: Make additions to the tty module #23546
Closed
@8vasu

8vasu commented Dec 8, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@8vasu
Improve termios.tcsetwinsize().
0d2d35b 
Signed-off-by: Soumendra Ganguly <[email protected]>
@8vasu 8vasu mentioned this pull request Dec 11, 2020
bpo-41818: Add os.login_tty() for *nix. #23740
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zoulasc
zoulasc reviewed Dec 14, 2020
View reviewed changes
Comment thread Modules/termios.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
8vasu added 2 commits December 13, 2020 22:15
@8vasu
Add TIOCGSIZE support to termios.tcgetwinsize()
628b7dc 
Signed-off-by: Soumendra Ganguly <[email protected]>
@8vasu
Add TIOCSSIZE support to termios.tcsetwinsize()
6ef8a29 
Signed-off-by: Soumendra Ganguly <[email protected]>
zoulasc
zoulasc approved these changes Dec 14, 2020
View reviewed changes

@zoulasc zoulasc left a comment

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Dec 14, 2020
@8vasu

8vasu commented Dec 14, 2020

Copy link
Copy Markdown
Contributor Author

@zoulasc Thank you for your time.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@8vasu
The SunOS struct ttysize had fields of type "int"; update termios.tcg…
91374ea 
…etwinsize().

Signed-off-by: Soumendra Ganguly <[email protected]>
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Feb 28, 2021
gpshead
gpshead requested changes Mar 15, 2021
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/termios.c Outdated
}

PyObject *v;
if (!(v = PyList_New(2))) {

@gpshead gpshead Mar 15, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

When returning a constant number of values, it is more typical to use a tuple than a list. PyTuple_New and equivalent item setting calls. also update the docstrings in the comment above and on the next function to mention "two item tuple" and use ().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@8vasu 8vasu Aug 12, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@gpshead Sir, thank you for the review. I should mention that I used PyList_New() instead of PyTuple_New() because I wanted this function to resemble the other low level function termios.tcgetattr(), which also returns a list. Do you confirm this change?

Added: I should have been more specific: termios.tcgetattr() also returns a list of fixed length, and that cannot (?) be changed (backward compatibility?). I was wondering if termios.tcgetwinsize() should also behave similarly.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@8vasu 8vasu Aug 14, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I ended up making the changes!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/termios.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting core review labels Mar 15, 2021
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead gpshead self-assigned this Mar 15, 2021
@gpshead gpshead added the type-feature A feature request or enhancement label Mar 15, 2021
@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 :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@8vasu
termios.tcgetwinsize() and termios.tcsetwinsize() should return/accep…
bb0b2b1 
…t two-item tuples instead of lists.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Aug 14, 2021
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot
bedevere-bot requested a review from gpshead August 14, 2021 06:38
@gpshead
Refactor tcsetwinsize to share common code and accept any two item se…
eb0be09 
…quence, with overflow checking.
gpshead
gpshead reviewed Aug 27, 2021
View reviewed changes
Comment thread Modules/termios.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
parenthesis on my if statements :)
5ab0d4c
gpshead
gpshead approved these changes Aug 27, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Aug 27, 2021
@gpshead gpshead removed the stale Stale PR or inactive for long period of time. label Aug 27, 2021
gpshead
gpshead approved these changes Aug 27, 2021
View reviewed changes
@gpshead
gpshead merged commit ae224bb into python:main Aug 27, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label Aug 27, 2021
gpshead
gpshead reviewed Aug 27, 2021
View reviewed changes
Comment thread Doc/library/termios.rst
output, :const:`TCIOFF` to suspend input, or :const:`TCION` to restart input.


.. function:: tcgetwinsize(fd)

@gpshead gpshead Aug 27, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

one follow up PR as I forgot to fix it up here, these need versionadded tags. I'll take care of it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@8vasu 8vasu Aug 28, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@gpshead Thank you for making the improvements!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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
Update ACKS #27988
Merged
@ambv

ambv commented Aug 27, 2021

Copy link
Copy Markdown
Contributor

Awesome to see this landed 👏🏻👏🏻👏🏻
gpshead and 8vasu reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@8vasu 8vasu mentioned this pull request Nov 20, 2021
bpo-41818: Add os.login_tty() for *nix. #29658
Merged
@8vasu 8vasu mannequin mentioned this pull request May 5, 2022
Lib/pty.py major revision #85984
Open
@mattip mattip mentioned this pull request Nov 11, 2024
termios.tcget/setwinsize pypy/pypy#5108
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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@8vasu @bedevere-bot @ambv @gpshead @zoulasc @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.