Incomplete linking of managesieve Perl module

[jasontibbitts]

In the current Fedora development, perl 5.26 has just landed and this has somehow broken the managesieve perl module. I wouldn't have noticed except that a cassandane test fails.

What ends up happening is that you can't load the managesieve module because the Perl-internal managesieve.so library is missing symbols:

There was 1 failure:
1) test_badscript_timsieved(Cassandane::Cyrus::Sieve)
 Can't load '/builddir/build/BUILDROOT/cyrus-imapd-3.0.1-5.fc27.x86_64/usr/lib64/perl5/vendor_perl/auto/Cyrus/SIEVE/managesieve/managesieve.so' for module Cyrus::SIEVE::managesieve: /builddir/build/BUILDROOT/cyrus-imapd-3.0.1-5.fc27.x86_64/usr/lib64/perl5/vendor_perl/auto/Cyrus/SIEVE/managesieve/managesieve.so: undefined symbol: sqlite3_column_text at /usr/lib64/perl5/DynaLoader.pm line 193.
� at /builddir/build/BUILDROOT/cyrus-imapd-3.0.1-5.fc27.x86_64/usr/bin/installsieve line 64.
Compilation failed in require at /builddir/build/BUILDROOT/cyrus-imapd-3.0.1-5.fc27.x86_64/usr/bin/installsieve line 64.
BEGIN failed--compilation aborted at /builddir/build/BUILDROOT/cyrus-imapd-3.0.1-5.fc27.x86_64/usr/bin/installsieve line 64.

What's happened is that Fedora compiles Perl differently now, and the compiler flags that Perl will use when building compiled modules have changed. There's no implicit linking now, so if you link against a library that will call into another library, you need to link against both.

I'm patching around this by just adding -lsqlite3 -lpq to the LIBS line of Makefile.PL, but that's because I can't comprehend autoconf. If I was building with mysql support, I expect I'd need it there as well. I suspect there's some set of @LIB_WHATEVER@ macros which should be used instead in Makefile.PL.in, but I wasn't successful in finding them.

[elliefm]

Does this help? You'll need to autoreconf and rebuild from scratch after applying:

diff --git a/configure.ac b/configure.ac
index c23032a..224a02c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1949,6 +1949,7 @@ if test "x$use_sqlite" = xyes; then
     LIBS="${LIBS} ${SQLITE_LIBADD}"
     CPPFLAGS="${SQLITE_INCADD} ${CPPFLAGS}"
     AC_DEFINE(HAVE_SQLITE,[],[Build in SQLite support?])
+    AC_SUBST(SQLITE_LIBADD)
   else
     AC_MSG_ERROR([SQLite required but not found])
   fi
