On Mon, Sep 02, 2013 at 07:04:22PM +0200, Lukas Slebodnik wrote: > ehlo > > Patches are attached. > > LS
(I started this review yesterday in the evening and then didn't had the energy to finish it so some comments overlap with Sumit's) The SSSD build still works fine with these patches on Fedora 19 (master) and for patches which could be applied on top of 1.9, I also tested RHEL-5. I haven't tested RHEL-6 or any other distribution. > From 6e3b789f4b24198b2ec4b40fb09e8b97e578044a Mon Sep 17 00:00:00 2001 > From: Lukas Slebodnik <[email protected]> > Date: Sat, 31 Aug 2013 01:12:01 +0200 > Subject: [PATCH 2/9] AUTOTOOLS: add check for type intptr_t > > We check whether HAVE_INTPTR_T is defined in definition of macro > discard_const_p, but autootols macro AC_CHECK_TYPE did not generate it. > --- > src/external/sizes.m4 | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/src/external/sizes.m4 b/src/external/sizes.m4 > index > 53df61dedc3ae748c50ca9a3935632087834155d..1018d846016541043d81fb2a53609ad9c562071a > 100644 > --- a/src/external/sizes.m4 > +++ b/src/external/sizes.m4 > @@ -37,8 +37,7 @@ AC_CHECK_SIZEOF(off_t) > AC_CHECK_SIZEOF(size_t) > AC_CHECK_SIZEOF(ssize_t) > > +AC_CHECK_TYPES([intptr_t]) > AC_CHECK_TYPE(intptr_t, long long) > AC_CHECK_TYPE(uintptr_t, unsigned long long) > AC_CHECK_TYPE(ptrdiff_t, unsigned long long) I think this is OK but maybe we should have a ticket to get rid of the obsolete AC_CHECK_TYPE invocation? See https://www.gnu.org/software/autoconf/manual/autoconf-2.67/html_node/Obsolete-Macros.html#Obsolete-Macros for some details. The documentation explicitly cites using AC_CHECK_TYPE for detecting pointer types as broken. I would also like a comment added or the "AC_CHECK_TYPE(intptr_t" removed because currently the code looks a little odd with two subsequent lines checking for intptr_t. > From e5d01ae5690d52b83dbdb621bf4e4077fc65abf4 Mon Sep 17 00:00:00 2001 > From: Lukas Slebodnik <[email protected]> > Date: Sat, 31 Aug 2013 05:16:58 +0200 > Subject: [PATCH 3/9] AUTOTOOLS: Add -LLIBDIR to PYTHON_LIBS > > Detect directory with python libraries and add this > directory to the list of directories to be searched for linker. > --- > src/external/python.m4 | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/external/python.m4 b/src/external/python.m4 > index > 00487a746a44a945e7bcd92a8f1f2ddd2b4eba80..5f48bc92618b2fbc23610f67f86e38f659977c5a > 100644 > --- a/src/external/python.m4 > +++ b/src/external/python.m4 > @@ -19,7 +19,8 @@ dnl versions of python > PYTHON_LIBS="`$PYTHON -c \"from distutils import sysconfig; \ > print \\\" \\\".join(sysconfig.get_config_var('LIBS').split() + \ > sysconfig.get_config_var('SYSLIBS').split()) + \ > - ' -lpython' + sysconfig.get_config_var('VERSION')\"`" > + ' -lpython' + sysconfig.get_config_var('VERSION') + \ > + ' -L' + sysconfig.get_config_var('LIBDIR')\"`" > AC_MSG_RESULT([yes]) > else > AC_MSG_ERROR([no. Please install python devel package]) > -- > 1.8.3.1 > ACK. Out of curiosity, what directory is Python's LIBDIR on FreeBSD? /usr/local/lib ? > From: Lukas Slebodnik <[email protected]> > Date: Sat, 31 Aug 2013 05:36:49 +0200 > Subject: [PATCH 4/9] AUTOTOOLS: Add missing AC_MSG_RESULT Ack, but Sumit is right about adding []. > From c338c7d721d6bef4fda6ad84321dd3096a14612a Mon Sep 17 00:00:00 2001 > From: Lukas Slebodnik <[email protected]> > Date: Sat, 31 Aug 2013 07:08:32 +0200 > Subject: [PATCH 5/9] AUTOMAKE: Use portable way to link with dlopen I couldn't apply this patch on top of 1.9. It works on Fedora 19, though. > > --- > Makefile.am | 4 ++-- > configure.ac | 1 + > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/Makefile.am b/Makefile.am > index > 9e68ad9b3a16c9106311eb833a97b01714656a61..1d08dd08b19a56017fbff908811e6a58ab0f7a74 > 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -719,7 +719,7 @@ sssd_be_SOURCES = \ > src/providers/dp_refresh.c \ > $(SSSD_FAILOVER_OBJ) > sssd_be_LDADD = \ > - -ldl \ > + $(LIBADD_DL) \ > $(SSSD_LIBS) \ > $(CARES_LIBS) \ > $(SSSD_INTERNAL_LTLIBS) > @@ -1096,7 +1096,7 @@ simple_access_tests_CFLAGS = \ > $(CHECK_CFLAGS) \ > -DUNIT_TESTING > simple_access_tests_LDADD = \ > - -ldl \ > + $(LIBADD_DL) \ > $(SSSD_LIBS) \ > $(CARES_LIBS) \ > $(CHECK_LIBS) \ > diff --git a/configure.ac b/configure.ac > index > 19bd1928f8a2149d3bf8e8f7a8d46ab7343a0bd6..d16054efae73e8844ac236d0220f274684c5ce8d > 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -20,6 +20,7 @@ m4_ifdef([AM_PROG_AR], [AM_PROG_AR]) > AC_DISABLE_STATIC > AC_PROG_INSTALL > AC_PROG_LIBTOOL > +LT_LIB_DLLOAD I thought that LT_* macros were low-level libtool macros? Can you check if AC_LIBTOOL_DLOPEN would do the same thing? Also I would expect that any macro that would set $LD should be called before AC_PROG_LIBTOOL not after. > From a98852a669c5df89a0c611cc9324be32e6d05c0e Mon Sep 17 00:00:00 2001 > From: Lukas Slebodnik <[email protected]> > Date: Sat, 31 Aug 2013 08:50:47 +0200 > Subject: [PATCH 6/9] AUTOMAKE: Use portable way to link with gettext ACK > From beb84b3339a79de8f0f4a57b2e61642542b0e4ef Mon Sep 17 00:00:00 2001 > From: Lukas Slebodnik <[email protected]> > Date: Sat, 31 Aug 2013 11:15:03 +0200 > Subject: [PATCH 7/9] AUTOTOOLS: Add directories for searching ldap headers and > libs > > --- > src/external/ldap.m4 | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/external/ldap.m4 b/src/external/ldap.m4 > index > 8899df9d7c73dae25b97347022651d768986333e..3a99ddfcc7e7ebee94272d382e2876ec8649770a > 100644 > --- a/src/external/ldap.m4 > +++ b/src/external/ldap.m4 > @@ -9,14 +9,14 @@ dnl > --------------------------------------------------------------------------- > dnl - Check for Mozilla LDAP or OpenLDAP SDK > dnl > --------------------------------------------------------------------------- > > -for p in /usr/include/openldap24; do > +for p in /usr/include/openldap24 /usr/local/include; do > if test -f "${p}/ldap.h"; then > OPENLDAP_CFLAGS="${OPENLDAP_CFLAGS} -I${p}" > break; > fi > done > > -for p in /usr/lib64/openldap24 /usr/lib/openldap24; do > +for p in /usr/lib64/openldap24 /usr/lib/openldap24 /usr/local/lib ; do > if test -f "${p}/libldap.so"; then > OPENLDAP_LIBS="${OPENLDAP_LIBS} -L${p}" > break; Don't we also need to add /usr/local/lib64 ? (Sorry, I don't know how multilib works on FreeBSD).. > From 2e939d50ce4837a76bb4850e6fb6b1c1435c27c1 Mon Sep 17 00:00:00 2001 > From: Lukas Slebodnik <[email protected]> > Date: Sat, 31 Aug 2013 12:17:38 +0200 > Subject: [PATCH 8/9] AUTOTOOLS: Use pkg-config to detect libraries. > > We used pkg-config only as a fallback if header files was not found, > but detection of library failed in case of available header file and > linking problem (missing -Ldir). > > This patch prefers pkg-config. The other reason is that in the past, some of the libraries didn't ship a .pc file. Since you would like to have these patches in 1.9 (I assume), can you check that they have pkg-config files in the older versions, too? > From faa431369b66684024272392da507d374695aaea Mon Sep 17 00:00:00 2001 > From: Lukas Slebodnik <[email protected]> > Date: Sat, 31 Aug 2013 15:32:39 +0200 > Subject: [PATCH 9/9] AUTOTOOLS: Refactor unicode library detection Seems to work fine, but see Sumit's comment in his response.. _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
