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 <lslebodn(a)redhat.com>
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/Obso...
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 <lslebodn(a)redhat.com>
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 <lslebodn(a)redhat.com>
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 <lslebodn(a)redhat.com>
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 <lslebodn(a)redhat.com>
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 <lslebodn(a)redhat.com>
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 <lslebodn(a)redhat.com>
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 <lslebodn(a)redhat.com>
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..