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

[clutton]

On Fri, 2016-07-08 at 17:52 +0200, Felix Natter wrote:
> clutton <address@hidden> writes:
[snip]

> > 
> 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?)

Nonono, that's not what I meant to commit, it depicts my final
intention.I need to determine where ncursesw.h resides and add this
directory to -I. Then I make clear branch/patch. May be you can give me
some suggestions how to do it using autotools? (since pkg-config would
be redundancy...) It'll save some time...

> 
> - 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?

I think I missed that, sorry.

>   
> 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.

Indeed, it's a bad practice. Developers must build using -Wetc. Release
shouldn't have such flags.

> 
> 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>

I'll make a clean one. That branch were no more then testing and
investigation.

> 
> 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@

No, wait a little, till I finish with that stupid ncursesw thing, it
should be simpler now, I have more time.

> 
> > 
> > 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.


Ok, I see, using pkg-config is not an option.

My steps would be:

1) Proper ncursesw header inclusion, using autotools
2) I'll make the project build with -Wall/etc flags.
3) Iconv, as you suggested, although when we reach this point you could
add it by yourself.

Ok, 3 tasks. You will review and merge. Deal?

signature.asc
Description: This is a digitally signed message part