Automake Support for cyrus-imapd 2.5

[Дилян Палаузов]

Hello,

> commit "imap/Makefile.in: add .c.snmp: recipe"
>
> The old makefile will rebuild pushstats.c if the snmpgen script changes.
>   The new one won't.

Please feel entitled to fix it.

> commit "lib/mkchartable.pl move output from stderr to stdout"
>
> In mkchartable.pl, you need to handle the open() failing.
>
> Also, mkchartable.pl can fail partway through, e.g. if one of the
> charset/*.t files is not readable.  In the old makefile, such a failure
> would trigger this code
>
> -        || (rm -f chartable.c&&  exit 1)
>
> which would break the build and avoid half-creating a chartable.c so
> that the next attempt to build was also (correctly) fail.  In the new
> makefile there is no such safeguard: a partly finished chartable.c would
> be created and the next attempt to build would (incorrectly) not fail at
> this point.

Please feel entitled to fix it.

> commit "Prepend the path in #include to .et ->  .h files"
>
> I am deeply confused about why this is necessary, can you explain?
>
> Getting rid of these is obviously useful:
>
> -#include "../lib/imapurl.h"
> -#include "../lib/util.h"
>
> but I don't see why you would change
>
> -#include "imap_err.h"
> +#include "imap/imap_err.h"
>
> without changing any of the other #include lines around it.
>
> Also, here
>
> -CPPFLAGS = -I.. -I$(srcdir)/../sieve -I$(srcdir)/../imap
> -I$(srcdir)/../lib @COM_ERR_CPPFLAGS@ @CPPFLAGS@ @SASLFLAGS@
> +CPPFLAGS = -I.. -I$(srcdir)/../lib @COM_ERR_CPPFLAGS@ @CPPFLAGS@
> @SASLFLAGS@
>
> it might be a good idea to use paths down from $(top_srcdir) instead of
> up-and-around from $(srcdir), like this
>
> CPPFLAGS = -I$(top_srcdir) -I$(top_srcdir)/lib @COM_ERR_CPPFLAGS@
> @CPPFLAGS@ @SASLFLAGS@
>
> Generally, I think it would be a good thing to have a clear consistent
> and documented policy for how we #include headers.  We don't really have
> that now, and I don't think this commit gets us any closer.

The point is, that some source files are always in $(srcdir), while 
others might be in $(srcdir) or in $(builddir).  Let's say if the files 
are not in $(srcdir), they are generated in $(builddir).  These files 
are prefixed with the path, as in #include "imap/imap_err.h", so that 
the build works (this was my conclusion, when I was trying to build with 
VPATH).

The #include policy, which I have followed was: CPPFLAGS = 
$(top_srcdir), $(top_builddir), $(top_srcdir)/lib and 
$(top_builddir)/lib + external dependencies.

The reason to include both $(top_srcdir) and $(top_builddir) is that 
some headers might be in srcdir, but others might be generated in 
$(builddir).

The reason to include lib/ is that those files get at the end installed 
under /usr/include/cyrus . My logic is, the files which depend on files 
in installed in /usr/include/cyrus, need CPPFLAGS=-I  pointing to the 
place where the headers files are.  This might be $(srcdir)/lib, but it 
might be /usr/cyrus/include .

> commit "Move def. Makefile.in:(PACKAGE/VERSION) to configure.ac:AC_INIT"
>
> Looks good, but here
>
> +AC_INIT([cyrus-imapd], [2.5],
> [http://bugzilla.cyrusimap.org],,[http://www.cyrusimap.org])
>
> it might be nice to have the version on it's own line, as we know that
> the version string will be modified once for every release.

You have to put the version anyway on some line.  Currently it is on the 
AC_INIT line, you can move it to another line, but I do not see reasons 
to choose a line different from AC_INIT.

> commit "lib/Makefile.in: Reorder LIBCYR(M)_(OBJS, HDRS) alphabetically"
>
> Looks good.  While you're doing that, how about splitting them up one
> per line?  We have all sorts of headaches doing git rebases of commits
> which add or remove .c files because of this.

If you check the final Makefile.am, you will see that nearly everything 
is ordered alphabetically.  First are the files from the imap directory, 
then the files from imtest, lib, man, master, netnews, notify... Within 
each directory, the targets are also orderted: first idled, them imap, 
ipurge, lmtpd.  For each target, the sources files are also ordered 
alphabetically.

This makes it very easy to find something, whatever you are looking for.

With the unordered LIBCYR(M)_OBJS it was very hard to find that some 
files are included in both LIBCYRM_OBJS and LIBCYR_OBJS, which does not 
make sense, since all targets linking with libcyrus.a link also with 
libcyrus_min.a .

Okay, we can split them one per line, if you want.  But you can also put 
them all on the same line.  Having them all on the same line, it is 
quite easy to find where the change is.
	
> commit "lib/Makefile.in: Reorder LIBCYR(M)_(OBJS, HDRS) alphabetically"
>
> +       $(srcdir)/brearch.h
>
> s/brearch/bsearch/
Please fix it.

>
> +LIBCYRM_OBJS = assert.o hash.o imapopts.o libconfig.o mappedfile.o
> mpool.o \
> +       retry.o signals.o strhash.o util.o xmalloc.o xstrlcat.o
> xstrlcpy.o \
> +       @IPV6_OBJS@ map_ at WITH_MAP@.o
>
> map_ at WITH_MAP@.o  is out of alphabetical order.

Yes, but I put the @..@ files temporary at the end.  In the final 
Makefile.am the order is alphabetical.

> commit "lib/Makefile.in: expand ACL and AUTH"
>
> +LIBCYR_OBJS = acl.o acl_afs.o auth.o auth_krb.o auth_unix.o auth_krb5.o
> \
>
> auth_krb5.o and auth_pts.o are out of alphabetical order.

Yes.  In the final Makefile.am they are reordered correctly.

>
> commit "lib/prot.h: Add #include "config.h", remove from LIBCYR_HRDS"
>
> I'm a little confused by your approach here.  You say
>
>>     Since lib/prot.h #include "config.h" it was removed from LIBCYR_HDRS,
>
> but you also remove the #include config.h from lib/prot.h?  Given that
> lib/prot.h will not compile correctly unless HAVE_SSL and HAVE_ZLIB are
> correctly defined, this seems unwise.

I add #include config.h to lib/prot.h and remove lib/prot.h from 
LIBCYR_HDRS .

As lib/prot.h uses HAVE_SSL, it shall include config.h
As lib/prot.h includes config.h, it shall not be installed.

> commit "remove inserting of files in both libcyrus and libcyrus_min"
>
> I find this change confusing but I can't see anything wrong with it.
> However as long as it's a static library I think you could definitely go
> one step further and eliminate libcyrus_min entirely.

What is confusing?

> commit "lib/Makefile.am: move strarray from libcyrus.a to
> libcyrus_min.a"
>
> Looks good, but here
>
> +master: master.o masterconf.o cyrusMasterMIB.o
>
> the master program needs to depend on libcyrus.a in some way.

That is not true.  The master program is the only program, that depends 
only on libcyrus_min.a, but not on libcyrus.a . This was so before my 
changes.

> commit "lib/cyrusdb_twoskip.c: undef VERSION from config.h"
>
> Isn't it PACKAGE_VERSION now?

It is PACKAGE_VERSION, but config.h for some reason still defines VERSION.
>
> Also, it might be a good idea to rename the VERSION define in twoskip.c
> to something like TWOSKIP_VERSION
It might be a good idea.

Със здраве
   Дилян