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.

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

Restore original TTY state after Sandbox #11990

Merged
merged 4 commits into from
Sep 8, 2021

Conversation

sullivan-sean
Copy link

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Fixes #11984

This PR switches from the current method of using raw! and then cooked! to reset the Sandbox TTY to just using raw which automatically restores the original TTY state (and also is the recommended method in the docs). Using this approach the TTY state should always be properly restored, including the echonl and istrip flags that are not currently reset in all cases.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks, looks good so far! Could you add more comments here explaining what's being done and why? Thanks!

@sullivan-sean
Copy link
Author

To add some more context, the current behavior on 3.2.10 is that the TTY state is modified by the sandbox when installing a package with brew install and are not properly reset afterwards. This can result in unexpected artifacts in a user's terminal after running brew install, such as an extra newline being printed before running each command.

Current behavior

On my machine I see the following differences in stty -a before and after running brew install:

% stty -a | grep echonl
lflags: icanon isig iexten echo echoe echok echoke -echonl echoctl
% brew install tree-sitter
==> Downloading https://ghcr.io/v2/homebrew/core/tree-sitter/manifests/0.20.0-1
Already downloaded: /Users/seansullivan/Library/Caches/Homebrew/downloads/caf4df26418c0c4664fed59578fe8257f590731241bbbc342433585a2bab3694--tree-sitter-0.20.0-1.bottle_manifest.json
==> Downloading https://ghcr.io/v2/homebrew/core/tree-sitter/blobs/sha256:cbf44029649cea921fe333454
Already downloaded: /Users/seansullivan/Library/Caches/Homebrew/downloads/e30e408fcc1bd5715be4a06aefd102e2e64d2209249d2fec6f41c7d3d115b0c1--tree-sitter--0.20.0.arm64_big_sur.bottle.1.tar.gz
==> Pouring tree-sitter--0.20.0.arm64_big_sur.bottle.1.tar.gz
🍺  /opt/homebrew/Cellar/tree-sitter/0.20.0: 18 files, 12.6MB
% stty -a | grep echonl

lflags: icanon isig iexten echo echoe echok echoke echonl echoctl

The echonl flag is OFF (-echonl) in the stty -a output before hand and ON afterwards, which has the side effect of an extra newline before the output of every command in this TTY after running brew install.

Proposed Solution

In tracking down the source of this changed behavior in 3.2.10, I found it was related to the io/console Ruby gem, which was not used by Homebrew until #11914. In particular, the change to start the sandbox in a PTY requires setting the parent TTY to "raw" mode to be able to pass through input such as terminal escape codes directly to the parent TTY. The approach in #11914 is to set the TTY to "raw" mode with io.raw! method and to restore the original TTY state with the io.cooked! method.

This PR changes the Sandbox PTY to use the io.raw method from io/console instead of io.raw!. The documentation for io.raw! highlights io.raw as the preferred method for properly restoring the original TTY state.

While the current approach with io.raw! might work in some cases (i.e. when the original TTY state exactly matches that of io.cooked!), using io.raw will work in all cases.

@carlocab
Copy link
Member

carlocab commented Sep 8, 2021

Thanks for the clear explanation -- can we summarise that a little and add that in comments in the code?

@MikeMcQuaid MikeMcQuaid merged commit 4b68787 into Homebrew:master Sep 8, 2021
@MikeMcQuaid
Copy link
Member

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @sullivan-sean!

@github-actions github-actions bot added the outdated PR was locked due to age label Oct 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TTY not properly reset after brew install
3 participants