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.

Sign up for GitHub

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

Jump to bottom

gh-85984: Utilize new "winsize" functions from termios in pty tests. #101831

Merged
merged 5 commits into from
Feb 12, 2023
Merged

gh-85984: Utilize new "winsize" functions from termios in pty tests. #101831

merged 5 commits into from
Feb 12, 2023

Conversation

8vasu
Copy link
Contributor

@8vasu 8vasu commented Feb 11, 2023 •
edited
Loading

This follows #92365. This is one in a series of PRs aimed at cleaning-up, fixing bugs in, introducing new features in, and updating the code in "Lib/pty.py".

This is a dependency of #101833.

#101832 is another dependency of #101833.

Signed-off-by: Soumendra Ganguly soumendraganguly@gmail.com

@bedevere-bot bedevere-bot mentioned this pull request Feb 11, 2023
Open
@bedevere-bot bedevere-bot added the tests Tests in the Lib/test dir label Feb 11, 2023
@8vasu 8vasu mentioned this pull request Feb 11, 2023
Draft
@8vasu
Copy link
Contributor Author

8vasu commented Feb 11, 2023

@gpshead Thank you for being a major part of this project for so long. This is another PR for you. There is no rush; please take a look at this when you are free.

8vasu
8vasu commented Feb 11, 2023
View reviewed changes
Lib/test/test_pty.py Outdated

# We expect the function to return without error.
self.assertEqual(pty._copy(masters[0]), None)
# def test__copy_eof_on_all(self):
Copy link
Contributor Author

@8vasu 8vasu Feb 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

test__copy_eof_on_all probably does not make sense due to the current structure of the _copy() function. Please correct me if I am wrong.

Copy link
Member

Choose a reason for hiding this comment

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

agreed, this test doesn't make sense as written, please delete it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@8vasu
Utilize new functions termios.tcgetwinsize() and termios.tcsetwinsize…
63fb4d8 
… in test_pty.py.

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
gpshead
gpshead reviewed Feb 11, 2023
View reviewed changes
Lib/test/test_pty.py Outdated Show resolved Hide resolved
gpshead
gpshead reviewed Feb 11, 2023
View reviewed changes
Lib/test/test_pty.py Outdated Show resolved Hide resolved
gpshead
gpshead reviewed Feb 11, 2023
View reviewed changes
Lib/test/test_pty.py Outdated Show resolved Hide resolved
gpshead
gpshead reviewed Feb 11, 2023
View reviewed changes
Lib/test/test_pty.py Outdated Show resolved Hide resolved
gpshead
gpshead requested changes Feb 11, 2023
View reviewed changes
Lib/test/test_pty.py Outdated

# We expect the function to return without error.
self.assertEqual(pty._copy(masters[0]), None)
# def test__copy_eof_on_all(self):
Copy link
Member

Choose a reason for hiding this comment

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

agreed, this test doesn't make sense as written, please delete it.

@bedevere-bot
Copy link

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.

@gpshead gpshead self-assigned this Feb 11, 2023
@8vasu
Copy link
Contributor Author

8vasu commented Feb 12, 2023

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from gpshead February 12, 2023 02:44
@gpshead
Copy link
Member

gpshead commented Feb 12, 2023

!buildbot wasm32

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @gpshead for commit 4ae71ad 🤖

The command will test the builders whose names match following regular expression: wasm32

The builders matched are:

  • wasm32-wasi PR
  • wasm32-emscripten node (dynamic linking) PR
  • wasm32-emscripten node (pthreads) PR
  • wasm32-emscripten browser (dynamic linking, no tests) PR

@gpshead
Copy link
Member

gpshead commented Feb 12, 2023

!buildbot wasm32

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @gpshead for commit aaff7ce 🤖

The command will test the builders whose names match following regular expression: wasm32

