diff mbox

[PATCHSET,4/4] Fix various issue in TUI

Message ID 83y4p1zq7k.fsf@gnu.org
State New
Headers show

Commit Message

Eli Zaretskii Jan. 17, 2015, 9:55 a.m. UTC
> Date: Mon, 05 Jan 2015 22:06:07 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: gdb-patches@sourceware.org
> 
> > Date: Mon, 05 Jan 2015 19:54:42 +0000
> > From: Pedro Alves <palves@redhat.com>
> > 
> > On 12/31/2014 05:56 PM, Eli Zaretskii wrote:
> > > Well, one patch is Windows-specific after all.  This patch makes sure
> > > windows-termcap is not compiled when GDB is linked against ncurses,
> > 
> > ...
> > 
> > > and also makes the file a no-op should it compile in that
> > > configuration.  
> > 
> > With the configure.ac change, how can that happen?
> 
> It shouldn't.
> 
> > > --- gdb/configure.ac~0	2014-10-29 21:45:50 +0200
> > > +++ gdb/configure.ac	2014-12-30 07:42:27 +0200
> > > @@ -627,9 +627,10 @@
> > >      ac_cv_search_tgetent="none required"
> > >      ;;
> > >    *mingw32*)
> > > -    ac_cv_search_tgetent="none required"
> > > -    CONFIG_OBS="$CONFIG_OBS windows-termcap.o"
> > > -    ;;
> > > +    if test x"$prefer_curses" = xyes; then
> > > +      ac_cv_search_tgetent="none required"
> > > +      CONFIG_OBS="$CONFIG_OBS windows-termcap.o"
> > 
> > I'm confused on the predicate here.  How can we tell
> > from $prefer_curses that the curses library doesn't include
> > termcap?  AFAICS, if the TUI is enabled, it'll always be
> > "yes".  But isn't that the case where you _don't_
> > want windows-termcap.o?  And shouldn't this be checking
> > $curses_found?
> 
> Sorry, I sent the wrong patch here.  It should be !=, of course.
> 
> > > --- gdb/windows-termcap.c~0	2014-06-11 18:34:41 +0300
> > > +++ gdb/windows-termcap.c	2014-12-29 15:42:44 +0200
> > > @@ -19,6 +19,11 @@
> > >     You should have received a copy of the GNU General Public License
> > >     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> > >  
> > > +
> > > +#include "config.h"
> > 
> > All files in gdb/ should start with "defs.h" instead.
> 
> OK.

I just realized that I cannot push this changeset because Autoconf I
have installed isn't version 2.64, and so I cannot regenerate
'configure'.  Could someone please do that for me?  The patch is
below.

Or I could push the patch below, and then someone else could do the
generated files as a separate commit.

Thanks in advance.

Don't use windows-termcap.c when linking against a curses library

gdb/
2015-01-17  Eli Zaretskii  <eliz@gnu.org>

	* configure.ac [*mingw32*]: Only add windows-termcap.o to
	CONFIG_OBS if not building with a curses library.

	* windows-termcap.c: Include defs.h.  Make the whole body empty if
	either one of HAVE_CURSES_H or HAVE_NCURSES_H or
	HAVE_NCURSES_NCURSES_H is defined.

Comments

Joel Brobecker Jan. 19, 2015, 2:50 p.m. UTC | #1
> I just realized that I cannot push this changeset because Autoconf I
> have installed isn't version 2.64, and so I cannot regenerate
> 'configure'.  Could someone please do that for me?  The patch is
> below.
> 
> Or I could push the patch below, and then someone else could do the
> generated files as a separate commit.

I will take care of it all.
Joel Brobecker Jan. 19, 2015, 3:43 p.m. UTC | #2
> I just realized that I cannot push this changeset because Autoconf I
> have installed isn't version 2.64, and so I cannot regenerate
> 'configure'.  Could someone please do that for me?  The patch is
> below.
> 
> Or I could push the patch below, and then someone else could do the
> generated files as a separate commit.
> 
> Thanks in advance.
> 
> Don't use windows-termcap.c when linking against a curses library
> 
> gdb/
> 2015-01-17  Eli Zaretskii  <eliz@gnu.org>
> 
> 	* configure.ac [*mingw32*]: Only add windows-termcap.o to
> 	CONFIG_OBS if not building with a curses library.
> 
> 	* windows-termcap.c: Include defs.h.  Make the whole body empty if
> 	either one of HAVE_CURSES_H or HAVE_NCURSES_H or
> 	HAVE_NCURSES_NCURSES_H is defined.

