Page MenuHomePhabricator

Fallback option for colorized output when terminfo isn't available
ClosedPublic

Authored by phosek on Jan 15 2018, 2:06 AM.

Details

Summary

Try to detect the terminal color support by checking the value of the
TERM environment variable. This is not great, but it's better than
nothing when terminfo library isn't available, which may still be the
case on some Linux distributions.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Jan 15 2018, 2:06 AM

FWIW: Someone filed an issue against the Android NDK complaining about the ncurses/libtinfo dependency. It prevented Clang from running out-of-the-box on Arch Linux: https://github.com/android-ndk/ndk/issues/574.

I'd like it better if you just removed the "#ifdef HAVE_TERMINFO" branch of code entirely, along the cmake support for causing it to be defined and adding TERMINFO_LIBS to llvm. (lldb still looks like it uses it, so can't just remove everything related to terminfo from the cmake support files)

Both GCC and Git simply use the TERM is set and != "dumb" check, and never touch curses. If it's good enough for them, it's good enough for llvm too, and we don't need to extra complexity and dependencies. Also, the hacks in this code are ugly (writing our own function signatures for these functions instead of using a proper #include!).

I'd like it better if you just removed the "#ifdef HAVE_TERMINFO" branch of code entirely, along the cmake support for causing it to be defined and adding TERMINFO_LIBS to llvm. (lldb still looks like it uses it, so can't just remove everything related to terminfo from the cmake support files)

Both GCC and Git simply use the TERM is set and != "dumb" check, and never touch curses. If it's good enough for them, it's good enough for llvm too, and we don't need to extra complexity and dependencies. Also, the hacks in this code are ugly (writing our own function signatures for these functions instead of using a proper #include!).

This is exactly what LLVM did prior to r187874 so this would mean basically reverting that change, I'm fine with that.

joerg added a subscriber: joerg.Jan 18 2018, 1:36 PM

I completely disagree with this approach. A lot of GNU tools (including GCC) are completely broken. We shouldn't follow them. There are a lot more terminals around than just "dumb", "xterm" and "linux". It is completely non-acceptable to just assume ANSI escape sequences work. If Android doesn't ship a usable terminfo implementation, I consider that an Android bug. Wouldn't be the first portability nightmare with Android.

jyknight accepted this revision.Jan 18 2018, 3:09 PM

I completely disagree with this approach. A lot of GNU tools (including GCC) are completely broken. We shouldn't follow them. There are a lot more terminals around than just "dumb", "xterm" and "linux". It is completely non-acceptable to just assume ANSI escape sequences work. If Android doesn't ship a usable terminfo implementation, I consider that an Android bug. Wouldn't be the first portability nightmare with Android.

I completely disagree with your disagreement. Also, this is not an android issue.

Again note that Clang doesn't even _use_ the terminfo database, it just checks it to see if it has an attribute named "colors". Of course this is totally wrong if you are actually using a non-VTxxx-compatible terminal that supports colors. Which I assure you, do exist. Of course, nobody _uses_ any of these other crazy non-VTxxx terminals. (No, I don't care if you have such a paperweight sitting under your desk...) We should delete this code.

Nevertheless, the patch proposed here to add a fallback case is clearly fine, at a minimum. So, fine, let's not let a side-argument about removing useless code hold this up.

This revision is now accepted and ready to land.Jan 18 2018, 3:09 PM
joerg requested changes to this revision.Jan 18 2018, 3:17 PM

That's no excuse for making the situation even worse.

This revision now requires changes to proceed.Jan 18 2018, 3:17 PM

Ehhhhh? This patch (without my proposed change of entirely removing the HAVE_TERMINFO codepath) certainly does not.

joerg added a comment.Jan 18 2018, 3:46 PM

Do you see the comment just following the code? The patch completely violates that basic design principle. It would be perfectly sensible to hard-code a list of dumb terminals and explicitly default to no colors for them. The reverse (hard-coding a list and assuming it is fine for everything else) is not.

jyknight removed a subscriber: jyknight.Jan 18 2018, 4:04 PM

You win by virtue of using up my willingness to argue on this subject.

phosek updated this revision to Diff 130548.Jan 18 2018, 7:18 PM
phosek edited the summary of this revision. (Show Details)

I've implemented what @joerg suggested, is this acceptable?

joerg accepted this revision.Jan 19 2018, 2:36 AM

Good enough for me.

This revision is now accepted and ready to land.Jan 19 2018, 2:36 AM
This revision was automatically updated to reflect the committed changes.