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

[clutton]

On Fri, 2016-07-08 at 20:46 +0200, Felix Natter wrote:
> clutton <address@hidden> writes:
> 
> > 
> > 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...
> I think this comes close to it:
> http://stackoverflow.com/questions/23405853/how-to-check-for-the-exis
> tence-of-a-header-of-a-package
> 
> > 
> > > 
> > > 
> > > - 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.
> No problem.
> 
> > 
> > > 
> > >   
> > > 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.
> +1
> 
> > 
> > > 
> > > 
> > > 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.
> +1
> 
> > 
> > > 
> > > 
> > > 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.ht
> > > ml
> > > https://lists.gnu.org/archive/html/bug-gtypist/2014-01/threads.ht
> > > ml
> > > (especially the resolution:
> > > https://lists.gnu.org/archive/html/bug-gtypist/2014-01/msg00009.h
> > > tml)
> > > 
> > > 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?
> Sure, I've always done that :-)
> 
> Cheers and Best Regards,


I've deleted old autoall branch and created clear new one.
Please review. I tested this on several Linuxe's and on
Free/Net/Open|BSD. Would like to see it tested on MacOS (should be fine
too). And windows, I have no idea how you guys make it build for
windows...

The approach is of more extreme nature the previous one, but it seems
good with autotools philosophy (testing capabilities).

If everything is ok, the branch is ready for merging. If not: there are
my few thoughts: the test code could be separated to m4 directory (I
tried using ax_with_curses.m4 but it's more Linux only). Maybe using
resolv.h for headers (perfect autotools solution), but I'm ok with how
my patch solves everything now...

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