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 — bpo-41818: Fix test_master_read() so that it succeeds on all platforms that either raise OSError or return b"" upon reading from master by 8vasu · Pull Request #23536 · python/cpython

Conversation

@8vasu

@8vasu 8vasu commented Nov 28, 2020 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

This replaces #23533. Build was failing on Solaris since, like the BSDs and Darwin, Solaris does not raise OSError in test_master_read(). Therefore, now we make test_master_read() succeed on all platforms that either raise OSError [ such as Linux ] or return b"" [ such as BSDs, Darwin, and possibly Solaris ] upon reading from master when the slave is closed. Any platform that does not exhibit such behavior can now be detected using test_master_read(). On any given platform, exiting from pty.spawn()'s copy loop depends on our exact knowledge of the behavior of test_master_read() on that platform.

Signed-off-by: Soumendra Ganguly [email protected]

https://bugs.python.org/issue41818

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@8vasu
Fix test_master_read() so that it succeeds on all platforms that
a550369 
either raise OSError or return b"" upon reading from master

Signed-off-by: Soumendra Ganguly <[email protected]>
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 28, 2020
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Nov 28, 2020
@blurb-it
📜🤖 Added by blurb_it.
f994d09
@8vasu

8vasu commented Nov 28, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@asvetlov this follows #23526. My original solution was #23533, but this is much better.

Edit: to be extra safe, I request you to run the buildbots again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kulikjak
kulikjak approved these changes Nov 28, 2020
View reviewed changes

@kulikjak kulikjak left a comment

Copy link
Copy Markdown
Contributor

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.

With this patch, test_pty on Solaris is green again.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Nov 28, 2020
@asvetlov asvetlov added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 28, 2020
@bedevere-bot

bedevere-bot commented Nov 28, 2020

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @asvetlov for commit f994d09 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 28, 2020
@asvetlov

asvetlov commented Nov 28, 2020

Copy link
Copy Markdown
Contributor

to be extra safe, I request you to run the buildbots again.

Done
8vasu reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
asvetlov
asvetlov approved these changes Nov 28, 2020
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Nov 28, 2020
@asvetlov
asvetlov merged commit 74311ae into python:master Nov 28, 2020
@bedevere-bot bedevere-bot removed the awaiting merge label Nov 28, 2020
@asvetlov

asvetlov commented Nov 28, 2020

Copy link
Copy Markdown
Contributor

Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

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

8vasu commented Nov 28, 2020

Copy link
Copy Markdown
Contributor Author

You're welcome!
All reactions

Sorry, something went wrong.

Uh oh!

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

8vasu commented Nov 29, 2020

Copy link
Copy Markdown
Contributor Author

@asvetlov #23546 is the next one. Of course, you can take your time to review it :D The tty module will now need a documentation update; I have not done that yet. Also, please let me know if I need to make tests for the tty module. After we finish working on that one, and before starting to make changes to the pty module, I will add one more nice test for the pty module which will check if pty.spawn() is capable of adjusting slave window size dynamically [ SIGWINCH ].
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@8vasu 8vasu mentioned this pull request Nov 29, 2020
bpo-41818: Make additions to the tty module #23546
Closed
This was referenced Dec 8, 2020
bpo-41818: Add termios.tcgetwinsize(), termios.tcsetwinsize(). Update docs. #23686
Merged
bpo-41818: Add os.login_tty() for *nix. #23740
Closed
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@8vasu @adorilson
bpo-41818: Fix test_master_read() so that it succeeds on all platform…
1779aa3 
…s that either raise OSError or return b"" upon reading from master (pythonGH-23536)

Signed-off-by: Soumendra Ganguly <[email protected]>
@8vasu 8vasu mentioned this pull request Nov 20, 2021
bpo-41818: Add os.login_tty() for *nix. #29658
Merged
@8vasu 8vasu mannequin mentioned this pull request May 5, 2022
Lib/pty.py major revision #85984
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asvetlov asvetlov asvetlov approved these changes
+1 more reviewer
@kulikjak kulikjak kulikjak approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

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 @asvetlov @kulikjak @the-knights-who-say-ni
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.