progress: detect real terminal width for messages by lzap · Pull Request #316 · osbuild/image-builder-cli

lzap · 2025-09-23T15:30:12Z

There is a common pattern in the progress bar code to shorten strings to fit into terminal width. This commit introduces a new utility function for that. It also introduces a fallback terminal width of 80 columns in case the terminal size cannot be determined.

Added a test for the function and made sure it never returns string longer than the specified length. That was not the case for the previous implementation.

Additionally, the progress bar implementation was updated to dynamically detect the terminal width and adjust message lengths accordingly. This ensures that progress messages are fully visible without being truncated, enhancing user experience during long-running operations.

There is few characters left for the spinner and padding, so we subtract 10 from the terminal width when setting message lengths.

A new dependency was introduced, but it was already an indirect one.

Also, turns out the container test does not actually run with any terminal size variables, so I added a fixed terminal size to the podman run command.

Split out of: #303

supakeen · 2025-09-23T15:35:16Z

This will need some (manual) testing and verification since we run in many different places where there might, or might not be any terminal attached; or the terminal might misreport its width, etc.

Thank you for splitting this off ❤️.

bcl

Seems to work ok running it locally. But I also don't see anything wrong, so I'm mostly saying it didn't obviously break things :)

mvo5

Thank you! This looks nice, some idea inline for your consideration but I think we could merge as is (but I think its worth thinking about the inline comments)

mvo5 · 2025-09-24T06:35:42Z

pkg/util/util.go

+// newlines with spaces. If the string is longer than length, it appends
+// a unicode ellipsis character. If length is 0, it returns the unmodified
+// string.
+func ShortenString(msg string, length int) string {


(nitpick) I'm not sure we should move it out, the side-effect of \n->" " for a general util is a bit weird.

If we move it out, maybe into strutil to not have a kitchen-sink util? I think you mention that you generally preferred it this way in another PR. And maybe strutil.ShortenAndNormalize() or something (this one is a bit long) to indicate that there is this other side-effect

I am a bit thorn on this one, so I am going for a hybrid approach.

Modified the utility function to be generic, no \n replacement.

Added a tiny function in progress.go which utilizes this function plus replaces newlines.

No appetite for strutil :) ? Its fine, just wondering.

If you do not like it in util i can move it back to progress. I do not like util or XXXutil at all, package name should represent what it does. In this case I would consider strings if you ask me.

Its fine as is, thanks for the tweaks about the \n

lzap · 2025-09-24T12:32:08Z

Locally everything works, so not sure what is wrong on the CI/CD. So I am trying out something crazy. I updated the progress integration test to use asciinema utility that will record both sessions into a "cast" file. I also updated the GHA so the recordings are archived as artifacts. These are in fact JSON Seq files and they can be played via asciinema utility (in Fedora, Ubuntu etc) or pasted (they are relatively small) via https://asciinema.org online tool for viewing.

supakeen · 2025-09-24T13:20:59Z

Locally everything works, so not sure what is wrong on the CI/CD. So I am trying out something crazy. I updated the progress integration test to use asciinema utility that will record both sessions into a "cast" file. I also updated the GHA so the recordings are archived as artifacts. These are in fact JSON Seq files and they can be played via asciinema utility (in Fedora, Ubuntu etc) or pasted (they are relatively small) via https://asciinema.org online tool for viewing.

Using asciicinema runs a small risk of it setting up a pty and the error disappearing. It is possible that in CI (and other cases) that there's no controlling pty at all (in normal cases) and maybe no tty on the receiving end either.

What was the exact error you were seeing?

lzap · 2025-09-24T13:32:49Z

What was the exact error you were seeing?

It was this job: https://github.com/osbuild/image-builder-cli/actions/runs/17951169988/job/51050562633

lzap · 2025-09-24T13:35:38Z

Ha I wonder if --terminal-size 80x24 option broke podman which returned 125. Yeah that is it, so it was just this option is probably unsupported.

So you do not like asciinama then? Since terminal size cannot be set via podman, it actually turns useful now.

supakeen · 2025-09-24T13:42:24Z

Ha I wonder if --terminal-size 80x24 option broke podman which returned 125. Yeah that is it, so it was just this option is probably unsupported.

So you do not like asciinama then? Since terminal size cannot be set via podman, it actually turns useful now.

It's not really about asciinema. More that it might influence how terminal detection and things work since it might spawn (another) pty.

lzap · 2025-09-24T13:54:39Z

It's not really about asciinema. More that it might influence how terminal detection and things work since it might spawn (another) pty.

But that is the goal, podman does not allow too much configuration of pty. Asciinema allows setting the size, other than that it does not change anything. I do not see any problem here, explain please.

Here is the recording which I downloaded from the latest (successful) run:

https://asciinema.org/a/DImVCmlxcIsyWSY2iTdwRPQyD

supakeen · 2025-09-24T14:03:08Z

It's not really about asciinema. More that it might influence how terminal detection and things work since it might spawn (another) pty.