I pushed this patch after having regenerated the configure script.
I edited the ChangeLog entry to add a line for configure being
regenerated.

> --- a/libiberty/ChangeLog
> +++ b/libiberty/ChangeLog
> @@ -1,3 +1,8 @@
> +2015-01-17  Eli Zaretskii  <eliz@gnu.org>
> +
> +	* strerror.c <sys_nerr, sys_errlist>: Declare only if they aren't
> +	macros.

I pushed this change separately (and first).

To answer your question, such changes should be pushed to both
the GCC repository (SVN), and the binutils-gdb repository (git).

The commits are now in master; I can't remember whether these were
approved for 7.9, but if they were, then you can cherry-pick them
from master.
Eli Zaretskii Jan. 19, 2015, 4:26 p.m. UTC | #3
> Date: Mon, 19 Jan 2015 15:50:40 +0100
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: palves@redhat.com, gdb-patches@sourceware.org
> 
> > I just realized that I cannot push this changeset because Autoconf I
> > have installed isn't version 2.64, and so I cannot regenerate
> > 'configure'.  Could someone please do that for me?  The patch is
> > below.
> > 
> > Or I could push the patch below, and then someone else could do the
> > generated files as a separate commit.
> 
> I will take care of it all.

Thank you.

Btw, what is the reason we insist on that particular version of
Autoconf, and not on "2.64 or later"?  It's a pain to have an old
version installed (already there are packages for which 2.65 is too
old).
Joel Brobecker Jan. 19, 2015, 4:54 p.m. UTC | #4
> Btw, what is the reason we insist on that particular version of
> Autoconf, and not on "2.64 or later"?  It's a pain to have an old
> version installed (already there are packages for which 2.65 is too
> old).

I am not absolutely certain. I have a feeling from past experience
that upgrading to new versions is not necessarily a big deal, but
something that needs to be done with care, as it might not work
with new versions of configure. I know that I accidently used
a newer version of configuring in the past, and got errors, so
it doesn't always work, at least for me.

But other than that, one of my reasons is that we know it works, and
using the exact version also allows you to generate changes that are
directly related to your changes and only your changes - which makes
reviewing the patch for the generated file a little easier.
Eli Zaretskii Jan. 19, 2015, 5:31 p.m. UTC | #5
> Date: Mon, 19 Jan 2015 16:43:01 +0100
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: palves@redhat.com, gdb-patches@sourceware.org
> 
> > Don't use windows-termcap.c when linking against a curses library
> > 
> > gdb/
> > 2015-01-17  Eli Zaretskii  <eliz@gnu.org>
> > 
> > 	* configure.ac [*mingw32*]: Only add windows-termcap.o to
> > 	CONFIG_OBS if not building with a curses library.
> > 
> > 	* windows-termcap.c: Include defs.h.  Make the whole body empty if
> > 	either one of HAVE_CURSES_H or HAVE_NCURSES_H or
> > 	HAVE_NCURSES_NCURSES_H is defined.
> 
> I pushed this patch after having regenerated the configure script.
> I edited the ChangeLog entry to add a line for configure being
> regenerated.

Thanks!

> > --- a/libiberty/ChangeLog
> > +++ b/libiberty/ChangeLog
> > @@ -1,3 +1,8 @@
> > +2015-01-17  Eli Zaretskii  <eliz@gnu.org>
> > +
> > +	* strerror.c <sys_nerr, sys_errlist>: Declare only if they aren't
> > +	macros.
> 
> I pushed this change separately (and first).

Thanks again.

> To answer your question, such changes should be pushed to both
> the GCC repository (SVN), and the binutils-gdb repository (git).

OK, so I presume DJ will commit to the GCC repository?  I don't think
I have write access there.  Or did you do that already?

> The commits are now in master; I can't remember whether these were
> approved for 7.9, but if they were, then you can cherry-pick them
> from master.

Done.
Joel Brobecker Jan. 19, 2015, 5:54 p.m. UTC | #6
> > To answer your question, such changes should be pushed to both
> > the GCC repository (SVN), and the binutils-gdb repository (git).
> 
> OK, so I presume DJ will commit to the GCC repository?  I don't think
> I have write access there.  Or did you do that already?

