Skip to content
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

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

Closed
wants to merge 5 commits into from
Closed

Conversation

8vasu
Copy link
Contributor

@8vasu 8vasu commented Dec 11, 2020

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

@8vasu
Copy link
Contributor Author

8vasu commented Dec 11, 2020

@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
Copy link
Contributor Author

8vasu commented Dec 14, 2020

@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.

Copy link

@zoulasc zoulasc left a comment

Choose a reason for hiding this comment

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

LGTM

@8vasu
Copy link
Contributor Author

8vasu commented Dec 14, 2020

@zoulasc Thank you, sir.

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@8vasu
Copy link
Contributor Author

8vasu commented Dec 17, 2020

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

@github-actions
Copy link

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
Copy link
Contributor Author

8vasu commented Jan 27, 2021

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
Copy link

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 Feb 28, 2021
@8vasu
Copy link
Contributor Author

8vasu commented Aug 27, 2021

@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.

@@ -12735,7 +12735,7 @@ $as_echo "no" >&6; }
fi
rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext

# check for openpty and forkpty
# check for openpty, login_tty, and forkpty
Copy link
Member

Choose a reason for hiding this comment

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

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 gpshead self-assigned this Aug 27, 2021
@8vasu
Copy link
Contributor Author

8vasu commented Nov 15, 2021

@gpshead Any updates on this?

@asvetlov
Copy link
Contributor

I see a merge conflict now. Please fix.

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@8vasu 8vasu deleted the os_login_tty branch November 20, 2021 07:20
@8vasu
Copy link
Contributor Author

8vasu commented Nov 20, 2021

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review stale Stale PR or inactive for long period of time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants