Hello,
Third and final tranche of review comments.
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@
It is now fixed.
imap/imap_err.h imap/imap_err.c: imap/imap_err.et
cd imap && $(COMPILE_ET) ../$(top_srcdir)/imap/imap_err.et
and the same for sieve/sieve_err.et, imap/mupdate_err.et and
imap/nntp_err.et
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.
Greg, I admit that you have more understanding about Perl than me. I
kindly ask you to fix the Perl stuff, including the
https://bugzilla.cyrusimap.org/show_bug.cgi?id=3678 issue.
Why are those EXTRACFLAGS necessary at all? Can't AM_CFLAGS be used
instead?
+#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.
Yes, but in order for "make dist" to put the all files in the tarball,
these files need to mentioned somehow. Anyway, once the
automake-building works, I suggest to have a separate discussion on how
to implement "make snapshot"/"make dist"/generation of xversion.h, etc.
Let's stick now to the dependancy and compilation issues.
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/
I renamed all sieve/test/ to sieve/tests/ .
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.
notifytest is build by "make check".
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.
I deleted the syslog/ files and the SYSLOG Automake conditional.
Please check if the syslog checks in configure.ac can be deleted, too.
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/
Fixed.
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.
I deleted installsieve/* .
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.
I deleted it.
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
The dependency on cyradm.sh is fixed now.
Also, why the rm??
master:doc/Makefile.in contains:
dist:
mkdir -p man
for man in ../man/*.[1-9]; \
do \
echo "=== $$man ==="; \
groff -man -Thtml $$man > man/`basename $$man`.html; \
done
pod2html ../perl/imap/cyradm.sh > man/cyradm.1.html
pod2man ../perl/sieve/scripts/sieveshell.pl > ../man/sieveshell.1
pod2html ../perl/sieve/scripts/sieveshell.pl >
man/sieveshell.1.html
rm -f groff-html-*.png pod2htm*
fig2dev -L png murder.fig murder.png
fig2dev -L png netnews.fig netnews.png
(cd text; make)
And the rm comes from there.
+ @$(MKDIR_P) doc
+ @$(MKDIR_P) doc/text
The first of these lines is redundant.
Fixed.
commit "Makefile.am: merge imap/Makefile.in"
+ $(LN_S) -f lmtpd lmtpprodyx
s/lmtpprodyx/lmtpproxy/
Fixed.
+ cd imap&& $(top_builddir)/../$(COMPILE_ET)
../$(top_srcdir)/imap/imap_err.et
This is wrong in so many ways.
See above.
+AM_CONDITIONAL([WITH_OPENSSL], [test "$with_openssl" != "no"])
For consistency, the name of the conditional should be OPENSSL
Fixed.
Със здраве
Дилян
