On (03/09/13 08:32), Stephen Gallagher wrote:
-----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 removed these teo lines and
replaced with:
+AC_CHECK_TYPES([intptr_t],
+ [],
+ [AC_DEFINE_UNQUOTED([intptr_t], [long long],
+ [Define to `long long'
+ if <stdint.h does not define.])])
>
> 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...
>
Yes, We should file a ticket to get rid of 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 <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 ?
>
Yes, but portable way is to call
"sysconfig.get_config_var('LIBDIR')"
or to use pkg-config (I don't like second option)
>> 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 [].
>
Changed.
>> 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.
>
I din't have a problem with this, beacuse I cherry-picked all patches
directly from
master. I don't want to complicate this.
So you have two options either to use "cherry-pick" form master or "git am
--3way".
I created patch from master and 1-9 branch and patches was very similar.
I don't know why it can not be applied cleanly.
>>
>> --- 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.
>
Macro LT_LIB_DLLOAD can be safely use, it is not private.
5.1 Autoconf macros exported by libtool
http://www.gnu.org/software/libtool/manual/html_node/Autoconf-macros.html
>> 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)..
>
I have a 64-bit virtual machine and directory /usr/local/lib64 does not
exist.
>> 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?
>
FreeBSD have .pc files.
popt-devel does not ship a .pc file on fedora 19 :-)
We need fallback.
>> 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..
Changed
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.
I tested srpm from master and sssd-1-9 with
various mock configs. So it must work.
Patch 0002: See Sumit and Jakub's comments about the macro.
Patch 0003: Ack
Patch 0004: See earlier comments about escaping.
As I mentioned before. It is
changed.
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.
Standard directories are /usr/include and /usr/lib, but all
packages
are installed to /usr/local/.
I would like to push these patches also to 1-9-branch.
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.
I think we can leave
it as it. libunistring can be untested.
The similar situation is with nss and libcrypto.
I have also fixed all Sumit's comments
Thank you very much for review.
Updated patches are attached.
LS