-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
bpo-41818: Make additions to the tty module #23546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@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 |
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 suggest esablish public function names first, update the tty
module documentation, and make the final review.
@jonathanslenders I very respect your work on https://github.com/prompt-toolkit/python-prompt-toolkit
Maybe you want to review this (and follow-up) PR? Maybe you can suggest something? I really like to hear voices from experts in PTY/TTY area.
Lib/tty.py
Outdated
mode[CFLAG] = mode[CFLAG] | CS8 | ||
mode[LFLAG] = mode[LFLAG] & ~(ECHO | ICANON | IEXTEN | ISIG) | ||
try: | ||
CHECK_WINSZ = TIOCGWINSZ |
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.
Duplicate
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.
As you suggested, I will use hasattr()
for this instead.
Lib/tty.py
Outdated
If fd is not a descriptor of | ||
a tty, then OSError is raised.""" | ||
ioctl(fd, TIOCSWINSZ, winsz) | ||
except NameError: |
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.
Explicit check for hasattr(termios, "TIOCSWINSZ")
or something like this is better than except NameError
.
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.
It did feel like there had to be a better way of doing this. I did not like the except NameError
either. Thank you so much for this suggestion.
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 have one concern. To be able to use hasattr(termios, "TIOCSWINSZ")
, I think I need to import termios
. Am I correct about this? Since we already have from termios import *
, which cannot be removed because of backward compatibility, is it good practice to also have an import termios
there just for that single purpose? I was thinking that we could use
try:
from termios import TIOCGWINSZ, TIOCSWINSZ
...
except ImportError:
...
which seems to be an explicit check as well.
Lib/tty.py
Outdated
ioctl(fd, TIOCSWINSZ, winsz) | ||
except NameError: | ||
HAVE_WINSZ = False | ||
getwinsz = None |
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 think better to make a stub that raises NotImplementedError
is better than setting the function name to None
.
In std lib we never use this pattern.
Sometimes we don't declare unsupported function at all, but in this case we usually mark in docs: "Availability: Windows" or "Availability: Linux kernel >= 4.5 or glibc >= 2.27".
I think here is better to support stubs that always fail.
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 am working on this.
Lib/tty.py
Outdated
getwinsz = None | ||
setwinsz = None | ||
|
||
def mode_echo(mode, echo=True): |
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.
mode_echo
is not the best name I guess.
Looking on the function body I can suggest renaming with enable_echo()
.
enable_echo(mode)
and enable_echo(mode, False)
.
Maybe others have better idea?
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.
enable_echo()
does sound better than mode_echo()
. If you agree with makeraw()
instead of mode_raw()
, then we could also change this to makeecho()
; however, I think that enable_echo()
sounds better than either. I am leaning toward enable_raw()
, and the updated commit will use that name unless someone else suggests something better.
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.
How about mkecho()
? See next section.
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.
Sounds good
Lib/tty.py
Outdated
else: | ||
mode[LFLAG] &= ~ECHO | ||
|
||
def mode_raw(mode): |
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.
enable_raw()
maybe if we agree with enable_echo()
?
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.
Yes, that was a temporary name. This is supposed to function like cfmakeraw(3)
. While enable_raw()
does sound better than mode_raw()
, we could also change it to tty.makeraw()
if you permit me to do so, and if there are no licensing issues.
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.
How about mkraw()
instead of makeraw()
? Reasoning: mkecho()
seems better than makeecho()
.
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.
Ok
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 have changed the names to cfmakeraw()
, cfmakeecho()
, ... so that they conform to standard naming convention.
Lib/tty.py
Outdated
tcsetattr(fd, when, new) | ||
return mode | ||
|
||
def login(fd): |
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.
The naming again.
For me, the function does not login but makes the calling process a session leader as you correctly wrote in docstring.
Maybe make_session_leader()
or set_session_leader()
is better?
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.
Perfect! I will use one of those, or a variant of one of those. I agree that tty.login()
was poor choice on my part; I wanted it to sound like login_tty(3)
.
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.
Update: since I had written these a while ago, I had forgotten what this function exactly does. I did some research, and it turns out that this function is actually doing a bit more than making the calling process a session leader.
According to https://pubs.opengroup.org/onlinepubs/9699919799/functions/setsid.html, the setsid()
call in our function is responsible for making the calling process a session leader; moreover
Upon return the calling process shall be the session leader of this new session, shall be the process group leader of a new process group, and shall have no controlling terminal.
Our function is also making fd
the stdin/stdout/stderr and controlling tty of the calling process, which is why it belongs in the tty module.
Nevertheless, as you said, login()
sounds like a bad choice; I completely agree with that. Should I change it to login_tty()
? This function IS actually a clone of login_tty(3)
.
Additional note: on my Debian system, man login_tty
says
These are BSD functions, present in glibc. They are not standardized in POSIX.
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.
Maybe we should add pty.login_tty()
function instead which calls the native login_tty(3)
?
It can do some platform-specific things which we don't want to learn and reimplement in Python code.
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.
Sir, this sounds like a great idea. I have removed tty.login_tty()
from this pull request. I have also done restructuring/cleanup. Please check the comments on the diff.
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.
Please see #23740 for my implementation of os.login_tty()
.
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 |
@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. |
16a0c70
to
ce6c808
Compare
@asvetlov : Thanks! A few thoughts:
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:
I did this to avoid
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? |
Lib/tty.py
Outdated
try: | ||
ioctl(STDOUT_FILENO, TIOCSCTTY) | ||
except (NameError, OSError): | ||
tmp_fd = os.open(os.ttyname(STDOUT_FILENO), os.O_RDWR) |
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'm not sure what this line is doing exactly? Maybe add a comment here?
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.
Here is the source of login_tty(3)
: https://sourceware.org/git/?p=glibc.git;a=blob;f=login/login_tty.c
The
except (NameError, OSError):
tmp_fd = os.open(os.ttyname(STDOUT_FILENO), os.O_RDWR)
in our code is equivalent to their
{
/* This might work. */
char *fdname = ttyname (fd);
int newfd;
if (fdname)
{
if (fd != 0)
(void) close (0);
if (fd != 1)
(void) close (1);
if (fd != 2)
(void) close (2);
newfd = open (fdname, O_RDWR);
(void) close (newfd);
}
}
I contacted the glibc people a while ago and this is what they had to say
Since this was added by Roland almost 25 years ago, it seems to be trying to
emulate the TIOCSCTTY for system derived from System V (as indicated by this
stackflow thread [1]).
Back in the day glibc tries to enclose fallbacks to be portable to the various
Unix implementations. Currently this is just a dead code with various
shortcomings (as for a lot of the glibc fallback implementation where proper
kernel support was the correct answer) since neither Linux nor Hurd uses it.
And it seems that Unixes derived from System V also has moved on from this
interface, neither Solaris nor Illumos seems to provide it.
TL;DR: TIOCSCTTY
is available everywhere now. Therefore, I too think that it would be safe to remove the except (NameError, OSError):
block. Should I remove it?
I am going to use tty.login_tty()
[ or whatever we end up naming it ] to replace the fallback code in pty.fork()
, which still uses the old System V approach. tty.login_tty()
will also be used in pty.spawn()
to make it easier for us to implement some new/improved functionality.
Lib/tty.py
Outdated
def login_tty(fd): | ||
"""Makes the calling process a session leader; if fd is a | ||
descriptor of a tty, then that tty becomes the stdin, stdout, | ||
stderr, and controlling tty of the calling process. Closes fd.""" |
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.
This comment implies that fd can either be or not be a descriptor of a tty.
Not sure what would happen if it's not a tty descriptor.
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.
You are right. I will have to rephrase that. Actually, it will fail if fd
is not a tty. Do you think it would be a good idea to instead mention/document what exception would be raised if fd was not a tty?
682510f
to
db202cb
Compare
@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 old comment 1
old comment 2@asvetlov @jonathanslenders I have made the requested changes; please review again. Notes follow.
old comment 3https://pubs.opengroup.org/onlinepubs/7908799/xcurses/intov.html#tag_001_005_002 mentions that cbreak Mode must additionally have old comment 4I compared the following definitions/implementations of "raw mode tty" and "cbreak mode tty": raw
cbreak
I would like to put down what I learned about 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 old comment 5New commit: 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: Edit: to ease the review process, here are the various implementations of
Edit2: Here is where I am using 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
They have decided to include functions
Therefore, now I am highly tempted to change the Additional notes:
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 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. |
6033b85
to
a2d2d1d
Compare
80fbded
to
a639728
Compare
… cfmakeecho(), cfmakeraw(), and cfmakecbreak(). The functions setcbreak() and setraw() now return original termios to save an extra tcgetattr() call. Signed-off-by: Soumendra Ganguly <[email protected]>
a639728
to
a684142
Compare
@jonathanslenders Sir, can you please review this again? I have removed a lot of the code [ |
Signed-off-by: Soumendra Ganguly <[email protected]>
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 New summary for this PR: add |
This follows #23536. Also, see #23686, #23740.
Add
cfmakeraw()
,cfmakeecho()
, andcfmakecbreak()
; makesetraw()
andsetcbreak()
return saved termios attributes to avoid extratcgetattr()
calls.Post #23686, #23546, #23740 goals:
test_winsize()
to "Lib/test/test_pty.py";Notes
cfmakeraw()
is not POSIX compliant; it is available on the BSDs and in glibc. However, POSIX has functions with similar names; for example, considercfgetispeed()
. Please see https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/termios.h.html.test_winch()
will be resubmitted as a part of a later pull request.class winsize
.tty.login_tty()
.Signed-off-by: Soumendra Ganguly [email protected]
https://bugs.python.org/issue41818