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

PtyProcess: add NO_CTTY flag by petreeftime · Pull Request #101 · rust-cli/rexpect · GitHub

/ rexpect Public

PtyProcess: add NO_CTTY flag - #101

Closed
petreeftime wants to merge 6 commits into
rust-cli:masterfrom
petreeftime:ptyprocess-fix
ConversationCommits (6)ChecksFiles changed
Closed

PtyProcess: add NO_CTTY flag#101
petreeftime wants to merge 6 commits into
rust-cli:masterfrom
petreeftime:ptyprocess-fix

Conversation

petreeftime commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator

The default behavior of posix_openpt seems to be to replace the controlling terminal for the calling process, and PtyProcess should only change it for child instead. I think this might be a reason why some of the tests were failing non-deterministacally sometimes, although I am not 100% confident in this fix.

Signed-off-by: Petre Eftime petre.eftime@gmail.com

petreeftime commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator Author

bors try

bors Bot added a commit that referenced this pull request Apr 23, 2023
Try #101:
5a374e6

bors Bot commented Apr 23, 2023

Copy link
Copy Markdown
Contributor

try

Build succeeded:

petreeftime commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator Author

@matthiasbeyer so far this passes 100% of the tests.

petreeftime mentioned this pull request Apr 23, 2023
Closed

petreeftime commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator Author

bors try

bors Bot added a commit that referenced this pull request Apr 23, 2023
Try #101:
6ccd1c1

petreeftime commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator Author

Ah. This failed as well.

bors Bot commented Apr 23, 2023

Copy link
Copy Markdown
Contributor

try

Build failed:

petreeftime marked this pull request as draft April 23, 2023 17:59
update to swatinem/rust-cache@v2
2fac169 
rust-cache@v1 seems to generate some deprecation warnings, so better
update to v2.

Signed-off-by: Petre Eftime <petre.eftime@gmail.com>
PtyProcess: add NO_CTTY flag
1f75263 
The default behavior of posix_openpt seems to be to replace the
controlling terminal for the calling process, and PtyProcess should
only change it for child instead. I think this might be a reason why
some of the tests were failing non-deterministacally sometimes, although
I am not 100% confident in this fix.

Signed-off-by: Petre Eftime <petre.eftime@gmail.com>
make new terminal controlling terminal
ab53025 
Signed-off-by: Petre Eftime <petre.eftime@gmail.com>

petreeftime commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator Author

bors try

bors Bot added a commit that referenced this pull request Apr 23, 2023
Try #101:
3e85ca2

bors Bot commented Apr 23, 2023

Copy link
Copy Markdown
Contributor

try

Build failed:

try wait echo
2e292c1 
Signed-off-by: Petre Eftime <petre.eftime@gmail.com>

petreeftime commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator Author

bors try

bors Bot added a commit that referenced this pull request Apr 23, 2023
Try #101:
7f7fb3d

bors Bot commented Apr 23, 2023

Copy link
Copy Markdown
Contributor

try

Build succeeded:

petreeftime commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator Author

bors try

bors Bot added a commit that referenced this pull request Apr 23, 2023
Try #101:
6d9290c

bors Bot commented Apr 23, 2023

Copy link
Copy Markdown
Contributor

try

Build failed:

petreeftime commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator Author

In pexpect, a similar issue is called out: https://github.com/pexpect/pexpect/blob/2532721644781543ca660e52d48a35bd93872fc1/doc/commonissues.rst#timing-issue-with-send-and-sendline

wait after send
7156464 
Signed-off-by: Petre Eftime <petre.eftime@gmail.com>

petreeftime commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator Author

bors try

bors Bot added a commit that referenced this pull request Apr 23, 2023
Try #101:
39645d8

bors Bot commented Apr 23, 2023

Copy link
Copy Markdown
Contributor

try

Build succeeded:

petreeftime commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator Author

bors try

bors Bot added a commit that referenced this pull request Apr 23, 2023
Try #101:
82acf94

bors Bot commented Apr 23, 2023

Copy link
Copy Markdown
Contributor

try

Build failed:

remove echo on master_fd
62b314c 
Signed-off-by: Petre Eftime <petre.eftime@gmail.com>

petreeftime commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator Author

bors try

bors Bot added a commit that referenced this pull request Apr 23, 2023
Try #101:
c57158c

bors Bot commented Apr 23, 2023

Copy link
Copy Markdown
Contributor

try

Build failed:

matthiasbeyer requested changes Apr 24, 2023
View reviewed changes

matthiasbeyer left a comment

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

I am fine with the patch per se, if it helps. Only one annotation to simplify a bit of code here.

But as far as I can see, CI still fails? Or am I missing something?

Comment thread src/process.rs
Comment on lines +121 to +126
unsafe {
match ioctl(master_fd.as_raw_fd(), TIOCSCTTY) {
0 => Ok(()),
_ => Err(nix::Error::last()),
}?;
}

matthiasbeyer Apr 24, 2023

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

I assume the unsafe block is only needed for the ioctl call here, isn't it?
also, we don't need to match here, I think.

Suggested change
unsafe {
match ioctl(master_fd.as_raw_fd(), TIOCSCTTY) {
0 => Ok(()),
_ => Err(nix::Error::last()),
}?;
}
if 0 != unsafe { ioctl(master_fd.as_raw_fd(), TIOCSCTTY) } {
return Err(nix::Error::last())
}

matthiasbeyer mentioned this pull request Apr 24, 2023
Merged

petreeftime commented Apr 24, 2023

Copy link
Copy Markdown
Collaborator Author

Yes, so far I was reading through how pexpect or some other terminal emulators set this up and see if we're missing anything. So far, not luck with getting the issue fixed unfortunately, and no matter what I do I can't seem to be able to reproduce it on my setup.

matthiasbeyer commented Apr 24, 2023

Copy link
Copy Markdown
Member

Yeah that also was the case for me... it seems like we only have this issue in CI, which makes it even worse.

ekarlsn mentioned this pull request Apr 26, 2023
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

matthiasbeyer matthiasbeyer requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL