Automake Support for cyrus-imapd 2.5

Greg Banks gnb at fastmail.fm
Tue Apr 17 03:14:34 EDT 2012


G'day,

Third and final tranche of review comments.

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 .

commit "Makefile.am: merge com_err/et/Makefile.in"

I think this has already been covered, but using top_builddir in the
Makefile.in rather than in configure.ac will break when the library is
actually found in /usr/lib

+LIBS = ${top_builddir}/@COM_ERR_LIBS@

commit "Makefile.am: replace mkdir with"

Looks good

commit "Makefile.am: merge lib/Makefile.in"

+AM_CFLAGS = @PERL_CCCDLFLAGS@ $(EXTRACFLAGS)

This is wrong.  PERL_CCCDLFLAGS should only be used for the code which
is built into the perl extension shared library.  In particular, it
contains -fPIC which we do not want anywhere else if we can avoid it.

+#this is from lib/test/Makefile.in, however testglob.c does not exist
in
+#/lib/test, instead testglob2.c is there.
+#lib_test_testglob_LIRBARIES = lib/libcyrus.a lib/libcyrus_min.a
-ldb-4.0

Everything in lib/test/ will be either moved to cunit/ or removed,
eventually, and is currently not built. You probably shouldn't bother
with it.

commit "Makefile.am: merge sieve/Makefile.in"

+       sieve/test/testExtension ....

Not only is this line far too long, but the directory is sieve/tests/
not sieve/test/

commit "Makefile.am: merge master/Makefile.in"

Looks good.

commit "Makefile.am: merge perl/sieve/lib/Makefile.in"

Looks good

commit "Makefile.am: merge netnews/Makefile.in"

Looks good

commit "Makefile.am: merge depot/Makefile.in"

Looks good.

commit "Makefile.am: merge notifyd/Makefile.in"

Looks good, although I note that notifytest is not built at all
currently but is built in the new Makefile.

commit "Makefile.am: merge timsieved/Makefile.in"

+       timsieved/TODO

Given that a) this file has not been updated since 2003, and b) we have
Bugzilla, I think we can delete this.

commit "Makefile.am: merge syslog/Makefile.in"

I think it's time we deleted the entire syslog/ directory.  I don't
believe there's any platform on which it will be built, nor be useful.

commit "Makefile.am: merge ptclient/Makefile.in"

+#ptclient_ptextract_SOURCES = imap/cli_fatal.o imap/mutex_fake.o
ptclient/ptextrac.c

s/ptextrac/ptextract/

+ptclient_ptloader_SOURCES = ptclient/ptloader.c ptclient/ptloader.h
ptclient/afskrb.oc ptclient/ldap.c imap/mutex_fake.o
master/service-thread.c

s/afskrb.oc/afskrb.c/

commit "Makefile.am: merge installsieve/Makefile.in"

>    It is unclear why this directory exist, since there is no place, that
>    installs installsieve.

Another relic, let's remove it too.

commit "Remove makedepend"

Looks good.

commit "Include tools/* and snmp/* in "make dist""

Looks good

commit "Include the files from / in "make dist""

Looks good

commit "Include SIEVE/scripts/sieveshell.pl in "make dist""

>     It is unclear why this script is neede (and installsieve/installsieve)
>    in parallel to perl/sieve/scripts/(sieveshell|installsieve).pl

I don't see any good reason for this to continue existing.

commit "Include the files from contrib/ in "make dist""

Looks good

commit "Makefile.am: merge imtest/Makefile.in"

Looks good

commit "Makefile.am: merge doc/(Makefile.dist, text/Makefile)"

+man/cyradm.1.html:
+       @$(MKDIR_P) man
+       pod2html $(top_srcdir)/perl/imap/cyradm.sh > man/cyradm.1.html
+       rm man/pod2htm*


man/cyradm.1.html should depend on $(top_srcdir)/perl/imap/cyradm.sh

Also, why the rm??

+       @$(MKDIR_P) doc
+       @$(MKDIR_P) doc/text

The first of these lines is redundant.

commit "Makefile.am: merge perl/(sieve,.)/Makefile.in"

No comment, this was reworked later.

commit "Makefile.am: order alphabetically build of service programs"

Looks good

commit "Makefile.am: merge imap/Makefile.in"

+         $(LN_S) -f lmtpd lmtpprodyx

s/lmtpprodyx/lmtpproxy/

+       cd imap && $(top_builddir)/../$(COMPILE_ET)
../$(top_srcdir)/imap/imap_err.et

This is wrong in so many ways.

+AM_CONDITIONAL([WITH_OPENSSL], [test "$with_openssl" != "no"])

For consistency, the name of the conditional should be OPENSSL


-- 
Greg.


More information about the Cyrus-devel mailing list