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.
