[Spice-devel] [PATCH] m4: Add macro for --with-sasl

[Pavel Grunt]

Hi,

On Wed, 2015-11-25 at 14:58 +0100, Christophe Fergeau wrote:
> Hey,
> 
> On Tue, Nov 24, 2015 at 11:38:26AM +0100, Pavel Grunt wrote:
> > Imported from spice-gtk e3c455b86dba0f924165fe0dc51583f48aecc8fb.
> > It is not used by spice-common, but both server and client can use it.
> 
> Couple of comments, most of them are more things which could be added on
> top of this change
> 
> > ---
> >  m4/spice-deps.m4 | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> > 
> > diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> > index cb2b4c0..3579db5 100644
> > --- a/m4/spice-deps.m4
> > +++ b/m4/spice-deps.m4
> > @@ -263,3 +263,56 @@ AS_IF([test "x$1" != x],
> >               [missing_gstreamer_elements="no"])
> >        ])
> >  ])
> > +
> > +# SPICE_CHECK_SASL
> > +# ----------------
> > +# Adds a --with-sasl switch to allow using SASL for authentication.
> > +# Checks whether the required library is available. If it is present,
> > +# it will return the flags to use in SASL_CFLAGS and SASL_LIBS variables,
> > +# and it will define a HAVE_SASL preprocessor symbol.
> 
> And a HAVE_SASL conditional
> 
> > +# ----------------
> > +AC_DEFUN([SPICE_CHECK_SASL], [
> > +    AC_ARG_WITH([sasl],
> > +      [AS_HELP_STRING([--with-sasl=@<:@yes/no/auto@:>@],
> > +                      [use cyrus SASL for authentication
> > @<:@default=auto@:>@])],
> > +                      [],
> > +                      [with_sasl="auto"])
> > +
> > +    SASL_CFLAGS=
> > +    SASL_LIBS=
> > +    enable_sasl=no
> > +    if test "x$with_sasl" != "xno"; then
> > +      if test "x$with_sasl" != "xyes" && test "x$with_sasl" != "xauto";
> > then
> > +        SASL_CFLAGS="-I$with_sasl"
> > +        SASL_LIBS="-L$with_sasl"
> 
> This block seems to be meant to pass --with-sasl=/prefix/to/libsasl ,
> which would make the help string incorrect.
> 
> 
> > +      fi
> > +      old_cflags="$CFLAGS"
> > +      old_libs="$LIBS"
> > +      CFLAGS="$CFLAGS $SASL_CFLAGS"
> > +      LIBS="$LIBS $SASL_LIBS"
> > +      AC_CHECK_HEADER([sasl/sasl.h],[with_sasl=yes])
> 
> The spice-server code had a bit more sophistication which caused
> configure to fail if --with-sasl is passed and the header is not found
> (I changed sasl/sasl.h to something non-existing in order to test that).
> However, if autodetection is used, the failed detection of the header is
> ignored, and if the library is found, then sasl will be enabled.
> 
> > +      if test "x$with_sasl" = "xyes" ; then
> > +        AC_CHECK_LIB([sasl2],
> > [sasl_client_init],[with_sasl2=yes],[with_sasl2=no])
> > +      fi
> > +      if test "x$with_sasl2" = "xno" && test "x$with_sasl" = "xyes" ; then
> > +        AC_CHECK_LIB([sasl],
> > [sasl_client_init],[with_sasl=yes],[with_sasl=no])
> > +      fi
> 
> I'd say it's time to drop support for -lsasl which corresponds to
> cyrus-sasl 1.5 versions, 1.5.28 was released in 2002 (and it seems to be
> the most recent release).
> 
> One additional remark is that cyrus-sasl ships a .pc file since its most
> recent release (2.1.26), which was released 3 years ago. This release is
> available in debian stable, supported fedoras and el7. el6 does not have
> this .pc file though. I'd still switch to it and drop this big block of
> detection code.
Ok, everything will be easier

> 
> This would make it easier to get the 'usual' behaviour:
> - configure autodetects when nothing is specified
> - configure fails if --with-sasl is specified but cyrus-sasl cannot be
>   detected
> - configure does not try to detect cyrus-sasl if --without-sasl is used.
> 
> The spice-deps.m4 macro you introduced could use the .pc file from the
> start I think, do you mind updating this patch to do that?

I will update it,

Thanks,
Pavel

> (alternatively I can do it, using one of the other checks in this file
> for inspiration ;)
> 
> Christophe