But that is the goal, podman does not allow too much configuration of tty. Asciinema allows setting the size, other than that it does not change anything.

Here is the recording which I downloaded from the latest (successful) run:

This code should work in environments where there isn't a size (or that don't report a size) right? It can fall back to non-fancy (verbose) output in those cases.

lzap · 2025-09-24T14:09:11Z

This code should work in environments where there isn't a size (or that don't report a size) right? It can fall back to non-fancy (verbose) output in those cases.

Absolutely, I can extend the test and add with and without cases so both are tested in non-tty non-interactive environments. In fact, not only verbose should work, I also think progress should not break the terminal. Ideally it should detect that and do nothing. I think this test is relatively fast compared to others, locally it takes few seconds the only slow part is building the fake container.

supakeen · 2025-09-24T14:16:21Z

pkg/progress/progress.go

-		return msg[:60] + "..."
+func getTerminalWidth() int {
+	width, _, err := term.GetSize(int(os.Stdout.Fd()))
+	if err != nil {


It might be safer to assume that the terminal is in fact broken if it's not responding correctly to TIOCGWINSZ and instead downgrade to a non-interactive terminal. That's not really something we can do when we get to this point of the code so:

If progress is set to auto we currently only check if isatty returns true on the fd; we should probably call GetSize there as well to see if that terminal is behaving correctly.

supakeen · 2025-09-24T14:20:25Z

pkg/progress/progress.go

+}
+
+func shortenString(msg string) string {
+	width := getTerminalWidth()


Note that doing this repeatedly can cause bogus reads; some terminals can return weird values during resizing (very large, very small). It might be better to keep some state and listen to SIGWINCH instead though that can also be racy I guess.

Feel free to ignore this one since we always erase the entire line and don't do any moves relative to the line length or such so it's unlikely to cause us any issues.

supakeen · 2025-09-24T14:23:10Z

I put some small comments from my experiences of doing things with terminals, I need to do a deeper look and rack my brain for other issues (I distinctly remember cases where nested pty's are used from another project).

Note that these things are all very 'maybe' and very environment dependent. I'm just slightly worried that suddenly image-builder breaks in some environment. I don't think the odds of that happening are high since we always pass --progress=verbose in Koji and don't really do anything relative to the window sizes we read but only clear a line and write a new line.

It might be good to be a bit defensive and then I'm happy enough to merge since I'm probably being a bit too difficult on this.

lzap · 2025-09-24T16:09:42Z

It might be good to be a bit defensive and then I'm happy enough to merge since I'm probably being a bit too difficult on this.

You are presenting great points, don't worry.

I drafted everything you mentioned, the process now listens to the UNIX signal and updates w/h accordingly. I should probably move this into the Progress instance and introduce some proper Close() method that can be deferred from main tho, opinions?

Moreover, the initial check in progress.New also checks for size and if that is not set, it falls back to simple mode. Also updated the test, it tests with and without pty. I had to introduce a dataclass because of the pesky linter tho.

Also I found a bug in tests - those mocked functions had defer in a loop, therefore they were actually overwriting the global value and never returning the original value. Fixed with t.Run.

test/test_container.py

supakeen · 2025-09-30T12:19:08Z

@lzap Could you fix the conflict here, I'm about ready to ack it but otherwise I have to do it a few times.

lzap · 2025-09-30T13:03:27Z

Could you fix the conflict here

Of course, thanks for the heads-up. Rebased.

supakeen · 2025-09-30T14:30:23Z

test/test_container.py

-    output = subprocess.check_output(podman_run + [
-        "-t",
+
+    podman_command = [ podman_run +


Needs to be podman_run + [ instead of [ podman_run +.

supakeen · 2025-09-30T14:30:57Z

One comment (introduced by the rebase) and linters need appeasement.

There is a common pattern in the progress bar code to shorten strings to fit into terminal width. This commit introduces a new utility function for that. It also introduces a fallback terminal width of 80 columns in case the terminal size cannot be determined. Added a test for the function and made sure it never returns string longer than the specified length. That was not the case for the previous implementation. Additionally, the progress bar implementation was updated to dynamically detect the terminal width and adjust message lengths accordingly. This ensures that progress messages are fully visible without being truncated, enhancing user experience during long-running operations. There is few characters left for the spinner and padding, so we subtract 10 from the terminal width when setting message lengths. A new dependency was introduced, but it was already an indirect one. Testing is difficult, so this change also adds asciinema recordings of the progress bar in action. The recordings are uploaded as artifacts of the CI run. The test verifies that the expected progress messages are present in the output.

lzap · 2025-10-22T09:14:22Z

Rebased, fixed test dependency and linter.

lzap · 2025-10-23T12:40:55Z

Any chance to get another one @thozza or @bcl ? Thanks!

bcl

Looks good!

This test is no longer working, it seems its a result of the merge of osbuild/image-builder-cli#316 Drop it for now so that we can move forward with the merge. There is a similar test in https://github.com/osbuild/image-builder-cli/pull/316/files#diff-57c49abc8a31b46856c2eecb2edfeff6072cd8d0edf769a67b6d4fdaabd8321aR100 that does not cover "auto" but any fixes/regression should be done in the ibcli repo.

This test is no longer working, it seems its a result of the merge of osbuild#316 Drop it for now so that we can move forward with the merge. There is a similar test in https://github.com/osbuild/image-builder-cli/pull/316/files#diff-57c49abc8a31b46856c2eecb2edfeff6072cd8d0edf769a67b6d4fdaabd8321aR100 that does not cover "auto" but any fixes/regression should be done in the ibcli repo.

This test is no longer working, it seems its a result of the merge of osbuild#316 Drop it for now so that we can move forward with the merge. There is a similar test in https://github.com/osbuild/image-builder-cli/pull/316/files#diff-57c49abc8a31b46856c2eecb2edfeff6072cd8d0edf769a67b6d4fdaabd8321aR100 that does not cover "auto" but any fixes/regression should be done in the ibcli repo. See also osbuild/bootc-image-builder#1157

This test is no longer working, it seems its a result of the merge of #316 Drop it for now so that we can move forward with the merge. There is a similar test in https://github.com/osbuild/image-builder-cli/pull/316/files#diff-57c49abc8a31b46856c2eecb2edfeff6072cd8d0edf769a67b6d4fdaabd8321aR100 that does not cover "auto" but any fixes/regression should be done in the ibcli repo. See also osbuild/bootc-image-builder#1157

lzap requested a review from a team as a code owner September 23, 2025 15:30

lzap requested review from mvo5, supakeen and thozza and removed request for a team September 23, 2025 15:30

bcl previously approved these changes Sep 24, 2025

View reviewed changes

mvo5 reviewed Sep 24, 2025

View reviewed changes

lzap dismissed bcl’s stale review via 60f2dac September 24, 2025 12:28

lzap force-pushed the term-width branch from ea64b10 to 60f2dac Compare September 24, 2025 12:28

lzap force-pushed the term-width branch 2 times, most recently from f79b8f5 to d8006e7 Compare September 24, 2025 12:43

supakeen reviewed Sep 24, 2025

View reviewed changes

lzap force-pushed the term-width branch from d8006e7 to 9c2e3cc Compare September 24, 2025 16:11

lzap commented Sep 24, 2025

View reviewed changes

test/test_container.py Outdated Show resolved Hide resolved

lzap commented Sep 24, 2025

View reviewed changes

test/test_container.py Outdated Show resolved Hide resolved

lzap force-pushed the term-width branch from 9c2e3cc to 5195931 Compare September 25, 2025 14:30

lzap force-pushed the term-width branch from 5195931 to 9d30d32 Compare September 30, 2025 13:03

supakeen reviewed Sep 30, 2025

View reviewed changes

lzap force-pushed the term-width branch from 9d30d32 to f187427 Compare October 22, 2025 07:59

supakeen self-assigned this Oct 22, 2025

lzap force-pushed the term-width branch from f187427 to 87a4954 Compare October 22, 2025 09:14

supakeen approved these changes Oct 22, 2025

View reviewed changes

lzap requested review from bcl and mvo5 October 22, 2025 11:43

bcl approved these changes Oct 23, 2025

View reviewed changes

bcl added this pull request to the merge queue Oct 23, 2025

Merged via the queue into osbuild:main with commit 87b9ded Oct 24, 2025
38 checks passed

lzap deleted the term-width branch October 24, 2025 12:59

Conversation

lzap commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

supakeen commented Sep 23, 2025

Uh oh!

bcl left a comment

Choose a reason for hiding this comment

Uh oh!

mvo5 left a comment

Choose a reason for hiding this comment

Uh oh!

mvo5 Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

lzap Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

mvo5 Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

lzap Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

mvo5 Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

lzap commented Sep 24, 2025

Uh oh!

supakeen commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lzap commented Sep 24, 2025

Uh oh!

lzap commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

supakeen commented Sep 24, 2025

Uh oh!

lzap commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

supakeen commented Sep 24, 2025

Uh oh!

lzap commented Sep 24, 2025

Uh oh!

supakeen Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

supakeen Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

supakeen commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lzap commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

supakeen commented Sep 30, 2025

Uh oh!

lzap commented Sep 30, 2025

Uh oh!

supakeen Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

supakeen commented Sep 30, 2025

Uh oh!

lzap commented Oct 22, 2025

Uh oh!

lzap commented Oct 23, 2025

Uh oh!

bcl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

lzap commented Sep 23, 2025 •

edited

Loading

supakeen commented Sep 24, 2025 •

edited

Loading

lzap commented Sep 24, 2025 •

edited

Loading

lzap commented Sep 24, 2025 •

edited

Loading

supakeen Sep 24, 2025 •

edited

Loading

supakeen Sep 24, 2025 •

edited

Loading

supakeen commented Sep 24, 2025 •

edited

Loading

lzap commented Sep 24, 2025 •

edited

Loading