[PATCHES] Improve portability of autoconf and automake

[Lukas Slebodnik]

ehlo

Patches are attached.

LS

[Sumit Bose]

On Mon, Sep 02, 2013 at 07:04:22PM +0200, Lukas Slebodnik wrote:
> ehlo
>
> Patches are attached.

I haven't tested to patches so far but already have some comments.

bye,
Sumit
> LS
> From 6e3b789f4b24198b2ec4b40fb09e8b97e578044a Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik 
> 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 guess we missed the change of AC_CHECK_TYPE in Autoconf 2.14 where the
style used above still worked. If I understand the docs correctly it
should still work, but you should remove the old AC_CHECK_TYPE for
intptr_t.

Would you mind to update the other old-style AC_CHECK_TYPE usages as
well or open a trac ticket to do so?
> -- 
> 1.8.3.1
> From af2721b0120e9b48e09a9372362a57b8de5f1bf2 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik 
> Date: Sat, 31 Aug 2013 05:36:49 +0200
> Subject: [PATCH 4/9] AUTOTOOLS: Add missing AC_MSG_RESULT
>
> AC_MSG_RESULT was not used everywhere after AC_MSG_CHECKING.
> Therefore two lines from configure output was mixed in some cases.
>
> src/external/krb5.m4          | 3 ++-
>  src/external/nsupdate.m4      | 6 ++++--
>  src/external/pac_responder.m4 | 1 +
>  src/external/python.m4        | 3 ++-
>  4 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/src/external/krb5.m4 b/src/external/krb5.m4
> index bdbd71ba1f395eb4f78047f75841729e0289ae81..9cb161d7208c108f744eb34b8e08eb6b3a0fc267 100644
> --- a/src/external/krb5.m4
> +++ b/src/external/krb5.m4
> @@ -16,8 +16,9 @@ if test -x "$KRB5_CONFIG"; then
>    KRB5_LIBS="`$KRB5_CONFIG --libs`"
>    AC_MSG_RESULT(yes)
>  else
> AC_MSG_RESULT([no])
> if test x$KRB5_PASSED_LIBS = x; then
>    AC_MSG_ERROR(no. Please install MIT kerberos devel package)
>    AC_MSG_ERROR(Please install MIT kerberos devel package)

can you add [] ?
>  fi
>
> fi
>
> diff --git a/src/external/nsupdate.m4 b/src/external/nsupdate.m4
> index 9ccff615927664afd7d9a6412159791ff79c5485..b7048d58a3f50dfd25c20754f71386646e2e3dce 100644
> --- a/src/external/nsupdate.m4
> +++ b/src/external/nsupdate.m4
> @@ -9,9 +9,11 @@ if test -x "$NSUPDATE"; then
>      AC_MSG_RESULT([yes])
>      AC_DEFINE_UNQUOTED([HAVE_NSUPDATE_REALM], 1, [Whether to use the 'realm' directive with nsupdate])
>    else
> AC_MSG_WARN([no. Will build without the 'realm' directive])
> AC_MSG_RESULT([no])
> AC_MSG_WARN([Will build without the 'realm' directive])
> fi
>
> else
> AC_MSG_ERROR([no. nsupdate is not available])
> AC_MSG_RESULT([no])
> AC_MSG_ERROR([nsupdate is not available])
>
> fi
> diff --git a/src/external/pac_responder.m4 b/src/external/pac_responder.m4
> index 733241a1322f4338be0345b2bb969b7a7c5c35ce..f4f4a34e3256d18dc6387e388a1fbebb223f3566 100644
> --- a/src/external/pac_responder.m4
> +++ b/src/external/pac_responder.m4
> @@ -25,6 +25,7 @@ then
>              AC_MSG_RESULT(yes)
>              ;;
>          *)
>        AC_MSG_RESULT(no)
>        AC_MSG_WARN([Cannot build authdata plugin with this version of
>                     MIT Kerberos, please use 1.9.x or later])
> esac
>
> diff --git a/src/external/python.m4 b/src/external/python.m4
> index 5f48bc92618b2fbc23610f67f86e38f659977c5a..cfe9961c75bbd191b78c8cdc21008b11b179548f 100644
> --- a/src/external/python.m4
> +++ b/src/external/python.m4
> @@ -23,7 +23,8 @@ dnl versions of python
>              ' -L' + sysconfig.get_config_var('LIBDIR')"`"
>              AC_MSG_RESULT([yes])
>      else
>    AC_MSG_ERROR([no. Please install python devel package])
>    AC_MSG_RESULT([no])
>    AC_MSG_ERROR([Please install python devel package])
> fi
>
> ])
>
> -- 
> 1.8.3.1
> From 2e939d50ce4837a76bb4850e6fb6b1c1435c27c1 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik 
> 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.

