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_@WITH_MAP@.o map_@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_@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.
