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

FIX: Don't invoke tput if --columns is passed #6

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

Conversation

inkarkat
Copy link
Contributor

@inkarkat inkarkat commented Aug 4, 2020

Under certain circumstances (e.g. when run as a cron job), tput is not able to determine the columns (tput: unknown terminal "unknown"). For that, the columns can be explicitly set via --columns=N, and nowrap can still do its job. However, the tput lookup is still performed, and results in an error message (which even triggers an email for the cron job). Lookup of available terminal columns should only happen if the columns are not specified on the command-line; this also slightly improves performance.
Move the $column initialization to after the command-line argument parsing.

inkarkat and others added 2 commits August 4, 2020 09:55
Under certain circumstances (e.g. when run as a cron job), tput is not able to determine the columns (tput: unknown terminal "unknown"). For that, the columns can be explicitly set via --columns=N, and nowrap can still do its job. However, the tput lookup is still performed, and results in an error message (which even triggers an email for the cron job). Lookup of available terminal columns should only happen if the columns are not specified on the command-line; this also slightly improves performance.
Move the $column initialization to after the command-line argument parsing.
See goodell#6 for Ingo's contribution and
more context.
@goodell
Copy link
Owner

goodell commented Aug 24, 2020

Thanks for the fix, Ingo, sorry it's taken me a while to take a proper look at it. Fix looks good to me, but I wanted a regression test too so I've added one as 6abdeb2. I'll merge the combo if my delta looks good to you.

Looks like the Travis CI is having a problem with the perl5.8 build for some reason, but that seems to be unrelated to your patch. I think I've got a workaround for it separately, should get posted later today.

@goodell
Copy link
Owner

goodell commented Aug 24, 2020

perl5.8 CI is fixed as of 7e8c912

@inkarkat
Copy link
Contributor Author

Thanks for adding a test; that's a good practice! I wonder why you've used nohup; unsetting TERM should be enough to make tput fail, isn't it?

The Perl 5.8 build seems to be still failing; I hope you can figure out that problem as well. Take your time; I'm more than happy that you're still maintaining this nice little tool!

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