Would it make sense to add SSS_AC_EXPAND_LIB_DIR to the non-pkg-config
paths as well?
> From faa431369b66684024272392da507d374695aaea Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik 
> Date: Sat, 31 Aug 2013 15:32:39 +0200
> Subject: [PATCH 9/9] AUTOTOOLS: Refactor unicode library detection
>
> If $libdir is not in default library path libunistring cannot be
> found. (pkg-config can not be used in this case).
> This patch helps to search libunistring in "$libdir" directory.
>
> In refactoring part, indentation was updated to be more readable
> and some duplicated parts were removed.
>
> configure.ac                 | 15 +++++++--------
>  src/build_macros.m4          | 13 +++++++++++++
>  src/external/libunistring.m4 | 31 ++++++++++++++++++++++++-------
>  3 files changed, 44 insertions(+), 15 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index d16054efae73e8844ac236d0220f274684c5ce8d..c8803b16bc8bf95c6902a18fd5a9be3965d9c59e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -158,16 +158,15 @@ m4_include([src/external/libndr_nbt.m4])
>
> WITH_UNICODE_LIB
>  if test x$unicode_lib = xlibunistring; then
> m4_include([src/external/libunistring.m4])
>        AC_DEFINE_UNQUOTED(HAVE_LIBUNISTRING, 1, [Using libunistring for unicode])
>        UNICODE_LIBS=-lunistring
>        AC_SUBST(UNICODE_LIBS)
> m4_include([src/external/libunistring.m4])
> AC_DEFINE_UNQUOTED(HAVE_LIBUNISTRING, 1, [Using libunistring for unicode])
> UNICODE_LIBS=$UNISTRING_LIBS
>
> else
> m4_include([src/external/glib.m4])
>        AC_DEFINE_UNQUOTED(HAVE_GLIB2, 1, [Using libunistring for unicode])
>        UNICODE_LIBS=$GLIB2_LIBS
>        AC_SUBST(UNICODE_LIBS)
> m4_include([src/external/glib.m4])
> AC_DEFINE_UNQUOTED(HAVE_GLIB2, 1, [Using libunistring for unicode])

Can you fix the old libunistring/glib copy-and-paste error?
> UNICODE_LIBS=$GLIB2_LIBS
>
> fi
> +AC_SUBST(UNICODE_LIBS)
>
> WITH_LIBNL

[Lukas Slebodnik]

On (03/09/13 12:15), Sumit Bose wrote:
> On Mon, Sep 02, 2013 at 07:04:22PM +0200, Lukas Slebodnik wrote:
> > ehlo
> >
> > Patches are attached.
>
> I haven't tested to patches so far but already have some comments.
>
> bye,
> Sumit
> > LS
> > From 6e3b789f4b24198b2ec4b40fb09e8b97e578044a Mon Sep 17 00:00:00 2001
> > From: Lukas Slebodnik 
> > 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 guess we missed the change of AC_CHECK_TYPE in Autoconf 2.14 where the
> style used above still worked. If I understand the docs correctly it
> should still work, but you should remove the old AC_CHECK_TYPE for
> intptr_t.

just a comment to this point.

It may be litle bit confusing.
There are 3 macros with similar names.
AC_CHECK_TYPE (type, [action-if-found], [action-if-not-found],
                     [includes = ‘AC_INCLUDES_DEFAULT’])
AC_CHECK_TYPES (types, [action-if-found], [action-if-not-found],
                     [includes = ‘AC_INCLUDES_DEFAULT’])
And obsolete macro:
AC_CHECK_TYPE (type, default)
   --If the type type is not defined, define it to be the C (or C++) builtin
     type default, e.g., ‘short int’ or ‘unsigned int’.
   --missing types are defined using #define and not typedef

I hope we used obsolete version. There is a patter how to rewrite it using
new macro. We can ged rid of them.

And now about non obsolete versions.

There is a difference between AC_CHECK_TYPE and AC_CHECK_TYPES
The second version: For each type of the types that is defined,
    define HAVE_type
> > +AC_CHECK_TYPES([intptr_t])

-> this generate HAVE_INTPTR_T if exists intptr_t
> > AC_CHECK_TYPE(intptr_t, long long)

-> this generate "#define intptr_t longlong" unless exists intptr_t

And here is how discard_const_p is defined.

#ifndef discard_const_p
#if defined(__intptr_t_defined) || defined(HAVE_INTPTR_T)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^     ^^^^^^^^^^^^^^^^^^^^^
    This is not portable          This was not generated by autoconf
# define discard_const_p(type, ptr) ((type *)((intptr_t)(ptr)))
#else
# define discard_const_p(type, ptr) ((type *)(ptr))
#endif
#endif

If intptr_t is not default(builtin) type, we will define it in macro
AC_CHECK_TYPE (new one or obsolete).
Which branch of discard_const_p should be used?

I need to know how to use macro.

