On Thu, May 10, 2012, at 01:04 AM, Дилян Палаузов wrote: > Hello, > > I wanted to upload a new branch for testing libtool/shared libraries > functionality, but something went wrong and the 13 commits were uploaded > on top of 'master' instead of creating a separate branch. I am sorry > for the surprises this might cause. Anyway...
Review follows. commit "Makefile.am: do not link notifyd/notifytest with libimap" Looks good. commit "Makefile.am: imap_libimap_a_SOURCES += index/index.c" Looks good. And yes, Cyrus has a bad case of circular link dependencies which we should address properly at some time in the future. There's also the truly horrible state of fatal() and a number of things around the config file code. commit "configure.ac, Makefile.am: Add libtool support" Looks good. I'm surprised you need the rule in the Makefile.am but it seems harmless. So, with no arguments to LT_INIT() are we getting both shared and static versions of the libraries by default? commit "libcyrus_min:libconfig:config_read: add parameter config_need_data" Ok, so config_need_data() needs fixing. As far as I can see all it does is enable an extra semantic check of the config file in config_read(). It seems silly for some Cyrus executables to be doing that check and others not to; either the file is correct or it's not. The check really isn't that expensive either, not with today's CPUs. I can't see any good reason for it; the git history just shows that it arrived in the 2.2 merge back in CVS days. I think you should just remove config_need_data entirely. commit "convert lib/libcyrus_min.a to a libtool archive" Looks good. commit "convert lib/libcyrus.a to a libtool archive" Looks good. commit "convert com_err/et/libcom_err.a to a libtool archive" You should also fix the 2nd argument to db_panic() in lib/cyrusdb_berkeley.c. Also, I notice that libcom_err.la is 'noinst'. How do you expect that cyrus executable will be able to find this code at runtime on a platform which has shared libraries but no system com_err library? commit "convert sieve/libsieve.a to a libtool archive" Traditionally in automake $pkglibdir is $prefix/lib/$package/ which would be /usr/lib/cyrus/. I don't think it's a good idea to muck around with this convention. If you must install things in that directory, create a new variable like foobardir and use foobar_LTLIBRARIES in Makefile.am. Having said that, I don't see any reason to install these libraries in a different place to where libcyrus.la is installed, i.e. $libdir. It's less code and more sensible and consistent. commit "convert imap/libimap.a to a libtool archive" Same comments as previous commit. I notice you're updating doc/internal/unit-tests.html. Quite a lot of that file has been obsoleted by the automake conversion, and needs to be updated. commit "autoconf: replace all AC_ERROR with AC_MSG_ERROR" Looks good. commit "Makefile.am: remove LD_(BASIC,SERVER,SIEVE,UTILITY)_FLAGS" Looks good. commit "/.gitignore: update to ingore .la, .lo and .libs/ files" Looks good. commit "Makefile.PL.in: MYEXTLIB: prefix libcyrus(_min).a with .libs/" Yuck. Still I don't see a better way. So far so good, but there's still a number of unresolved issues. - The Makefile.am still has AM_CFLAGS = @PERL_CCCDLFLAGS@, which will result in -fPIC being used for compiling both the shared and the static libraries. - Cyrus has multiple definitions of fatal(), with three different sets of parameters. Apart from the basic sillyness of this arrangement, it's just *asking* for unnecessary drama once you add shared libraries into the mix. At the very least we should ensure they all have exactly the same parameters. Better would be to push the definition into libcyrus_min() and provide an atfatal() which behaves like atexit(), i.e. allows code to register cleanup handlers to be called from fatal(). Then all the calls to foo_done() all over the place could use that technique. We could also add __attribute__((noreturn)) and provide a printf-like variant (like fatalf() in masterconf.c) too. - Cassandane runs Cyrus executables from an installation staging tree, i.e. the directory that you would use a DESTDIR when doing a make install. With shared libraries, Cassandane will need to be adjusted to set up $LD_LIBRARY_PATH appropriately. Search for the code which sets up $PERL5LIB. - We need to have a README for downstream package maintainers that describes what changed about the Cyrus build. > I have updated cyrus-imapd to use libtool and build shared (and static) > libraries, and the binaries to use the shared libraries. I hope you > like the results. > > Some issues: > -- where shall the libraries be installed? Currently libcyrus and > libcyrus_min go under $libdir (e.g. /usr/lib), as this was before with > using libtool > > -- libimap and libsieve are installed under pkglibdir = > $cyrus_prefix/lib, e.g. /usr/cyrus/lib . If the libraries are moved to > $libdir, then they shall probably be renamed to libcyrus_imap and > libcyrus_sieve to avoid conflicts with other libraries with the same > (common) name All the libraries should go in the same directory and have consistent naming. Either $exec_prefix/lib/cyrus/libfoo.so or $exec_prefix/lib/libcyrus-foo.so is fine by me, but the latter is probably marginally easier to build as it's in the default runtime linker paths when $prefix = /usr, and it's the traditional location for shared libraries which are linked to executables rather than plugins which are dlopen()ed by code. > -- I have problems with MakeMaker, it would be very, very nice if > somebody considers to change the build system for the perl files, e.g. > switch to something more modern and developed like Module::Build (to > what I read MakeMaker is not developed any more). In any case, the perl > modules are linked with the static libraries of libcyrus_min and > libcyrus, so currently for the binaries the shared libraries are used, > for the perl-stuff the static libraries are used and cyrus-imapd > compiles by default both shared and static libraries. Agreed, MakeMaker sucks. I'm happy to do the work to update to anything else, but I don't know enough about Perl to choose a good system to update to. > > -- in the future library versioning could be used, e.g. to associate > library-version to a cyrus-imapd release or so, at the moment my idea > was just to make shared libraries and these are un-versioned. We have no meaningful ABI guarantee between versions of Cyrus, so it's actually a bad idea to ship a shared library with no versioning information in it at all. I think the best way forward is probably to use libtool's -release option with $VERSION from autoconf. -- Greg.