Sorry - I wasn't clear. I did push it to gcc.
Pedro Alves Jan. 22, 2015, 12:07 p.m. UTC | #7
On 01/17/2015 09:55 AM, Eli Zaretskii wrote:
>> > Date: Mon, 05 Jan 2015 22:06:07 +0200
>> > From: Eli Zaretskii <eliz@gnu.org>
>> > Cc: gdb-patches@sourceware.org
>> > 
>>> > > Date: Mon, 05 Jan 2015 19:54:42 +0000
>>> > > From: Pedro Alves <palves@redhat.com>
>>> > > 
>>> > > On 12/31/2014 05:56 PM, Eli Zaretskii wrote:
>>>> > > > Well, one patch is Windows-specific after all.  This patch makes sure
>>>> > > > windows-termcap is not compiled when GDB is linked against ncurses,
>>> > > 
>>> > > ...
>>> > > 
>>>> > > > and also makes the file a no-op should it compile in that
>>>> > > > configuration.  
>>> > > 
>>> > > With the configure.ac change, how can that happen?
>> > 
>> > It shouldn't.

I'd have preferred to drop that hunk then.  It just seems to
be either pointless or hiding some problem with the
configure.ac check.

> Don't use windows-termcap.c when linking against a curses library
> 
> gdb/
> 2015-01-17  Eli Zaretskii  <eliz@gnu.org>
> 
> 	* configure.ac [*mingw32*]: Only add windows-termcap.o to
> 	CONFIG_OBS if not building with a curses library.
> 
> 	* windows-termcap.c: Include defs.h.  Make the whole body empty if
> 	either one of HAVE_CURSES_H or HAVE_NCURSES_H or
> 	HAVE_NCURSES_NCURSES_H is defined.
> 
> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 8dd7f8f..b852b93 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -611,9 +611,10 @@ case $host_os in
>      ac_cv_search_tgetent="none required"
>      ;;
>    *mingw32*)
> -    ac_cv_search_tgetent="none required"
> -    CONFIG_OBS="$CONFIG_OBS windows-termcap.o"
> -    ;;
> +    if test x"$prefer_curses" != xyes; then
> +      ac_cv_search_tgetent="none required"
> +      CONFIG_OBS="$CONFIG_OBS windows-termcap.o"
> +    fi ;;

I'm still confused and not convinced this is the right
predicate.  You said:

 "This patch makes sure windows-termcap is not compiled
  when GDB is linked against ncurses".

But that doesn't look to be what the patch does.

I asked about $curses_found before because AFAICS, it's quite
possible for $prefer_curses to be set, but $curses_found
to be "no".  By default, we try configuring the TUI:

 # Enable TUI.
 AC_ARG_ENABLE(tui,
 AS_HELP_STRING([--enable-tui], [enable full-screen terminal user interface (TUI)]),
   [case $enableval in
     yes | no | auto)
       ;;
     *)
       AC_MSG_ERROR([bad value $enableval for --enable-tui]) ;;
   esac],enable_tui=auto)

And that always results in $prefer_curses set to yes:

 # For the TUI, we need enhanced curses functionality.
 if test x"$enable_tui" != xno; then
   prefer_curses=yes
 fi


But then we check if curses is really found:

 curses_found=no
 if test x"$prefer_curses" = xyes; then
 ...
   AC_SEARCH_LIBS(waddstr, [ncurses cursesX curses])

   if test "$ac_cv_search_waddstr" != no; then
     curses_found=yes
   fi
 fi


And then it's $curses_found that we check:

# Check whether we should enable the TUI, but only do so if we really
# can.
if test x"$enable_tui" != xno; then
  if test -d $srcdir/tui; then
    if test "$curses_found" != no; then
...
    else
      if test x"$enable_tui" = xyes; then
	AC_MSG_ERROR([no enhanced curses library found; disable TUI])
      else
	AC_MSG_WARN([no enhanced curses library found; disabling TUI])
      fi
    fi
  fi
fi


So shouldn't the right check be this:

    if test x"$curses_found" != xyes; then
      ac_cv_search_tgetent="none required"
      CONFIG_OBS="$CONFIG_OBS windows-termcap.o"
    fi ;;

?

Thanks,
Pedro Alves
Eli Zaretskii Jan. 22, 2015, 4:26 p.m. UTC | #8
> Date: Thu, 22 Jan 2015 12:07:26 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: gdb-patches@sourceware.org
> 
> >>> > > On 12/31/2014 05:56 PM, Eli Zaretskii wrote:
> >>>> > > > Well, one patch is Windows-specific after all.  This patch makes sure
> >>>> > > > windows-termcap is not compiled when GDB is linked against ncurses,
> >>> > > 
> >>> > > ...
> >>> > > 
> >>>> > > > and also makes the file a no-op should it compile in that
> >>>> > > > configuration.  
> >>> > > 
> >>> > > With the configure.ac change, how can that happen?
> >> > 
> >> > It shouldn't.
> 
> I'd have preferred to drop that hunk then.  It just seems to
> be either pointless or hiding some problem with the
> configure.ac check.

At the very least how about an #error in those conditions?  Otherwise,
any bit-rot (something that happens now and then with the MinGW build)
will silently do the wrong thing.

>  "This patch makes sure windows-termcap is not compiled
>   when GDB is linked against ncurses".
> 
> But that doesn't look to be what the patch does.
> 
> I asked about $curses_found before because AFAICS, it's quite
> possible for $prefer_curses to be set, but $curses_found
> to be "no".  By default, we try configuring the TUI:
> 
>  # Enable TUI.
>  AC_ARG_ENABLE(tui,
>  AS_HELP_STRING([--enable-tui], [enable full-screen terminal user interface (TUI)]),
>    [case $enableval in
>      yes | no | auto)
>        ;;
>      *)
>        AC_MSG_ERROR([bad value $enableval for --enable-tui]) ;;
>    esac],enable_tui=auto)
> 
> And that always results in $prefer_curses set to yes:
> 
>  # For the TUI, we need enhanced curses functionality.
>  if test x"$enable_tui" != xno; then
>    prefer_curses=yes
>  fi
> 
> 
> But then we check if curses is really found:
> 
>  curses_found=no
>  if test x"$prefer_curses" = xyes; then
>  ...
>    AC_SEARCH_LIBS(waddstr, [ncurses cursesX curses])
> 
>    if test "$ac_cv_search_waddstr" != no; then
>      curses_found=yes
>    fi
>  fi
> 
> 
> And then it's $curses_found that we check:
> 
> # Check whether we should enable the TUI, but only do so if we really
> # can.
> if test x"$enable_tui" != xno; then
>   if test -d $srcdir/tui; then
>     if test "$curses_found" != no; then
> ...
>     else
>       if test x"$enable_tui" = xyes; then
> 	AC_MSG_ERROR([no enhanced curses library found; disable TUI])
>       else
> 	AC_MSG_WARN([no enhanced curses library found; disabling TUI])
>       fi
>     fi
>   fi
> fi
> 
> 
> So shouldn't the right check be this:
> 
>     if test x"$curses_found" != xyes; then
>       ac_cv_search_tgetent="none required"
>       CONFIG_OBS="$CONFIG_OBS windows-termcap.o"
>     fi ;;
> 
> ?

I'm not sure.  GDB can be configured --without-tui --with-curses.
Does your logic work then?
Pedro Alves Jan. 22, 2015, 5:15 p.m. UTC | #9
On 01/22/2015 04:26 PM, Eli Zaretskii wrote:
>> Date: Thu, 22 Jan 2015 12:07:26 +0000
>> From: Pedro Alves <palves@redhat.com>
>> CC: gdb-patches@sourceware.org
>>
>>>>>>> On 12/31/2014 05:56 PM, Eli Zaretskii wrote:
>>>>>>>>> Well, one patch is Windows-specific after all.  This patch makes sure
>>>>>>>>> windows-termcap is not compiled when GDB is linked against ncurses,
>>>>>>>
>>>>>>> ...
>>>>>>>
>>>>>>>>> and also makes the file a no-op should it compile in that
>>>>>>>>> configuration.  
>>>>>>>
>>>>>>> With the configure.ac change, how can that happen?
>>>>>
>>>>> It shouldn't.
>>
>> I'd have preferred to drop that hunk then.  It just seems to
>> be either pointless or hiding some problem with the
>> configure.ac check.
> 
> At the very least how about an #error in those conditions?  Otherwise,
> any bit-rot (something that happens now and then with the MinGW build)
> will silently do the wrong thing.

