Re: [bug-inetutils] Readline code

[Simon Josefsson]

Mats Erik Andersson <address@hidden> writes:

> fredag den 18 november 2011 klockan 09:55 skrev Simon Josefsson detta:
>> Mats Erik Andersson <address@hidden> writes:
>> 
>> > måndag den 14 november 2011 klockan 11:21 skrev Simon Josefsson detta:
>> >> Bruno Haible <address@hidden> writes:
>> >> > In other words, you need to add $(LIBREADLINE) to the value of LDADD
>> >> > in telnet/Makefile.am.
>> >> >
>> >> > Likewise in telnetd/Makefile.am.
>> >> 
>> >> Thanks for analyzing, I agree and pushed this tiny change in your name.
>> >> 
>> >> > Quite odd situation, but IMO the readline macro should not set
>> >> > HAVE_LIBREADLINE
>> >> > if the include files are missing, or the code in ftp/cmds.c should
>> >> > be testing
>> >> >   HAVE_LIBREADLINE && HAVE_READLINE_READLINE_H
>> >> > instead of just
>> >> >   HAVE_LIBREADLINE
>> >> 
>> >> Yes.  However, wouldn't an even better solution be to use
>> >> AC_LIB_LINKFLAGS here instead of the current check for readline?  That
>> >> should be more reliable.
>> >
>> > Related to this is the fact that <term.h> must be prepended by <curses.h>
>> > in all standards. Also <stropts.h> is needed with HAVE_STREAMSPTY.
>> >
>> > The commit 0a83fb1 attends to these matters and it achieves correct
>> > building of telnet and telnetd for
>> >
>> >   GNU/Linux, GNU/kFreeBSD, GNU/OpenSolaris, OpenBSD, and OpenIndiana.
>> 
>> I noticed that there is a gnulib module called 'readline' that InetUtils
>> is already including (!) so I have removed all cruft in configure.ac
>> regarding readline and fixed some usage for ftp.  The one part I feel
>> less confident about is the libhistory stuff -- but I reckon that if
>> something more is needed, we should handle it in a modern way (i.e.,
>> either AC_LIB_LINKFLAGS or gnulib).
>> 
>> Please test this whether it builds now.  Eventually we should make
>> another snapshot, but there are more things that needs to be attended to
>> from the first platform-testers response, so let's hold off that until
>> we have fixed more issues.
>
> You forgot to include a patch suggestion, or to push the change!
> I will test when it arrives.

It is in there now.  However I did notice a problem: the gnulib readline
module is a minimal implementation and there is this comment in the
gnulib readline module:

/* This module is intended to be used when the application only needs
   the readline interface.  If you need more functions from the
   readline library, it is recommended to require the readline library
   (or improve this module) rather than #if-protect part of your
   application (doing so would add assumptions of this module into
   your application).  The application should use #include
   "readline.h", that header file will include <readline/readline.h>
   if the real library is present on the system. */

I think that makes sense.  So I think we shouldn't really use the gnulib
readline module, since InetUtils also needs readline/history.h and
add_history which aren't provided by the gnulib module.

Any objections to just have InetUtils require that libreadline is
installed on the system and refuse to build if it is not?
Alternatively, instead of refusing to build, we could disable the ftp
client since it is the only tool using the readline library right now.

We still want gnulib's check of libreadline though since it searches for
termcap etc.  This should probably be refactored in gnulib, so you can
get readline.m4 without readline.c.

/Simon