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

[Installer] One-step install command leads to minimal whiptail sizes #3323

Closed
3 tasks done
MichaIng opened this issue May 10, 2020 · 16 comments
Closed
3 tasks done

[Installer] One-step install command leads to minimal whiptail sizes #3323

MichaIng opened this issue May 10, 2020 · 16 comments
Labels
WIP Work in progress.

Comments

@MichaIng
Copy link
Contributor

MichaIng commented May 10, 2020

In raising this issue, I confirm the following:

How familiar are you with the the source code relevant to this issue?:

10


Expected behaviour:

  • The installer should estimate terminal dimensions correctly to show whiptail dialogs in intended size.

Actual behaviour:

  • When calling the installer with the official one-step command curl -sSL https://install.pi-hole.net | bash, the whiptail dialogs have always minimal size.

Steps to reproduce:

  • Check actual terminal dimensions: stty size
    • Increase screen/terminal dimensions if it is equal or below 40x140 to assure that the effect is visible.
  • Run installer: curl -sSL https://install.pi-hole.net | bash
  • Verify that first whiptail dialog has neither half height nor half width of terminal dimensions, as intended, instead it is 20x70, the forced minimum:
    pihole

Troubleshooting undertaken, and/or other relevant information:

2020-05-10 14:55:12 root@VM-Buster:~# echo '[ -t 0 ] && echo STDIN available || echo STDIN not available' | bash
STDIN not available
2020-05-10 14:56:19 root@VM-Buster:~# bash <<< '[ -t 0 ] && echo STDIN available || echo STDIN not available'
STDIN not available
2020-05-10 14:56:46 root@VM-Buster:~# bash -c '[ -t 0 ] && echo STDIN available || echo STDIN not available'
STDIN available
  • This makes sense of course since in the first two cases STDIN is blocked for bash code input.
  • Because of this the installer defaults to 24 80 which leads to minimum 20x70 whiptail dimensions.
  • It totally makes sense to check for STDIN since this is indeed required for stty to work:
2020-05-10 14:56:55 root@VM-Buster:~# echo 'stty size' | bash
stty: 'standard input': Inappropriate ioctl for device
2020-05-10 14:59:05 root@VM-Buster:~# bash <<< 'stty size'
stty: 'standard input': Inappropriate ioctl for device
2020-05-10 14:59:18 root@VM-Buster:~# bash -c 'stty size'
71 237
  • tput is an alternative to get terminal dimensions, which does not rely on SDTIN, hence can be called from within a pipe:
2020-05-10 14:59:26 root@VM-Buster:~# echo 'tput lines; tput cols' | bash
71
237
2020-05-10 15:01:08 root@VM-Buster:~# bash <<< 'tput lines; tput cols'
71
237
2020-05-10 15:01:16 root@VM-Buster:~# bash -c 'tput lines; tput cols'
71
237
  • However at least on Debian/Ubuntu it is part of the ncurses-bin package. It has the "required" tag, hence can be expected in most cases, but it is not marked as "essential" (stronger than "required") like coreutils, which contains the stty command.
  • Moreover tput has another issue, at least on Debian until including Stretch. Instead of STDIN it requires either STDOUT or STDERR to be attached to the terminal. If both are redirected, e.g. someone pipes installer output to tee to create a log file, it does not work:
2020-05-10 15:08:37 root@VM-Stretch:~# tput lines
71
2020-05-10 15:08:47 root@VM-Stretch:~# tput lines 2>1 | tee
24
2020-05-10 15:08:52 root@VM-Stretch:~# tput lines | tee
71
2020-05-10 15:08:56 root@VM-Stretch:~# tput lines 2>/dev/null
71
2020-05-10 15:09:08 root@VM-Stretch:~# tput lines 2>1 > log
2020-05-10 15:09:25 root@VM-Stretch:~# cat log
24
2020-05-10 15:09:28 root@VM-Stretch:~# tput lines > log
2020-05-10 15:09:31 root@VM-Stretch:~# cat log
71
  • Since Debian Buster this is not an issue anymore.

Suggested solution:

Since stty simply cannot work without free STDIN and there might be other commands affected as well, and since tput has another limitation on older distro versions + adds a package dependency, I suggest to change the one-line install command:

bash -c "$(curl -sSfL https://install.pi-hole.net)"
  • bash -c allows to preserve all file descriptors (STDIN, STDOUT, STDERR) for best compatibility with commands that interact with the terminal in any way.
  • The additional curl -f option leads to 40X responses to be treated as errors, so that curl exits, instead of forwarding a possible error page html code to be executed by bash: https://manpages.debian.org/buster/curl/curl.1.en.html#OPTIONS
@dschaper
Copy link
Member