Well, I fear that the #ifdefery can bit rot just as well.  So having
two places that can bitrot (configure and #ifdef) seems worse than
one.

>>
>> So shouldn't the right check be this:
>>
>>     if test x"$curses_found" != xyes; then
>>       ac_cv_search_tgetent="none required"
>>       CONFIG_OBS="$CONFIG_OBS windows-termcap.o"
>>     fi ;;
>>
>> ?
> 
> I'm not sure.  GDB can be configured --without-tui --with-curses.
> Does your logic work then?

It's the same thing.  That sets $prefer_curses:

 opt_curses=no
 AC_ARG_WITH(curses, AS_HELP_STRING([--with-curses], [use the curses library instead of the termcap library]), opt_curses=$withval)

 prefer_curses=no
 if test "$opt_curses" = "yes"; then
   prefer_curses=yes
 fi


But that doesn't mean that curses will be used.  We'll again fall here:

curses_found=no
if test x"$prefer_curses" = xyes; then
...
  AC_SEARCH_LIBS(waddstr, [ncurses cursesX curses])

  if test "$ac_cv_search_waddstr" != no; then
    curses_found=yes
  fi
fi

And if waddstr is not found, meaning curses is not really
available, even though it'd be preferred, $prefer_curses is
yes, but $curses_found is no.

Thanks,
Pedro Alves
Eli Zaretskii Jan. 22, 2015, 5:29 p.m. UTC | #10
> Date: Thu, 22 Jan 2015 17:15:00 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: gdb-patches@sourceware.org
> 
> >> So shouldn't the right check be this:
> >>
> >>     if test x"$curses_found" != xyes; then
> >>       ac_cv_search_tgetent="none required"
> >>       CONFIG_OBS="$CONFIG_OBS windows-termcap.o"
> >>     fi ;;
> >>
> >> ?
> > 
> > I'm not sure.  GDB can be configured --without-tui --with-curses.
> > Does your logic work then?
> 
> It's the same thing.

Then please install your variant (on the branch as well).  I trust you
know this stuff much better than I do.

Thanks.
diff mbox

Patch

diff --git a/gdb/configure.ac b/gdb/configure.ac
index 8dd7f8f..b852b93 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -611,9 +611,10 @@  case $host_os in
     ac_cv_search_tgetent="none required"
     ;;
   *mingw32*)
-    ac_cv_search_tgetent="none required"
-    CONFIG_OBS="$CONFIG_OBS windows-termcap.o"
-    ;;
+    if test x"$prefer_curses" != xyes; then
+      ac_cv_search_tgetent="none required"
+      CONFIG_OBS="$CONFIG_OBS windows-termcap.o"
+    fi ;;
 esac
 
 # These are the libraries checked by Readline.
diff --git a/gdb/windows-termcap.c b/gdb/windows-termcap.c
index 026c3d2..b366f65 100644
--- a/gdb/windows-termcap.c
+++ b/gdb/windows-termcap.c
@@ -19,6 +19,11 @@ 
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
+
+#include "defs.h"
+
+#if !defined HAVE_CURSES_H && !defined HAVE_NCURSES_H && !defined HAVE_NCURSES_NCURSES_H
+
 #include <stdlib.h>
 
 /* -Wmissing-prototypes */
@@ -71,3 +76,5 @@ 
 {
   return NULL;
 }
+
+#endif	/* !HAVE_CURSES_H && !HAVE_NCURSES_H && !HAVE_NCURSES_NCURSES_H */
diff --git a/libiberty/ChangeLog b/libiberty/ChangeLog
index ac23875..832ce0b 100644
--- a/libiberty/ChangeLog
+++ b/libiberty/ChangeLog
@@ -1,3 +1,8 @@ 
+2015-01-17  Eli Zaretskii  <eliz@gnu.org>
+
+	* strerror.c <sys_nerr, sys_errlist>: Declare only if they aren't
+	macros.
+
 2014-12-24  Uros Bizjak  <ubizjak@gmail.com>
 	    Ben Elliston  <bje@au.ibm.com>
 	    Manuel Lopez-Ibanez  <manu@gcc.gnu.org>
diff --git a/libiberty/strerror.c b/libiberty/strerror.c
index 0efadc3..a8a0bd1 100644
--- a/libiberty/strerror.c
+++ b/libiberty/strerror.c
@@ -469,8 +469,13 @@  struct error_info
 
 #else
 
+
+#ifndef sys_nerr
 extern int sys_nerr;
+#endif
+#ifndef sys_errlist
 extern char *sys_errlist[];
+#endif
 
 #endif