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

[Felix Natter]

clutton <address@hidden> writes:

> On Tue, 2016-06-28 at 14:41 +0200, Felix Natter wrote:
>> clutton <address@hidden> writes:
>> 
> [snip]

hello clutton,

> TL/DR: Using "-D_XOPEN_SOURCE_EXTENDED -I/usr/include/ncursesw" and
> #include <curses.h>
> would resolve all ncursesw.h functionality.
> The meter of discussion is how to achieve that.

Agreed.

> ---------------answering to Felix----------------------
> Why my last patch works: It's the same case when you don't include
> <stdio.h> and use printf. The linker would find your printf eventually
> by linking to standard libraries. We at the end linking to libncursesw,
> although the processor know nothing, it works because linking resolve
> Unknown symbols. The approach is unacceptable, anyway.
> ----------------end---------------------------

At first I couldn't believe it, so I tried myself [1] and you're right
:-)

[1] for the record:
#include <curses.h>

int main()
{
    initscr();
    noecho();
    int ch;
    int retcode = get_wch(&ch);
    endwin();
    printf("ch = %d\n", ch);
    return 0;
}

$ gcc -o test test.c -lncursesw

$ gcc -o test test.c -lncurses
/tmp/cchTWg1l.o: In function `main':
test.c:(.text+0x1f): undefined reference to `get_wch'
collect2: error: ld returned 1 exit status

It's unacceptable, agreed.

> Take a look at my recent commit, I made it work with -Wall. -Wall
> -Werror are flags which should be used by developers while coding to
> avoid such situations.

For the record:
$ gcc -Wall -o test test.c -lncurses
[...]
test.c: In function ‘main’:
test.c:9:5: warning: implicit declaration of function ‘get_wch’ 
[-Wimplicit-function-declaration]
     int retcode = get_wch(&ch);
[...]

We should definitely use -Wall! Maybe hardening flags
(https://wiki.debian.org/Hardening) as well...

> For now, omit the configure.ac file, and push/[push after reviewing]
> the C code. Or should I make a cleaning up as a final step?

Ok, here's a review:

- configure.ac: looks good, but do we need to add it to CFLAGS as well?
  (why is CPPFLAGS honored by plain gcc anyway?)

- of course cmdline.[ch] and getopt.c are not under our control, but I
  realize the changes are necessary to be able to work with -Wall.

- gtypist.c, infoview.c, script.h, utf8.c:
  If a variable isn't used, why not just remove (instead of uncomment)
  the declaration?
  
Also, do we want to enable "-Wall" in configure.ac (maybe -ansi
-pedantic as well)? This might break compilation with non-gcc compilers
though, so it's probably better to do this for development only.

Once this is fixed (should be easy), do you want to merge the branch
yourself (no conflicts)?
$ git checkout master
$ git merge autoall
$ <add+commit src/ChangeLog entry>

Next time it would be good to have cleaner branches (i.e. do not name
commits "curses.h testing").

[2] Tim added i18n support for GNU gengetopt, which unfortunately is not
released yet, so I commited the files to git temporarily.

BTW: Shall I commit this change? I think you've said that it
fixes your problem?

--- a/src/Makefile.am
+++ b/src/Makefile.am
-gtypist_LDADD = @LIBINTL@ # TODO: do we need @LIBICONV@ here?
+gtypist_LDADD = @LIBINTL@ @LIBICONV@

> Back to topic, after hacking a little I've found that nowadays projects
> resolve such issues by using pkg-config. With pkg-config it'll awfully
> simple to do proper checks. Something like:
>
> pkg-config --cflags-only-I ncursesw
> pkg-config --cflags ncursesw
>
> People hack it like that
>
> PKG_CHECK_MODULES([NCURSES], [ncursesw], [LIBS="$LIBS $NCURSES_LIBS";
> ncurses=ncursesw],
>   [AC_CHECK_LIB([ncursesw],
>                [initscr],
>                [LIBS="$LIBS -lncursesw"; ncurses=ncursesw],
>                [ncurses=ncurses])
> ])
>
> But, adding another dependence... What do you think? Should we left
> everything as it is now? Or use some hack instead of pkg-config?
>
> ncursesw has it's own utility for such a thing:ncursesw6-
> config, ncursesw5-config. But I vote against using them.

We've discussed using pkg-config before (after David from gentoo
reported that cbreak() sometimes is in -ltinfo):

See the thread "[bug-gtypist] gtypist fails to build with ncurses[tinfo]":
https://lists.gnu.org/archive/html/bug-gtypist/2013-09/threads.html
https://lists.gnu.org/archive/html/bug-gtypist/2014-01/threads.html
(especially the resolution:
https://lists.gnu.org/archive/html/bug-gtypist/2014-01/msg00009.html)

BTW: We also need to make sure that gentoo compilation still works,
but I think it should.

Cheers and Best Regards,
-- 
Felix Natter