Re: [bug-gtypist] ideas, implementations, patches.

[Felix Natter]

clutton <address@hidden> writes:

helloo clutton,

> On Sat, 2016-05-28 at 11:45 +0200, Felix Natter wrote:
>> clutton <address@hidden> writes:
>> 
>> > 
>> > On Sun, 2016-05-15 at 10:00 +0200, Felix Natter wrote:
>> > > 
>> > > clutton <address@hidden> writes:
>> hello Clutton,
>> 
>> > 
>> > Could you guys please check this patch out?
>> Now I am doing this, sorry for the delay.
>> 
>> > 
>> > 1) malloc->stdlib obvious.
>> Should be ok.
>
> Ok, it's part of of stdlib.
>
>> 
>> > 
>> > 2) the change I was talking about, include <curses.h> instead of
>> > <ncursesw/ncursesw.h>, if you still don't believe that this is the
>> > thing to do check out if your curses.h has function prototypes
>> > like add_wch, etc.
>> This does not work for Debian/GNU (only works for BSD curses as far
>> as I
>> understand you):
>> 
>> In Debian/GNU, ncurses and ncursesw are separated.
>> /usr/include/curses.h
>> does not have add_wch & co:
>> 
>> $ dpkg -S /usr/include/curses.h 
>> libncurses5-dev:amd64: /usr/include/curses.h
>> 
>> $ grep add_wch /usr/include/curses.h
>> 
>> // this is what we need on GNU:
>> $ dpkg -S /usr/include/ncursesw/ncurses.h
>> libncursesw5-dev:amd64: /usr/include/ncursesw/ncurses.h
>> 
>> $ grep add_wch /usr/include/ncursesw/ncurses.h
>> extern NCURSES_EXPORT(int) add_wch (const cchar_t *);
>> [...]
>> 
>> So if you need /usr/include/curses.h (and don't have
>> ncursesw/ncurses.h), then we need to do sth like:
>> 
>> # check for ncursesw (GNU)
>> AC_CHECK_HEADER(ncursesw/ncurses.h, HAVE_NCURSESW_H=1)
>> if test -n "$HAVE_NCURSESW_H";  then
>>    AC_CHECK_HEADER(curses.h, HAVE_CURSES_H=1)
>>    if test -n "$HAVE_CURSES_H";  then
>>       echo -e "Couldn't find ncursesw/curses header."\
>>           "On Debian/Ubuntu you need to install libncursesw5-dev."
>>       exit 1;
>>    fi
>> fi
>> 
>> which is not perfect because it will silently use curses.h if
>> ncursesw
>> is not installed on GNU systems. But I guess we will catch that
>> problem
>> when doing AC_CHECK_LIB(ncursesw, add_wch) afterwards.
>> 
>> Then we need this:
>> 
>> #if defined(HAVE_PDCURSES) || !defined(HAVE_NCURSESW_H)
>> #include <curses.h>
>> #else
>> #include <ncursesw/ncurses.h>
>> #endif
>
> Understood, let it leave as it is. Doing gnu projects, I wish I had all
> those OS's near me to see what is going on.

If you want to, you could set up a virtualbox or xen vm :-)