curl -sSL https://install.pi-hole.net | bash hasn't ever had a problem with whiptail for me. This is the first issue mentioning it, so "whiptail dialogs have always minimal size" seems to be a bit of a misstatement?

@dschaper
Copy link
Member

Is that a terminal emulator screencap? Is it really 150x300? Or do you just have it set to really small fonts?

@MichaIng
Copy link
Contributor Author

MichaIng commented May 10, 2020

@dschaper
In my case it's via PuTTY, made fullscreen on full HD display, see stty size output above: 71 lines 237 chars
Ah, did you mix up pixels with lines/chars? E.g. 1920x1200 pixels (my screen) means with usual font size (16x8) 75 lines height and 240 characters width, minus PuTTY window borders this fits the 71x237 available characters I got.

Please rerun the troubleshooting commands I've posted above, so you can try to replicate the underlying issues on your setup. I don't believe that this is different on different distros but at least I never tested it on Fedora or CentOS.

@dschaper
Copy link
Member

I have 4k display, my terminal emulator is set to 140x50, this seems more of a self-inflicted issue.

The information in the dialog boxes won't change, you'll just have huge whiptail boxes with a single sentence and acres of whitespace. I'd rather have small boxes.

For me this isn't an issue.

@dschaper
Copy link
Member

And really, you don't need to @ me, I read everything.

@PromoFaux
Copy link
Member

I mean, I rarely fullscreen my terminal on my 3840x1080 monitor, but just for fun:

Current one liner command:
image

With the proposed:

image

Personally I prefer the former. I don't need the whiptail to expand to fit the screen, the information within stays at the same size, and looks better in the smaller window.

Besides which, I think the proposed command is arguably more prone to user error (obviously not if they copy paste), but there is more to go wrong when typing it out.

@MichaIng
Copy link
Contributor Author

MichaIng commented May 10, 2020

If you mean the minimum dimensions are enough and beautiful for all dialog boxes, why the whole effort of estimating terminal dimensions is done? Basically it is true, besides in some cases (upstream DNS selection) one needs to scroll through the selection menu while with bigger dialog it is shown fully expended.

But there is one dialog for which the minimum is too small, when you reset/refresh the install with the one-liner:
pihole

Running pihole -r btw leads to this bigger whiptail dialogs of course. So if this is not wanted, I suggest to remove the terminal size estimation and instead set dimensions for all whiptail dialogues individually so that everything fits inside nicely.

@dschaper
Copy link
Member

If your argument is "Why even do it?" my response will be to remove it completely instead of changing it for your edge case.

@dschaper
Copy link
Member

We just need to make the Reconfigure response shorter or break it to two lines.

@MichaIng
Copy link
Contributor Author

MichaIng commented May 10, 2020

@dschaper
This is not "my edge case". You can simply run the troubleshooting commands I did above and verify yourself that the current estimation is simply broken with the one-liner. However yeah I agree that it is probably easier to remove this, indeed very large whiptails look ugly as well. It should be just assured that every whiptail fits into a reasonable minimum screen, e.g. the 20x70 that is used as minimum already.

@dschaper
Copy link
Member

@MichaIng

Great, will look it over.

@dschaper
Copy link
Member

@MichaIng

It's whiptail, not a responsive webpage.

@dschaper
Copy link
Member

@MichaIng

I prefer the smaller dialog box.

@MichaIng
Copy link
Contributor Author

MichaIng commented May 10, 2020

I only know this from Debian for sure, but actually whiptail allows to auto-adjust the dimensions based on content, by using 0 0 0 for the dimensions, e.g.:

whiptail --menu 'short' 0 0 0 1 one 2 two

pihole

whiptail --menu 'looooooooooooooooooooooooooooooooooooooooooooooooooonger\nand\nwith\nmultiple\nlines' 0 0 0 1 one 2 two 3 three 4 four 5 five

pihole
First the menu, then the text is made scrollable automatically when content exceeds terminal dimensions:
pihole

Only a few very long lines on large terminal make it again ugly, so line breaks need to be added manually then:
pihole

This is undocumented, hence not sure if supported on all distros/newt/whiptail versions, but easier then verifying whiptail size on every content change?

@MichaIng MichaIng changed the title [Installer] One-step install command leads to wrong whiptail sizes [Installer] One-step install command leads to minimal whiptail sizes May 10, 2020
@yubiuser yubiuser added the WIP Work in progress. label Aug 12, 2021
@yubiuser
Copy link
Member

yubiuser commented Oct 3, 2021

@MichaIng

Is this issue fixed by your PR #4229

@MichaIng
Copy link
Contributor Author

MichaIng commented Oct 3, 2021

Ah yes, it is 👍.

@MichaIng MichaIng closed this as completed Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress.
Projects
None yet
Development

No branches or pull requests

4 participants