On Mon, Sep 09, 2013 at 04:21:50PM +0200, Jakub Hrozek wrote: > On Mon, Sep 09, 2013 at 04:11:01PM +0200, Jakub Hrozek wrote: > > On Tue, Sep 03, 2013 at 08:14:09PM +0200, Lukas Slebodnik wrote: > > > 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 <[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 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.])]) > > > > ^^^^^^^^^ > > you missed a closing bracket > > > > Otherwise ACK. > > > > If you agree, I will just add the bracket before pushing. > > > > > > > > >> > > > >> 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 > > > >> > > > 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 <[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 ? > > > >> > > > Yes, but portable way is to call "sysconfig.get_config_var('LIBDIR')" > > > or to use pkg-config (I don't like second option) > > > > Yes, I like LIBDIR, better, too. > > > > ACK > > > > > > > > >>> 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 []. > > > >> > > > Changed. > > > > ACK > > > > > > > > >>> 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. > > > >> > > > 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 > > > > Right, I think I was confused by the LT_ prefix. > > > > ACK > > > > > > > > >>> 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 > > > > Still 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).. > > > >> > > > I have a 64-bit virtual machine and directory /usr/local/lib64 does not > > > exist. > > > > OK, thank you for checking. ACK. > > > > > > > > > > > >>> 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? > > > >> > > > FreeBSD have .pc files. > > > popt-devel does not ship a .pc file on fedora 19 :-) > > > We need fallback. > > > > ACK > > > > > > > > >>> 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.. > > > Changed > > > > > > > ACK > > > > > > > > > > > > > >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 also suppose that some distributions that prefer lower footprint would > > like to keep libunistring support in. It was tested in RHEL and the code > > didn't change afterwards. > > btw I also just tested mock builds with F-19 and RHEL-6 and they passed > just fine.
I amended the comment and pushed all patches to master, sssd-1-10 and sssd-1-9 _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
