tput errors when running "drush" script in cron

Agree that this is annoying. My one thought is that we once had some code that suppressed this error but we had to back it out because it broke something somewhere. I searched but couldn't find the issue. Maybe one of the Aegir guys remembers (anarcat?). Anyway, seems like a good patch to me.

that prior rollback was unrelated.

Committed. thanks owen.

I was still getting the notice so I did one modification to the logic:

changed

if [ -z $COLUMNS ] && [ -n "$TERM" ]; then

to

if [ -z $COLUMNS ] && [ -n "$TERM" ] && [ $TERM != "dumb" ]; then

For some reason $TERM is set to dumb in some cases. I'm using ubuntu 8.04 (not in my hands to upgrade).

I am having the same issue as jm.federico, but on CentOS 5.4. During the crontab run $TERM is set to 'dumb'.

I also have the same issue like jm.federico

my 5cts to find out why:

$ (echo $TERM; unset TERM; bash -c 'echo $TERM')
screen
dumb
$ find | xargs grep TERM 
./includes/environment.inc:  $nocolor = (drush_get_option(array('nocolor'), FALSE) || !getenv('TERM'));
./drush:if [ -z $COLUMNS ] && [ -n "$TERM" ]; then

and a quick-fix:

$ cvs diff -u drush
Index: drush
===================================================================
RCS file: /cvs/drupal-contrib/contributions/modules/drush/drush,v
retrieving revision 1.20
diff -u -r1.20 drush
--- drush       17 Jun 2010 04:39:41 -0000      1.20
+++ drush       7 Sep 2010 10:20:11 -0000
@@ -28,6 +28,8 @@
     SCRIPT_PATH=$(cygpath -w -a -- "$SCRIPT_PATH") ;;
 esac
 
+[ "$TERM" = dumb ] && unset TERM
+
 # If not exported and term is set determine and export the number of columns.
 if [ -z $COLUMNS ] && [ -n "$TERM" ]; then
   # Note to cygwin users: if you are getting "tput: command not found", 

#784342: Drush should't use colors if there is no terminal attached should also fix this; re-open if it does not.

No, sorry, #784342 doesn't fix this issue here.

Also, I think the test weather stdout is "not a terminal" ([ ! -t 1 ]) is not the best thing to base the "--nocolor" decision on. What if somebody does $ drush | less -R (-R for "Raw control chars" - makes color codes work in the less pager). In that case the colors will be suppressed because stdout will be a pipe while the user actually intended to have colors enabled. And besides that i don't like the "set -- ..." line, that changes the effective command line arguments, at all. That sort of manipulations can make debugging really hard.

Checking the TERM variable for being "not present" OR "'dumb'" is better, I think. It has already been done this way. It was just not "done right", because TERM was only tested for being "not present" and checking for "dumb" was neglected. As bash sets "TERM" to "dumb" on startup if it is not present, this is necessary. Switching to [ -t 1 ] type testing now would be inconsistent with existing code.

See the attached proposed patch, which implements my suggested changes.

the same patch in diff -u format

Okay, I've just skimmed this and the other issue, but I think I'm convinced. Will return to this later.

Edit: I was insane when I wrote the comments here, so I've deleted them. :p

Subscribing.

I just noticed that my new drush crontab (which is run every minute), fills the mailbox with errors. I actually tried to fix it in crontab, like this guy:
http://www.practicalweb.co.uk/blog/10/02/18/drush-tput-no-value-term-and...

It didnt work for me though. It's definitely not critical, but annoying. 'm just waiting until this is fixed in drush.

Thanks for the awesomeness of drush btw! Makes my life so much easier :)

In reply to comment #11 by owen barton:

Yes, you ar right of course! Fixed that.

(And forget what I originally said in this comment, please. Must have been infected with insanity. Sorry.)

Detected the same behaviour in Drush v.3.3.

First, sorry for #8 -- these two issues are unrelated, except for both relating to the tput command. I re-opened #784342: Drush should't use colors if there is no terminal attached to discuss the -t vs --nocolor issue there.

