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

Progress bar width calculation does not recognize double-width characters #12014

Open
roberth opened this issue Dec 5, 2024 · 6 comments · May be fixed by #12066
Open

Progress bar width calculation does not recognize double-width characters #12014

roberth opened this issue Dec 5, 2024 · 6 comments · May be fixed by #12066
Labels
bug localization Nix should work well in every place new-cli Relating to the "nix" command

Comments

@roberth
Copy link
Member

roberth commented Dec 5, 2024

Describe the bug

When the progress bar is rendered wider than expected, an unexpected line break occurs, causing scrolling, and a failure to clear the bar whose content is on the second to last line for the most part.

unicode.nix

with import <nixpkgs> { };
runCommand
"unicod"
{ }
''
  echo 1🔍4---------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  sleep 1
  echo a零b---------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  sleep 1
  echo ok
  sleep 1
  touch $out
''

$ nix build -f unicode.nix
[1/0/1 built, 0.0 MiB DL] building unicod on ssh://agent-2: 1🔍4----------------------------------------------------------------------------
[1/0/1 built, 0.0 MiB DL] building unicod on ssh://agent-2: a零b----------------------------------------------------------------------------

$

Note -L was not passed, so it is not expected for log lines to remain on screen, and indeed the ok line did not.

Steps To Reproduce

  1. Write a build that prints some 🔍 to the log and sleeps to make sure the line makes it into the progress bar.
  2. Build it.
  3. Observe a unintentional snapshot of the log on the second-to-last line of the terminal

Expected behavior

Double-width unicode characters count double when deciding where to cut off the progress bar content.

In Rust, I would use unicode_width. I am not aware of a C/C++ alternative.

Assuming this logic will come from a dependency, we'll want to make it optional, in order to keep the compile-from-scratch footprint small.

I personally wouldn't mind introducing optional Rust code into the project, but that would have to be a team decision.

Metadata

Additional context

This affects non-western users and users of tools with fancy logging.

Loosely related:

Checklist

Add 👍 to issues you find important.
@roberth roberth added bug new-cli Relating to the "nix" command localization Nix should work well in every place labels Dec 5, 2024
@Mic92
Copy link
Member

Mic92 commented Dec 6, 2024 •
edited
Loading

Can we not just use wcwidth? Nothing against rust, but it seems this would be easier to integrate at the current state.

@roberth
Copy link
Member Author

roberth commented Dec 6, 2024

wchar_t is above my pay grade :)
Probably?

@Mic92
Copy link
Member

Mic92 commented Dec 6, 2024 •
edited
Loading

If you have a string, you probably want this instead: int wcswidth(const wchar_t *s, size_t n);. Anyway, I don't think it's rocket science. It should be technology from the 90s.

@NaN-git
Copy link

NaN-git commented Dec 9, 2024 •
edited
Loading

@Mic92 This seems to be trickier than calling wcwidth or wcswidth, i.e.

The behavior of wcswidth() depends on the LC_CTYPE category of the current locale.

I tried different locales, but wcwidth just returned -1 for the non-ASCII characters in the test case.
🔍 seems to be a special case already. According to https://github.com/ridiculousfish/widecharwidth/blob/master/widechar_width_c.h#L39 the width of the character was 1 in older Unicode versions, but now it is 2 (in accordance to the behaviour of my terminal). Another library returned 1...

Some East-Asian characters do not seem to have a well defined width either. It will be difficult to support this correctly, unless the cursor position is queried from the terminal after printing a line.

If https://github.com/ridiculousfish/widecharwidth/blob/master/widechar_width_c.h would be used, then for ambitious cases a character width of 1 can be assumed or the maximum width possible, i.e. 2 for cases like 🔍.

The question is whether it makes sense to introduce this library as build time dependency of Nix.

@roberth
Copy link
Member Author

roberth commented Dec 9, 2024

The question is whether it makes sense to introduce this library as build time dependency of Nix.

We like to preserve the ability to bootstrap a Nix from source in a non-Nix environment where dependencies are more expensive, especially in terms of human overhead.
For an essentially single-function dependency like this it is easy to provide a "good enough" placeholder that makes the dependency optional. Specifically in this case we could provide an implementation that returns the current, potentially wrong answer. The failure mode, ugly output, isn't all that relevant for this use case, because it's easy enough to build a Nix-packaged Nix when the "bootstrap" Nix is good enough.

The other use case that suffers a bit more is packaging into other distros, where users may not use Nix to get a better Nix. We'd be putting more responsibility on those distro packagers, but it's actually no worse than the status quo in this case.
Even this could potentially be mitigated with vendoring, but that makes it costlier for us to maintain. A tradeoff.

@Mic92
Copy link
Member

Mic92 commented Dec 10, 2024 •
edited
Loading

Many Linux distribution don't allow vendoring of dependencies as a policy btw. So we should make it easy in meson to override vendored dependencies. Our libgit2 patches also caused some headaches.

@NaN-git NaN-git linked a pull request Dec 15, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug localization Nix should work well in every place new-cli Relating to the "nix" command
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

try to calculate width of UTF-8 encoded characters NaN-git/nix
3 participants
@Mic92 @roberth @NaN-git