And the second question. Do wee need defined HAVE_type for all types?
(only HAVE_INTPTR_T is used in code)

LS

[Jakub Hrozek]

On Tue, Sep 03, 2013 at 04:33:03PM +0200, Lukas Slebodnik wrote:
> On (03/09/13 12:15), Sumit Bose wrote:
> > On Mon, Sep 02, 2013 at 07:04:22PM +0200, Lukas Slebodnik wrote:
> > > ehlo
> > >
> > > Patches are attached.
> >
> > I haven't tested to patches so far but already have some comments.
> >
> > bye,
> > Sumit
> > > LS
> > > From 6e3b789f4b24198b2ec4b40fb09e8b97e578044a Mon Sep 17 00:00:00 2001
> > > From: Lukas Slebodnik 
> > > 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 guess we missed the change of AC_CHECK_TYPE in Autoconf 2.14 where the
> > style used above still worked. If I understand the docs correctly it
> > should still work, but you should remove the old AC_CHECK_TYPE for
> > intptr_t.
>
> just a comment to this point.
>
> It may be litle bit confusing.
> There are 3 macros with similar names.
> AC_CHECK_TYPE (type, [action-if-found], [action-if-not-found],
>                      [includes = ‘AC_INCLUDES_DEFAULT’])
> AC_CHECK_TYPES (types, [action-if-found], [action-if-not-found],
>                      [includes = ‘AC_INCLUDES_DEFAULT’])
> And obsolete macro:
> AC_CHECK_TYPE (type, default)
>    --If the type type is not defined, define it to be the C (or C++) builtin
>      type default, e.g., ‘short int’ or ‘unsigned int’.
>    --missing types are defined using #define and not typedef
>
> I hope we used obsolete version.

Yes we did.
> There is a patter how to rewrite it using
> new macro. We can ged rid of them.

I'm always in favor of getting rid of obsolete stuff, but I don't think
we need to do it now (unless it's trivial).
> And now about non obsolete versions.
>
> There is a difference between AC_CHECK_TYPE and AC_CHECK_TYPES
> The second version: For each type of the types that is defined,
>     define HAVE_type
> > > +AC_CHECK_TYPES([intptr_t])
> -> this generate HAVE_INTPTR_T if exists intptr_t
> > > AC_CHECK_TYPE(intptr_t, long long)
> -> this generate "#define intptr_t longlong" unless exists intptr_t

Hm, I wonder if there are any systems out there that depend on the
compat #define generated by AC_CHECK_TYPE. Perhaps if we get rid of the
macros, we should do it in backwards-compatible way.
> And here is how discard_const_p is defined.
>
> #ifndef discard_const_p
> #if defined(__intptr_t_defined) || defined(HAVE_INTPTR_T)
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^     ^^^^^^^^^^^^^^^^^^^^^
>     This is not portable          This was not generated by autoconf
> # define discard_const_p(type, ptr) ((type *)((intptr_t)(ptr)))
> #else
> # define discard_const_p(type, ptr) ((type *)(ptr))
> #endif
> #endif
>
> If intptr_t is not default(builtin) type, we will define it in macro
> AC_CHECK_TYPE (new one or obsolete).
> Which branch of discard_const_p should be used?

I would prefer the portable branch guarded by defined.
> I need to know how to use macro.
>
> And the second question. Do wee need defined HAVE_type for all types?
> (only HAVE_INTPTR_T is used in code)

I think it would be more defensive but not strictly needed. If we had
the macro then we could always use HAVE_$type and not worry about
forgetting that some types have the HAVE_$type definition and some not.
But then again, it wasn't an issue until now.

[Jakub Hrozek]

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 
> 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/Obsolet...
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 
> 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 
> 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 
> 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 
> 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 
> 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 
> 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 
> 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..

[Stephen Gallagher]

-----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  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/Obsolet...

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  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  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  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  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  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  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  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-----

[Jakub Hrozek]

On Tue, Sep 03, 2013 at 08:32:49AM -0400, 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  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/Obsolet...
>
> 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  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  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  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  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  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  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  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.

Great, thanks for the RHEL-6 test. The reason I also tested on RHEL-5 was
that I'd like to push the patches to sssd-1-9 as well as it makes sense
to upgrade the FreeBSD port to 1.9. Currently they're stuck on 1.7.x (sic).

[Lukas Slebodnik]

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  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/Obsolet...

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  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  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  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  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  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  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  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

[Jakub Hrozek]

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  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/Obsolet...
>
> 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  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  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  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  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  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  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  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.
> I have also fixed all Sumit's comments
>
> Thank you very much for review.
>
> Updated patches are attached.
>
> LS

[Jakub Hrozek]

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  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/Obsolet...
> >
> > 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  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  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  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  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  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  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  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.

[Jakub Hrozek]

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  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/Obsolet...
> > >
> > > 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  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  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  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  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  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  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  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