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

_sandboxbuildboxrun.py: Restore terminal after exit of interactive child #1786

Merged
merged 2 commits into from
Aug 6, 2024
Merged

_sandboxbuildboxrun.py: Restore terminal after exit of interactive child #1786

merged 2 commits into from
Aug 6, 2024

Conversation

abderrahim
Copy link
Contributor

This is a port of 8f40112 to the buildbox-run sandbox

Thanks to @adoakley for investigating the issue. This was previously #41

Fixes #1690

@abderrahim
Copy link
Contributor Author

@juergbi I guess you're the best placed to review as the author of the original commit.

@abderrahim
Copy link
Contributor Author

Seems a bit more complicated than this: there are a few test failures with Inappropriate ioctl for device. I wonder why, since I don't think we have tests that run bst interactively.

@abderrahim abderrahim force-pushed the abderrahim/background branch from 7393276 to 9e1854d Compare January 27, 2023 07:56
@cs-shadow
Copy link
Member

the logs have disappeared now so it's difficult to say now but I just wanted to respond to this bit:

Seems a bit more complicated than this: there are a few test failures with Inappropriate ioctl for device. I wonder why, since I don't think we have tests that run bst interactively.

We actually do! I was very happy to add the first batch of them in https://gitlab.com/BuildStream/buildstream/-/merge_requests/1706 :)

it's possible pexpect didn't like deal well with this change.

@abderrahim abderrahim force-pushed the abderrahim/background branch from 9e1854d to 95c5b81 Compare December 15, 2023 17:24
@abderrahim abderrahim force-pushed the abderrahim/background branch from 95c5b81 to dd96570 Compare March 2, 2024 16:12
@abderrahim abderrahim force-pushed the abderrahim/background branch from dd96570 to 793405c Compare May 21, 2024 08:19
abderrahim and others added 2 commits August 2, 2024 12:03
@abderrahim
_sandboxbuildboxrun.py: Restore terminal after exit of interactive child
eeb6e1f 
This is a port of 8f40112 to the
buildbox-run sandbox

Fixes #1690
@abderrahim
_sandboxbuildboxrun: don't try to restore terminal if not on a terminal
aa319b4 
This happens in our tests where we don't have a terminal attached even
for interactive operations
@abderrahim abderrahim force-pushed the abderrahim/background branch from 793405c to aa319b4 Compare August 2, 2024 11:03
@abderrahim
Copy link
Contributor Author

I pushed a fix to also check that stdin is a terminal before calling ioctl.

juergbi
juergbi approved these changes Aug 6, 2024
View reviewed changes
Copy link
Contributor

@juergbi juergbi left a comment

Choose a reason for hiding this comment

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

LGTM and seems to work fine in a manual test.

@abderrahim abderrahim merged commit 8c8bb1a into master Aug 6, 2024
16 checks passed
@abderrahim abderrahim deleted the abderrahim/background branch August 6, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juergbi juergbi juergbi approved these changes

@gtristan gtristan Awaiting requested review from gtristan gtristan is a code owner

@BenjaminSchubert BenjaminSchubert Awaiting requested review from BenjaminSchubert BenjaminSchubert is a code owner

@cs-shadow cs-shadow Awaiting requested review from cs-shadow cs-shadow is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

buildstream goes into the background after entering a shell
3 participants
@abderrahim @cs-shadow @juergbi