-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 09/03/2013 07:32 AM, Jakub Hrozek wrote:
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..
I ran these patches in a scratch-build against RHEL 6 (which is the
oldest supported OS we have for master). It builds successfully, from
which I will assume that all of the macros we are relying on either
exist or are falling-back gracefully.
Patch 0002: See Sumit and Jakub's comments about the macro.
Patch 0003: Ack
Patch 0004: See earlier comments about escaping.
Patch 0005: Ack
Patch 0006: Ack
Patch 0007: are /usr/local/include and /usr/local/lib not standard
lookup paths on BSD? The reason for this entry here is entirely for
support of RHEL 5 (which we have dropped) because we were carrying a
parallel-installable version of the openldap client libraries in a
non-standard location.
Patch 0008: I *think* the original reasoning for this behavior was
that we intended a Solaris port which didn't support pkg-config
properly at the time. I'm not sure whether Solaris/Illumos today has
since implemented it properly. That said, we're not currently focusing
on that OS, so I'm willing to leave that as "patches welcome".
Patch 0009: Can we drop libunistring in 1.11/1.12? We only originally
supported it because we were wary that pulling in Glib would have
issues with dueling implementations of D-BUS and a mainloop, but we've
since proven that this isn't a problem. Of course, if Glib is not
available on our target platforms, this becomes less obvious.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird -
http://www.enigmail.net/
iEYEARECAAYFAlIl1vEACgkQeiVVYja6o6MZIwCcD8chfSRBKU1P8tSzfd+xyXJz
HncAnRg1xpUiCtgym3VCIvLyyLCGwFzG
=X0kQ
-----END PGP SIGNATURE-----