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
