| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
@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 |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Duplicate
Sorry, something went wrong.
There was a problem hiding this comment.
As you suggested, I will use hasattr() for this instead.
Sorry, something went wrong.
There was a problem hiding this comment.
Explicit check for hasattr(termios, "TIOCSWINSZ") or something like this is better than except NameError.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
I am working on this.
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
How about mkecho()? See next section.
Sorry, something went wrong.
There was a problem hiding this comment.
Sounds good
Sorry, something went wrong.
There was a problem hiding this comment.
enable_raw() maybe if we agree with enable_echo()?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
How about mkraw() instead of makeraw()? Reasoning: mkecho() seems better than makeecho().
Sorry, something went wrong.
There was a problem hiding this comment.
Ok
Sorry, something went wrong.
There was a problem hiding this comment.
I have changed the names to cfmakeraw(), cfmakeecho(), ... so that they conform to standard naming convention.
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
There was a problem hiding this comment.
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).
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Please see #23740 for my implementation of os.login_tty().
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@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.) |
Sorry, something went wrong.
|
@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:
try:
from termios import TIOCGWINSZ
...
except ImportError:
...
I did this to avoid import termios, since we already have a from termios import * which cannot be removed because of backward compatibility reasons. I understand that from termios import * and import termios can coexist; it just felt a little strange doing that :D If you think that using import termios + hasattr() is the better option, then let me know; I will make the changes.
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! |
Sorry, something went wrong.
|
Thanks for making the requested changes! @asvetlov: please review the changes made to this pull request. |
Sorry, something went wrong.
|
Could we return a namedtuple from getwinsize? |
Sorry, something went wrong.
There was a problem hiding this comment.
I'm not sure what this line is doing exactly? Maybe add a comment here?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
|
@asvetlov @jonathanslenders I have made the requested changes; please review again. |
Sorry, something went wrong.
|
Thanks for making the requested changes! @asvetlov: please review the changes made to this pull request. |
Sorry, something went wrong.
|
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 class winsize, tty.login_tty() etc) by making a force-push. 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 ICRNL cleared. The document is about "xcurses"; however, I checked using stty that even the python curses.cbreak() clears ICRNL. I am going to make another updated commit with very detailed comments! 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 IXOFF today: IXON enables Xon/Xoff "software flow control" on output, and IXOFF controls that on the input. Therefore, enabling or disabling IXOFF has no effect on xterm(1) or the ttys provided by the linux kernel (/dev/tty1, /dev/tty2,...); it is probably more relevant when working with a hardware terminal; with a terminal emulator on a fast pc, that does not matter. See https://tldp.org/HOWTO/Text-Terminal-HOWTO-11.html, https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap11.html#tag_11_01_09. Both C-s and C-q are only affected when IXON are turned on or off; if IXANY is also enabled, then any character can be used to resume the flow control as opposed to just C-q (or whatever is the START character). 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 IXON or IXOFF. old comment 5New commit: winsize is now a separate class to enforce nonnegative integer entries. 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: winsize is now a class so that we can force the fields to be nonnegative. As you had hinted before, the ws_xpixel and ws_ypixel might be less useful now. I am not even sure if there exists an OS that still actually uses them. Should we remove them? They are certainly not hurting other functionality at the moment. The "get size" functionality already exists as os.get_terminal_size(): please see os_get_terminal_size_impl() in https://github.com/python/cpython/blob/master/Modules/posixmodule.c. However, that function also supports windows CONIO; further, there is no function for setting window size. I need both "get" and "set" window size functions (only on *nix) to be able to fix some problems with pty: see https://bugs.python.org/issue41818. Further, note that shutil.get_terminal_size() is not appropriate for this since it simply queries the variables LINES and COLUMNS instead of using TIOCGWINSZ. Thank you for you time. Edit: to ease the review process, here are the various implementations of login_tty(3):
Edit2: Here is where I am using tty.login_tty() [ it is still named tty.login() there ]: https://github.com/8vasu/pypty2/blob/master/pty2.py. This will be a part of the next (which is possibly the final) PR in this sequence. 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 7TIOCGWINSZ and TIOCSWINSZ are widely available, but they are not POSIX compliant since POSIX generally does not standardize the ioctl() except for the STREAMS interface: see https://www.austingroupbugs.net/view.php?id=1151 They have decided to include functions tcgetwinsize() and tcsetwinsize() in IEEE Std 1003.1 ("POSIX.1") issue 8 [ the upcoming version of POSIX ]; see https://www.austingroupbugs.net/view.php?id=1151#c3856. NetBSD has already implemented them:
Therefore, now I am highly tempted to change the winsize class back to a couple of functions named tcsetwinsize(), tcgetwinsize() OR at least rename the methods to tcsetwinsize(), tcgetwinsize(). I need some help in making this decision. The benefit of having a class is that we can then have named attributes AND enforce nonnegative integer values for them. 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 pty to this pull request; please look for test_winch(). This test fails when current stdin is a tty and when SIGWINCH is available; it pinpoints the reason why pty.spawn() is unable to dynamically resize the slave window, which leads to gibberish output after, say, the current stdin is resized because of the creation of a tmux pane, or an emacs window in a frame while using ansi-term, or when resizing the GUI [X/Wayland] terminal window. See before and after pictures here: https://bugs.python.org/file49455/before.png, https://bugs.python.org/file49456/after.png 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. |
Sorry, something went wrong.
… cfmakeecho(), cfmakeraw(), and cfmakecbreak(). The functions setcbreak() and setraw() now return original termios to save an extra tcgetattr() call. Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
|
@jonathanslenders Sir, can you please review this again? I have removed a lot of the code [ winsize and login_tty() ]. Thank you in advance. |
Sorry, something went wrong.
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
|
Still waiting for change review. |
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
|
@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 termios.tcgetwinsize(), termios.tcsetwinsize(), and os.login_tty(). New summary for this PR: add cfmakeraw(), cfmakeecho(), and cfmakecbreak(); make setraw() and setcbreak() return saved termios attributes to avoid extra tcgetattr() calls. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This follows #23536. Also, see #23686, #23740.
Add cfmakeraw(), cfmakeecho(), and cfmakecbreak(); make setraw() and setcbreak() return saved termios attributes to avoid extra tcgetattr() calls.
Post #23686, #23546, #23740 goals:
Notes
Signed-off-by: Soumendra Ganguly soumendraganguly@gmail.com
https://bugs.python.org/issue41818