On (15/10/13 15:42), Benjamin Franzke wrote: >Hi Lukas, > > >2013/10/15 Lukas Slebodnik <[email protected]> > >> On (15/10/13 11:47), Benjamin Franzke wrote: >> >Hi, >> > >> >These two patches add missing CFLAGS/LIBS to Makefile.am: >> > >> >[PATCH 1/2] BUILD: Link libsss_ad.so to sasl libs >> >[PATCH 2/2] BUILD: Use OPENLDAP_CFLAGS instead of LDAP_CFLAGS >> ACK to 2nd patch >> > >> >This underlinking was noticed in make check (dlopen-test). >> > >> >Note: >> >It failed for me since my openldap build had no sasl support, >> >which would otherwise have pulled in libsasl2.so. >> >Of course, that support should be in place, but the linking should still >> be >> >fixed. >> > >> >BTW: It would propably be nice to have a configure check whether >> >openldap has sasl support, but it seems that would need a check if >> >ldap_sasl_interactive_bind returns LDAP_NOT_SUPPORTED. >> > >> >Regards, Ben >> >> I have a little problem with the first patch. >> >> >> From d26a15de098df2b42582cda590e184c69f48bb7f Mon Sep 17 00:00:00 2001 >> From: Benjamin Franzke <[email protected]> >> Date: Tue, 15 Oct 2013 10:27:36 +0200 >> Subject: [PATCH 1/2] BUILD: Link libsss_ad.so to sasl libs >> >> This is for the sasl_client_init symbol. >> Introducted in commit fb945a2c. >> --- >> Makefile.am | 2 ++ >> configure.ac | 2 +- >> 2 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/Makefile.am b/Makefile.am >> index >> ff1e71e72d90a6eff658a40e2ca24c1929b31aa5..8c919d40481720e4661a42a188cea2fd179282d4 >> 100644 >> --- a/Makefile.am >> +++ b/Makefile.am >> @@ -1713,12 +1713,14 @@ libsss_ad_la_CFLAGS = \ >> $(AM_CFLAGS) \ >> $(SYSTEMD_LOGIN_CFLAGS) \ >> $(LDAP_CFLAGS) \ >> + $(SASL_CFLAGS) \ >> $(DHASH_CFLAGS) \ >> $(KRB5_CFLAGS) \ >> $(NDR_NBT_CFLAGS) >> libsss_ad_la_LIBADD = \ >> $(SYSTEMD_LOGIN_LIBS) \ >> $(OPENLDAP_LIBS) \ >> + $(SASL_LIBS) \ >> $(DHASH_LIBS) \ >> $(KEYUTILS_LIBS) \ >> $(KRB5_LIBS) \ >> diff --git a/configure.ac b/configure.ac >> index >> d28d55f3b0eb35cc4ecc02ae9b1fafbcb9588dcf..7e3819a86ad94176e95f1ded07b88d25399888de >> 100644 >> --- a/configure.ac >> +++ b/configure.ac >> @@ -260,7 +260,7 @@ fi >> >> AM_CHECK_INOTIFY >> >> -AC_CHECK_HEADERS([sasl/sasl.h],,AC_MSG_ERROR([Could not find SASL >> headers])) >> +PKG_CHECK_MODULES([SASL], [libsasl2], [], [AC_MSG_ERROR([Could not find >> SASL library])]) >> >> pkg-config file needn't be available one some distribution (platforms). >> For example popt-devel on fedora 19 doesn't have pkg-config file. >> So it is better to fallback to AC_CHECK_HEADER. >> >> 1. And it does not mean that openldap was build with sasl support if >> libsasl2 >> is installed on the machine. Detection should more complex. >> > >Yes, thats why the dea to detect whether ldap has sasl support, which is >not easy, as Stephen already said. > >> >> 2. According to man ldap_sasl_interactive_bind_s, we should only link with >> library "OpenLDAP LDAP (libldap, -lldap)" and we does not directly use >> any sasl function. What kind of distribution do you use? >> > >As written in the commit message, since commit fb945a2c sssd uses sasl >unconditionally: >git grep sasl_client_init >src/providers/ad/ad_init.c: (void)sasl_client_init(ad_sasl_callbacks); > >So if sssd should be build without sasl that'd need to be fixed. > >> >> 3. IIRC, sssd can be build with ldap without sasl support, but ipa provider >> will not work. And it should be similar with ad provider. I would prefer >> to use conditional build instead of failing if ldap does not support >> sasl. >> Because with your patch, sssd could not be build without sasl library >> and >> someone can use sssd only with ldap provider (and he doesn't care about >> sasl) >> > >How did it not fail before, when sasl was not installed? I mean there was >error too.. > I realized sssd cannot be built without header file sasl.h, because we include header file in sdap_async_connection.c src/providers/ldap/sdap_async_connection.c:#include <sasl/sasl.h> But we needn't link ldap_common with sasl library, because sasl functions are not used in module sdap_async_connection.c
The last problem is in your patch, that you replaced macro AC_CHECK_HEADERS with pkg-config detection. Sasl pkg-config file needn't be available on some distributions. For example, the latest ubuntu does not have sasl.pc in the package libsasl2-dev. http://packages.ubuntu.com/saucy/amd64/libsasl2-dev/filelist LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