The builders matched are:

  • wasm32-wasi PR
  • wasm32-emscripten node (dynamic linking) PR
  • wasm32-emscripten node (pthreads) PR
  • wasm32-emscripten browser (dynamic linking, no tests) PR

@gpshead
Copy link
Member

gpshead commented Feb 12, 2023

(specifically it was the wasm32 encripten node bots that crashed on test_pty)

@gpshead gpshead merged commit da2fb92 into python:main Feb 12, 2023
@8vasu
Copy link
Contributor Author

8vasu commented Feb 12, 2023

@gpshead Thank you for the final change. Some info: maybe the reason why it failed with both the Python fcntl module and the current termios.tc{get/set}winsize() functions is that ioctl()+TIOCGWINSZ/TIOCSWINSZ is problematic on wasm32-emcripten. When I googled "emscripten ioctl", this was the very second link: emscripten-core/emscripten#6463.

carljm added a commit to carljm/cpython that referenced this pull request Feb 13, 2023
@carljm
Merge branch 'main' into inlinecomp2
ae0bd02 
* main:
  pythongh-101810: Remove duplicated st_ino calculation (pythonGH-101811)
  pythongh-92547: Purge sqlite3_enable_shared_cache() detection from configure (python#101873)
  pythonGH-100987: Refactor `_PyInterpreterFrame` a bit, to assist generator improvement. (pythonGH-100988)
  pythonGH-87849: Simplify stack effect of SEND and specialize it for generators and coroutines. (pythonGH-101788)
  Correct trivial grammar in reset_mock docs (python#101861)
  pythongh-101845: pyspecific: Fix i18n for availability directive (pythonGH-101846)
  pythongh-89792: Limit test_tools freeze test build parallelism based on the number of cores (python#101841)
  pythongh-85984: Utilize new "winsize" functions from termios in pty tests. (python#101831)
  pythongh-89792: Prevent test_tools from copying 1000M of "source" in freeze test (python#101837)
  Fix typo in test_fstring.py (python#101823)
  pythonGH-101797: allocate `PyExpat_CAPI` capsule on heap (python#101798)
  pythongh-101390: Fix docs for `imporlib.util.LazyLoader.factory` to properly call it a class method (pythonGH-101391)
@8vasu 8vasu mentioned this pull request Mar 4, 2023
Merged
@ambv ambv added the needs backport to 3.11 only security fixes label May 19, 2023
@miss-islington
Copy link
Contributor

Thanks @8vasu for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry @8vasu and @gpshead, I had trouble checking out the 3.11 backport branch.
Please retry by removing and re-adding the "needs backport to 3.11" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker da2fb9264315dc30ac3012b4dbf5ba76d3f34433 3.11

@ambv ambv added needs backport to 3.11 only security fixes and removed needs backport to 3.11 only security fixes labels May 19, 2023
@miss-islington
Copy link
Contributor

Thanks @8vasu for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-104652 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label May 19, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 19, 2023
@8vasu @gpshead @miss-islington
pythongh-85984: Utilize new "winsize" functions from termios in pty t…
fdcdd65 
…ests. (pythonGH-101831)

Utilize new functions termios.tcgetwinsize() and termios.tcsetwinsize in test_pty.py.

(cherry picked from commit da2fb92)

Co-authored-by: Soumendra Ganguly <67527439+8vasu@users.noreply.github.com>
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
ambv pushed a commit that referenced this pull request May 19, 2023
@miss-islington @8vasu @gpshead
[3.11] gh-85984: Utilize new "winsize" functions from termios in pty …
aaeaf01 
…tests. (GH-101831) (#104652)

Utilize new functions termios.tcgetwinsize() and termios.tcsetwinsize in test_pty.py.

(cherry picked from commit da2fb92)

Co-authored-by: Soumendra Ganguly <67527439+8vasu@users.noreply.github.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpshead gpshead Awaiting requested review from gpshead

Assignees

@gpshead gpshead

Labels
tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@8vasu @bedevere-bot @gpshead @miss-islington @ambv