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

PTY rows/cols increased #2806

Merged
merged 1 commit into from
Jun 5, 2024
Merged

PTY rows/cols increased #2806

merged 1 commit into from
Jun 5, 2024

Conversation

pda
Copy link
Member

@pda pda commented May 30, 2024

The size of the fake PTY allocated to build jobs was previously unspecified, which generally led to a default of 80x24. That meant programs which checked the size of the terminal would assume it was very small, and that programs which default to using a pager (e.g. less/more) for long output, lines would be truncated at 80 characters, and showing more than ~24 lines of text would often result in a blocking prompt to show more.

Increasing the cols by 2x and rows/lines by ~4x seems reasonable, it comes closer to matching actual modern terminal window sizes, shows longer lines, and is less likely to trigger a blocking pager. Perhaps larger values would be good, especially for Rows. Maybe there's no harm and some upside to e.g. 1,000 rows?

This doesn't fundamentally solve the issue, but perhaps reduces the likelihood and impact of pager interference.

Before & after:

image

Previously unspecified, which generally led to a default of 80x24.

This meant that programs which default to using a pager (e.g. less/more)
for long output, lines would be truncated at 80 characters, and showing
more than ~24 lines of text would often result in a blocking prompt to
show more.

Increasing the cols by 2x and rows/lines by ~4x seems reasonable, it
comes _closer_ to matching actual modern terminal window sizes, shows
longer lines, and is less likely to trigger a blocking pager. Perhaps
larger values would be good, especially for Rows.

This doesn't fundamentally solve the issue, but perhaps reduces the
likelihood and impact of pager interference.
@pda
Copy link
Member Author

pda commented May 30, 2024

A slightly more compelling before/after, where I emulate a command producing 30 lines of output through a pager (less) in the common --quit-if-one-screen (-F) mode …

Pipeline:

steps:
  - command:
      - tput cols
      - tput lines
      - seq 30 | less -F
    timeout_in_minutes: 1

❌ Before:

image

✅ After:

image

@pda
Copy link
Member Author

pda commented May 30, 2024

Thinking a bit more about the right number of rows to claim, I have two ideas:

The terminal output of a modern CLI command will likely be designed around the assumption it's being viewed in a modern terminal emulator on a modern display. Mine at fullscreen on a 27" monitor shows about 90 lines. Obviously a smaller window, or a pane in an editor/IDE is likely to show less, but I think 100 lines is a reasonable size to claim; it's generous enough to avoid pager for any output that would normally fit on a real screen, but not so large/rare/unrealistic as to trigger any edge-cases.

The Buildkite user interface when showing the log for a build job shows roughly 80 lines of terminal output on my display. This also feels to me like it ~lines up with the 100 line display claimed by this PR.

So, I'm feeling pretty good about 100 lines, as opposed to increasing it to e.g. 1,000 or more.

Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

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

100 rows seems pretty reasonable to me. Another consideration is: we probably shouldn't go over 300 without being sure to raise the (recently added) max-lines default in terminal-to-html.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants