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

Add bounds to termHeight when BUILDKIT_TTY_LOG_LINES is set #4767

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add bounds to termHeight when BUILDKIT_TTY_LOG_LINES is set #4767

wants to merge 2 commits into from
+12 −5

Conversation

holzman
Copy link
Contributor

@holzman holzman commented Mar 15, 2024

In order to print tty output, the termHeight for the vt100 module needs to be between 6 and (current window size - 7).

Fixes #4766.

@holzman
Copy link
Contributor Author

holzman commented Mar 15, 2024

Can test the dropped output with export BUILDKIT_TTY_LOG_LINES=$(tput lines)

@jedevc
Copy link
Member

jedevc commented Mar 18, 2024

Needs go mod tidy/go mod vendor - looks like the vendoring is messed up somehow 😢

@holzman
Copy link
Contributor Author

holzman commented Mar 18, 2024

Thanks @jedevc - I'll do that and push a new commit.

tonistiigi
tonistiigi reviewed Mar 18, 2024
View reviewed changes
util/progress/progressui/init.go Outdated
@@ -5,6 +5,7 @@ import (
"runtime"
"strconv"

"github.com/moby/term"
Copy link
Member

Choose a reason for hiding this comment

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

We already import github.com/container/console for this functionality

util/progress/progressui/init.go Outdated
@@ -41,9 +42,13 @@ func init() {
// Make the terminal height configurable at runtime.
termHeightStr := os.Getenv("BUILDKIT_TTY_LOG_LINES")
if termHeightStr != "" {
fd := uintptr(os.Stdout.Fd())
Copy link
Member

Choose a reason for hiding this comment

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

os.Stderr looks more correct for the progress output.

util/progress/progressui/init.go Outdated
termHeight = termHeightVal
winSize, err := term.GetWinsize(fd)
if err == nil {
termHeight = max(termHeight, min(termHeightVal, int(winSize.Height)-termHeight-1))
Copy link
Member

Choose a reason for hiding this comment

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

I think this condition should be moved to ttyDisplay.print(). There you already have the terminal height for correct console (without reading os pkg in util what may not match the writer) and it also picks up the resizes.

@holzman
Copy link
Contributor Author

holzman commented Mar 19, 2024

Thanks for the review @tonistiigi! I'll make the changes and resubmit.

tonistiigi
tonistiigi reviewed Mar 27, 2024
View reviewed changes
util/progress/progressui/display.go Outdated
@@ -1056,6 +1056,7 @@ func (disp *ttyDisplay) print(d displayInfo, width, height int, all bool) {
lineCount++
if j.showTerm {
term := j.vertex.term
termHeight = max(termHeightMin, min(termHeight, height-termHeightMin-1))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be a new scoped variable, as I think previous computation shouldn't change the calculation for the next round.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point that every round having the same result. termHeight is used in both print and update though, so should it be a package-level variable? Then I'd change it to something like:

const termHeightMin
var termHeightInitial
var termHeight

?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure you get what would be the difference between termHeightInitial and termHeightMin. If you need another var that sgtm though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, here's what I was thinking:

termHeightMin is a const (and set to 6).
termHeightInitial is set to BUILDKIT_TTY_LOG_LINES if set (otherwise defaults to termHeightMin).

We then check bounds on termHeightInitial - if it passes, we set termHeight to it, otherwise it gets set to the larger of termHeightMin or height-7. This way, the computation doesn't depend on the previous value for termHeight.

BTW I appreciate the many looks as I fumble around on this!

util/progress/progressui/init.go Outdated
@@ -13,7 +13,8 @@ var colorCancel aec.ANSI
var colorWarning aec.ANSI
var colorError aec.ANSI

var termHeight = 6
var termHeightMin = 6
Copy link
Member

Choose a reason for hiding this comment

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

This can be const

@thompson-shaun
Copy link
Collaborator

@holzman - Do we still want to advance this? If so, there appear to be some open items ;)

@holzman
Copy link
Contributor Author

holzman commented May 9, 2024

We should. But I had a question: any objection to adding an additional package-level variable instead of scoping it to the method (since termHeight is used in other methods)? I was thinking termHeightInitial.

@holzman holzman force-pushed the tty-log-lines-bounds branch 2 times, most recently from e835b55 to 96db1f0 Compare May 10, 2024 19:51
@holzman
Copy link
Contributor Author

holzman commented May 10, 2024

Sending this back to the drawing board for a bit: I'm managing to crash the display by aggressively resizing the window.

@holzman holzman marked this pull request as draft May 10, 2024 20:36
@holzman
Copy link
Contributor Author

holzman commented May 14, 2024

FWIW, I think the crashes were actually due to a bug in the underlying vt100 module.
Once tonistiigi/vt100#3 gets merged, I'll test and ask again for review.

@holzman
vendor: update vt100 with fix for resizing height
e3beb5f
@holzman
Add bounds to termHeight when BUILDKIT_TTY_LOG_LINES is set
3884559 
In order to print tty output, the termHeight for the vt100
module needs to be between 6 and (current window size - 7).

Signed-off-by: Burt Holzman <burt@fnal.gov>
@holzman holzman marked this pull request as ready for review May 16, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonistiigi tonistiigi tonistiigi left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@tonistiigi tonistiigi

Labels
status/code-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No output if BUILDKIT_TTY_LOG_LINES is too big
4 participants
@holzman @jedevc @thompson-shaun @tonistiigi