FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-41818: Add os.login_tty() for *nix. by 8vasu · Pull Request #23740 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

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

Closed
8vasu wants to merge 5 commits into
python:mainfrom
8vasu:os_login_tty
ConversationCommits (5)ChecksFiles changed
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 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 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 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 approved these changes Dec 14, 2020
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

8vasu commented Dec 14, 2020

Copy link
Copy Markdown
Contributor Author

@zoulasc Thank you, sir.

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

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 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 Bot added the stale Stale PR or inactive for long period of time. label Jan 17, 2021

8vasu commented Jan 27, 2021

Copy link
Copy Markdown
Contributor Author

Still waiting for core review.

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

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 Bot added the stale Stale PR or inactive for long period of time. label Feb 28, 2021

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 reviewed Aug 27, 2021
View reviewed changes
Comment thread configure
rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext

# check for openpty and forkpty
# check for openpty, login_tty, and forkpty

gpshead Aug 27, 2021

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It looks like you're directly editing configure? this is generated code. edit the input configure.ac instead and regenerate configure. (i suspect might have just forgotten to commit your configure.ac change to the PR branch?)

8vasu Aug 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sir, thank you for the prompt review. I will be honest; I actually did manually edit configure. I will learn how to do it and make the requested changes to configure.ac by Saturday morning.

8vasu Aug 28, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I have edited configure.ac and generated configure using autoconf now. Are there any steps that I might have missed/done improperly? When I generated the configure script, it added the runstatedir option to it which was not there before; please let me know if that was not supposed to happen/how to fix it. Also, it says that this branch has "conflicts that must be resolved"; Modules/clinic/posixmodule.c.h is the conflicting file; what is the best way to fix this problem; I want to avoid the XY problem this time!

gpshead self-assigned this Aug 27, 2021

8vasu commented Nov 15, 2021

Copy link
Copy Markdown
Contributor Author

@gpshead Any updates on this?

asvetlov commented Nov 18, 2021

Copy link
Copy Markdown
Contributor

I see a merge conflict now. Please fix.

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

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 mannequin mentioned this pull request May 5, 2022
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

gpshead gpshead left review comments
+1 more reviewer
zoulasc zoulasc approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

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

Back | FazBrowse Home | New Git URL