On Mon, 2020-02-03 at 22:29 +0000, Bernd Edlinger wrote: > On 2/3/20 10:05 PM, Segher Boessenkool wrote: > > On Mon, Feb 03, 2020 at 08:16:52PM +0000, 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 <dmalc...@redhat.com> > Bernd Edlinger <bernd.edlin...@hotmail.de> > > 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; > +} > + > +/* 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 *t, *p; > + > + /* First check the terminal is capable of printing color escapes, > + if not URLs won't work either. */ > + if (!should_colorize ()) > + return false; > + > + t = getenv ("TERM"); > + p = getenv ("COLORTERM"); Could "t" and "p" be renamed to "term" and "colorterm", and be moved to immediately before they're used? > + /* xfce4-terminal is known to not implement URLs at this time. > + Recently new installations (0.8) will safely ignore the URL escape > + sequences, but a large number of legacy installations (0.6.3) print > + garbage when URLs are printed. Therefore we lose nothing by > + disabling this feature for that specific terminal type. */ > + if (p && !strcmp (p, "xfce4-terminal")) > + return false; > + > + /* Old versions of gnome-terminal where URL escapes cause screen > + corruptions set COLORTERM="gnome-terminal", recent versions > + with working URL support set this to "truecolor". */ > + if (p && !strcmp (p, "gnome-terminal")) > + return false; > + > + /* Since the following checks are less specific than the ones > + above, let GCC_URLS and TERM_URLS override the decision. */ > + if (getenv ("GCC_URLS") || getenv ("TERM_URLS")) > + return true; > + > + /* In an ssh session the COLORTERM is not there, but TERM=xterm > + can be used as an indication of a incompatible terminal while > + TERM=xterm-256color appears to be a working terminal. */ > + if (!p && t && !strcmp (t, "xterm")) > + return false; > + > + /* When logging in a linux over serial line, we see TERM=linux > + and no COLORTERM, it is unlikely that the URL escapes will > + work in that environmen either. */ > + if (!p && t && !strcmp (t, "linux")) > + return false; > + > + return true; > +#endif > +} > + > +/* 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 > +parse_env_vars_for_urls (diagnostic_url_rule_t rule) > { I think this introduces overloading here, given that a no-arguments parse_env_vars_for_urls is declared above. I'd prefer them to have different names from each other. I like the name "parse_env_vars_for_urls" for the subroutine that actually parses the env vars, so perhaps rename this one-argument one to "determine_url_format" or somesuch? > switch (rule) > { > case DIAGNOSTICS_URL_NO: > - return false; > + return URL_FORMAT_NONE; > case DIAGNOSTICS_URL_YES: > - return true; > + return parse_env_vars_for_urls (); > case DIAGNOSTICS_URL_AUTO: > - return should_colorize (); > + if (auto_enable_urls ()) > + return parse_env_vars_for_urls (); > + else > + return URL_FORMAT_NONE; > default: > gcc_unreachable (); > } [...] OK for master with the nitpicks above fixed. Thanks Dave