Automake Support for cyrus-imapd 2.5

[Greg Banks]

G'day,

On Sun, Apr 15, 2012, at 01:31 AM, Дилян Палаузов wrote:
> Hello,
> 
> at git://demo.aegee.org:143/cyrus-imapd.git , branch dpa/automake I have 
> patched cyrus-imapd/master to support Automake .
> 

Some reviews follow.

commit "rename configure.in to configure.ac"

Looks good, but there's little point updating the $Id$ lines in
configure.ac, in fact they should be removed.

commit "Update requirements for Autoconf from version 2.54 to 2.68"

Looks good.  I guess I'm going to be upgrading soon, I'm on 2.67 :(

commit "Add AS_HELP_STRING to AC_ARG_WITH and AC_ARG_ENABLE"

Looks good.

+        [AS_HELP_STRING([--enable-replication], [enable replication
support (experimental)])],

Hmm, I don't think this still qualifies as "experimental"

commit ".gitignore: remove sieve/, add perl/sieve/managesieve/"

Looks good.

commit "*/Makefile.in: .c.o.: removed newline before $< for consistency"

Looks good,

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.

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.

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.

commit "*/Makefile.in replace IMAP_COM_ERR_LIBS with COM_ERR_LIBS"

Looks good.

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.

commit "configure.ac: replace AC_CONFIG_HEADER with AC_CONFIG_HEADERS"

Looks good.

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.

commit "lib/Makefile.in: Reorder LIBCYR(M)_(OBJS, HDRS) alphabetically"

+       $(srcdir)/brearch.h

s/brearch/bsearch/

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

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.

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.

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.

Oh, I see you fixed the ordering issue with map_ at WITH_LOCK@.o.

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.  For
example, in imapd/Makefile.in we do

DEPLIBS = ../lib/libcyrus.a ../lib/libcyrus_min.a @DEPLIBS@

imapd: $(IMAPDOBJS) mutex_fake.o libimap.a $(DEPLIBS) $(SERVICE)
        $(CC) $(LDFLAGS) -o imapd ...

commit "lib/libcyr_cfg.h: remove #include <config.h>"

Looks good.

commit "lib/cyrusdb_sql.c : typo: rename struct db to struct dbengine"

Looks good

commit "lib/cyrusdb_twoskip.c: undef VERSION from config.h"

Isn't it PACKAGE_VERSION now?

Also, it might be a good idea to rename the VERSION define in twoskip.c
to something like TWOSKIP_VERSION

commit "master/cyrusMasterMIB.c: resolve double #include <config.h>"

Looks good

commit "lib/libconfig.c: imapopts.h double included (via libconfig.h)"

Looks good

commit "lib/Makefile.in: consolidate two install: loops"

Looks good

commit "com_err/et/Makefile.in: remove LOCALINCLUDE as it is not needed"

Looks good

Have to go now, more later.

-- 
Greg.