>> 
>> > 
>> > 2) AM_ICONV doesn't work for me somehow, does it add -liconv for
>> > you?
>> > It looks like everything is ok
>> Using AM_ICONV seems to be a good idea because it handles corner
>> cases:
>>   https://www.gnu.org/software/gettext/manual/html_node/AM_005fICONV.
>> html
>> 
>> > 
>> > checking for iconv... yes
>> > checking for working iconv... yes
>> > checking how to link with libiconv... /usr/local/lib/libiconv.so
>> > -Wl,-
>> > rpath -Wl,/usr/local/lib
>> > checking for iconv declaration... 
>> >          extern size_t iconv (iconv_t cd, char * *inbuf, size_t
>> > *inbytesleft, char * *outbuf, size_t *outbytesleft);
>> > checking whether NLS is requested... yes
>> > checking for msgfmt... /usr/local/bin/msgfmt
>> > checking for gmsgfmt... /usr/local/bin/msgfmt
>> > checking for xgettext... /usr/local/bin/xgettext
>> > checking for msgmerge... /usr/local/bin/msgmerge
>> > checking for CFPreferencesCopyAppValue... no
>> > checking for CFLocaleCopyCurrent... no
>> > checking for GNU gettext in libc... no
>> > checking for iconv... (cached) yes
>> > checking for working iconv... (cached) yes
>> > checking how to link with libiconv... /usr/local/lib/libiconv.so
>> > -Wl,-
>> > rpath -Wl,/usr/local/lib
>> > 
>> >     But then I have a linking error:
>> > 
>> > gcc  -g -O2   -o gtypist gtypist.o cursmenu.o  script.o error.o
>> > getopt.o  getopt1.o infoview.o speedbox.o  banner.o cmdline.o
>> > utf8.o
>> > /usr/local/lib/libintl.so -Wl,-rpath -Wl,/usr/local/lib -lncursesw
>> > utf8.o: In function `convertUTF8ToCurrentEncoding':
>> > /home/develop/c/gtypist/src/utf8.c:65: undefined reference to
>> > `libiconv_open'
>> > /home/develop/c/gtypist/src/utf8.c:76: undefined reference to
>> > `libiconv'
>> > /home/develop/c/gtypist/src/utf8.c:95: undefined reference to
>> > `libiconv_close'
>> > collect2: error: ld returned 1 exit status
>> > 
>> > 
>> > Any ideas about this? One way must be removed, either AM_ICONV or
>> > mine,
>> > which is:
>> > AC_CHECK_LIB(iconv, iconv_open, [], [AC_MSG_ERROR(
>> >     [iconv library is missing: See INSTALL file for further
>> > information])])
>> The gcc command line is missing -liconv. Could you please try this
>> patch?
>> 
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index e0a6ad3..03fafed 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -12,7 +12,7 @@ gtypist_SOURCES = gtypist.c cursmenu.c script.c
>> error.c getopt.c getopt1.c           \
>>      infoview.c speedbox.c banner.c cmdline.c cursmenu.h error.h
>> getopt.h             \
>>      gettext.h gtypist.h script.h utf8.c utf8.h infoview.h
>> speedbox.h banner.h  \
>>      cmdline.h cmdline.ggo
>> -gtypist_LDADD = @LIBINTL@ # TODO: do we need @LIBICONV@ here?
>> +gtypist_LDADD = @LIBINTL@ @LIBICONV@
>>  
>>  AM_CPPFLAGS = -I../intl \
>>    -DDATADIR=\"$(pkgdatadir)\" \
>> 
>> I cannot test it on GNU since iconv is included in GNU libc.
>
> This is the problem, -liconv should be used on systems which doesn't
> have iconv in libc. And seems AM_ICONV doesn't do the job. Recent
> iconv.m4 doesn't work also.
> I'll look what can be done.

Could you please try this simply change in Makefile.am?
-gtypist_LDADD = @LIBINTL@ # TODO: do we need @LIBICONV@ here?
+gtypist_LDADD = @LIBINTL@ @LIBICONV@

I think this should add -liconv on your system.

> Anyway I'd like to have this code instead of current. It's more
> auto* way, and we are doing it by hands. I'll change curses.h ->
> ncursesw.h

ncursesw/ncurses.h ;-)

> # check for courses.h
> AC_CHECK_HEADER(curses.h, [], [AC_MSG_ERROR(
>     [curses.h header is missing: See INSTALL file for further
> information])])
> # check for libncursesw
> AC_CHECK_LIB(ncursesw, add_wch, [], [AC_MSG_ERROR(
>     [ncursesw library is missing: See INSTALL file for further
> information])])
>
> Is it ok?

Yeah that's fine, but maybe we want to keep a short message like
"On Debian/Ubuntu you need to install libncursesw5-dev." because
that is a common case?

> Could you please point me to current master branch location? The branch
> I'm using is git://git.savannah.gnu.org/gtypist.git(master)

This is fine.

> May be a little bit earlier, but, could I have some dev branch to push
> my work so you can see it directly?

I couldn't ask Tim yet, but I think he will agree, too. The first step
is to register on http://savannah.gnu.org.

Cheers and Best Regards,
-- 
Felix Natter