Use ioctl() to determine width of controlling terminal #415
Conversation
| height, width = struct.unpack("hh", ioctl(f.fileno(), TIOCGWINSZ, "1234")) | ||
| except (IOError, OSError, struct.error): | ||
| # return default size if actual size can't be determined | ||
| return 80 |
There was a problem hiding this comment.
Should an error be printed (once?) if the size cannot be determined? Might avoid users not understanding why catkin_tools output is limited to 80 chars in case the ioctl doesn't work for them.
There was a problem hiding this comment.
I'd say the 80 column default should work well enough that no error message is necessary. It's not difficult to add though.
There was a problem hiding this comment.
Maybe an error was a bit strong, a warning would be better.
I'm not worried about it not working, I'm just thinking that it might avoid some confusion when users are somehow ending up with 80 col output on a 200+ col screen (or catkin insisting on 80 col output on a 30 col window) and they have no way of finding out why other than looking through the source code.
|
I added 2e5695b, let me know what you guys think. |
| __warn_terminal_width_once_has_printed = True | ||
| print('WARNING: Could not determine the width of the terminal. ' | ||
| 'This warning will only be printed once.', | ||
| file=sys.stderr) |
There was a problem hiding this comment.
Should this be: "WARNING: Could not determine the width of the terminal, will use default of 80 columns. This warning will only be printed once"?
There was a problem hiding this comment.
That's a good idea, I added a note about the default of 80 in the message in 7d586d4.
I also changed the WARNING to NOTICE, but that's just to check a hunch about the failing CI.
There was a problem hiding this comment.
I prefer notice to warning anyway :-)
Speaking of noticing, I saw that only the OSX build is failing, maybe the terminal ioctl() does not work with that OS, and the warning messes with the unit test somehow?
There was a problem hiding this comment.
In my latest commit it is passing, I think it was picking up the WARNING in a check for whether or not there were warnings 😄. So changing it to NOTICE was an easy workaround.
|
Ok, that looks good to me. Thanks @roehling and @gavanderhoorn, if you guys have more comments, we can pick them up in a follow up pr. |
|
A question: is there a specific reason there are three places a default is returned? Is that just to keep |
This PR gets rid of the
tput colssubprocess, which is slow and pollutes log files withtput: No value for $TERM and no -T specifiedmessages if no controlling terminal is present.