GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-85984: Utilize new "winsize" functions from termios in pty tests. by 8vasu · Pull Request #101831 · python/cpython

Conversation

@8vasu

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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 [email protected]

  • Issue: Lib/pty.py major revision #85984

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added the awaiting review label Feb 11, 2023
@bedevere-bot bedevere-bot mentioned this pull request Feb 11, 2023
Lib/pty.py major revision #85984
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
gh-85984: Major revision of the pty library. #101833
Closed
@8vasu
8vasu force-pushed the test_pty_winsize branch from edc9762 to 1bad1ca Compare February 11, 2023 21:32
@8vasu

8vasu commented Feb 11, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
8vasu
8vasu commented Feb 11, 2023
View reviewed changes
Comment thread 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):

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gpshead gpshead Feb 11, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@8vasu 8vasu Feb 12, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@8vasu
Utilize new functions termios.tcgetwinsize() and termios.tcsetwinsize…
63fb4d8 
… in test_pty.py.

Signed-off-by: Soumendra Ganguly <[email protected]>
@8vasu
8vasu force-pushed the test_pty_winsize branch from 4821827 to 63fb4d8 Compare February 11, 2023 22:34
@blurb-it
📜🤖 Added by blurb_it.
5082b4a
gpshead
gpshead reviewed Feb 11, 2023
View reviewed changes
Comment thread Lib/test/test_pty.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
gpshead
gpshead reviewed Feb 11, 2023
View reviewed changes
Comment thread Lib/test/test_pty.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
gpshead
gpshead reviewed Feb 11, 2023
View reviewed changes
Comment thread Lib/test/test_pty.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
gpshead
gpshead reviewed Feb 11, 2023
View reviewed changes
Comment thread Lib/test/test_pty.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
gpshead
gpshead reviewed Feb 11, 2023
View reviewed changes
Comment thread Lib/test/test_pty.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
gpshead
gpshead requested changes Feb 11, 2023
View reviewed changes
Comment thread 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):

@gpshead gpshead Feb 11, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Feb 11, 2023
@bedevere-bot

bedevere-bot commented Feb 11, 2023

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead gpshead self-assigned this Feb 11, 2023
8vasu added 2 commits February 11, 2023 20:07
@8vasu
Remove import_module("fcntl") and test__copy_eof_on_all().
cd0742c 
Signed-off-by: Soumendra Ganguly <[email protected]>
@8vasu
Merge branch 'test_pty_winsize' of https://github.com/8vasu/cpython
4ae71ad 
Get the News entry.
@8vasu

8vasu commented Feb 12, 2023

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Feb 12, 2023
@bedevere-bot

bedevere-bot commented Feb 12, 2023

Copy link
Copy Markdown

Thanks for making the requested changes!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot
bedevere-bot requested a review from gpshead February 12, 2023 02:44
@gpshead

gpshead commented Feb 12, 2023

Copy link
Copy Markdown
Member

!buildbot wasm32
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Feb 12, 2023

Copy link
Copy Markdown

🤖 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
restore the import_module("fcntl") thing to avoid crashing on wasm32
aaff7ce
@gpshead

gpshead commented Feb 12, 2023

Copy link
Copy Markdown
Member

!buildbot wasm32
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Feb 12, 2023

Copy link
Copy Markdown

🤖 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead

gpshead commented Feb 12, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
gpshead merged commit da2fb92 into python:main Feb 12, 2023
@bedevere-bot bedevere-bot removed the awaiting change review label Feb 12, 2023
@8vasu

8vasu commented Feb 12, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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
gh-85984: Add POSIX pseudo-terminal functions. #102413
Merged
@ambv ambv added the needs backport to 3.11 only security fixes label May 19, 2023
@miss-islington

miss-islington commented May 19, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington

miss-islington commented May 19, 2023

Copy link
Copy Markdown
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

miss-islington commented May 19, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented May 19, 2023

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 <[email protected]>
Signed-off-by: Soumendra Ganguly <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
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 <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@8vasu @bedevere-bot @gpshead @miss-islington @ambv
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.