On (07/11/13 20:43), Lukas Slebodnik wrote: >On (31/10/13 11:12), Lukas Slebodnik wrote: >>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 > >I am attaching patches with more robust detection of SASL, >because pkg-config file is not available in older versions of >cysrus sasl <= 2.1.25. (Fedora<=18, RHEL<=6, all versions of ubuntu) > >Configure failed with Benjamin's version on RHEL6. > >LS
>From 8b4746d7e2d5c53cbe5d29f1ce984a16aefb5a54 Mon Sep 17 00:00:00 2001 >From: Lukas Slebodnik <[email protected]> >Date: Tue, 15 Oct 2013 10:27:36 +0200 >Subject: [PATCH 1/2] BUILD: Explicitly link libsss_ad.so with sasl libs > >If openldap is not built with sasl support >libsss_ad.so will not be linked with libsasl2 although >sasl_client_init is called by function ad_sasl_initialize. >--- > Makefile.am | 2 ++ > configure.ac | 3 +-- > src/external/sasl.m4 | 17 +++++++++++++++++ > 3 files changed, 20 insertions(+), 2 deletions(-) > create mode 100644 src/external/sasl.m4 > Could someone push 1st patch also to the 1-11 branch? LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
