GNOME Bugzilla – Bug 674320
Various improvements for logging on W32
Last modified: 2013-07-18 12:33:50 UTC
1) Log coloring on W32 uses W32 console API. Which is fine when you're logging into console, but NOT fine when you need to log colored output into a file for later viewing. On W32 such file can be viewed in MSys (cat, less, vim etc). Or it can be sent to someone running an OS with a terminal that understands these codes. W32 console API affects only the console; when logging into a file it produces colorless log. 2) On Windows (at least) __FILE__ is the absolute source filename. It might be a bug in the buildsystem, i wouldn't know. However, there's a workaround for it (calling basename()), but it's only enabled for MSVC. 3) I've noticed that after fixing (2) i've got black sources and filenames in colored logs. Black text doesn't work well on W32 when printed over black background. The code that is responsible for not printing black over black* checks that color definition is 0 as a way to detect black color. However, when color definition contains formatting (bold, underline) it becomes != 0, but its lower byte is still 0 (black), but the code doesn't catch that. Also, that code ignores underline format. *Actually, it's not true that W32 console can only have black background, black is just the default.
Created attachment 212279 [details] [review] Add the ability to force the use of UNIX colors
Created attachment 212280 [details] [review] 0002-Cut-down-src-file-names-for-MinGW-too.patch
Created attachment 212281 [details] [review] Fix black and underline coloring on W32
It all look fine to me except this: -gst_debug_set_colored (gboolean colored) +gst_debug_set_colored (gint colored) It should be something like: gst_debug_set_color_mode (gint mode); or even: typedef enum { GST_DEBUG_COLOR_MODE_OFF = 0, GST_DEBUG_COLOR_MODE_ON = 1, GST_DEBUG_COLOR_MODE_UNIX = 2 } GstDebugColorMode; gst_debug_set_color_mode (GstDebugColorMode mode); You want to change this?
Created attachment 212854 [details] [review] Add the ability to force the use of UNIX colors Adjusted the patch as suggested.
Review of attachment 212281 [details] [review]: ::: gst/gstinfo.c @@ +830,3 @@ }; /* we draw black as white, as cmd.exe can only have black bg */ If this comment is wrong, could you please correct it too.
Review of attachment 212854 [details] [review]: ::: gst/gst.c @@ +334,3 @@ + (gpointer) parse_goption_arg, N_("Force colored debugging output to " + "use UNIX terminal color codes"), + NULL}, This needs an update of the man page. @@ +576,1 @@ Shouldn't we also use the enum here. GST_DEBUG_COLOR_MODE={OFF,ON,UNIX}, same for the command line option.
Created attachment 220442 [details] [review] Add the ability to force the use of UNIX colors (with doc) Added doc changes That last comment about the use of enum was not entirely clear to me. Can you elaborate?
Created attachment 220443 [details] [review] Add the ability to force the use of UNIX colors (REALLY with doc, i swear) Uh, sorry. Wrong patch.
As for my comment about black background...it's more complicated that i thought (for example - i've failed to make a Windows console have persistent white background; it might be possible to achieve with edited palette, but that is too much of a trouble). I think it's better to let retain the comment.
OK, managed to make a black-text-on-white-background console, and make it to be persistent. However, running gst-anything with debug logging enabled on this console STILL outputs everything as colored-text-over-black-background-with-some-exceptions, i.e. the same thing you'd have if you had default black console. Once application finishes, console returns back to white. If anyone *cares* about debug gst ouput on non-black console on W32, i would encourage that person to bugzill it separately. And in current circumstances the comment is still valid.
Patches look good to me. Does anyone else has an opinion here?
Comment on attachment 220443 [details] [review] Add the ability to force the use of UNIX colors (REALLY with doc, i swear) First two patches look fine to me. I would like to see one single unified interface for all the debug color stuff. More precisely: - get rid of GST_DEBUG_NO_COLOR env var - don't add GST_DEBUG_FORCE_*_COLOR env var - add new GST_DEBUG_COLOR_MODE=xyz environment variable and then allow: COLOR_MODE={on,off,disable,auto,windows,unix} with on=auto and off=disable. Also: - rename gst_debug_set_color(mode) to gst_debug_set_color_mode(mode) as suggested by Wim in comment #4.
Created attachment 221547 [details] [review] Debug color mode option Adjusted as suggested. Note that i've decided to keep the "apply platform-specific logic when you get to do stuff" approach instead of defaulting coloring mode to the appropriate value depending on the platform. But i'm open to suggestions. "windows" mode is not there, because it's not portable. The code is always #ifdef G_OS_WIN32 no matter what, so setting --gst-debug-color-mode=windows on *nix would have had the effect of --gst-debug-color-mode=off. But again, if you think that mode naming symmetry is more important than uniform behaviour, then that is easily changeable.
Is there anything blocking pushing those patches ?
No, please do after another review
Isn't the 3rd patch breaking compatibility ? It's replacing GST_DEBUG_NO_COLOR with GST_DEBUG_COLOR_MODE ? The other ones look fine.
Well, it should be possible to retain the support for GST_DEBUG_NO_COLOR, just treat it as GST_DEBUG_COLOR_MODE=off. If it's compatibility that bothers you.
Yes, the third patch should be kept backwards compatible. Keep behaviour but if useful deprecate stuff
Created attachment 249490 [details] [review] Debug color mode option v2 Same as before, but GST_DEBUG_NO_COLOR and --gst-debug-no-color remain available. I've tried to fix documentation as well, but i'm not sure how much redundancy is allowed there.
commit 797fcd1d499348b77b21d4f21414e4898237fa00 Author: Руслан Ижбулатов <lrn1986@gmail.com> Date: Thu Jul 18 15:10:10 2013 +0400 info: Add debug color mode option This allows to explicitely set the debug output color mode to UNIX on every platform, enable it (use platform default color mode) or enable it. https://bugzilla.gnome.org/show_bug.cgi?id=674320 commit 46106ebe8f3fbe166575639936a8591d279fa746 Author: Руслан Ижбулатов <lrn1986@gmail.com> Date: Wed Apr 18 14:35:32 2012 +0400 info: Fix black and underline coloring on W32 Fixes #674320 commit 143f30bfcb6251e38d7a855dde6f92d47d189cfe Author: Руслан Ижбулатов <lrn1986@gmail.com> Date: Wed Apr 18 14:12:16 2012 +0400 info: Cut down src file names for MinGW too Fixes #674320