diff --git a/perl/sieve/managesieve/Makefile.PL.in b/perl/sieve/managesieve/Makefile.PL.in
index 2bb715d..a0c591e 100644
--- a/perl/sieve/managesieve/Makefile.PL.in
+++ b/perl/sieve/managesieve/Makefile.PL.in
@@ -69,7 +69,7 @@ WriteMakefile(
     'ABSTRACT'  => 'Cyrus Sieve management interface',
     'VERSION_FROM' => "@top_srcdir@/perl/sieve/managesieve/managesieve.pm", # finds $VERSION
     'MYEXTLIB'  => '../lib/.libs/libisieve.a @top_builddir@/perl/.libs/libcyrus.a @top_builddir@/perl/.li
-    'LIBS'     => ["$LIB_SASL @SSL_LIBS@ @LIB_RT@ @LIB_UUID@ @ZLIB@"],
+    'LIBS'     => ["$LIB_SASL @SSL_LIBS@ @LIB_RT@ @LIB_UUID@ @ZLIB@ @SQLITE_LIBADD@"],
     'CCFLAGS'  => '@GCOV_CFLAGS@',
     'DEFINE'   => '-DPERL_POLLUTE',     # e.g., '-DHAVE_SOMETHING' 
     'INC'      => "-I@top_srcdir@/lib -I@top_srcdir@/perl/sieve -I@top_srcdir@/perl/sieve/lib @SASLFLAGS@

(This patch is slightly hand-doctored because I first wrote it against master instead of 3.0, so if it doesn't apply cleanly you might need to fiddle with it a little, but hopefully it applies cleanly!)

[elliefm]

I'd also be curious to see if/how it fails if you try to build with mysql support. The various sql library detection bits are pretty nasty IMO, I have vague plans to rewrite that stuff at some point.

[jasontibbitts]

Ah, so you have to add an AC_SUBST. I guess that makes sense. I'm going to assume that autoconf will get the proper libraries from pkgconfig if that's available.

You'll need one for the postgres libs as well; if you fix just the sqlite issue then you'll find missing symbols that are in libpq. For me that's all; I sadly just can't build with mysql at the moment because of the openssl version mismatch thing but I hope that will be improved in the near future.

[jasontibbitts]

I just wanted to note that this is still an issue for me in 3.0.7; I have to add some extra bits to LIBS or I get errors from loading managesieve.so. Right now I'm just hardcoding -lsqlite -lpq -lmariadb so I can avoid thinking about autoconf.. All three are needed or I get errors when loading managesieve.o.

BTW, the error I get when I add just the sqlite and pq libs but not mariadb is this:

Can't load '/builddir/build/BUILDROOT/cyrus-imapd-3.0.7-1.fc29.x86_64/usr/lib64/perl5/vendor_perl/auto/Cyrus/SIEVE/managesieve/managesieve.so' for module Cyrus::SIEVE::managesieve: /builddir/build/BUILDROOT/cyrus-imapd-3.0.7-1.fc29.x86_64/usr/lib64/perl5/vendor_perl/auto/Cyrus/SIEVE/managesieve/managesieve.so: undefined symbol: mysql_field_count at /usr/lib64/perl5/DynaLoader.pm line 193.

[elliefm]

Here's an updated version of my previous patch that also does the same thing for MySQL. I don't experience the issue myself on Debian, so I have no idea if it works or not! :)

I had a look at doing the same sort of thing for Postgres, but our configure.ac handles the Postgres dependencies quite differently from the way it handles SQLite and MySQL (it's nasty, but Postgres might actually just work, by accident). But I can't verify.

diff --git a/configure.ac b/configure.ac
index b33728020..1ef38c545 100644
--- a/configure.ac
+++ b/configure.ac
@@ -273,6 +273,7 @@ if test "$with_mysql" != "no"; then
     LIBS="${LIBS} ${MYSQL_LIBADD}"
     CPPFLAGS="${MYSQL_INCADD} ${CPPFLAGS}"
     AC_DEFINE(HAVE_MYSQL,[],[Build in MySQL support?])
+    AC_SUBST(MYSQL_LIBADD)
 fi
 AC_MSG_RESULT($with_mysql)
 
@@ -1957,6 +1958,7 @@ if test "x$use_sqlite" = xyes; then
     LIBS="${LIBS} ${SQLITE_LIBADD}"
     CPPFLAGS="${SQLITE_INCADD} ${CPPFLAGS}"
     AC_DEFINE(HAVE_SQLITE,[],[Build in SQLite support?])
+    AC_SUBST(SQLITE_LIBADD)
   else
     AC_MSG_ERROR([SQLite required but not found])
   fi
diff --git a/perl/sieve/managesieve/Makefile.PL.in b/perl/sieve/managesieve/Makefile.PL.in
index 2bb715d3f..891807da8 100644
--- a/perl/sieve/managesieve/Makefile.PL.in
+++ b/perl/sieve/managesieve/Makefile.PL.in
@@ -69,7 +69,7 @@ WriteMakefile(
     'ABSTRACT'  => 'Cyrus Sieve management interface',
     'VERSION_FROM' => "@top_srcdir@/perl/sieve/managesieve/managesieve.pm", # finds $VERS
     'MYEXTLIB'  => '../lib/.libs/libisieve.a @top_builddir@/perl/.libs/libcyrus.a @top_bu
-    'LIBS'     => ["$LIB_SASL @SSL_LIBS@ @LIB_UUID@ @ZLIB@"],
+    'LIBS'     => ["$LIB_SASL @SSL_LIBS@ @LIB_UUID@ @ZLIB@ @SQLITE_LIBADD@ @MYSQL_LIBADD@
     'CCFLAGS'  => '@GCOV_CFLAGS@',
     'DEFINE'   => '-DPERL_POLLUTE',     # e.g., '-DHAVE_SOMETHING' 
     'INC'      => "-I@top_srcdir@/lib -I@top_srcdir@/perl/sieve -I@top_srcdir@/perl/sieve

[jasontibbitts]

It looks like all of the lnes in that patch are truncated. I patched in the missing "], to the LIBS line, but I think something else is missing. I get:

Can't load '/builddir/build/BUILDROOT/cyrus-imapd-3.0.7-2.fc29.x86_64/usr/lib64/perl5/vendor_perl/auto/Cyrus/SIEVE/managesieve/managesieve.so' for module Cyrus::SIEVE::managesieve: /builddir/build/BUILDROOT/cyrus-imapd-3.0.7-2.fc29.x86_64/usr/lib64/perl5/vendor_perl/auto/Cyrus/SIEVE/managesieve/managesieve.so: undefined symbol: PQresultStatus at /usr/lib64/perl5/DynaLoader.pm line 193.

I thought maybe I needed `@PGSQL_LIBADD@ there (since that's also defined), so I ended up with the following, which works.

diff --git a/configure.ac b/configure.ac
index b337280..f9f0960 100644
--- a/configure.ac
+++ b/configure.ac
@@ -273,6 +273,8 @@ if test "$with_mysql" != "no"; then
     LIBS="${LIBS} ${MYSQL_LIBADD}"
     CPPFLAGS="${MYSQL_INCADD} ${CPPFLAGS}"
     AC_DEFINE(HAVE_MYSQL,[],[Build in MySQL support?])
+    AC_SUBST(MYSQL_LIBADD)
+
 fi
 AC_MSG_RESULT($with_mysql)

@@ -318,6 +320,7 @@ if test "$use_pgsql" != "no"; then
   CPPFLAGS="${PGSQL_INCADD} ${CPPFLAGS}"

   AC_DEFINE(HAVE_PGSQL,[],[Build in PgSQL support?])
+  AC_SUBST(PGSQL_LIBADD)
 fi

 dnl End PgSQL Detection
@@ -1957,6 +1960,7 @@ if test "x$use_sqlite" = xyes; then
     LIBS="${LIBS} ${SQLITE_LIBADD}"
     CPPFLAGS="${SQLITE_INCADD} ${CPPFLAGS}"
     AC_DEFINE(HAVE_SQLITE,[],[Build in SQLite support?])
+    AC_SUBST(SQLITE_LIBADD)
   else
     AC_MSG_ERROR([SQLite required but not found])
   fi
diff --git a/perl/sieve/managesieve/Makefile.PL.in b/perl/sieve/managesieve/Makefile.PL.in
index 2bb715d..2a3415a 100644
--- a/perl/sieve/managesieve/Makefile.PL.in
+++ b/perl/sieve/managesieve/Makefile.PL.in
@@ -69,7 +69,7 @@ WriteMakefile(
     'ABSTRACT'  => 'Cyrus Sieve management interface',
     'VERSION_FROM' => "@top_srcdir@/perl/sieve/managesieve/managesieve.pm", # finds $VERSION
     'MYEXTLIB'  => '../lib/.libs/libisieve.a @top_builddir@/perl/.libs/libcyrus.a @top_builddir@/perl/.libs/libcyrus_min.a',
-    'LIBS'     => ["$LIB_SASL @SSL_LIBS@ @LIB_UUID@ @ZLIB@"],
+    'LIBS'     => ["$LIB_SASL @SSL_LIBS@ @LIB_UUID@ @ZLIB@ @SQLITE_LIBADD@ @MYSQL_LIBADD@ @PGSQL_LIBADD@"],
     'CCFLAGS'  => '@GCOV_CFLAGS@',
     'DEFINE'   => '-DPERL_POLLUTE',     # e.g., '-DHAVE_SOMETHING'
     'INC'      => "-I@top_srcdir@/lib -I@top_srcdir@/perl/sieve -I@top_srcdir@/perl/sieve/lib @SASLFLAGS@ @SSL_CPPFLAGS@",

My understanding was that Debian was also switching to requiring explicit DSO links. See https://wiki.debian.org/ToolChain/DSOLinking (where they reference the Fedora change) but Debian moves glacially so I have no idea if they have fully implemented all of that. In any case, I would expect to see an identical problem on most modern Linux distributions, but you have to actually run the Perl managesieve extension (or run Cassandane) to actually see it. I suspect that most people don't do that, and so they wouldn't have noticed.

[elliefm]

Oh sorry, copy and paste failure!

diff --git a/configure.ac b/configure.ac
index b33728020..1ef38c545 100644
--- a/configure.ac
+++ b/configure.ac
@@ -273,6 +273,7 @@ if test "$with_mysql" != "no"; then
     LIBS="${LIBS} ${MYSQL_LIBADD}"
     CPPFLAGS="${MYSQL_INCADD} ${CPPFLAGS}"
     AC_DEFINE(HAVE_MYSQL,[],[Build in MySQL support?])
+    AC_SUBST(MYSQL_LIBADD)
 fi
 AC_MSG_RESULT($with_mysql)
 
@@ -1957,6 +1958,7 @@ if test "x$use_sqlite" = xyes; then
     LIBS="${LIBS} ${SQLITE_LIBADD}"
     CPPFLAGS="${SQLITE_INCADD} ${CPPFLAGS}"
     AC_DEFINE(HAVE_SQLITE,[],[Build in SQLite support?])
+    AC_SUBST(SQLITE_LIBADD)
   else
     AC_MSG_ERROR([SQLite required but not found])
   fi
diff --git a/perl/sieve/managesieve/Makefile.PL.in b/perl/sieve/managesieve/Makefile.PL.in
index 2bb715d3f..891807da8 100644
--- a/perl/sieve/managesieve/Makefile.PL.in
+++ b/perl/sieve/managesieve/Makefile.PL.in
@@ -69,7 +69,7 @@ WriteMakefile(
     'ABSTRACT'  => 'Cyrus Sieve management interface',
     'VERSION_FROM' => "@top_srcdir@/perl/sieve/managesieve/managesieve.pm", # finds $VERSION
     'MYEXTLIB'  => '../lib/.libs/libisieve.a @top_builddir@/perl/.libs/libcyrus.a @top_builddir@/perl/.libs/libcyrus_min.a',
-    'LIBS'	=> ["$LIB_SASL @SSL_LIBS@ @LIB_UUID@ @ZLIB@"],
+    'LIBS'	=> ["$LIB_SASL @SSL_LIBS@ @LIB_UUID@ @ZLIB@ @SQLITE_LIBADD@ @MYSQL_LIBADD@"],
     'CCFLAGS'	=> '@GCOV_CFLAGS@',
     'DEFINE'	=> '-DPERL_POLLUTE',     # e.g., '-DHAVE_SOMETHING' 
     'INC'	=> "-I@top_srcdir@/lib -I@top_srcdir@/perl/sieve -I@top_srcdir@/perl/sieve/lib @SASLFLAGS@ @SSL_CPPFLAGS@",

[elliefm]

I missed the PGSQL_LIBADD in my version (???) but I'll include it when I eventually merge this.

I run Debian stable these days (I got fed up with testing always needing lots of duct tape/handholding, and stable has everything I need since Jessie) so that's probably why I haven't seen the linkage changes yet.

But it's definitely good to get this fixed even, if Debian doesn't need it yet.

[elliefm]

Oh I see, I skipped PGSQL_LIBADD because it's already been added to the global LIBS (this is the nasty bit). But I think explicitly adding it for managesieve is the right thing to do anyway, then it won't get forgotten if/when someone (probably me, heh) nicens up the PGSQL configuration someday.