◐ Shell
reader mode source ↗
Skip to content

bpo-41818: Add os.login_tty() for *nix.#23740

Closed
8vasu wants to merge 5 commits into
python:mainfrom
8vasu:os_login_tty
Closed

bpo-41818: Add os.login_tty() for *nix.#23740
8vasu wants to merge 5 commits into
python:mainfrom
8vasu:os_login_tty

Conversation

@8vasu

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

Copy link
Copy Markdown
Contributor

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

os.login_tty() calls the native login_tty() when it is available; otherwise, if setsid() is available and if TIOCSCTTY or ttyname() are available, it runs generic code to emulate login_tty().

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

This was referenced Dec 11, 2020
Closed
Merged
@8vasu

8vasu commented Dec 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@asvetlov Here is the implementation of os.login_tty() that you had suggested. Thank you for being patient.

Edit: This will help us make the code in "Lib/pty.py" and "Lib/test/test_pty.py" much simpler. cc: @ethanfurman @aeros @gpshead

@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 posixmodule.c? os_login_tty_impl() is the function that I mentioned earlier. Thank you for your kind help.

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, sir.

@8vasu
Soumendra Ganguly: add os.login_tty() for *nix. …
ddc3219 
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@8vasu

8vasu commented Dec 17, 2020

Copy link
Copy Markdown
Contributor Author

There were 2 commits that only changed the wording in the news file; rebased to have a single commit.

@github-actions

github-actions Bot commented Jan 17, 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 17, 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
@8vasu

8vasu commented Aug 27, 2021

Copy link
Copy Markdown
Contributor Author

@gpshead This follows #23686; I need to update Modules/clinic/posixmodule.c.h but that can be done quickly using argument clinic when necessary. I request you to please review this if/when you have time.

@gpshead gpshead self-assigned this Aug 27, 2021
@8vasu

8vasu commented Nov 15, 2021

Copy link
Copy Markdown
Contributor Author

@gpshead Any updates on this?

@asvetlov

asvetlov commented Nov 18, 2021

Copy link
Copy Markdown
Contributor

I see a merge conflict now. Please fix.

@8vasu
Fix merge conflict in Modules/clinic/posixmodule.c.h …
ec3b44b 
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@8vasu 8vasu closed this Nov 20, 2021
@8vasu 8vasu deleted the os_login_tty branch November 20, 2021 07:20
@8vasu 8vasu mentioned this pull request Nov 20, 2021
Merged
@8vasu

8vasu commented Nov 20, 2021

Copy link
Copy Markdown
Contributor Author

@asvetlov @gpshead The merge conflict in posixmodule.c.h was easy to fix but the ones in configure were a little harder; I realized that I had unknowingly added some unnecessary changes to configure since I did not have the "autoconf-archive" package installed; I decided to start fresh instead of rebasing; please check #29658. All other code remains unchanged.

@8vasu 8vasu mannequin mentioned this pull request May 5, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead left review comments

+1 more reviewer

@zoulasc zoulasc zoulasc approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gpshead gpshead

Labels

awaiting core review stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

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