Regarding this issue, it seems that the only thing that we want to fix here is to suppress the message that tput prints to stderr when there is no terminal. I don't like testing for "dumb"; I don't know that $TERM might be "mudo" or "物の言えない" or something else. I know that drush does not need to support every language and every distro, but if there's an alternative that is distro-agnostic, then we should use it.

The enclosed patch simply suppresses everything that tput writes to stderr. This should not interfere with any correct value of TERM, and calling tput should not cause any adverse affects (other than the msg to stderr) when it is called with a "missing" value for TERM. In short, tput is better than drush at detecting whether or not the value of TERM is meaningful.

The TERM tests in environment.inc are not necessary, as stderr is already redirected to stdout there, and the is_numeric test prevents undesirable outcomes when the stderr message is interpreted as a column count.

This seems to clear everything up for me; does anyone know of other cases I missed, where calling tput still causes problems with a missing TERM?

I am fine with greg's patch. It's simple and fixes the issue.

The fact that COLUMNS could be exported while containing just an empty string (in case "tput columns" fails because TERM=dumb) could maybe be ironed out... ?

Greg should commit this when he is satisfied.

Committed.

Sorry, but this is not fixed. Both Ubuntu and Debian (dash and bash) default to 80. For example:

$ echo $(tput cols 2>/dev/null)
80
$ echo $(tput cols)
141

I'll check later what's going on.

Ok, got it now.

Since both stderr (2>/dev/null) and stdout (implicitly via $()) are redirected, tput gets confused and doesn't know for what terminal do we want info for, and returns it's default, hence 80. And currently it will always return 80, regardless of the terminal size.

So, the best approach would be to check for $TERM: if there is no $TERM, then COLUMNS get to be 80. Otherwise, let tput figure out what to do (ie. read termcap file and give us result).

Will provide a patch.

See #7 & c.; sometimes $TERM is "dumb". Sorry that I tested on an 80-column terminal and didn't notice that it was still broken.

Term being dumb, doesn't confuse tput

$ export TERM=dumb; echo $(tput cols)
141

Dumb is defined (/lib/terminfo/d/dumb) and tput can handle it. Here's the patch that fixes this.

Per #7, your test is not adequate for the situation where TERM is not set.

Try this:

echo $TERM
unset TERM
bash
echo $TERM
tput cols

On Debian (Ubuntu), this prints:

xterm
dumb
tput: No value for $TERM and no -T specified

Other distros might print different values, but the fact remains that there are some instances where $TERM is non-empty, and tput cols prints an error to stderr. I'm a bit at a loss on how to deal with this; if we redirect stderr to /dev/null or to stdout, then tput cols returns the wrong value when $TERM is set. If we don't redirect stderr, then we get an undesirable message.

I can think of only two solutions, neither of which make me terribly happy.

1. special checking for 'dumb', per #17
2. call $(tput cols 2>/dev/null), and if the result is non-empty, call $(tput cols) again to get the correct value for COLUMNS

#1 is subject to failure if 'dumb' is ever localized (and frankly I don't know if it is or isn't)
#2 is a little inelegant, as it involves multiple calls to tput

Pick your ugly. I think I prefer #2, but I'm ready to settle on either one and just move on.

I guess you're right with this approach. I'll provide a patch.

I can go with that. Sorry for #19.

Looks like we can basically go with #17, although this does bring up the question of whether exec('tput colors 2>&1'); returns the correct value, or the default value for colors per #25. Might want to avoid setting COLUMNS unless necessary per #20.

Since this issue has turned out to be so convoluted, a goodly amount of commenting would be nice too.

subscribe

I just tried to make my terminal wider to see more info, and got annoyed by my bug in #19. Here's a patch that fixes it, as we discussed above.

Patch tested and seems that works fine.

Committed. Thanks.

Thank you everybody for fixing this! Perfect.

Cheers