agora inbox for pgsql-bugs@postgresql.org
help / color / mirror / Atom feedBUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
14+ messages / 6 participants
[nested] [flat]
* BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
@ 2019-11-12 08:29 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 14+ messages in thread
From: PG Bug reporting form @ 2019-11-12 08:29 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: tanghy.fnst@cn.fujitsu.com
The following bug has been logged on the website:
Bug reference: 16108
Logged by: Haiying Tang
Email address: tanghy.fnst@cn.fujitsu.com
PostgreSQL version: 12.0
Operating system: Windows
Description:
Hello
I found the following release notes in PG12 is not working properly at
Windows.
> •Add colorization to the output of command-line utilities
Following the release note, I've set the the environment variable PG_COLOR
to auto, then I run pg_dump command with an incorrect passwd.
However, the command-line output is not colorized as the release notes
said.
Before PG_COLOR=auto is set: pg_dump: error: connection to database
"tanghy.fnst" failed: FATAL:
After PG_COLOR=auto is set: [01mpg_dump: [0m[01;31merror: [0mconnection
to database "tanghy.fnst" failed: FATAL
I think the colorization to the output of command-line is not supported at
Windows.
Maybe function "pg_logging_init" at source "src\common\logging.c" should add
a platform check.
Besides, the related release note of PG12 should add some description about
it.
Best Regards,
Tang
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
@ 2019-11-12 08:39 Thomas Munro <thomas.munro@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Munro @ 2019-11-12 08:39 UTC (permalink / raw)
To: PG Bug reporting form <noreply@postgresql.org>; +Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; tanghy.fnst@cn.fujitsu.com
On Tue, Nov 12, 2019 at 9:30 PM PG Bug reporting form
<noreply@postgresql.org> wrote:
> The following bug has been logged on the website:
>
> Bug reference: 16108
> Logged by: Haiying Tang
> Email address: tanghy.fnst@cn.fujitsu.com
> PostgreSQL version: 12.0
> Operating system: Windows
> Description:
>
> Hello
>
> I found the following release notes in PG12 is not working properly at
> Windows.
> > •Add colorization to the output of command-line utilities
>
> Following the release note, I've set the the environment variable PG_COLOR
> to auto, then I run pg_dump command with an incorrect passwd.
> However, the command-line output is not colorized as the release notes
> said.
>
> Before PG_COLOR=auto is set: pg_dump: error: connection to database
> "tanghy.fnst" failed: FATAL:
> After PG_COLOR=auto is set: [01mpg_dump: [0m [01;31merror: [0mconnection
> to database "tanghy.fnst" failed: FATAL
>
> I think the colorization to the output of command-line is not supported at
> Windows.
> Maybe function "pg_logging_init" at source "src\common\logging.c" should add
> a platform check.
> Besides, the related release note of PG12 should add some description about
> it.
Based on this:
https://en.wikipedia.org/wiki/ANSI_escape_code#DOS_and_Windows
... I wonder if it works if you use the new Windows Terminal, and I
wonder if it would work on the older thing if we used the
SetConsoleMode() flag it mentions.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
@ 2019-11-12 18:59 Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
parent: Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Juan José Santamaría Flecha @ 2019-11-12 18:59 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: PG Bug reporting form <noreply@postgresql.org>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; tanghy.fnst@cn.fujitsu.com
On Tue, Nov 12, 2019 at 9:39 AM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> ... I wonder if it works if you use the new Windows Terminal, and I
> wonder if it would work on the older thing if we used the
> SetConsoleMode() flag it mentions.
>
>
In order to make it work both things are needed, setting the console mode
and a terminal that supports it. Please find attached a patch for so.
Regards,
Juan José Santamaría Flecha
Attachments:
[application/octet-stream] 0001-command-line-colorization-on-windows.patch (1.1K, ../../CAC+AXB2LDsO_vB-yNW8b9VBW=onGPwJiUuxAQ23TDcm8JbJ8NQ@mail.gmail.com/3-0001-command-line-colorization-on-windows.patch)
download | inline diff:
diff --git a/src/common/logging.c b/src/common/logging.c
index 895da71..655db70 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -32,6 +32,31 @@ static const char *sgr_locus = NULL;
#define ANSI_ESCAPE_FMT "\x1b[%sm"
#define ANSI_ESCAPE_RESET "\x1b[0m"
+#ifdef WIN32
+/*
+ * Check Windows support for VT100
+ */
+static bool
+enable_vt_mode()
+{
+ /* Check stderr */
+ HANDLE hOut = GetStdHandle(STD_ERROR_HANDLE);
+ DWORD dwMode = 0;
+
+ if (hOut == INVALID_HANDLE_VALUE)
+ return false;
+
+ if (!GetConsoleMode(hOut, &dwMode))
+ return false;
+
+ dwMode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
+ if (!SetConsoleMode(hOut, dwMode))
+ return false;
+
+ return true;
+}
+#endif
+
/*
* This should be called before any output happens.
*/
@@ -49,8 +74,13 @@ pg_logging_init(const char *argv0)
if (pg_color_env)
{
+#ifdef WIN32
+ bool vt_mode = enable_vt_mode();
+#else
+ bool vt_mode = isatty(fileno(stderr));
+#endif
if (strcmp(pg_color_env, "always") == 0 ||
- (strcmp(pg_color_env, "auto") == 0 && isatty(fileno(stderr))))
+ (strcmp(pg_color_env, "auto") == 0 && vt_mode))
log_color = true;
}
^ permalink raw reply [nested|flat] 14+ messages in thread
* RE: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
@ 2019-11-15 04:23 Tang, Haiying <tanghy.fnst@cn.fujitsu.com>
parent: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Tang, Haiying @ 2019-11-15 04:23 UTC (permalink / raw)
To: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>; Thomas Munro <thomas.munro@gmail.com>; +Cc: PG Bug reporting form <noreply@postgresql.org>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
>In order to make it work both things are needed, setting the console mode and a terminal that supports it.
Your patch worked fine on windows which supports VT100. But the bug still happened when set PG_COLOR="always" at Windows Terminal that not support VT100. Please see the attached file “Test_result.png” for the NG result. (I used win7 for this test)
To fix the above bug, I made some change to your patch. The new one works fine on my win7(VT100 not support) and win10(VT100 support).
Also, in this new patch(v1), I added some doc change for Windows not support Colorization. Please find the attached patch for so.
Regards,
Tang
From: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
Sent: Wednesday, November 13, 2019 4:00 AM
To: Thomas Munro <thomas.munro@gmail.com>
Cc: PG Bug reporting form <noreply@postgresql.org>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Tang, Haiying/唐 海英 <tanghy.fnst@cn.fujitsu.com>
Subject: Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
On Tue, Nov 12, 2019 at 9:39 AM Thomas Munro <thomas.munro@gmail.com<mailto:thomas.munro@gmail.com>> wrote:
... I wonder if it works if you use the new Windows Terminal, and I
wonder if it would work on the older thing if we used the
SetConsoleMode() flag it mentions.
In order to make it work both things are needed, setting the console mode and a terminal that supports it. Please find attached a patch for so.
Regards,
Juan José Santamaría Flecha
Attachments:
[application/octet-stream] v1-0001-command-line-colorization-on-windows.patch (1.6K, ../../43AA0560298D1A4893896FDC52840EA1D9C74F7D@G08CNEXMBPEKD03.g08.fujitsu.local/3-v1-0001-command-line-colorization-on-windows.patch)
download | inline diff:
diff --git a/doc/src/sgml/release-12.sgml b/doc/src/sgml/release-12.sgml
index 68949b2026..26b94bf4e4 100644
--- a/doc/src/sgml/release-12.sgml
+++ b/doc/src/sgml/release-12.sgml
@@ -3825,6 +3825,10 @@ Author: Peter Eisentraut <peter@eisentraut.org>
For example, the default behavior is equivalent to
<literal>PG_COLORS="error=01;31:warning=01;35:locus=01"</literal>.
</para>
+
+ <para>
+ Notably, VT100 support is required for the colorization on Windows.
+ </para>
</listitem>
</itemizedlist>
diff --git a/src/common/logging.c b/src/common/logging.c
index 895da7150e..8cb59fb609 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -32,6 +32,31 @@ static const char *sgr_locus = NULL;
#define ANSI_ESCAPE_FMT "\x1b[%sm"
#define ANSI_ESCAPE_RESET "\x1b[0m"
+#ifdef WIN32
+/*
+ * Check Windows support for VT100
+ */
+static bool
+enable_vt_mode()
+{
+ /* Check stderr */
+ HANDLE hOut = GetStdHandle(STD_ERROR_HANDLE);
+ DWORD dwMode = 0;
+
+ if (hOut == INVALID_HANDLE_VALUE)
+ return false;
+
+ if (!GetConsoleMode(hOut, &dwMode))
+ return false;
+
+ dwMode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
+ if (!SetConsoleMode(hOut, dwMode))
+ return false;
+
+ return true;
+}
+#endif
+
/*
* This should be called before any output happens.
*/
@@ -47,6 +72,13 @@ pg_logging_init(const char *argv0)
progname = get_progname(argv0);
__pg_log_level = PG_LOG_INFO;
+#ifdef WIN32
+ bool vt_mode = enable_vt_mode();
+
+ if(!vt_mode)
+ pg_color_env = NULL;
+#endif
+
if (pg_color_env)
{
if (strcmp(pg_color_env, "always") == 0 ||
[image/png] Test_result.png (86.8K, ../../43AA0560298D1A4893896FDC52840EA1D9C74F7D@G08CNEXMBPEKD03.g08.fujitsu.local/4-Test_result.png)
download | view image
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
@ 2019-11-15 08:14 Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
parent: Tang, Haiying <tanghy.fnst@cn.fujitsu.com>
0 siblings, 1 reply; 14+ messages in thread
From: Juan José Santamaría Flecha @ 2019-11-15 08:14 UTC (permalink / raw)
To: Tang, Haiying <tanghy.fnst@cn.fujitsu.com>; +Cc: Thomas Munro <thomas.munro@gmail.com>; PG Bug reporting form <noreply@postgresql.org>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Thanks for testing. I am opening a new item in the next commitfest for this
topic.
On Fri, Nov 15, 2019 at 5:23 AM Tang, Haiying <tanghy.fnst@cn.fujitsu.com>
wrote:
> >In order to make it work both things are needed, setting the console mode
> and a terminal that supports it.
>
>
>
> Your patch worked fine on windows which supports VT100. But the bug still
> happened when set PG_COLOR="always" at Windows Terminal that not support
> VT100. Please see the attached file “Test_result.png” for the NG result. (I
> used win7 for this test)
>
>
> To fix the above bug, I made some change to your patch. The new one works
> fine on my win7(VT100 not support) and win10(VT100 support).
>
My understanding of the "always" logic is that it has to be enabled no
matter what, even if not supported in current output.
Also, in this new patch(v1), I added some doc change for Windows not
> support Colorization. Please find the attached patch for so.
>
>
>
You cannot change the release notes, if anything it will be added to 12.2
patch notes. It should be added to the 21 (!) utilities that specify the
PG_COLOR usage, but I am not so sure that adding a note stating this
feature requires Windows 10 >= 1511 update is really a Postgres business.
Please find attached a version that supports older Mingw versions and SDKs.
Regards,
Juan José Santamaría Flecha
Attachments:
[application/octet-stream] v2-0001-command-line-colorization-on-windows.patch (1.2K, ../../CAC+AXB1yZXb_=DzRoPV8jY0cUX5EFYLHtmOgu7is3d3M31GppQ@mail.gmail.com/3-v2-0001-command-line-colorization-on-windows.patch)
download | inline diff:
diff --git a/src/common/logging.c b/src/common/logging.c
index 895da71..6c82fb6 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -32,6 +32,36 @@ static const char *sgr_locus = NULL;
#define ANSI_ESCAPE_FMT "\x1b[%sm"
#define ANSI_ESCAPE_RESET "\x1b[0m"
+#ifdef WIN32
+
+#ifndef ENABLE_VIRTUAL_TERMINAL_PROCESSING
+#define ENABLE_VIRTUAL_TERMINAL_PROCESSING 0x0004
+#endif
+
+/*
+ * Check Windows support for VT100
+ */
+static bool
+enable_vt_mode()
+{
+ /* Check stderr */
+ HANDLE hOut = GetStdHandle(STD_ERROR_HANDLE);
+ DWORD dwMode = 0;
+
+ if (hOut == INVALID_HANDLE_VALUE)
+ return false;
+
+ if (!GetConsoleMode(hOut, &dwMode))
+ return false;
+
+ dwMode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
+ if (!SetConsoleMode(hOut, dwMode))
+ return false;
+
+ return true;
+}
+#endif
+
/*
* This should be called before any output happens.
*/
@@ -49,8 +79,13 @@ pg_logging_init(const char *argv0)
if (pg_color_env)
{
+#ifdef WIN32
+ bool is_terminal = enable_vt_mode();
+#else
+ bool is_terminal = isatty(fileno(stderr));
+#endif
if (strcmp(pg_color_env, "always") == 0 ||
- (strcmp(pg_color_env, "auto") == 0 && isatty(fileno(stderr))))
+ (strcmp(pg_color_env, "auto") == 0 && is_terminal))
log_color = true;
}
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
@ 2020-02-18 22:39 Michail Nikolaev <michail.nikolaev@gmail.com>
parent: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
0 siblings, 2 replies; 14+ messages in thread
From: Michail Nikolaev @ 2020-02-18 22:39 UTC (permalink / raw)
To: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>; +Cc: Tang, Haiying <tanghy.fnst@cn.fujitsu.com>; Thomas Munro <thomas.munro@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hello everyone.
> Please find attached a version that supports older Mingw versions and SDKs.
I have checked the patch source code and it seems to be working. But a
few moments I want to mention:
I think it is not good idea to mix the logic of detecting the fact of
TTY with enabling of the VT100 mode. Yeah, it seems to be correct for
current case but a little confusing.
Maybe is it better to detect terminal using *isatty* and later call
*enable_vt_mode*?
Also, it seems like if GetConsoleMode returns
ENABLE_VIRTUAL_TERMINAL_PROCESSING flag already set - we could skip
SetConsoleMode call (not a big deal of course).
Thanks,
Michail.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
@ 2020-02-18 23:01 Michail Nikolaev <michail.nikolaev@gmail.com>
parent: Michail Nikolaev <michail.nikolaev@gmail.com>
1 sibling, 0 replies; 14+ messages in thread
From: Michail Nikolaev @ 2020-02-18 23:01 UTC (permalink / raw)
To: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>; +Cc: Tang, Haiying <tanghy.fnst@cn.fujitsu.com>; Thomas Munro <thomas.munro@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
P.S.
Also, should we enable vt100 mode in case of PG_COLOR=always? I think yes.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
@ 2020-02-19 16:16 Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
parent: Michail Nikolaev <michail.nikolaev@gmail.com>
1 sibling, 1 reply; 14+ messages in thread
From: Juan José Santamaría Flecha @ 2020-02-19 16:16 UTC (permalink / raw)
To: Michail Nikolaev <michail.nikolaev@gmail.com>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: Tang, Haiying <tanghy.fnst@cn.fujitsu.com>; Thomas Munro <thomas.munro@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Feb 18, 2020 at 11:39 PM Michail Nikolaev <
michail.nikolaev@gmail.com> wrote:
>
> I have checked the patch source code and it seems to be working. But a
> few moments I want to mention:
>
Thanks for looking into this.
> I think it is not good idea to mix the logic of detecting the fact of
> TTY with enabling of the VT100 mode. Yeah, it seems to be correct for
> current case but a little confusing.
> Maybe is it better to detect terminal using *isatty* and later call
> *enable_vt_mode*?
>
Most of what enable_vt_mode() does is actually detecting the terminal, but
I can see why that is confusing without better comments.
> Also, it seems like if GetConsoleMode returns
> ENABLE_VIRTUAL_TERMINAL_PROCESSING flag already set - we could skip
> SetConsoleMode call (not a big deal of course).
>
Agreed.
The patch about making color by default [1] introduces the
function terminal_supports_color(), that I think is relevant for this
issue. Please find attached a new version based on that idea.
Also, adding Peter to weight on this approach.
[1] https://commitfest.postgresql.org/27/2406/
Regards,
Juan José Santamaría Flecha
Attachments:
[application/octet-stream] v3-0001-command-line-colorization-on-windows.patch (2.3K, ../../CAC+AXB3cTZKR8ry-T6-ui0qLMij+K_auxO9Lq9dHy1DBj+HDHQ@mail.gmail.com/3-v3-0001-command-line-colorization-on-windows.patch)
download | inline diff:
diff --git a/src/common/logging.c b/src/common/logging.c
index c78ae79..8c0377f 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -32,6 +32,60 @@ static const char *sgr_locus = NULL;
#define ANSI_ESCAPE_FMT "\x1b[%sm"
#define ANSI_ESCAPE_RESET "\x1b[0m"
+#define str_starts_with(str, substr) (strncmp(str, substr, strlen(substr)) == 0)
+#define str_ends_with(str, substr) (strcmp(str + strlen(str) - strlen(substr), substr) == 0)
+#ifndef ENABLE_VIRTUAL_TERMINAL_PROCESSING
+#define ENABLE_VIRTUAL_TERMINAL_PROCESSING 0x0004
+#endif
+
+static bool
+terminal_supports_color(void)
+{
+#ifndef WIN32
+ const char *term_env = getenv("TERM");
+
+ if (!term_env)
+ return false;
+ else if (strcmp(term_env, "ansi") == 0)
+ return true;
+ else if (strcmp(term_env, "cygwin") == 0)
+ return true;
+ else if (strcmp(term_env, "linux") == 0)
+ return true;
+ else if (str_starts_with(term_env, "rxvt"))
+ return true;
+ else if (str_starts_with(term_env, "screen"))
+ return true;
+ else if (str_starts_with(term_env, "xterm"))
+ return true;
+ else if (str_starts_with(term_env, "vt100"))
+ return true;
+ else if (str_ends_with(term_env, "color"))
+ return true;
+ else
+ return false;
+#else
+/*
+ * Windows supports color on console's VT100 mode.
+ * It is disabled by default, so it must be enabled to use color outpout.
+ */
+ /* Check stderr */
+ HANDLE hOut = GetStdHandle(STD_ERROR_HANDLE);
+ DWORD dwMode = 0;
+
+ if (hOut == INVALID_HANDLE_VALUE)
+ return false;
+ if (!GetConsoleMode(hOut, &dwMode))
+ return false;
+ if (dwMode & ENABLE_VIRTUAL_TERMINAL_PROCESSING)
+ return true;
+ dwMode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
+ if (!SetConsoleMode(hOut, dwMode))
+ return false;
+ return true;
+#endif
+}
+
/*
* This should be called before any output happens.
*/
@@ -40,6 +94,8 @@ pg_logging_init(const char *argv0)
{
const char *pg_color_env = getenv("PG_COLOR");
bool log_color = false;
+ bool color_terminal = isatty(fileno(stderr)) &&
+ terminal_supports_color();
/* usually the default, but not on Windows */
setvbuf(stderr, NULL, _IONBF, 0);
@@ -50,7 +106,7 @@ pg_logging_init(const char *argv0)
if (pg_color_env)
{
if (strcmp(pg_color_env, "always") == 0 ||
- (strcmp(pg_color_env, "auto") == 0 && isatty(fileno(stderr))))
+ (strcmp(pg_color_env, "auto") == 0 && color_terminal))
log_color = true;
}
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
@ 2020-02-22 20:08 Michail Nikolaev <michail.nikolaev@gmail.com>
parent: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Michail Nikolaev @ 2020-02-22 20:08 UTC (permalink / raw)
To: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>; +Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Tang, Haiying <tanghy.fnst@cn.fujitsu.com>; Thomas Munro <thomas.munro@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hello.
> The patch about making color by default [1] introduces the function terminal_supports_color(), that I think is relevant for this issue. Please find attached a new version based on that idea.
I am not sure it is good idea to mix both patches because it adds some
confusion and makes it harder to merge each.
Maybe is it better to update current patch the way to reuse some
function later in [1]?
Also, regarding comment
> It is disabled by default, so it must be enabled to use color outpout.
It is not true for new terminal, for example. Maybe it is better to
rephrase it to something like: "Check if TV100 support if enabled and
attempt to enable if not".
[1] https://www.postgresql.org/message-id/flat/bbdcce43-bd2e-5599-641b-9b44b9e0add4@2ndquadrant.com
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
@ 2020-02-24 17:56 Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
parent: Michail Nikolaev <michail.nikolaev@gmail.com>
0 siblings, 2 replies; 14+ messages in thread
From: Juan José Santamaría Flecha @ 2020-02-24 17:56 UTC (permalink / raw)
To: Michail Nikolaev <michail.nikolaev@gmail.com>; +Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Tang, Haiying <tanghy.fnst@cn.fujitsu.com>; Thomas Munro <thomas.munro@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Sat, Feb 22, 2020 at 9:09 PM Michail Nikolaev <michail.nikolaev@gmail.com>
wrote:
>
> I am not sure it is good idea to mix both patches because it adds some
> confusion and makes it harder to merge each.
> Maybe is it better to update current patch the way to reuse some
> function later in [1]?
>
The patch was originaly reported for Windows, but looking into Peter's
patch, I think this issue affects other systems unless we use stricter
logic to detect a colorable terminal when using the "auto" option.
Probably, the way to go is leaving this patch as WIN32 only and thinking
about a future patch.
> Also, regarding comment
> > It is disabled by default, so it must be enabled to use color outpout.
>
> It is not true for new terminal, for example. Maybe it is better to
> rephrase it to something like: "Check if TV100 support if enabled and
> attempt to enable if not".
>
The logic I have seen on new terminals is that VT100 is supported but
disabled. Would you find clearer? "Attempt to enable VT100 sequence
processing. If it is not possible consider it as unsupported."
Please find attached a patch addressing these comments.
Regards,
Juan José Santamaría Flecha
Attachments:
[application/octet-stream] v4-0001-command-line-colorization-on-windows.patch (1.6K, ../../CAC+AXB0LSzuE7veUrw0-N=cxKEV_hfZTT_RaPMfMeRY3co-RQA@mail.gmail.com/3-v4-0001-command-line-colorization-on-windows.patch)
download | inline diff:
diff --git a/src/common/logging.c b/src/common/logging.c
index c78ae79..d59ec88 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -32,6 +32,35 @@ static const char *sgr_locus = NULL;
#define ANSI_ESCAPE_FMT "\x1b[%sm"
#define ANSI_ESCAPE_RESET "\x1b[0m"
+#ifdef WIN32
+#ifndef ENABLE_VIRTUAL_TERMINAL_PROCESSING
+#define ENABLE_VIRTUAL_TERMINAL_PROCESSING 0x0004
+#endif
+
+/*
+ * Attempt to enable VT100 sequence processing.
+ * If it is not possible consider it as unsupported.
+ */
+static bool
+enable_vt_processing(void)
+{
+ /* Check stderr */
+ HANDLE hOut = GetStdHandle(STD_ERROR_HANDLE);
+ DWORD dwMode = 0;
+
+ if (hOut == INVALID_HANDLE_VALUE)
+ return false;
+ if (!GetConsoleMode(hOut, &dwMode))
+ return false;
+ if (dwMode & ENABLE_VIRTUAL_TERMINAL_PROCESSING)
+ return true;
+ dwMode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
+ if (!SetConsoleMode(hOut, dwMode))
+ return false;
+ return true;
+}
+#endif /* WIN32 */
+
/*
* This should be called before any output happens.
*/
@@ -40,6 +69,10 @@ pg_logging_init(const char *argv0)
{
const char *pg_color_env = getenv("PG_COLOR");
bool log_color = false;
+ bool color_terminal = isatty(fileno(stderr));
+#ifdef WIN32
+ color_terminal = color_terminal && enable_vt_processing();
+#endif
/* usually the default, but not on Windows */
setvbuf(stderr, NULL, _IONBF, 0);
@@ -50,7 +83,7 @@ pg_logging_init(const char *argv0)
if (pg_color_env)
{
if (strcmp(pg_color_env, "always") == 0 ||
- (strcmp(pg_color_env, "auto") == 0 && isatty(fileno(stderr))))
+ (strcmp(pg_color_env, "auto") == 0 && color_terminal))
log_color = true;
}
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
@ 2020-02-26 10:48 Michail Nikolaev <michail.nikolaev@gmail.com>
parent: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
1 sibling, 1 reply; 14+ messages in thread
From: Michail Nikolaev @ 2020-02-26 10:48 UTC (permalink / raw)
To: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>; +Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Tang, Haiying <tanghy.fnst@cn.fujitsu.com>; Thomas Munro <thomas.munro@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hello.
Looks totally fine to me now.
So, I need to mark it as "ready to commiter", right?
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
@ 2020-02-26 10:58 Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
parent: Michail Nikolaev <michail.nikolaev@gmail.com>
0 siblings, 0 replies; 14+ messages in thread
From: Juan José Santamaría Flecha @ 2020-02-26 10:58 UTC (permalink / raw)
To: Michail Nikolaev <michail.nikolaev@gmail.com>; +Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Tang, Haiying <tanghy.fnst@cn.fujitsu.com>; Thomas Munro <thomas.munro@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, Feb 26, 2020 at 11:48 AM Michail Nikolaev <
michail.nikolaev@gmail.com> wrote:
>
> Looks totally fine to me now.
>
> So, I need to mark it as "ready to commiter", right?
>
Yes, that's right. Thanks for reviewing it.
Regards
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
@ 2020-03-02 06:48 Michael Paquier <michael@paquier.xyz>
parent: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
1 sibling, 1 reply; 14+ messages in thread
From: Michael Paquier @ 2020-03-02 06:48 UTC (permalink / raw)
To: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>; +Cc: Michail Nikolaev <michail.nikolaev@gmail.com>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Tang, Haiying <tanghy.fnst@cn.fujitsu.com>; Thomas Munro <thomas.munro@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Feb 24, 2020 at 06:56:05PM +0100, Juan José Santamaría Flecha wrote:
> The patch was originaly reported for Windows, but looking into Peter's
> patch, I think this issue affects other systems unless we use stricter
> logic to detect a colorable terminal when using the "auto" option.
> Probably, the way to go is leaving this patch as WIN32 only and thinking
> about a future patch.
It is better to not mix issues. You can actually bump on similar
coloring issues depending on your configuration, with OSX or even
Linux.
> The logic I have seen on new terminals is that VT100 is supported but
> disabled. Would you find clearer? "Attempt to enable VT100 sequence
> processing. If it is not possible consider it as unsupported."
>
> Please find attached a patch addressing these comments.
I was reading the thread for the first time, and got surprised first
with the argument about "always" which gives the possibility to print
incorrect characters even if the environment does not allow coloring.
However, after looking at logging.c, the answer is pretty clear what
always is about as it enforces colorization, so this patch looks
correct to me.
On top of that, and that's a separate issue, I have noticed that we
have exactly zero documentation about PG_COLORS (the plural flavor,
not the singular), but we have code for it in common/logging.c..
Anyway, committed down to 12, after tweaking a few things.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200302064842.GE32059@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
@ 2020-03-02 09:01 Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 14+ messages in thread
From: Juan José Santamaría Flecha @ 2020-03-02 09:01 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Michail Nikolaev <michail.nikolaev@gmail.com>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Tang, Haiying <tanghy.fnst@cn.fujitsu.com>; Thomas Munro <thomas.munro@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Mar 2, 2020 at 7:48 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On top of that, and that's a separate issue, I have noticed that we
> have exactly zero documentation about PG_COLORS (the plural flavor,
> not the singular), but we have code for it in common/logging.c..
>
Yeah, there is nothing about it prior to [1]. So, this conversation will
have to be carried over there.
> Anyway, committed down to 12, after tweaking a few things.
>
Thank you.
[1]
https://www.postgresql.org/message-id/bbdcce43-bd2e-5599-641b-9b44b9e0add4@2ndquadrant.com
Regards,
Juan José Santamaría Flecha
^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2020-03-02 09:01 UTC | newest]
Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-11-12 08:29 BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform PG Bug reporting form <noreply@postgresql.org>
2019-11-12 08:39 ` Thomas Munro <thomas.munro@gmail.com>
2019-11-12 18:59 ` Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
2019-11-15 04:23 ` Tang, Haiying <tanghy.fnst@cn.fujitsu.com>
2019-11-15 08:14 ` Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
2020-02-18 22:39 ` Michail Nikolaev <michail.nikolaev@gmail.com>
2020-02-18 23:01 ` Michail Nikolaev <michail.nikolaev@gmail.com>
2020-02-19 16:16 ` Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
2020-02-22 20:08 ` Michail Nikolaev <michail.nikolaev@gmail.com>
2020-02-24 17:56 ` Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
2020-02-26 10:48 ` Michail Nikolaev <michail.nikolaev@gmail.com>
2020-02-26 10:58 ` Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
2020-03-02 06:48 ` Michael Paquier <michael@paquier.xyz>
2020-03-02 09:01 ` Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox