GNOME Bugzilla – Bug 775468
Improve log write supports color method on windows
Last modified: 2018-05-24 19:17:40 UTC
This tries to improve the color support detection for windows. From Windows 10 it is suppose to support ansi colors so support it only in that case. In the future we might also want to try to detect if we are using bash on windows.
Created attachment 341151 [details] [review] Improve log write supports color method on windows
Some code could be lifted from this thread https://www.mail-archive.com/mingw-w64-public@lists.sourceforge.net/msg12928.html
Review of attachment 341151 [details] [review]: Breaks colour support in bash (and Powershell, ansicon?) in Windows 7, surely?
(In reply to Andrew Chadwick from comment #3) > Review of attachment 341151 [details] [review] [review]: > > Breaks colour support in bash (and Powershell, ansicon?) in Windows 7, > surely? Well initially I prefer to break support for bash, powershell etc if I avoid having weird chars on the command line of Windows 7
Created attachment 341206 [details] image of glib-using program run Hi, I think this issues is more like something a change within the isatty() call in Windows 10 or so, as this problem began to show up since the anniversary update, if I recall correctly. I vaguely recall that the original code does work in Windows 7 and 8.1, but I no longer have installations of those to re-verify this point. It also seems that it does not sometimes happen (see the attached image--it shows the situation when g-ir-compiler is being run[OK] and what shows up when one displays the About dialogue by hitting Help->About in the builder demoin gtk3-demo [funny message]). Perhaps [1][2] are worth a look in this issue, IMHO. [1]: https://github.com/fatih/color/issues/32 [2]: http://www.nivot.org/blog/post/2016/02/04/Windows-10-TH2-%28v1511%29-Console-Host-Enhancements My take at this, with blessings, thank you!
(Hmm, suddenly my English became really bad, sorry... the comment should read: "I think the issue is more like a change in the isatty() call/console subsystem of Windows 10 or so...<snip> It also seems that it sometimes does not happen...<snip>")
Created attachment 341212 [details] [review] gmessages.c/Windows: Improve g_log_writer_supports_color() Hi, This is my patch for this issue on Windows 10. I do need help from people still running Windows 7/8/8.1 to see whether colors show up in their terminals with the isatty() call. With blessings, thank you, and cheers!
Review of attachment 341212 [details] [review]: ::: glib/gmessages.c @@ +1881,3 @@ + + /* Disable the message box for assertions. */ + _CrtSetReportMode(_CRT_ASSERT, 0); Do we do anything with oldHandler? Like, maybe set it back? The situation with crt report mode is exactly opposite: we do set it back, but to a hardcoded value instead of the value it had previously (is there _CrtGetReportMode?).
Created attachment 341214 [details] [review] gmessages.c/Windows: Improve g_log_writer_supports_color() (take ii) Hi LRN, Indeed we should try to use the old mode (or at least the modes we have for the type we feed to _CrtSetReportMode()), which is actually retrieved by _CrtSetReportMode() in its first call, according to MSDN docs[1]. So here we go. Also, the new patch fixes some formatting bits and removed an 'extern'. [1]: https://msdn.microsoft.com/en-us/library/1y71x448(v=vs.90).aspx With blesings, thank you!
Created attachment 341215 [details] [review] gmessages.c/Windows: Improve g_log_writer_supports_color() (take ii) (oops, sorry, forgot to run git format-patch... d'oh)
Review of attachment 341215 [details] [review]: ::: glib/gmessages.c @@ +1879,3 @@ + _invalid_parameter_handler oldHandler, newHandler; + newHandler = myInvalidParameterHandler; + oldHandler = _set_invalid_parameter_handler (newHandler); Still missing a _set_invalid_parameter_handler (oldHandler); somewhere further down @@ +1902,3 @@ + return FALSE; + + GetConsoleMode (h_output, &dw_mode); What if GetConsoleMode fails? For example, if h_output is not a console handle at all (previous checks for h_output == h_stdout or h_stderr are not indicative, as std handles could also be non-console).
Review of attachment 341215 [details] [review]: More comments from my side ::: glib/gmessages.c @@ +218,3 @@ +#include <crtdbg.h> + +void should it be static? @@ +225,3 @@ + uintptr_t pReserved) +{ + return; no need for this return @@ +1865,3 @@ +#ifdef G_OS_WIN32 + if (!g_win32_check_windows_version (10, 0, 0, G_WIN32_OS_ANY)) + return isatty (output_fd); the problem of this is that on windows 7 we will see weird chars as it happens now on the console, which means we have to detect somehow if we are under mintty or something. IMHO we should just disable colors on something older than windows 10. At least for now until we have a good solution
Created attachment 341217 [details] [review] gmessages.c/Windows: Improve g_log_writer_supports_color() (take iii) Hi Nacho/LRN, (In reply to Ignacio Casal Quinteiro (nacho) from comment #12) > ::: glib/gmessages.c > @@ +218,3 @@ > +#include <crtdbg.h> > + > +void > > should it be static? I actually made it _GLIB_EXTERN instead since this is actually the same stub that was used to fix gspawn-win32-helper on Visual Studio 2005+ due to the use of _get_osfhandle(), so I think we might as well share it, since that helper program already depends on the GLib DLL. > > @@ +1865,3 @@ > +#ifdef G_OS_WIN32 > + if (!g_win32_check_windows_version (10, 0, 0, G_WIN32_OS_ANY)) > + return isatty (output_fd); > > the problem of this is that on windows 7 we will see weird chars as it > happens now on the console, which means we have to detect somehow if we are > under mintty or something. IMHO we should just disable colors on something > older than windows 10. At least for now until we have a good solution Oh, I see, thanks for the info as I don't have Windows 7 around anymore to try this out. Memory didn't serve me right apparently:| The other issues that was pointed out should be addressed at this point. With blessings, thank you, and cheers!
Review of attachment 341217 [details] [review]: I think the comment about the support might also need some fixes as the comment I added on my patch ::: glib/gmessages.c @@ +1909,3 @@ + return TRUE; + } + else no need for else if we are returning already inside of the if
Testing results: This works (output is colored) on Windows 10 with Cygwin, MSYS2 and cmd, when using Windows Console or ConEmu. Does not output color codes when output is redirected into a disk file (might be a desirable _optional_ feature; for example, one might use a file viewing program that supports color codes). Does not work (output is not colored) when using Cygwin mintty. Does not output color codes when piping output into "less" program. Also, might be a duplicate of bug 769859 (although *that* bug is about using Windows Console API, not color codes).
Hi, (In reply to LRN from comment #15) > Also, might be a duplicate of bug 769859 (although *that* bug is about using > Windows Console API, not color codes). I think we should make 769859 a dupe then, since this is about the same base issue. Let's make the items here (and there) more of a long-run thing here, and perhaps split the work in parts here, as we most probably want to have this work for Windows 7 and 8.1 as well. From the initial bug report in 769859 for references: --- Because currently coloring is achieved by inserting terminal codes that Windows console does not support and displays simply as weird characters. Outputting colored text can be achieved using two methods: 1) previous = GetConsoleScreenBufferInfo() -> SetConsoleTextAttribute(color) -> output text -> SetConsoleTextAttribute (previous) This requires locking, otherwise multithreaded output will be a mess (more so than it already is) 2) WriteConsoleOutput() This modifies console buffer directly and writes rectangular blocks into rectangular buffer space (no built-in line wrapping support). GStreamer uses (1). (in gstreamer/gst/gstinfo.c) Another thing of note: it might be desirable to be able to force glib to do coloring using terminal codes. This is useful when piping the output to `less' or other programs that *do* understand these codes. ---
*** Bug 769859 has been marked as a duplicate of this bug. ***
Created attachment 341383 [details] [review] gmessages.c/Windows: Improve g_log_writer_supports_color() (take iv) Hi, (In reply to LRN from comment #15) > > Does not output color codes when output is redirected into a disk file > (might be a desirable _optional_ feature; for example, one might use a file > viewing program that supports color codes). I think we need to investigate on this later. FIXME added for this. > Does not work (output is not colored) when using Cygwin mintty. The situation here IMHO is more complex, perhaps [1] has something to do with this. > Does not output color codes when piping output into "less" program. This should work now with this patch. We go back to isatty() in conjunction with this :). The formatting and FIXME issues should be fixed at this point. [1]: https://github.com/mintty/mintty/issues/482 With blessings, thank you!
Review of attachment 341383 [details] [review]: Let's get this in. Thanks a lot.
Hi Nacho, Thanks! I pushed the patch to master as 7a61a94. Wonder whether we push this to glib-2-50, as this exports (although strictly private) a small symbol to the GLib DLL (or should I just copy the stub into gmessages.c and therefore not export anything more)? With blessings, thank you!
Hey Fan, I would say go the safe way and copy the stub. Cheers.
Hello, > + * but not: > + * -Output in Cygwin via mintty (https://github.com/mintty/mintty/issues/482) Is this accurate? The linked thread states that it does work in Cygwin, but the problem is with MSYS2. Cheers!
Hi Nacho, I pushed the more conservative patch into glib-2-50 as 8d634fe. Thanks again! --- Hi Daniel, If you follow the threads in the link, you'll see the last 4 comments from MaSol and mintty, which would explain why. This is basically because mintty does not provide a Windows console environment, and the underlying implementation of Window CRTs isatty(). --- With blessings, and cheers!
(In reply to Fan, Chun-wei from comment #23) > Hi Daniel, > > If you follow the threads in the link, you'll see the last 4 comments from > MaSol and mintty, which would explain why. This is basically because mintty > does not provide a Windows console environment, and the underlying > implementation of Window CRTs isatty(). It's just the commit message I was unsure about - as it says this won't work in Cygwin, but the thread has a comment from "mintty" saying it works in Cygwin but it's MSYS2 that doesn't work. Just wanted to be sure I understood it all right! :)
I would like to, once again, point you in the direction of this mingw-w64-users thread (first message in the thread is here[1]), specifically - this post[2]. Is that name check applicable? [1] https://www.mail-archive.com/mingw-w64-public@lists.sourceforge.net/msg12928.html [2] https://www.mail-archive.com/mingw-w64-public@lists.sourceforge.net/msg12980.html
lrn: if having a iscygtty works I'd be ok having it as well
Created attachment 341535 [details] [review] Windows/tty emulators: Improve g_log_writer_supports_color() Hi, This adds support to detect tty emulators (such as mintty) on Windows Vista and later so that users of such terminals will see colors from the ANSI color codes. On Windows 10 we will still check for the results from the Windows CRT standard _isatty() before doing the check here. As NtQueryObject() is an internal API that is mentioned in MSDN to be subject to changes or removal, this uses GetFileInformationByHandleEx() to query the file/pipe name of the file handle from the file descriptor we have, and see whether the pattern of the name fits the pattern that we want for such terminal emulators--this is what vim uses. With blessings, thank you!
Review of attachment 341535 [details] [review]: Fine for me to push it only to master.
Actually, let's wait to see what LRN says here.
Now it does work (output is colored) when using Cygwin mintty. Push it.
Review of attachment 341535 [details] [review]: Hi Nacho/LRN, Glad the patch worked for you. I pushed the patch to master as 7518067. I think we should keep this open to support Windows 7/8/8.1 fully. With blessings, thank you, and cheers!
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1227.