[Spice-devel,spice-common,v2,1/2] spice-deps: Make LZ4 check depending on function

Submitted by Pavel Grunt on Nov. 24, 2016, 2:11 p.m.

Details

Message ID 20161124141125.2325-1-pgrunt@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Pavel Grunt Nov. 24, 2016, 2:11 p.m.
LZ4 changed versioning scheme from r131 to v1.7.3 making our configure
fail with (1.7.3 < 129).

Switch from version checking to checking that the necessary function
is available.
---
v2: Added some comments, Switched to AC_CHECK_FUNC
---
 m4/spice-deps.m4 | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
index adedec4..6827c7f 100644
--- a/m4/spice-deps.m4
+++ b/m4/spice-deps.m4
@@ -185,12 +185,26 @@  AC_DEFUN([SPICE_CHECK_LZ4], [
 
     have_lz4="no"
     if test "x$enable_lz4" != "xno"; then
-      PKG_CHECK_MODULES([LZ4], [liblz4 >= 129], [have_lz4="yes"], [have_lz4="no"])
+      # LZ4_compress_default is available in liblz4 >= 129, however liblz has changed
+      # versioning scheme making the check failing. Rather check for function definition
+      PKG_CHECK_MODULES([LZ4], [liblz4], [have_lz4="yes"], [have_lz4="no"])
 
       if test "x$have_lz4" = "xyes"; then
-        AC_DEFINE(USE_LZ4, [1], [Define to build with lz4 support])
-      elif test "x$enable_lz4" = "xyes"; then
-        AC_MSG_ERROR([lz4 support requested but liblz4 could not be found])
+        # For cross-compilers may be necessary to save & restore LIBS and CFLAGS before AC_SEARCH_LIBS
+        old_LIBS="$LIBS"
+        old_CFLAGS="$CFLAGS"
+        CFLAGS="$CFLAGS $LZ4_CFLAGS"
+        LIBS="$LIBS $LZ4_LIBS"
+
+        AC_CHECK_FUNC([LZ4_compress_default], [
+            AC_DEFINE(USE_LZ4, [1], [Define to build with lz4 support])],
+            [have_lz4="no"])
+
+        LIBS="$old_LIBS"
+        CFLAGS="$old_CFLAGS"
+      fi
+      if test "x$enable_lz4" = "xyes" && test "x$have_lz4" = "xno"; then
+        AC_MSG_ERROR([lz4 support requested but liblz4 >= 129 could not be found])
       fi
     fi
     AM_CONDITIONAL(HAVE_LZ4, test "x$have_lz4" = "xyes")

Comments

On Thu, Nov 24, 2016 at 03:11:24PM +0100, Pavel Grunt wrote:
> LZ4 changed versioning scheme from r131 to v1.7.3 making our configure
> fail with (1.7.3 < 129).
> 
> Switch from version checking to checking that the necessary function
> is available.
> ---
> v2: Added some comments, Switched to AC_CHECK_FUNC
> ---
>  m4/spice-deps.m4 | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> index adedec4..6827c7f 100644
> --- a/m4/spice-deps.m4
> +++ b/m4/spice-deps.m4
> @@ -185,12 +185,26 @@ AC_DEFUN([SPICE_CHECK_LZ4], [
>  
>      have_lz4="no"
>      if test "x$enable_lz4" != "xno"; then
> -      PKG_CHECK_MODULES([LZ4], [liblz4 >= 129], [have_lz4="yes"], [have_lz4="no"])
> +      # LZ4_compress_default is available in liblz4 >= 129, however liblz has changed
> +      # versioning scheme making the check failing. Rather check for function definition
> +      PKG_CHECK_MODULES([LZ4], [liblz4], [have_lz4="yes"], [have_lz4="no"])
>  
>        if test "x$have_lz4" = "xyes"; then
> -        AC_DEFINE(USE_LZ4, [1], [Define to build with lz4 support])
> -      elif test "x$enable_lz4" = "xyes"; then
> -        AC_MSG_ERROR([lz4 support requested but liblz4 could not be found])
> +        # For cross-compilers may be necessary to save & restore LIBS and CFLAGS before AC_SEARCH_LIBS

This is not limited to cross-compilers, you could have installed lz4 in
the /foo/bar/my-lz4 prefix and set PKG_CONFIG_PATH to accordingly.

Christophe
> 
> LZ4 changed versioning scheme from r131 to v1.7.3 making our configure
> fail with (1.7.3 < 129).
> 
> Switch from version checking to checking that the necessary function
> is available.
> ---
> v2: Added some comments, Switched to AC_CHECK_FUNC
> ---
>  m4/spice-deps.m4 | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> index adedec4..6827c7f 100644
> --- a/m4/spice-deps.m4
> +++ b/m4/spice-deps.m4
> @@ -185,12 +185,26 @@ AC_DEFUN([SPICE_CHECK_LZ4], [
>  
>      have_lz4="no"
>      if test "x$enable_lz4" != "xno"; then
> -      PKG_CHECK_MODULES([LZ4], [liblz4 >= 129], [have_lz4="yes"],
> [have_lz4="no"])
> +      # LZ4_compress_default is available in liblz4 >= 129, however liblz
> has changed
> +      # versioning scheme making the check failing. Rather check for
> function definition
> +      PKG_CHECK_MODULES([LZ4], [liblz4], [have_lz4="yes"], [have_lz4="no"])
>  
>        if test "x$have_lz4" = "xyes"; then
> -        AC_DEFINE(USE_LZ4, [1], [Define to build with lz4 support])
> -      elif test "x$enable_lz4" = "xyes"; then
> -        AC_MSG_ERROR([lz4 support requested but liblz4 could not be found])
> +        # For cross-compilers may be necessary to save & restore LIBS and
> CFLAGS before AC_SEARCH_LIBS
> +        old_LIBS="$LIBS"
> +        old_CFLAGS="$CFLAGS"
> +        CFLAGS="$CFLAGS $LZ4_CFLAGS"
> +        LIBS="$LIBS $LZ4_LIBS"
> +
> +        AC_CHECK_FUNC([LZ4_compress_default], [
> +            AC_DEFINE(USE_LZ4, [1], [Define to build with lz4 support])],
> +            [have_lz4="no"])
> +
> +        LIBS="$old_LIBS"
> +        CFLAGS="$old_CFLAGS"
> +      fi
> +      if test "x$enable_lz4" = "xyes" && test "x$have_lz4" = "xno"; then

I think this xyes/xno it's kind of a fix for shell bug quite long ago, a

   if test "$enable_lz4" = yes -a "$have_lz4" = no; then

will work. Even [ is more readable leading to

   if [ "$enable_lz4" = yes -a "$have_lz4" = no ]; then

configure try to use very old style to make sure compatibility is good
but this lead to really ugly syntax sometimes.

> +        AC_MSG_ERROR([lz4 support requested but liblz4 >= 129 could not be
> found])
>        fi
>      fi
>      AM_CONDITIONAL(HAVE_LZ4, test "x$have_lz4" = "xyes")

Frediano
On Thu, 2016-11-24 at 09:33 -0500, Frediano Ziglio wrote:
> > 
> > LZ4 changed versioning scheme from r131 to v1.7.3 making our
> > configure
> > fail with (1.7.3 < 129).
> > 
> > Switch from version checking to checking that the necessary
> > function
> > is available.
> > ---
> > v2: Added some comments, Switched to AC_CHECK_FUNC
> > ---
> >  m4/spice-deps.m4 | 22 ++++++++++++++++++----
> >  1 file changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> > index adedec4..6827c7f 100644
> > --- a/m4/spice-deps.m4
> > +++ b/m4/spice-deps.m4
> > @@ -185,12 +185,26 @@ AC_DEFUN([SPICE_CHECK_LZ4], [
> >  
> >      have_lz4="no"
> >      if test "x$enable_lz4" != "xno"; then
> > -      PKG_CHECK_MODULES([LZ4], [liblz4 >= 129], [have_lz4="yes"],
> > [have_lz4="no"])
> > +      # LZ4_compress_default is available in liblz4 >= 129,
> > however liblz
> > has changed
> > +      # versioning scheme making the check failing. Rather check
> > for
> > function definition
> > +      PKG_CHECK_MODULES([LZ4], [liblz4], [have_lz4="yes"],
> > [have_lz4="no"])
> >  
> >        if test "x$have_lz4" = "xyes"; then
> > -        AC_DEFINE(USE_LZ4, [1], [Define to build with lz4
> > support])
> > -      elif test "x$enable_lz4" = "xyes"; then
> > -        AC_MSG_ERROR([lz4 support requested but liblz4 could not
> > be found])
> > +        # For cross-compilers may be necessary to save & restore
> > LIBS and
> > CFLAGS before AC_SEARCH_LIBS
> > +        old_LIBS="$LIBS"
> > +        old_CFLAGS="$CFLAGS"
> > +        CFLAGS="$CFLAGS $LZ4_CFLAGS"
> > +        LIBS="$LIBS $LZ4_LIBS"
> > +
> > +        AC_CHECK_FUNC([LZ4_compress_default], [
> > +            AC_DEFINE(USE_LZ4, [1], [Define to build with lz4
> > support])],
> > +            [have_lz4="no"])
> > +
> > +        LIBS="$old_LIBS"
> > +        CFLAGS="$old_CFLAGS"
> > +      fi
> > +      if test "x$enable_lz4" = "xyes" && test "x$have_lz4" =
> > "xno"; then
> 
> I think this xyes/xno it's kind of a fix for shell bug quite long
> ago, a
> 
>    if test "$enable_lz4" = yes -a "$have_lz4" = no; then
> 
> will work. Even [ is more readable leading to
> 
>    if [ "$enable_lz4" = yes -a "$have_lz4" = no ]; then
> 
> configure try to use very old style to make sure compatibility is
> good
> but this lead to really ugly syntax sometimes.

it is a style used in all our configure.ac and m4 files. It may be
ugly/not easy to follow but I don't think it is worth spending time on
improving it everywhere... 


> 
> > +        AC_MSG_ERROR([lz4 support requested but liblz4 >= 129
> > could not be
> > found])
> >        fi
> >      fi
> >      AM_CONDITIONAL(HAVE_LZ4, test "x$have_lz4" = "xyes")
> 
> Frediano
> 
> On Thu, 2016-11-24 at 09:33 -0500, Frediano Ziglio wrote:
> > > 
> > > LZ4 changed versioning scheme from r131 to v1.7.3 making our
> > > configure
> > > fail with (1.7.3 < 129).
> > > 
> > > Switch from version checking to checking that the necessary
> > > function
> > > is available.
> > > ---
> > > v2: Added some comments, Switched to AC_CHECK_FUNC
> > > ---
> > >  m4/spice-deps.m4 | 22 ++++++++++++++++++----
> > >  1 file changed, 18 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> > > index adedec4..6827c7f 100644
> > > --- a/m4/spice-deps.m4
> > > +++ b/m4/spice-deps.m4
> > > @@ -185,12 +185,26 @@ AC_DEFUN([SPICE_CHECK_LZ4], [
> > >  
> > >      have_lz4="no"
> > >      if test "x$enable_lz4" != "xno"; then
> > > -      PKG_CHECK_MODULES([LZ4], [liblz4 >= 129], [have_lz4="yes"],
> > > [have_lz4="no"])
> > > +      # LZ4_compress_default is available in liblz4 >= 129,
> > > however liblz
> > > has changed
> > > +      # versioning scheme making the check failing. Rather check
> > > for
> > > function definition
> > > +      PKG_CHECK_MODULES([LZ4], [liblz4], [have_lz4="yes"],
> > > [have_lz4="no"])
> > >  
> > >        if test "x$have_lz4" = "xyes"; then
> > > -        AC_DEFINE(USE_LZ4, [1], [Define to build with lz4
> > > support])
> > > -      elif test "x$enable_lz4" = "xyes"; then
> > > -        AC_MSG_ERROR([lz4 support requested but liblz4 could not
> > > be found])
> > > +        # For cross-compilers may be necessary to save & restore
> > > LIBS and
> > > CFLAGS before AC_SEARCH_LIBS
> > > +        old_LIBS="$LIBS"
> > > +        old_CFLAGS="$CFLAGS"
> > > +        CFLAGS="$CFLAGS $LZ4_CFLAGS"
> > > +        LIBS="$LIBS $LZ4_LIBS"
> > > +
> > > +        AC_CHECK_FUNC([LZ4_compress_default], [
> > > +            AC_DEFINE(USE_LZ4, [1], [Define to build with lz4
> > > support])],
> > > +            [have_lz4="no"])
> > > +
> > > +        LIBS="$old_LIBS"
> > > +        CFLAGS="$old_CFLAGS"
> > > +      fi
> > > +      if test "x$enable_lz4" = "xyes" && test "x$have_lz4" =
> > > "xno"; then
> > 
> > I think this xyes/xno it's kind of a fix for shell bug quite long
> > ago, a
> > 
> >    if test "$enable_lz4" = yes -a "$have_lz4" = no; then
> > 
> > will work. Even [ is more readable leading to
> > 
> >    if [ "$enable_lz4" = yes -a "$have_lz4" = no ]; then
> > 
> > configure try to use very old style to make sure compatibility is
> > good
> > but this lead to really ugly syntax sometimes.
> 
> it is a style used in all our configure.ac and m4 files. It may be
> ugly/not easy to follow but I don't think it is worth spending time on
> improving it everywhere...
> 

I agree but usually when you decide to change a style you
define the new style and instead of changing everything
you change using the new style when you change the code.

By the way, not this not that important, patch is fine
even with this style. Perhaps the -a usage is compatible
with current style and more readable than test ... && test ...

> 
> > 
> > > +        AC_MSG_ERROR([lz4 support requested but liblz4 >= 129
> > > could not be
> > > found])
> > >        fi
> > >      fi
> > >      AM_CONDITIONAL(HAVE_LZ4, test "x$have_lz4" = "xyes")
> > 

Can you send an updated patch with everything? I'm a bit lost.

Frediano
On Thu, Nov 24, 2016 at 10:43:50AM -0500, Frediano Ziglio wrote:
> > 
> > On Thu, 2016-11-24 at 09:33 -0500, Frediano Ziglio wrote:
> > > > 
> > > > LZ4 changed versioning scheme from r131 to v1.7.3 making our
> > > > configure
> > > > fail with (1.7.3 < 129).
> > > > 
> > > > Switch from version checking to checking that the necessary
> > > > function
> > > > is available.
> > > > ---
> > > > v2: Added some comments, Switched to AC_CHECK_FUNC
> > > > ---
> > > >  m4/spice-deps.m4 | 22 ++++++++++++++++++----
> > > >  1 file changed, 18 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> > > > index adedec4..6827c7f 100644
> > > > --- a/m4/spice-deps.m4
> > > > +++ b/m4/spice-deps.m4
> > > > @@ -185,12 +185,26 @@ AC_DEFUN([SPICE_CHECK_LZ4], [
> > > >  
> > > >      have_lz4="no"
> > > >      if test "x$enable_lz4" != "xno"; then
> > > > -      PKG_CHECK_MODULES([LZ4], [liblz4 >= 129], [have_lz4="yes"],
> > > > [have_lz4="no"])
> > > > +      # LZ4_compress_default is available in liblz4 >= 129,
> > > > however liblz
> > > > has changed
> > > > +      # versioning scheme making the check failing. Rather check
> > > > for
> > > > function definition
> > > > +      PKG_CHECK_MODULES([LZ4], [liblz4], [have_lz4="yes"],
> > > > [have_lz4="no"])
> > > >  
> > > >        if test "x$have_lz4" = "xyes"; then
> > > > -        AC_DEFINE(USE_LZ4, [1], [Define to build with lz4
> > > > support])
> > > > -      elif test "x$enable_lz4" = "xyes"; then
> > > > -        AC_MSG_ERROR([lz4 support requested but liblz4 could not
> > > > be found])
> > > > +        # For cross-compilers may be necessary to save & restore
> > > > LIBS and
> > > > CFLAGS before AC_SEARCH_LIBS
> > > > +        old_LIBS="$LIBS"
> > > > +        old_CFLAGS="$CFLAGS"
> > > > +        CFLAGS="$CFLAGS $LZ4_CFLAGS"
> > > > +        LIBS="$LIBS $LZ4_LIBS"
> > > > +
> > > > +        AC_CHECK_FUNC([LZ4_compress_default], [
> > > > +            AC_DEFINE(USE_LZ4, [1], [Define to build with lz4
> > > > support])],
> > > > +            [have_lz4="no"])
> > > > +
> > > > +        LIBS="$old_LIBS"
> > > > +        CFLAGS="$old_CFLAGS"
> > > > +      fi
> > > > +      if test "x$enable_lz4" = "xyes" && test "x$have_lz4" =
> > > > "xno"; then
> > > 
> > > I think this xyes/xno it's kind of a fix for shell bug quite long
> > > ago, a
> > > 
> > >    if test "$enable_lz4" = yes -a "$have_lz4" = no; then
> > > 
> > > will work. Even [ is more readable leading to
> > > 
> > >    if [ "$enable_lz4" = yes -a "$have_lz4" = no ]; then
> > > 
> > > configure try to use very old style to make sure compatibility is
> > > good
> > > but this lead to really ugly syntax sometimes.
> > 
> > it is a style used in all our configure.ac and m4 files. It may be
> > ugly/not easy to follow but I don't think it is worth spending time on
> > improving it everywhere...
> > 
> 
> I agree but usually when you decide to change a style you
> define the new style and instead of changing everything
> you change using the new style when you change the code.
> 
> By the way, not this not that important, patch is fine
> even with this style. Perhaps the -a usage is compatible
> with current style and more readable than test ... && test ...

-a has portability issues iirc. Does not hurt to stick with the current
test ... && test ... in my opinion.

Christophe