Libtool and Support for Shared Libraries

Greg Banks gnb at fastmail.fm
Thu May 10 23:52:31 EDT 2012



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.


More information about the Cyrus-devel mailing list