Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On Mon, 2020-02-03 at 22:29 +, Bernd Edlinger wrote:
> On 2/3/20 10:05 PM, Segher Boessenkool wrote:
> > On Mon, Feb 03, 2020 at 08:16:52PM +, Bernd Edlinger wrote:
> > > So gnome terminal is a problem, since it depend heavily on the
> > > software
> > > version, VTE library, and gnome-terminal.
> > > Sometimes URLs are functional, sometimes competely buggy.
> > >
> > > But, wait a moment, here is the deal:
> > >
> > > I can detect old gnome terminals,
> > > they have COLORTERM=gnome-terminal (and produce garbage)
> > >
> > > but new gnome terminal with true URL-support have
> > >
> > > COLORTERM=truecolor
> > >
> > > So how about adding that to the detection logic ?
> >
> > It works on at least one of my older setups, too (will have to
> > check
> > the rest when I have time, unfortunately the weekend is just past).
> >
>
> Cool.
>
> so here is the next version, which removes tmux, and adds
> detection of old gnome-terminal, and linux console sessions,
> while also attempting to work with ssh sessions, where we
> do we have a bit less reliable information, but I would
> think that is still an improvement. I'd let TERM_URLS and
> GCC_URLS override the last two exceptions, as TERM=xterm
> can also mean, really xterm, but while that one does not
> print garbage, it does not handle the URLs either.
>
>
> How do you like it this way?
>
> Is it OK for trunk?
Thanks for the updated patch. I have various nitpicks inline below,
but I think that this has had enough iterations and ought to go into
master if you address the nitpicks (consider the fixes preapproved).
I manually tested the patch on my gnome-terminal and it continued to
give me working hyperlinks, and successfully disabled them within
screen.
> 2020-01-30 David Malcolm
> Bernd Edlinger
>
> PR 87488
> PR other/93168
> * config.in (DIAGNOSTICS_URLS_DEFAULT): New define.
> * configure.ac (--with-diagnostics-urls): New configuration
> option, based on --with-diagnostics-color.
> (DIAGNOSTICS_URLS_DEFAULT): New define.
> * config.h: Regenerate.
> * configure: Regenerate.
> * diagnostic.c (diagnostic_urls_init): Handle -1 for
> DIAGNOSTICS_URLS_DEFAULT from configure-time
> --with-diagnostics-urls=auto-if-env by querying for a GCC_URLS
> and TERM_URLS environment variable.
> * diagnostic-url.h (diagnostic_url_format): New enum type.
> (diagnostic_urls_enabled_p): rename to...
> (parse_env_vars_for_urls): ... this, and change return type.
> * diagnostic-color.c (parse_gcc_urls): New helper function.
> (auto_enable_urls): Disable URLs on xfce4-terminal, gnome-terminal,
> the linux console, and mingw.
> (parse_env_vars_for_urls): Adjust.
> * pretty-print.h (pretty_printer::show_urls): rename to...
> (pretty_printer::url_format): ... this, and change to enum.
> * pretty-print.c (pretty_printer::pretty_printer,
> pp_begin_url, pp_end_url, test_urls): Adjust.
> * doc/install.texi (--with-diagnostics-urls): Document the new
> configuration option.
The install.texi part of the patch also touches --with-diagnostics-
color, documenting the existing interaction with GCC_COLORS.
> * doc/invoke.texi (-fdiagnostics-urls): Add GCC_URLS and TERM_URLS
> vindex reference. Update description of defaults based on the above.
Likewise the invoke.texi part of the patch also updates the description
of how -fdiagnostics-color interacts with GCC_COLORS.
> ---
> gcc/config.in | 6 +++
> gcc/configure | 41 +++-
> gcc/configure.ac | 28 ++
> gcc/diagnostic-color.c | 102
> ++---
> gcc/diagnostic-url.h | 18 -
> gcc/diagnostic.c | 21 --
> gcc/doc/install.texi | 15 ++--
> gcc/doc/invoke.texi| 39 +--
> gcc/pretty-print.c | 44 ++---
> gcc/pretty-print.h | 5 ++-
> 10 files changed, 293 insertions(+), 26 deletions(-)
[...]
> @@ -239,20 +240,109 @@ colorize_init (diagnostic_color_rule_t rule)
> }
> }
>
> -/* Determine if URLs should be enabled, based on RULE.
> +/* Return URL_FORMAT_XXX which tells how we should emit urls
> + when in always mode.
> + We use GCC_URLS and if that is not defined TERM_URLS.
> + If neither is defined the feature is enabled by default. */
> +
> +static diagnostic_url_format
> +parse_env_vars_for_urls ()
> +{
> + const char *p;
> +
> + p = getenv ("GCC_URLS"); /* Plural! */
> + if (p == NULL)
> +p = getenv ("TERM_URLS");
> +
> + if (p == NULL)
> +return URL_FORMAT_DEFAULT;
> +
> + if (*p == '\0')
> +return URL_FORMAT_NONE;
> +
> + if (!strcmp (p, "no"))
> +return URL_FORMAT_NONE;
> +
> + if (!strcmp (p, "st"))
> +return URL_FORMAT_ST;
> +
> + if (!strcmp (p, "bel"))
> +return URL_FORMAT_BEL;
> +
> + return URL_FORMAT_DEFAULT;
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On 2/3/20 10:05 PM, Segher Boessenkool wrote:
> On Mon, Feb 03, 2020 at 08:16:52PM +, Bernd Edlinger wrote:
>> So gnome terminal is a problem, since it depend heavily on the software
>> version, VTE library, and gnome-terminal.
>> Sometimes URLs are functional, sometimes competely buggy.
>>
>> But, wait a moment, here is the deal:
>>
>> I can detect old gnome terminals,
>> they have COLORTERM=gnome-terminal (and produce garbage)
>>
>> but new gnome terminal with true URL-support have
>>
>> COLORTERM=truecolor
>>
>> So how about adding that to the detection logic ?
>
> It works on at least one of my older setups, too (will have to check
> the rest when I have time, unfortunately the weekend is just past).
>
Cool.
so here is the next version, which removes tmux, and adds
detection of old gnome-terminal, and linux console sessions,
while also attempting to work with ssh sessions, where we
do we have a bit less reliable information, but I would
think that is still an improvement. I'd let TERM_URLS and
GCC_URLS override the last two exceptions, as TERM=xterm
can also mean, really xterm, but while that one does not
print garbage, it does not handle the URLs either.
How do you like it this way?
Is it OK for trunk?
Thanks
Bernd.
From 35a6a850680907995e13dcd8497f5190710af0dd Mon Sep 17 00:00:00 2001
From: Bernd Edlinger
Date: Wed, 29 Jan 2020 15:31:10 +0100
Subject: [PATCH] PR 87488: Add --with-diagnostics-urls configuration option
2020-01-30 David Malcolm
Bernd Edlinger
PR 87488
PR other/93168
* config.in (DIAGNOSTICS_URLS_DEFAULT): New define.
* configure.ac (--with-diagnostics-urls): New configuration
option, based on --with-diagnostics-color.
(DIAGNOSTICS_URLS_DEFAULT): New define.
* config.h: Regenerate.
* configure: Regenerate.
* diagnostic.c (diagnostic_urls_init): Handle -1 for
DIAGNOSTICS_URLS_DEFAULT from configure-time
--with-diagnostics-urls=auto-if-env by querying for a GCC_URLS
and TERM_URLS environment variable.
* diagnostic-url.h (diagnostic_url_format): New enum type.
(diagnostic_urls_enabled_p): rename to...
(parse_env_vars_for_urls): ... this, and change return type.
* diagnostic-color.c (parse_gcc_urls): New helper function.
(auto_enable_urls): Disable URLs on xfce4-terminal, gnome-terminal,
the linux console, and mingw.
(parse_env_vars_for_urls): Adjust.
* pretty-print.h (pretty_printer::show_urls): rename to...
(pretty_printer::url_format): ... this, and change to enum.
* pretty-print.c (pretty_printer::pretty_printer,
pp_begin_url, pp_end_url, test_urls): Adjust.
* doc/install.texi (--with-diagnostics-urls): Document the new
configuration option.
* doc/invoke.texi (-fdiagnostics-urls): Add GCC_URLS and TERM_URLS
vindex reference. Update description of defaults based on the above.
---
gcc/config.in | 6 +++
gcc/configure | 41 +++-
gcc/configure.ac | 28 ++
gcc/diagnostic-color.c | 102 ++---
gcc/diagnostic-url.h | 18 -
gcc/diagnostic.c | 21 --
gcc/doc/install.texi | 15 ++--
gcc/doc/invoke.texi| 39 +--
gcc/pretty-print.c | 44 ++---
gcc/pretty-print.h | 5 ++-
10 files changed, 293 insertions(+), 26 deletions(-)
diff --git a/gcc/config.in b/gcc/config.in
index 4829286..01fb18d 100644
--- a/gcc/config.in
+++ b/gcc/config.in
@@ -76,6 +76,12 @@
#endif
+/* The default for -fdiagnostics-urls option */
+#ifndef USED_FOR_TARGET
+#undef DIAGNOSTICS_URLS_DEFAULT
+#endif
+
+
/* Define 0/1 if static analyzer feature is enabled. */
#ifndef USED_FOR_TARGET
#undef ENABLE_ANALYZER
diff --git a/gcc/configure b/gcc/configure
index 5fa565a..f55cdb8 100755
--- a/gcc/configure
+++ b/gcc/configure
@@ -1015,6 +1015,7 @@ enable_host_shared
enable_libquadmath_support
with_linker_hash_style
with_diagnostics_color
+with_diagnostics_urls
enable_default_pie
'
ac_precious_vars='build_alias
@@ -1836,6 +1837,11 @@ Optional Packages:
auto-if-env stands for -fdiagnostics-color=auto if
GCC_COLOR environment variable is present and
-fdiagnostics-color=never otherwise
+ --with-diagnostics-urls={never,auto,auto-if-env,always}
+ specify the default of -fdiagnostics-urls option
+ auto-if-env stands for -fdiagnostics-urls=auto if
+ GCC_URLS or TERM_URLS environment variable is
+ present and -fdiagnostics-urls=never otherwise
Some influential environment variables:
CC C compiler command
@@ -18974,7 +18980,7 @@ else
lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
lt_status=$lt_dlunknown
cat > conftest.$ac_ext <<_LT_EOF
-#line 18977 "configure"
+#line 18983 "configure"
#include "confdefs.h"
#if HAVE_DLFCN_H
@@ -19080,7 +19086,7 @@ else
lt_dlunknown=0;
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On Mon, Feb 03, 2020 at 08:16:52PM +, Bernd Edlinger wrote: > So gnome terminal is a problem, since it depend heavily on the software > version, VTE library, and gnome-terminal. > Sometimes URLs are functional, sometimes competely buggy. > > But, wait a moment, here is the deal: > > I can detect old gnome terminals, > they have COLORTERM=gnome-terminal (and produce garbage) > > but new gnome terminal with true URL-support have > > COLORTERM=truecolor > > So how about adding that to the detection logic ? It works on at least one of my older setups, too (will have to check the rest when I have time, unfortunately the weekend is just past). Segher
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On 2/3/20 9:26 PM, Jakub Jelinek wrote: > On Mon, Feb 03, 2020 at 08:16:52PM +, Bernd Edlinger wrote: >> Jakub, can you confirm that the COLORTERM on your working >> gnome-terminal is set to "truecolor" ? > > On the box where I have display attached to yes, but it isn't propagated > through ssh to the workstation that I do GCC development on, $COLORTERM > is unset there. Only $TERM is set there (to xterm-256color). > Yes, with an old gnome-terminal version (with display corruption), I see TERM=xterm COLORTERM=gnome-terminal When I use a serial terminal to log in a linux box, I have TERM=linux, and URLs do sometimes work, sometimes not, sometime cause glitches, that depends on the terminal where picocom is running. We could enable auto URLs with TERM=xterm-256color, Or better switch it off with TERM=linux and TERM=xterm, which leaves TERM=screen and TERM=screen.xterm-256color emit the URL escapes, since the screen can handle that as it looks now. I am not sure if this is too aggressive or not, what do you think? Thanks Bernd.
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On Mon, Feb 03, 2020 at 08:16:52PM +, Bernd Edlinger wrote: > Jakub, can you confirm that the COLORTERM on your working > gnome-terminal is set to "truecolor" ? On the box where I have display attached to yes, but it isn't propagated through ssh to the workstation that I do GCC development on, $COLORTERM is unset there. Only $TERM is set there (to xterm-256color). Jakub
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On 2/3/20 3:08 PM, Segher Boessenkool wrote: > On Sun, Feb 02, 2020 at 08:00:44AM +, Bernd Edlinger wrote: Okay, thanks. That is a strong indication that there is no need to interfere with screen, which proves that any auto-disabling should have a very specific terminal detection logic. >>> >>> Jakub says that he tested with a recent gnome-terminal. That works, of >>> course. Mnay other terminals will not, and switching what terminal is >>> attached to your screen session will not work well either, as far as I >>> can tell. >> >> I understood his statement, that the URLs are stripped from the data >> stream by screen and are no longer visible, even if the terminal would >> support them i.e. you connect from a gnome-terminal. > > It looks like tmux strips it as well. But in my earlier testing it did > not? Confused. Maybe this was the auto-detect thing, dunno. I guess > I'll test with more tmux versions when I find some more time for this. > Okay, I see, I will remove the tmux detection until further notice. >> But work fine when the compiler runs natively in a gnome-terminal. > > It is big garbage for me, both with bell (which is much worse on some > other terminals), and with the string terminator escape (ESC \). > So gnome terminal is a problem, since it depend heavily on the software version, VTE library, and gnome-terminal. Sometimes URLs are functional, sometimes competely buggy. But, wait a moment, here is the deal: I can detect old gnome terminals, they have COLORTERM=gnome-terminal (and produce garbage) but new gnome terminal with true URL-support have COLORTERM=truecolor So how about adding that to the detection logic ? Jakub, can you confirm that the COLORTERM on your working gnome-terminal is set to "truecolor" ? Obviously, my top priority is not breaking URLs on that working gnome version. Thanks Bernd.
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On Sun, Feb 02, 2020 at 08:00:44AM +, Bernd Edlinger wrote: > >> Okay, thanks. That is a strong indication that there is no need > >> to interfere with screen, which proves that any auto-disabling should > >> have a very specific terminal detection logic. > > > > Jakub says that he tested with a recent gnome-terminal. That works, of > > course. Mnay other terminals will not, and switching what terminal is > > attached to your screen session will not work well either, as far as I > > can tell. > > I understood his statement, that the URLs are stripped from the data > stream by screen and are no longer visible, even if the terminal would > support them i.e. you connect from a gnome-terminal. It looks like tmux strips it as well. But in my earlier testing it did not? Confused. Maybe this was the auto-detect thing, dunno. I guess I'll test with more tmux versions when I find some more time for this. > But work fine when the compiler runs natively in a gnome-terminal. It is big garbage for me, both with bell (which is much worse on some other terminals), and with the string terminator escape (ESC \). Segher
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On 2/2/20 1:28 AM, Sandra Loosemore wrote:
> On 2/1/20 8:43 AM, Bernd Edlinger wrote:
>
>> diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
>> index 6ffafac..1d3fec5 100644
>> --- a/gcc/doc/install.texi
>> +++ b/gcc/doc/install.texi
>> @@ -2097,9 +2097,18 @@ option (if not used explicitly on the command line).
>> @var{choice}
>> can be one of @samp{never}, @samp{auto}, @samp{always}, and
>> @samp{auto-if-env}
>> where @samp{auto} is the default. @samp{auto-if-env} means that
>> @option{-fdiagnostics-color=auto} will be the default if @code{GCC_COLORS}
>> -is present and non-empty in the environment, and
>> +is present and non-empty in the environment of the compiler, and
>> @option{-fdiagnostics-color=never} otherwise.
>>
>> +@item --with-diagnostics-urls=@var{choice}
>> +Tells GCC to use @var{choice} as the default for
>> @option{-fdiagnostics-urls=}
>> +option (if not used explicitly on the command line). @var{choice}
>> +can be one of @samp{never}, @samp{auto}, @samp{always}, and
>> @samp{auto-if-env}
>> +where @samp{auto} is the default. @samp{auto-if-env} means that
>> +@option{-fdiagnostics-urls=auto} will be the default if @env{GCC_URLS}
>> +or @env{TERM_URLS} is present and non-empty in the environment of the
>> +compiler, and @option{-fdiagnostics-urls=never} otherwise.
>> +
>
> It would be good to rewrite both of the above paragraphs to get rid of the
> future tense and put them in the active voice, like
>
> @samp{auto-if-env} makes ... the default if ...
>
done.
> Also @code{GCC_COLORS} ought to be @env{GCC_COLORS}, no?
>
right.
>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>> index b8ba8a3..59306bc 100644
>> --- a/gcc/doc/invoke.texi
>> +++ b/gcc/doc/invoke.texi
>> @@ -4032,14 +4032,41 @@ arguments in the C++ frontend.
>> @item -fdiagnostics-urls[=@var{WHEN}]
>> @opindex fdiagnostics-urls
>> @cindex urls
>> +@vindex GCC_URLS @r{environment variable}
>> +@vindex TERM_URLS @r{environment variable}
>> Use escape sequences to embed URLs in diagnostics. For example, when
>> @option{-fdiagnostics-show-option} emits text showing the command-line
>> option controlling a diagnostic, embed a URL for documentation of that
>> option.
>>
>> @var{WHEN} is @samp{never}, @samp{always}, or @samp{auto}.
>> -The default is @samp{auto}, which means to use URL escape sequences only
>> -when the standard error is a terminal.
>> +@samp{auto} means to use URL escape sequences only when the standard error
>> +is a terminal, and @env{TERM} is not set to "dumb".
>
> We should avoid literal quotes in the manual instead of proper markup. Maybe
> @samp{dumb}? Or is this trying to express something other than a literal
> string?
>
okay, in this case, maybe it gets clearer when I write:
"standard error is a terminal, and when not executing in an emacs shell."
I moved the "dumb" to a comment in the C-function doing that detection
now, and consider it an implementation detail for the emacs detection.
>> +
>> +The default depends on how the compiler has been configured.
>> +It can be any of the above @var{WHEN} options.
>> +
>> +GCC can also be configured (via the
>> +@option{--with-diagnostics-urls=auto-if-env} configure-time option)
>> +so that the default is affected by environment variables.
>> +Under such a configuration, GCC defaults to using @samp{auto}
>> +if either @env{GCC_URLS} or @env{TERM_URLS} environment variables are
>> +present and non-empty in the environment of the compiler, or @samp{never}
>> +if neither are.
>> +
>> +However, even with @option{-fdiagnostics-urls=always} the behavior will be
>
> s/will be/is/, unless this is something that hasn't been implemented yet. :-S
>
okay, thanks.
>> +dependent on those environment variables:
>> +If @env{GCC_URLS} set to empty or "no", do not embed URLs in diagnostics.
>
> s/set/is set/ ??
>
yes.
> And please avoid literal quotes in the text here too.
>
done.
>> +If set to "st", URLs use ST escape sequences.
>> +If set to "bel", the default, URLs use BEL escape sequences.
>
> I have no idea what ST and BEL escape sequences are
>
those are the acronyms of certain escape sequences, I added an explanation.
>> +Any other non-empty value enables the feature.
>> +If @env{GCC_URLS} is not set, use @env{TERM_URLS} as a fallback.
>> +
>> +At this time GCC tries to detect also a few terminals that are known to
>> +not implement the URL feature, and have an installed basis where the URL
>
> What is "an installed basis"?
>
I meant that old versions are still installed on a lot of places,
and we do not want to break them.
I re-worded that paragraph, to be hopefully clearer now.
>> +escapes are likely to mis-behave, i.e. print garbage on the screen.
>
> s/mis-behave/misbehave/
>
done.
>> +That list is currently xfce4-terminal and tmux and mingw, this can be
>> +overridden with the @option{-fdiagnostics-urls=always}.
>
> Are those things literal strings for something? How do you use that
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On 2/2/20 12:05 AM, Segher Boessenkool wrote: > On Sat, Feb 01, 2020 at 05:21:30PM +, Bernd Edlinger wrote: >> On 2/1/20 6:12 PM, Jakub Jelinek wrote: >>> On Sat, Feb 01, 2020 at 03:43:20PM +, Bernd Edlinger wrote: I seem to remember him saying that he always has to configure with --with-diagnostics-color=never, and the URLs are on top of that. But there was no configure option for that, which, given his explanation, made immediately sense to me. In the case of the xfce terminal, the color thing was always working fine, but beginning with last october, the warnings look just terrible. If that assumption turns out to be wrong, we can easily move that check from the auto_color to the auto_url code, or add more terminals which are of that kind. >>> >>> For me colors work just fine in screen, it really depends on which >>> terminals one is attaching the screen from. >>> URLs don't work, but show up exactly as if they were disabled, no visual nor >>> accoustic problems with those (and work fine in gnome-terminal which is >>> recent). >>> That said, my TERM is actually screen.xterm-256color rather than just screen >>> (and xterm-256color in gnome-terminal). >> >> Okay, thanks. That is a strong indication that there is no need >> to interfere with screen, which proves that any auto-disabling should >> have a very specific terminal detection logic. > > Jakub says that he tested with a recent gnome-terminal. That works, of > course. Mnay other terminals will not, and switching what terminal is > attached to your screen session will not work well either, as far as I > can tell. > I understood his statement, that the URLs are stripped from the data stream by screen and are no longer visible, even if the terminal would support them i.e. you connect from a gnome-terminal. But work fine when the compiler runs natively in a gnome-terminal. Or does screen pass the URL escapes thru to any terminal wether or not the produce garbage? Bernd.
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On 2/1/20 8:43 AM, Bernd Edlinger wrote:
diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
index 6ffafac..1d3fec5 100644
--- a/gcc/doc/install.texi
+++ b/gcc/doc/install.texi
@@ -2097,9 +2097,18 @@ option (if not used explicitly on the command line).
@var{choice}
can be one of @samp{never}, @samp{auto}, @samp{always}, and @samp{auto-if-env}
where @samp{auto} is the default. @samp{auto-if-env} means that
@option{-fdiagnostics-color=auto} will be the default if @code{GCC_COLORS}
-is present and non-empty in the environment, and
+is present and non-empty in the environment of the compiler, and
@option{-fdiagnostics-color=never} otherwise.
+@item --with-diagnostics-urls=@var{choice}
+Tells GCC to use @var{choice} as the default for @option{-fdiagnostics-urls=}
+option (if not used explicitly on the command line). @var{choice}
+can be one of @samp{never}, @samp{auto}, @samp{always}, and @samp{auto-if-env}
+where @samp{auto} is the default. @samp{auto-if-env} means that
+@option{-fdiagnostics-urls=auto} will be the default if @env{GCC_URLS}
+or @env{TERM_URLS} is present and non-empty in the environment of the
+compiler, and @option{-fdiagnostics-urls=never} otherwise.
+
It would be good to rewrite both of the above paragraphs to get rid of
the future tense and put them in the active voice, like
@samp{auto-if-env} makes ... the default if ...
Also @code{GCC_COLORS} ought to be @env{GCC_COLORS}, no?
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index b8ba8a3..59306bc 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -4032,14 +4032,41 @@ arguments in the C++ frontend.
@item -fdiagnostics-urls[=@var{WHEN}]
@opindex fdiagnostics-urls
@cindex urls
+@vindex GCC_URLS @r{environment variable}
+@vindex TERM_URLS @r{environment variable}
Use escape sequences to embed URLs in diagnostics. For example, when
@option{-fdiagnostics-show-option} emits text showing the command-line
option controlling a diagnostic, embed a URL for documentation of that
option.
@var{WHEN} is @samp{never}, @samp{always}, or @samp{auto}.
-The default is @samp{auto}, which means to use URL escape sequences only
-when the standard error is a terminal.
+@samp{auto} means to use URL escape sequences only when the standard error
+is a terminal, and @env{TERM} is not set to "dumb".
We should avoid literal quotes in the manual instead of proper markup.
Maybe @samp{dumb}? Or is this trying to express something other than a
literal string?
+
+The default depends on how the compiler has been configured.
+It can be any of the above @var{WHEN} options.
+
+GCC can also be configured (via the
+@option{--with-diagnostics-urls=auto-if-env} configure-time option)
+so that the default is affected by environment variables.
+Under such a configuration, GCC defaults to using @samp{auto}
+if either @env{GCC_URLS} or @env{TERM_URLS} environment variables are
+present and non-empty in the environment of the compiler, or @samp{never}
+if neither are.
+
+However, even with @option{-fdiagnostics-urls=always} the behavior will be
s/will be/is/, unless this is something that hasn't been implemented
yet. :-S
+dependent on those environment variables:
+If @env{GCC_URLS} set to empty or "no", do not embed URLs in diagnostics.
s/set/is set/ ??
And please avoid literal quotes in the text here too.
+If set to "st", URLs use ST escape sequences.
+If set to "bel", the default, URLs use BEL escape sequences.
I have no idea what ST and BEL escape sequences are
+Any other non-empty value enables the feature.
+If @env{GCC_URLS} is not set, use @env{TERM_URLS} as a fallback.
+
+At this time GCC tries to detect also a few terminals that are known to
+not implement the URL feature, and have an installed basis where the URL
What is "an installed basis"?
+escapes are likely to mis-behave, i.e. print garbage on the screen.
s/mis-behave/misbehave/
+That list is currently xfce4-terminal and tmux and mingw, this can be
+overridden with the @option{-fdiagnostics-urls=always}.
Are those things literal strings for something? How do you use that
option to override the list of terminals?
Frankly, this whole feature and the options that control it seem
horribly over-complex to me. :-S
-Sandra
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On Sat, Feb 01, 2020 at 05:21:30PM +, Bernd Edlinger wrote: > On 2/1/20 6:12 PM, Jakub Jelinek wrote: > > On Sat, Feb 01, 2020 at 03:43:20PM +, Bernd Edlinger wrote: > >> I seem to remember him saying that he always has to configure with > >> --with-diagnostics-color=never, and the URLs are on top of that. > >> But there was no configure option for that, which, given his explanation, > >> made immediately sense to me. > >> > >> In the case of the xfce terminal, the color thing was always working fine, > >> but beginning with last october, the warnings look just terrible. > >> > >> If that assumption turns out to be wrong, we can easily move that check > >> from > >> the auto_color to the auto_url code, or add more terminals which are > >> of that kind. > > > > For me colors work just fine in screen, it really depends on which > > terminals one is attaching the screen from. > > URLs don't work, but show up exactly as if they were disabled, no visual nor > > accoustic problems with those (and work fine in gnome-terminal which is > > recent). > > That said, my TERM is actually screen.xterm-256color rather than just screen > > (and xterm-256color in gnome-terminal). > > Okay, thanks. That is a strong indication that there is no need > to interfere with screen, which proves that any auto-disabling should > have a very specific terminal detection logic. Jakub says that he tested with a recent gnome-terminal. That works, of course. Mnay other terminals will not, and switching what terminal is attached to your screen session will not work well either, as far as I can tell. Segher
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On 1/31/20 11:54 PM, Segher Boessenkool wrote: > none of which use "TERM=fancy-pants-term-v42" either. (Did anyone ever > use "dumb", anyway?) > It seems like emacs shell does set TERM=dumb, and it is certainly the right thing not to emit any control codes in that environment. Bernd.
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On 2/1/20 6:12 PM, Jakub Jelinek wrote: > On Sat, Feb 01, 2020 at 03:43:20PM +, Bernd Edlinger wrote: >> I seem to remember him saying that he always has to configure with >> --with-diagnostics-color=never, and the URLs are on top of that. >> But there was no configure option for that, which, given his explanation, >> made immediately sense to me. >> >> In the case of the xfce terminal, the color thing was always working fine, >> but beginning with last october, the warnings look just terrible. >> >> If that assumption turns out to be wrong, we can easily move that check from >> the auto_color to the auto_url code, or add more terminals which are >> of that kind. > > For me colors work just fine in screen, it really depends on which > terminals one is attaching the screen from. > URLs don't work, but show up exactly as if they were disabled, no visual nor > accoustic problems with those (and work fine in gnome-terminal which is > recent). > That said, my TERM is actually screen.xterm-256color rather than just screen > (and xterm-256color in gnome-terminal). > Okay, thanks. That is a strong indication that there is no need to interfere with screen, which proves that any auto-disabling should have a very specific terminal detection logic. Bernd.
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On 2/1/20 4:54 PM, Segher Boessenkool wrote: > Hi! > > On Sat, Feb 01, 2020 at 05:02:18AM +, Bernd Edlinger wrote: Definitely, if the situation with tmux is like xfce4-terminal (reportedly the 0.8 version switched to a fixed VTE, which makes the URLs invisible, but the URL feature request is pending sine 2017, with no activity whatsoever), then detecting that and disabling the URLs until they finally implement the URLs is straight forward. I can add that to my patch if you confirm, the right detection logic: >>> >>> The situation is that it is a terminal multiplexer; you can connect any >>> terminal to it, and swap those out, etc. You might have one that has >>> support for the url thing at one time, but when you look at that session >>> from a different machine (from your phone, say), not anymore (or the >>> other way around). You can also connect multiple actual terminals at >>> the same time. >>> >>> In short, even *if* you could detect whether the terminal supports this >>> url thing (and you cannot), you cannot detect whether it will support it >>> two seconds from now, and even if the url thing you already displayed >>> will misbehave *in the future*! >> >> Ah, okay, this is a totally wrong assumption then. >> >> So if I understand you right, I should add a check for >> tmux in should_colorize, where the TERM=dumb thing is, >> and add TERM=screen, and TMUX=anything, to switch of >> auto-color off, which will take down URLs at the same time? > > Well, colourisation doesn't mess up the display so much, in practice; it > just makes it completely unreadable (for me anyway), so I have GCC_COLORS= > just like I need stuff for very many other programs to disable colours. > Something similar can of course work to turn off the url thing, but since > that make the display unusable on not very few systems, and the utility > of this is much lower as well (people just love those colours, for some > strange reason; urls is more meh), yes please, do not do the url thing > with TERM=dumb or TERM=ansi or TERM=screen and maybe some similar; but I > think many people will like their colours enabled. > > re-sending this as I accidentally did not "reply-to-all"... Okay in that case I would move the TUMX detection code to the auto_url function, that is easy. I would of course like to be as specific to a single terminal as possible here, since it is possible that tumx will implement URLs some day, at least there seems to be a patch available here: https://github.com/tmux/tmux/issues/911#issuecomment-554312483 so once there is a released tumx version, and that works reliably, which I may still doubt, we should reconsider this auto detection. So the situation may well arise that we would like to auto-enable tumx and auto-disable screen if I see that right. But certainly we should only disable a potentially useful feature when there is hard evidence that it is *only* causing real problems to real people. And what is the meaning of TERM=ansi does it imply it does not understand these OSC-8 escape codes ? So I moved the check for TUMX to the url code path only now, as Segher suggested. I am yet undecided what to make out of TERM=ansi, but it is hopefully only a theoretical issue. I attached the new version again. Is it OK for trunk? Bernd. From 68203865b77ec254324746316fc438fa65fbc2b3 Mon Sep 17 00:00:00 2001 From: Bernd Edlinger Date: Wed, 29 Jan 2020 15:31:10 +0100 Subject: [PATCH] PR 87488: Add --with-diagnostics-urls configuration option 2020-01-30 David Malcolm Bernd Edlinger PR 87488 PR other/93168 * config.in (DIAGNOSTICS_URLS_DEFAULT): New define. * configure.ac (--with-diagnostics-urls): New configuration option, based on --with-diagnostics-color. (DIAGNOSTICS_URLS_DEFAULT): New define. * config.h: Regenerate. * configure: Regenerate. * diagnostic.c (diagnostic_urls_init): Handle -1 for DIAGNOSTICS_URLS_DEFAULT from configure-time --with-diagnostics-urls=auto-if-env by querying for a GCC_URLS and TERM_URLS environment variable. * diagnostic-url.h (diagnostic_url_format): New enum type. (diagnostic_urls_enabled_p): rename to... (parse_env_vars_for_urls): ... this, and change return type. * diagnostic-color.c (parse_gcc_urls): New helper function. (auto_enable_urls): Disable URLs on tumx, xfce4-terminal, mingw. (parse_env_vars_for_urls): Adjust. * pretty-print.h (pretty_printer::show_urls): rename to... (pretty_printer::url_format): ... this, and change to enum. * pretty-print.c (pretty_printer::pretty_printer, pp_begin_url, pp_end_url, test_urls): Adjust. * doc/install.texi (--with-diagnostics-urls): Document the new configuration option. * doc/invoke.texi (-fdiagnostics-urls): Add GCC_URLS and TERM_URLS vindex reference. Update description of defaults based on the above. --- gcc/config.in | 6 gcc/configure | 41 +++-- gcc/configure.ac | 28 +
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On Sat, Feb 01, 2020 at 03:43:20PM +, Bernd Edlinger wrote: > I seem to remember him saying that he always has to configure with > --with-diagnostics-color=never, and the URLs are on top of that. > But there was no configure option for that, which, given his explanation, > made immediately sense to me. > > In the case of the xfce terminal, the color thing was always working fine, > but beginning with last october, the warnings look just terrible. > > If that assumption turns out to be wrong, we can easily move that check from > the auto_color to the auto_url code, or add more terminals which are > of that kind. For me colors work just fine in screen, it really depends on which terminals one is attaching the screen from. URLs don't work, but show up exactly as if they were disabled, no visual nor accoustic problems with those (and work fine in gnome-terminal which is recent). That said, my TERM is actually screen.xterm-256color rather than just screen (and xterm-256color in gnome-terminal). Jakub
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On Sat, Feb 01, 2020 at 08:41:15AM -0500, David Malcolm wrote: > Does our existing colorization code not work with TMUX, or is it just the > new URLs that are broken? Segher? I don't know, I have colourisation turned off in GCC pretty much always on systems I use with tmux. Some other programs that use colours look funny after you connect from another host, I think that is the "bold" thing, dunno. But all character cells are where they are supposed to be, and there is no beeping, etc., so the situation isn't nearly as bad in any case. Plus as noted, people really want colours apparently. Segher
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
Hi! On Sat, Feb 01, 2020 at 05:02:18AM +, Bernd Edlinger wrote: > >> Definitely, if the situation with tmux is like xfce4-terminal (reportedly > >> the 0.8 version switched to a fixed VTE, which makes the URLs invisible, > >> but the URL feature request is pending sine 2017, with no activity > >> whatsoever), > >> then detecting that and disabling the URLs until they finally implement > >> the URLs is straight forward. I can add that to my patch if you confirm, > >> the right detection logic: > > > > The situation is that it is a terminal multiplexer; you can connect any > > terminal to it, and swap those out, etc. You might have one that has > > support for the url thing at one time, but when you look at that session > > from a different machine (from your phone, say), not anymore (or the > > other way around). You can also connect multiple actual terminals at > > the same time. > > > > In short, even *if* you could detect whether the terminal supports this > > url thing (and you cannot), you cannot detect whether it will support it > > two seconds from now, and even if the url thing you already displayed > > will misbehave *in the future*! > > Ah, okay, this is a totally wrong assumption then. > > So if I understand you right, I should add a check for > tmux in should_colorize, where the TERM=dumb thing is, > and add TERM=screen, and TMUX=anything, to switch of > auto-color off, which will take down URLs at the same time? Well, colourisation doesn't mess up the display so much, in practice; it just makes it completely unreadable (for me anyway), so I have GCC_COLORS= just like I need stuff for very many other programs to disable colours. Something similar can of course work to turn off the url thing, but since that make the display unusable on not very few systems, and the utility of this is much lower as well (people just love those colours, for some strange reason; urls is more meh), yes please, do not do the url thing with TERM=dumb or TERM=ansi or TERM=screen and maybe some similar; but I think many people will like their colours enabled. Segher
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On 2/1/20 2:41 PM, David Malcolm wrote:
> On Sat, 2020-02-01 at 07:30 +, Bernd Edlinger wrote:
>>
>> On 1/31/20 11:06 PM, David Malcolm wrote:
>>> On Fri, 2020-01-31 at 16:59 +, Bernd Edlinger wrote:
Hi,
this is patch is heavily based on David's original patch here:
https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01409.html
and addresses Jakub's review comments here:
https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01412.html
So I hope you don't mind, when I pick up this patch since there
was not much activity recently on this issue, so I assumed you
would appreciate some help.
>>>
>>> Thanks Bernd; sorry, I got distracted by analyzer bug-fixing. It's
>>> appreciated.
>>>
I will try to improve the patch a bit, and hope you are gonna
like
it. I agree that this feature is fine, and should be enabled by
default, and just if it is positively clear that it won't work,
disabled in the auto mode.
Also as requested by Jakub this tries to be more compatible to
GCC_COLORS define, and adds the capability to switch between ST
and BEL string termination and also have a way to disable urls
even with -fdiagnostics-urls=always (like GCC_COLORS= disables
colors).
In addition to that I propose to use GCC_URLS and if that
is not defined use TERM_URLS to control that feature for
all applications willing to support that.
>
> [..]
>
> Thanks for the updated patch; here are some notes:
>
>> diff --git a/gcc/diagnostic-color.c b/gcc/diagnostic-color.c
>> index d554795..f406139 100644
>> --- a/gcc/diagnostic-color.c
>> +++ b/gcc/diagnostic-color.c
>> @@ -216,6 +216,10 @@ should_colorize (void)
>>&& GetConsoleMode (h, );
>> #else
>>char const *t = getenv ("TERM");
>> + /* Do not enable colors when TMUX is detected, since that is
>> + known to not work reliably. */
>> + if (t && strcmp (t, "screen") && getenv ("TMUX"))
>> +return false;
>>return t && strcmp (t, "dumb") != 0 && isatty (STDERR_FILENO);
>> #endif
>> }
>
> Does our existing colorization code not work with TMUX, or is it just the
> new URLs that are broken? Segher?
>
I seem to remember him saying that he always has to configure with
--with-diagnostics-color=never, and the URLs are on top of that.
But there was no configure option for that, which, given his explanation,
made immediately sense to me.
In the case of the xfce terminal, the color thing was always working fine,
but beginning with last october, the warnings look just terrible.
If that assumption turns out to be wrong, we can easily move that check from
the auto_color to the auto_url code, or add more terminals which are
of that kind.
>> @@ -239,20 +243,86 @@ colorize_init (diagnostic_color_rule_t rule)
>> }
>> }
>>
>> +/* Return URL_FORMAT_XXX which tells how we should emit urls
>> + when in always mode.
>> + We use GCC_URLS and if that is not defined TERM_URLS.
>> + If neither is defined the feature is enabled by default. */
>> +
>> +static diagnostic_url_format
>> +parse_gcc_urls()
>
> Perhaps rename this to parse_env_vars_for_urls or somesuch, given that
> it's not just GCC_URLS being parsed?
>
Okay, no problem.
>> +/* Return true if we should use urls when in auto mode, false otherwise. */
>> +
>> +static bool
>> +auto_enable_urls ()
>> +{
>> +#ifdef __MINGW32__
>> + return false;
>> +#else
>> + const char *p;
>> +
>> + /* First check the terminal is capable to print color escapes,
> ^^^
> grammar nit: "is capable of printing"
>
Thanks.
>> + if not URLs won't work either. */
>> + if (!should_colorize ())
>> +return false;
>> +
>> + p = getenv ("COLORTERM");
>> + if (p == NULL)
>> +return true;
>
> Is this part of the rejection of terminals that can't print color
> escapes, or is this some other heuristic based on the discussion?
>
Yes, it has nothing to do with color, the point that was made on the
Hyperlink discussion gist page, was that the correct value would have been
COLORTERM="truecolor", instead of "xfce4-terminal" so that seems to be
a bug. If they ever fix that, the workaround here will automatically
be disabled, but that is just fine, since they do already use a fixed
VTE version that is not disturbed by the URL escapes, whether or not
the URL feature is ever implemented by the XFCE maintainers.
>
>> + /* xfce4-terminal is known to not implement URLs at this time.
>> + Recently new installations will safely ignore the URL escape
>> + sequences, but a large number of legacy installations print
>> + garbage when URLs are printed. Therefore we loose nothing by
> ^
> "loose" -> "lose"
>
Fixed, thanks.
Note grep finds lots of places where the term "loose" is used all over
the gcc tree.
> If you have version numbers handy, it might be good to mention those
> in
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On Sat, 2020-02-01 at 07:30 +, Bernd Edlinger wrote:
>
> On 1/31/20 11:06 PM, David Malcolm wrote:
> > On Fri, 2020-01-31 at 16:59 +, Bernd Edlinger wrote:
> > > Hi,
> > >
> > > this is patch is heavily based on David's original patch here:
> > > https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01409.html
> > >
> > > and addresses Jakub's review comments here:
> > > https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01412.html
> > >
> > > So I hope you don't mind, when I pick up this patch since there
> > > was not much activity recently on this issue, so I assumed you
> > > would appreciate some help.
> >
> > Thanks Bernd; sorry, I got distracted by analyzer bug-fixing. It's
> > appreciated.
> >
> > > I will try to improve the patch a bit, and hope you are gonna
> > > like
> > > it. I agree that this feature is fine, and should be enabled by
> > > default, and just if it is positively clear that it won't work,
> > > disabled in the auto mode.
> > >
> > > Also as requested by Jakub this tries to be more compatible to
> > > GCC_COLORS define, and adds the capability to switch between ST
> > > and BEL string termination and also have a way to disable urls
> > > even with -fdiagnostics-urls=always (like GCC_COLORS= disables
> > > colors).
> > >
> > > In addition to that I propose to use GCC_URLS and if that
> > > is not defined use TERM_URLS to control that feature for
> > > all applications willing to support that.
[..]
Thanks for the updated patch; here are some notes:
> diff --git a/gcc/diagnostic-color.c b/gcc/diagnostic-color.c
> index d554795..f406139 100644
> --- a/gcc/diagnostic-color.c
> +++ b/gcc/diagnostic-color.c
> @@ -216,6 +216,10 @@ should_colorize (void)
> && GetConsoleMode (h, );
> #else
>char const *t = getenv ("TERM");
> + /* Do not enable colors when TMUX is detected, since that is
> + known to not work reliably. */
> + if (t && strcmp (t, "screen") && getenv ("TMUX"))
> +return false;
>return t && strcmp (t, "dumb") != 0 && isatty (STDERR_FILENO);
> #endif
> }
Does our existing colorization code not work with TMUX, or is it just the
new URLs that are broken? Segher?
> @@ -239,20 +243,86 @@ colorize_init (diagnostic_color_rule_t rule)
> }
> }
>
> +/* Return URL_FORMAT_XXX which tells how we should emit urls
> + when in always mode.
> + We use GCC_URLS and if that is not defined TERM_URLS.
> + If neither is defined the feature is enabled by default. */
> +
> +static diagnostic_url_format
> +parse_gcc_urls()
Perhaps rename this to parse_env_vars_for_urls or somesuch, given that
it's not just GCC_URLS being parsed?
> +/* Return true if we should use urls when in auto mode, false otherwise. */
> +
> +static bool
> +auto_enable_urls ()
> +{
> +#ifdef __MINGW32__
> + return false;
> +#else
> + const char *p;
> +
> + /* First check the terminal is capable to print color escapes,
^^^
grammar nit: "is capable of printing"
> + if not URLs won't work either. */
> + if (!should_colorize ())
> +return false;
> +
> + p = getenv ("COLORTERM");
> + if (p == NULL)
> +return true;
Is this part of the rejection of terminals that can't print color
escapes, or is this some other heuristic based on the discussion?
> + /* xfce4-terminal is known to not implement URLs at this time.
> + Recently new installations will safely ignore the URL escape
> + sequences, but a large number of legacy installations print
> + garbage when URLs are printed. Therefore we loose nothing by
^
"loose" -> "lose"
If you have version numbers handy, it might be good to mention those
in this comment, since references to "at this time" and "new" can
get dated.
> + disabling this feature for that specific terminal type. */
> + if (!strcmp (p, "xfce4-terminal"))
> +return false;
> +
> + return true;
> +#endif
> +}
> +
> /* Determine if URLs should be enabled, based on RULE.
> This reuses the logic for colorization. */
Maybe this:
/* Determine if URLs should be enabled, based on RULE,
and, if so, which format to use.
This reuses the logic for colorization. */
> -bool
> -diagnostic_urls_enabled_p (diagnostic_url_rule_t rule)
> +diagnostic_url_format
> +diagnostic_urls_enabled (diagnostic_url_rule_t rule)
[...]
> diff --git a/gcc/diagnostic-url.h b/gcc/diagnostic-url.h
> index 6be0569..22253fd 100644
> --- a/gcc/diagnostic-url.h
> +++ b/gcc/diagnostic-url.h
> @@ -31,6 +31,20 @@ typedef enum
>DIAGNOSTICS_URL_AUTO = 2
> } diagnostic_url_rule_t;
>
> -extern bool diagnostic_urls_enabled_p (diagnostic_url_rule_t);
> +/* Tells how URLs are to be emitted:
> + - URL_FORMAT_NONE means no URLs shall be emitted.
> + - URL_FORMAT_ST means use ST string termination.
> + - URL_FROMAT_BEL means use BEL string termination,
> + which is the default. */
> +enum diagnostic_url_format
> +{
> +
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On 2/1/20 9:55 AM, Jakub Jelinek wrote:
> On Sat, Feb 01, 2020 at 07:30:15AM +, Bernd Edlinger wrote:
>> @@ -239,20 +243,86 @@ colorize_init (diagnostic_color_rule_t rule)
>> }
>> }
>>
>> +/* Return URL_FORMAT_XXX which tells how we should emit urls
>> + when in always mode.
>> + We use GCC_URLS and if that is not defined TERM_URLS.
>> + If neither is defined the feature is enabled by default. */
>> +
>> +static diagnostic_url_format
>> +parse_gcc_urls()
>
> Formatting, missing space before (.
>
Gotcha :) this is openssl style sneaking in...
fixed.
>> diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
>> index 6ffafac..59d7ecd 100644
>> --- a/gcc/doc/install.texi
>> +++ b/gcc/doc/install.texi
>> @@ -2097,9 +2097,18 @@ option (if not used explicitly on the command line).
>> @var{choice}
>> can be one of @samp{never}, @samp{auto}, @samp{always}, and
>> @samp{auto-if-env}
>> where @samp{auto} is the default. @samp{auto-if-env} means that
>> @option{-fdiagnostics-color=auto} will be the default if @code{GCC_COLORS}
>> -is present and non-empty in the environment, and
>> +is present and non-empty in the environment at runtime, and
>> @option{-fdiagnostics-color=never} otherwise.
>
> I'm not sure "at runtime" is the right term here, usually we talk about
> runtime when running the program produced by gcc, not about compile time.
> So perhaps try to be even more verbose and say "in the environment of the
> compiler" or so?
>
way better, thanks.
>> +The default depends on how the compiler has been configured.
>> +it can be any of the above @var{WHEN} options.
>
> After full stop next sentence should start with capital letter.
>
fixed.
>> +
>> +GCC can also be configured (via the
>> +@option{--with-diagnostics-urls=auto-if-env} configure-time option)
>> +so that the default is affected by environment variables.
>> +Under such a configuration, GCC defaults to using @samp{auto}
>> +if either @env{GCC_URLS} or @env{TERM_URLS} environment variables are
>> +present in the environment, or @samp{never} if neither are.
>> +
>> +But even with @option{-fdiagnostics-urls=always} the behavior will be
>> +dependent on those environmen variables:
ah, typo...
>
> I know I start sentences with But all the time, but some style guides are
> against that, not sure if we want to do that in the documentation.
>
okay I can start the sentence now with "However, even with..."
In the sentence above I should probably also say are "present _and non-empty_
in the
environment _of the compiler_".
So here is just an incremental update with the improvements so far:
diff --git a/gcc/diagnostic-color.c b/gcc/diagnostic-color.c
index f406139..73ca771 100644
--- a/gcc/diagnostic-color.c
+++ b/gcc/diagnostic-color.c
@@ -249,7 +249,7 @@ colorize_init (diagnostic_color_rule_t rule)
If neither is defined the feature is enabled by default. */
static diagnostic_url_format
-parse_gcc_urls()
+parse_gcc_urls ()
{
const char *p;
diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
index 59d7ecd..1d3fec5 100644
--- a/gcc/doc/install.texi
+++ b/gcc/doc/install.texi
@@ -2097,7 +2097,7 @@ option (if not used explicitly on the command line).
@var{choice}
can be one of @samp{never}, @samp{auto}, @samp{always}, and @samp{auto-if-env}
where @samp{auto} is the default. @samp{auto-if-env} means that
@option{-fdiagnostics-color=auto} will be the default if @code{GCC_COLORS}
-is present and non-empty in the environment at runtime, and
+is present and non-empty in the environment of the compiler, and
@option{-fdiagnostics-color=never} otherwise.
@item --with-diagnostics-urls=@var{choice}
@@ -2106,8 +2106,8 @@ option (if not used explicitly on the command line).
@var{choice}
can be one of @samp{never}, @samp{auto}, @samp{always}, and @samp{auto-if-env}
where @samp{auto} is the default. @samp{auto-if-env} means that
@option{-fdiagnostics-urls=auto} will be the default if @env{GCC_URLS}
-or @env{TERM_URLS} is present and non-empty in the environment at runtime, and
-@option{-fdiagnostics-urls=never} otherwise.
+or @env{TERM_URLS} is present and non-empty in the environment of the
+compiler, and @option{-fdiagnostics-urls=never} otherwise.
@item --enable-lto
@itemx --disable-lto
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 6df0efd..59306bc 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -4044,18 +4044,19 @@ option.
is a terminal, and @env{TERM} is not set to "dumb".
The default depends on how the compiler has been configured.
-it can be any of the above @var{WHEN} options.
+It can be any of the above @var{WHEN} options.
GCC can also be configured (via the
@option{--with-diagnostics-urls=auto-if-env} configure-time option)
so that the default is affected by environment variables.
Under such a configuration, GCC defaults to using @samp{auto}
if either @env{GCC_URLS} or @env{TERM_URLS} environment variables are
-present in the environment, or
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On Sat, Feb 01, 2020 at 07:30:15AM +, Bernd Edlinger wrote:
> @@ -239,20 +243,86 @@ colorize_init (diagnostic_color_rule_t rule)
> }
> }
>
> +/* Return URL_FORMAT_XXX which tells how we should emit urls
> + when in always mode.
> + We use GCC_URLS and if that is not defined TERM_URLS.
> + If neither is defined the feature is enabled by default. */
> +
> +static diagnostic_url_format
> +parse_gcc_urls()
Formatting, missing space before (.
> diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
> index 6ffafac..59d7ecd 100644
> --- a/gcc/doc/install.texi
> +++ b/gcc/doc/install.texi
> @@ -2097,9 +2097,18 @@ option (if not used explicitly on the command line).
> @var{choice}
> can be one of @samp{never}, @samp{auto}, @samp{always}, and
> @samp{auto-if-env}
> where @samp{auto} is the default. @samp{auto-if-env} means that
> @option{-fdiagnostics-color=auto} will be the default if @code{GCC_COLORS}
> -is present and non-empty in the environment, and
> +is present and non-empty in the environment at runtime, and
> @option{-fdiagnostics-color=never} otherwise.
I'm not sure "at runtime" is the right term here, usually we talk about
runtime when running the program produced by gcc, not about compile time.
So perhaps try to be even more verbose and say "in the environment of the
compiler" or so?
> +The default depends on how the compiler has been configured.
> +it can be any of the above @var{WHEN} options.
After full stop next sentence should start with capital letter.
> +
> +GCC can also be configured (via the
> +@option{--with-diagnostics-urls=auto-if-env} configure-time option)
> +so that the default is affected by environment variables.
> +Under such a configuration, GCC defaults to using @samp{auto}
> +if either @env{GCC_URLS} or @env{TERM_URLS} environment variables are
> +present in the environment, or @samp{never} if neither are.
> +
> +But even with @option{-fdiagnostics-urls=always} the behavior will be
> +dependent on those environmen variables:
I know I start sentences with But all the time, but some style guides are
against that, not sure if we want to do that in the documentation.
Jakub
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On 1/31/20 11:06 PM, David Malcolm wrote:
> On Fri, 2020-01-31 at 16:59 +, Bernd Edlinger wrote:
>> Hi,
>>
>> this is patch is heavily based on David's original patch here:
>> https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01409.html
>>
>> and addresses Jakub's review comments here:
>> https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01412.html
>>
>> So I hope you don't mind, when I pick up this patch since there
>> was not much activity recently on this issue, so I assumed you
>> would appreciate some help.
>
> Thanks Bernd; sorry, I got distracted by analyzer bug-fixing. It's
> appreciated.
>
>> I will try to improve the patch a bit, and hope you are gonna like
>> it. I agree that this feature is fine, and should be enabled by
>> default, and just if it is positively clear that it won't work,
>> disabled in the auto mode.
>>
>> Also as requested by Jakub this tries to be more compatible to
>> GCC_COLORS define, and adds the capability to switch between ST
>> and BEL string termination and also have a way to disable urls
>> even with -fdiagnostics-urls=always (like GCC_COLORS= disables
>> colors).
>>
>> In addition to that I propose to use GCC_URLS and if that
>> is not defined use TERM_URLS to control that feature for
>> all applications willing to support that.
>
> I think I like the overall idea.
>
Thanks!
>> Since I have seen much garbage from the URLs in the xfce4-terminal
>> 0.6.3
>> admittedly an old Ubuntu installation, but still in LTS status,
>> and no attempt from the xfc4-terminal to _ever_ implement URLs, I
>> would
>> like to detect the xfce4-terminal, and use that in auto mode
>> to switch off the URLs feature, since in best case the URLs will
>> just be ignored, but can and will often create garbage on the screen.
>>
>> Likewise on MinGW, since the windows 10 cmd prompt does at best
>> ignore the URLs, but windows terminal and windows 7 cmd prompt
>> print garbage when URLs are output.
>
> That sounds reasonable, but we should document those exclusions, I
> think.
>
done.
> I think the ChangeLog should also refer to "PR other/93168":
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93168
>
done.
>> diff --git a/gcc/diagnostic-color.c b/gcc/diagnostic-color.c
>> index d554795..ad84d1f 100644
>> --- a/gcc/diagnostic-color.c
>> +++ b/gcc/diagnostic-color.c
>> @@ -239,6 +239,56 @@ colorize_init (diagnostic_color_rule_t rule)
>> }
>> }
>>
>> +bool diagnostic_urls_use_st = false;
>
> I don't like global variables (a pet peeve of mine); can you make this
> be a field of the pretty_printer instead?
>
> Even better: convert pretty_printer::show_urls from a bool to a new
> enum; something like:
>
> enum diagnostic_url_format
> {
> URL_FORMAT_NONE,
> URL_FORMAT_ST,
> URL_FORMAT_BEL
> };
>
> with suitable leading comments (probably a good place to summarize some
> of the compatibility info within the source).
>
> Then we could verify the differences between ST and BEL in the
> selftests, and also not have the selftests be affected by env vars.
>
Okay, I have to include diagnostic-url.h from pretty-print.h in that case,
since both files are included from the generated options.c in alphabetic order
/-).
I was first a bit reluctant to do so, since we usually don't have recursive
header files but probably that is okay then, alternatively I could use a
bitfield
in pretty-print.h and put defines for URL_FORMAT_xxx in diagnostic-url.h.
>
>> +/* Return true if we should use urls when in always mode, false otherwise.
>> + Additionally initialize diagnostic_urls_use_st to true if ST escapes
>> + should be used and false for BEL escapes. */
>
> Maybe have this return that 3-way enum instead?
>
okay, done.
>> +
>> +static bool
>> +parse_gcc_urls()
>> +{
>> + const char *p;
>> +
>> + p = getenv ("GCC_URLS"); /* Plural! */
>> + if (p == NULL)
>> +p = getenv ("TERM_URLS");
>> +
>> + if (p == NULL)
>> +return true;
>> + if (*p == '\0')
>> +return false;
>> +
>> + if (!strcmp (p, "no"))
>> +return false;
>> + if (!strcmp (p, "st"))
>> +diagnostic_urls_use_st = true;
>> + else if (!strcmp (p, "bel"))
>> +diagnostic_urls_use_st = false;
>> + return true;
>> +}
>> +
>> +/* Return true if we should use urls when in auto mode, false otherwise. */
>> +
>> +static bool
>> +auto_enable_urls ()
>> +{
>> +#ifdef __MINGW32__
>> + return false;
>> +#else
>> + const char *p;
>> +
>> + if (!should_colorize ())
>> +return false;
>> + p = getenv ("COLORTERM");
>> + if (p == NULL)
>> +return true;
>> + if (!strcmp (p, "xfce4-terminal"))
>> +return false;
>> + return true;
>> +#endif
>> +}
>
> I think this logic warrants a comment for each of the exclusions: why
> are we excluding them? I'd prefer to capture that in the source,
> rather than just in the mailing list.
>
done.
>> /* Determine if URLs should be enabled, based on RULE.
>> This reuses the logic for colorization. */
>>
>> @@ -250,9 +300,12 @@
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
Hi Segher, On 2/1/20 2:32 AM, Segher Boessenkool wrote: > On Fri, Jan 31, 2020 at 11:38:04PM +, Bernd Edlinger wrote: >> On 1/31/20 11:54 PM, Segher Boessenkool wrote: >>> about most, which caused me to open PR93168, is TERM=screen (which is >>> what tmux uses), so at least exclude that one? And doing all this >> >> Definitely, if the situation with tmux is like xfce4-terminal (reportedly >> the 0.8 version switched to a fixed VTE, which makes the URLs invisible, >> but the URL feature request is pending sine 2017, with no activity >> whatsoever), >> then detecting that and disabling the URLs until they finally implement >> the URLs is straight forward. I can add that to my patch if you confirm, >> the right detection logic: > > The situation is that it is a terminal multiplexer; you can connect any > terminal to it, and swap those out, etc. You might have one that has > support for the url thing at one time, but when you look at that session > from a different machine (from your phone, say), not anymore (or the > other way around). You can also connect multiple actual terminals at > the same time. > > In short, even *if* you could detect whether the terminal supports this > url thing (and you cannot), you cannot detect whether it will support it > two seconds from now, and even if the url thing you already displayed > will misbehave *in the future*! > Ah, okay, this is a totally wrong assumption then. So if I understand you right, I should add a check for tmux in should_colorize, where the TERM=dumb thing is, and add TERM=screen, and TMUX=anything, to switch of auto-color off, which will take down URLs at the same time? Bernd. >> >From >> >https://unix.stackexchange.com/questions/10689/how-can-i-tell-if-im-in-a-tmux-session-from-a-bash-script >> I read the safest way to find out if you are in a tmux session, is >> to look for TERM=screen and TMUX is set. > > Yes, but the same considerations are true for all screen-like programs. > I happen to like tmux best, some people swear by old-fashioned screen, > and there are other alternatives too I think. > >> Is that the case for your environment? > > It's true on at least six machines I tested just now, but all of those > are pretty similar anyway, so that doesn't say much. > >> Note that it is well possible that this environment values are >> not preserved in a ssh session, but nobody is perfect. > > The session you are looking at is not the ssh session; it will keep > running even if no actual terminal is connected, that's pretty much > the point of running it, in many cases :-) > Since I have seen much garbage from the URLs in the xfce4-terminal 0.6.3 admittedly an old Ubuntu installation, but still in LTS status, and no attempt from the xfc4-terminal to _ever_ implement URLs, >>> >>> This is true for *most* terminal emulators. >> >> Sadly, I would not do this if there is a chance that someone looses a >> working feature, so I was told that a more aggressive approach would >> "be straight harmful to the current state of things >> (i.e. hyperlinks at least not causing any problem, plus working >> out of the box wherever supported, for the vast majority of people)" >> https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda#gistcomment-3160953 > > I have seen not a *single* terminal emulator where this works 100% > correctly. I have seen many where it screws up display spectacularly > (or sound even :-/ ) > >> So I would try to detect known terminals which are so buggy that they >> print garbage instead of silently ignoring the URL escapes AND >> which may or may not have fixed the visual glitches but have not plan >> to implement URLs at all. > > That is not buggy at all. It is a bug to output the wrong escape codes, > instead. > >>> I have nothing against this feature, I just wish it wouldn't annoy me >>> on pretty much every system I use. None of which use "TERM=dumb", but >>> none of which use "TERM=fancy-pants-term-v42" either. (Did anyone ever >>> use "dumb", anyway?) >> >> The dumb thing was old code, I only took the freedom to document it ;-) > > I know :-) I mean, does anyone have "TERM=dumb" in the environment? > "TERM=ansi", sure, and I've used "TERM=vt220" many times, too, but I've > never seen "TERM=dumb" used. > > > Segher >
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On Fri, Jan 31, 2020 at 11:38:04PM +, Bernd Edlinger wrote: > On 1/31/20 11:54 PM, Segher Boessenkool wrote: > > about most, which caused me to open PR93168, is TERM=screen (which is > > what tmux uses), so at least exclude that one? And doing all this > > Definitely, if the situation with tmux is like xfce4-terminal (reportedly > the 0.8 version switched to a fixed VTE, which makes the URLs invisible, > but the URL feature request is pending sine 2017, with no activity > whatsoever), > then detecting that and disabling the URLs until they finally implement > the URLs is straight forward. I can add that to my patch if you confirm, > the right detection logic: The situation is that it is a terminal multiplexer; you can connect any terminal to it, and swap those out, etc. You might have one that has support for the url thing at one time, but when you look at that session from a different machine (from your phone, say), not anymore (or the other way around). You can also connect multiple actual terminals at the same time. In short, even *if* you could detect whether the terminal supports this url thing (and you cannot), you cannot detect whether it will support it two seconds from now, and even if the url thing you already displayed will misbehave *in the future*! > >From > >https://unix.stackexchange.com/questions/10689/how-can-i-tell-if-im-in-a-tmux-session-from-a-bash-script > I read the safest way to find out if you are in a tmux session, is > to look for TERM=screen and TMUX is set. Yes, but the same considerations are true for all screen-like programs. I happen to like tmux best, some people swear by old-fashioned screen, and there are other alternatives too I think. > Is that the case for your environment? It's true on at least six machines I tested just now, but all of those are pretty similar anyway, so that doesn't say much. > Note that it is well possible that this environment values are > not preserved in a ssh session, but nobody is perfect. The session you are looking at is not the ssh session; it will keep running even if no actual terminal is connected, that's pretty much the point of running it, in many cases :-) > >> Since I have seen much garbage from the URLs in the xfce4-terminal 0.6.3 > >> admittedly an old Ubuntu installation, but still in LTS status, > >> and no attempt from the xfc4-terminal to _ever_ implement URLs, > > > > This is true for *most* terminal emulators. > > Sadly, I would not do this if there is a chance that someone looses a > working feature, so I was told that a more aggressive approach would > "be straight harmful to the current state of things > (i.e. hyperlinks at least not causing any problem, plus working > out of the box wherever supported, for the vast majority of people)" > https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda#gistcomment-3160953 I have seen not a *single* terminal emulator where this works 100% correctly. I have seen many where it screws up display spectacularly (or sound even :-/ ) > So I would try to detect known terminals which are so buggy that they > print garbage instead of silently ignoring the URL escapes AND > which may or may not have fixed the visual glitches but have not plan > to implement URLs at all. That is not buggy at all. It is a bug to output the wrong escape codes, instead. > > I have nothing against this feature, I just wish it wouldn't annoy me > > on pretty much every system I use. None of which use "TERM=dumb", but > > none of which use "TERM=fancy-pants-term-v42" either. (Did anyone ever > > use "dumb", anyway?) > > The dumb thing was old code, I only took the freedom to document it ;-) I know :-) I mean, does anyone have "TERM=dumb" in the environment? "TERM=ansi", sure, and I've used "TERM=vt220" many times, too, but I've never seen "TERM=dumb" used. Segher
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On 1/31/20 11:54 PM, Segher Boessenkool wrote: > Hi! > > Thanks for working on this. > > On Fri, Jan 31, 2020 at 04:59:02PM +, Bernd Edlinger wrote: >> I will try to improve the patch a bit, and hope you are gonna like >> it. I agree that this feature is fine, and should be enabled by >> default, and just if it is positively clear that it won't work, >> disabled in the auto mode. > > Why does it not ask the terminfo, instead of only disabling this for > "dumb"? It should disable it for *most* terminals! The one I care Unfortunately I was told that terminfo is not willing to support URLs either, and it need to be administered centrally, which is per definition an impossible task... > about most, which caused me to open PR93168, is TERM=screen (which is > what tmux uses), so at least exclude that one? And doing all this Definitely, if the situation with tmux is like xfce4-terminal (reportedly the 0.8 version switched to a fixed VTE, which makes the URLs invisible, but the URL feature request is pending sine 2017, with no activity whatsoever), then detecting that and disabling the URLs until they finally implement the URLs is straight forward. I can add that to my patch if you confirm, the right detection logic: >From >https://unix.stackexchange.com/questions/10689/how-can-i-tell-if-im-in-a-tmux-session-from-a-bash-script I read the safest way to find out if you are in a tmux session, is to look for TERM=screen and TMUX is set. Is that the case for your environment? Note that it is well possible that this environment values are not preserved in a ssh session, but nobody is perfect. > beeping and screen-corrupting stuff for everything that sets TERM=xterm > is a bad idea imnsho. > >> Since I have seen much garbage from the URLs in the xfce4-terminal 0.6.3 >> admittedly an old Ubuntu installation, but still in LTS status, >> and no attempt from the xfc4-terminal to _ever_ implement URLs, > > This is true for *most* terminal emulators. > Sadly, I would not do this if there is a chance that someone looses a working feature, so I was told that a more aggressive approach would "be straight harmful to the current state of things (i.e. hyperlinks at least not causing any problem, plus working out of the box wherever supported, for the vast majority of people)" https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda#gistcomment-3160953 So I would try to detect known terminals which are so buggy that they print garbage instead of silently ignoring the URL escapes AND which may or may not have fixed the visual glitches but have not plan to implement URLs at all. Once they implement URLs it would be nice if they do something like set a TERM_URLS=yes so we know for sure, that the feature is positively available, for instance. > I have nothing against this feature, I just wish it wouldn't annoy me > on pretty much every system I use. None of which use "TERM=dumb", but > none of which use "TERM=fancy-pants-term-v42" either. (Did anyone ever > use "dumb", anyway?) > The dumb thing was old code, I only took the freedom to document it ;-) Bernd. > > Segher >
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
Hi! Thanks for working on this. On Fri, Jan 31, 2020 at 04:59:02PM +, Bernd Edlinger wrote: > I will try to improve the patch a bit, and hope you are gonna like > it. I agree that this feature is fine, and should be enabled by > default, and just if it is positively clear that it won't work, > disabled in the auto mode. Why does it not ask the terminfo, instead of only disabling this for "dumb"? It should disable it for *most* terminals! The one I care about most, which caused me to open PR93168, is TERM=screen (which is what tmux uses), so at least exclude that one? And doing all this beeping and screen-corrupting stuff for everything that sets TERM=xterm is a bad idea imnsho. > Since I have seen much garbage from the URLs in the xfce4-terminal 0.6.3 > admittedly an old Ubuntu installation, but still in LTS status, > and no attempt from the xfc4-terminal to _ever_ implement URLs, This is true for *most* terminal emulators. I have nothing against this feature, I just wish it wouldn't annoy me on pretty much every system I use. None of which use "TERM=dumb", but none of which use "TERM=fancy-pants-term-v42" either. (Did anyone ever use "dumb", anyway?) Segher
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
On Fri, 2020-01-31 at 16:59 +, Bernd Edlinger wrote:
> Hi,
>
> this is patch is heavily based on David's original patch here:
> https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01409.html
>
> and addresses Jakub's review comments here:
> https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01412.html
>
> So I hope you don't mind, when I pick up this patch since there
> was not much activity recently on this issue, so I assumed you
> would appreciate some help.
Thanks Bernd; sorry, I got distracted by analyzer bug-fixing. It's
appreciated.
> I will try to improve the patch a bit, and hope you are gonna like
> it. I agree that this feature is fine, and should be enabled by
> default, and just if it is positively clear that it won't work,
> disabled in the auto mode.
>
> Also as requested by Jakub this tries to be more compatible to
> GCC_COLORS define, and adds the capability to switch between ST
> and BEL string termination and also have a way to disable urls
> even with -fdiagnostics-urls=always (like GCC_COLORS= disables
> colors).
>
> In addition to that I propose to use GCC_URLS and if that
> is not defined use TERM_URLS to control that feature for
> all applications willing to support that.
I think I like the overall idea.
> Since I have seen much garbage from the URLs in the xfce4-terminal
> 0.6.3
> admittedly an old Ubuntu installation, but still in LTS status,
> and no attempt from the xfc4-terminal to _ever_ implement URLs, I
> would
> like to detect the xfce4-terminal, and use that in auto mode
> to switch off the URLs feature, since in best case the URLs will
> just be ignored, but can and will often create garbage on the screen.
>
> Likewise on MinGW, since the windows 10 cmd prompt does at best
> ignore the URLs, but windows terminal and windows 7 cmd prompt
> print garbage when URLs are output.
That sounds reasonable, but we should document those exclusions, I
think.
I think the ChangeLog should also refer to "PR other/93168":
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93168
> diff --git a/gcc/diagnostic-color.c b/gcc/diagnostic-color.c
> index d554795..ad84d1f 100644
> --- a/gcc/diagnostic-color.c
> +++ b/gcc/diagnostic-color.c
> @@ -239,6 +239,56 @@ colorize_init (diagnostic_color_rule_t rule)
> }
> }
>
> +bool diagnostic_urls_use_st = false;
I don't like global variables (a pet peeve of mine); can you make this
be a field of the pretty_printer instead?
Even better: convert pretty_printer::show_urls from a bool to a new
enum; something like:
enum diagnostic_url_format
{
URL_FORMAT_NONE,
URL_FORMAT_ST,
URL_FORMAT_BEL
};
with suitable leading comments (probably a good place to summarize some
of the compatibility info within the source).
Then we could verify the differences between ST and BEL in the
selftests, and also not have the selftests be affected by env vars.
> +/* Return true if we should use urls when in always mode, false otherwise.
> + Additionally initialize diagnostic_urls_use_st to true if ST escapes
> + should be used and false for BEL escapes. */
Maybe have this return that 3-way enum instead?
> +
> +static bool
> +parse_gcc_urls()
> +{
> + const char *p;
> +
> + p = getenv ("GCC_URLS"); /* Plural! */
> + if (p == NULL)
> +p = getenv ("TERM_URLS");
> +
> + if (p == NULL)
> +return true;
> + if (*p == '\0')
> +return false;
> +
> + if (!strcmp (p, "no"))
> +return false;
> + if (!strcmp (p, "st"))
> +diagnostic_urls_use_st = true;
> + else if (!strcmp (p, "bel"))
> +diagnostic_urls_use_st = false;
> + return true;
> +}
> +
> +/* Return true if we should use urls when in auto mode, false otherwise. */
> +
> +static bool
> +auto_enable_urls ()
> +{
> +#ifdef __MINGW32__
> + return false;
> +#else
> + const char *p;
> +
> + if (!should_colorize ())
> +return false;
> + p = getenv ("COLORTERM");
> + if (p == NULL)
> +return true;
> + if (!strcmp (p, "xfce4-terminal"))
> +return false;
> + return true;
> +#endif
> +}
I think this logic warrants a comment for each of the exclusions: why
are we excluding them? I'd prefer to capture that in the source,
rather than just in the mailing list.
> /* Determine if URLs should be enabled, based on RULE.
> This reuses the logic for colorization. */
>
> @@ -250,9 +300,12 @@ diagnostic_urls_enabled_p (diagnostic_url_rule_t rule)
> case DIAGNOSTICS_URL_NO:
>return false;
> case DIAGNOSTICS_URL_YES:
> - return true;
> + return parse_gcc_urls ();
> case DIAGNOSTICS_URL_AUTO:
> - return should_colorize ();
> + if (auto_enable_urls ())
> + return parse_gcc_urls ();
> + else
> + return false;
> default:
>gcc_unreachable ();
> }
> diff --git a/gcc/diagnostic-url.h b/gcc/diagnostic-url.h
> index 6be0569..06b7784 100644
> --- a/gcc/diagnostic-url.h
> +++ b/gcc/diagnostic-url.h
> @@ -32,5 +32,6 @@ typedef enum
> } diagnostic_url_rule_t;
>
> extern bool
Re: [PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var
Hi Andrew, I just saw your patch here: https://gcc.gnu.org/ml/gcc-patches/2020-01/msg01474.html Re: [PATCH] gcc: Add new configure options to allow static libraries to be selected Note: the artefacts in my patch below seem to be a missing re-generated gcc/configure from your patch? Is that right? I did not notice any problem from this, does this work for you this way? And, is it the right thing to push those changes along this patch, or do you want to take care of this by yourself? Thanks Bernd. On 1/31/20 5:58 PM, Bernd Edlinger wrote: > Hi, > > this is patch is heavily based on David's original patch here: > https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01409.html > > and addresses Jakub's review comments here: > https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01412.html > > So I hope you don't mind, when I pick up this patch since there > was not much activity recently on this issue, so I assumed you > would appreciate some help. > > I will try to improve the patch a bit, and hope you are gonna like > it. I agree that this feature is fine, and should be enabled by > default, and just if it is positively clear that it won't work, > disabled in the auto mode. > > Also as requested by Jakub this tries to be more compatible to > GCC_COLORS define, and adds the capability to switch between ST > and BEL string termination and also have a way to disable urls > even with -fdiagnostics-urls=always (like GCC_COLORS= disables colors). > > In addition to that I propose to use GCC_URLS and if that > is not defined use TERM_URLS to control that feature for > all applications willing to support that. > > Since I have seen much garbage from the URLs in the xfce4-terminal 0.6.3 > admittedly an old Ubuntu installation, but still in LTS status, > and no attempt from the xfc4-terminal to _ever_ implement URLs, I would > like to detect the xfce4-terminal, and use that in auto mode > to switch off the URLs feature, since in best case the URLs will > just be ignored, but can and will often create garbage on the screen. > > Likewise on MinGW, since the windows 10 cmd prompt does at best > ignore the URLs, but windows terminal and windows 7 cmd prompt > print garbage when URLs are output. > > I have one question though, regarding the autoconf version that > we use, it is autoconf-2.69, right, at least that is what I read in > gcc/doc/install.texi? > > I don't understand why the generated configure has additional > changes than what is changed in configure.ac. Should I commit the > generated version as is, or use a different autoconf version? > > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > Is it OK for trunk? > > > Thanks > Benrd. >
