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

Submitted by Pavel Grunt on Nov. 24, 2016, 8:57 a.m.

Details

Message ID 20161124085739.23168-1-pgrunt@redhat.com
State New
Headers show
Series "spice-deps: Make LZ4 check depending on function" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Pavel Grunt Nov. 24, 2016, 8:57 a.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.
---
 m4/spice-deps.m4 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
index adedec4..43a7d4a 100644
--- a/m4/spice-deps.m4
+++ b/m4/spice-deps.m4
@@ -185,12 +185,14 @@  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"])
+      PKG_CHECK_MODULES([LZ4], [liblz4], [
+          AC_SEARCH_LIBS([LZ4_compress_default], [lz4], [have_lz4="yes"], [have_lz4="no"])],
+          [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])
+        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

> 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.
> ---
>  m4/spice-deps.m4 | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> index adedec4..43a7d4a 100644
> --- a/m4/spice-deps.m4
> +++ b/m4/spice-deps.m4
> @@ -185,12 +185,14 @@ 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"])
> +      PKG_CHECK_MODULES([LZ4], [liblz4], [
> +          AC_SEARCH_LIBS([LZ4_compress_default], [lz4], [have_lz4="yes"],
> [have_lz4="no"])],
> +          [have_lz4="no"])
>  

have_lz4 by default is defined no so you could remove the assignment
to "no", but that's not a problem.

Using AC_SEARCH_LIBS and [lz4] as library you are assuming you can
link lz4 library with -llz4 (more or less) but this can be a problem
if you are using MingW or another cross compiler that needs other
options. Maybe this is hidden by the fact that we just require a
library path and is already included by another library.
Did you try to cross compile the client for Windows?

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

Beside the cross compilation issue patch works for me.

Frediano
On Thu, 2016-11-24 at 05:18 -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.
> > ---
> >  m4/spice-deps.m4 | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> > index adedec4..43a7d4a 100644
> > --- a/m4/spice-deps.m4
> > +++ b/m4/spice-deps.m4
> > @@ -185,12 +185,14 @@ 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"])
> > +      PKG_CHECK_MODULES([LZ4], [liblz4], [
> > +          AC_SEARCH_LIBS([LZ4_compress_default], [lz4],
> > [have_lz4="yes"],
> > [have_lz4="no"])],
> > +          [have_lz4="no"])
> >  
> 
> have_lz4 by default is defined no so you could remove the assignment
> to "no", but that's not a problem.

ok

> 
> Using AC_SEARCH_LIBS and [lz4] as library you are assuming you can
> link lz4 library with -llz4 (more or less) but this can be a problem
> if you are using MingW or another cross compiler that needs other
> options. Maybe this is hidden by the fact that we just require a
> library path and is already included by another library.
> Did you try to cross compile the client for Windows?

lz4 is not available for mingw so I haven't tried it

> 
> >        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])
> > +        AC_MSG_ERROR([lz4 support requested but liblz4 >= 129
> > could not be
> > found])
> >        fi
> >      fi
> >      AM_CONDITIONAL(HAVE_LZ4, test "x$have_lz4" = "xyes")
> 
> Beside the cross compilation issue patch works for me.
> 
> Frediano
> 
> On Thu, 2016-11-24 at 05:18 -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.
> > > ---
> > >  m4/spice-deps.m4 | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> > > index adedec4..43a7d4a 100644
> > > --- a/m4/spice-deps.m4
> > > +++ b/m4/spice-deps.m4
> > > @@ -185,12 +185,14 @@ 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"])
> > > +      PKG_CHECK_MODULES([LZ4], [liblz4], [
> > > +          AC_SEARCH_LIBS([LZ4_compress_default], [lz4],
> > > [have_lz4="yes"],
> > > [have_lz4="no"])],
> > > +          [have_lz4="no"])
> > >  
> > 
> > have_lz4 by default is defined no so you could remove the assignment
> > to "no", but that's not a problem.
> 
> ok
> 
> > 
> > Using AC_SEARCH_LIBS and [lz4] as library you are assuming you can
> > link lz4 library with -llz4 (more or less) but this can be a problem
> > if you are using MingW or another cross compiler that needs other
> > options. Maybe this is hidden by the fact that we just require a
> > library path and is already included by another library.
> > Did you try to cross compile the client for Windows?
> 
> lz4 is not available for mingw so I haven't tried it
>

Mumble... but probably is worth adding in the future.
As far as I know the code should be something like
https://cgit.freedesktop.org/spice/spice-gtk/commit/?id=f697933524b19186cb078b5fca43f748fd68cf9f
replacing LIBS and CFLAGS temporarily before doing the function
check but it's quite ugly (well, lot of configure code looks ugly
to me...)
 
> > 
> > >        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])
> > > +        AC_MSG_ERROR([lz4 support requested but liblz4 >= 129
> > > could not be
> > > found])
> > >        fi
> > >      fi
> > >      AM_CONDITIONAL(HAVE_LZ4, test "x$have_lz4" = "xyes")
> > 
> > Beside the cross compilation issue patch works for me.
> > 

Frediano
On Thu, 2016-11-24 at 06:25 -0500, Frediano Ziglio wrote:
> > 
> > On Thu, 2016-11-24 at 05:18 -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.
> > > > ---
> > > >  m4/spice-deps.m4 | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> > > > index adedec4..43a7d4a 100644
> > > > --- a/m4/spice-deps.m4
> > > > +++ b/m4/spice-deps.m4
> > > > @@ -185,12 +185,14 @@ 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"])
> > > > +      PKG_CHECK_MODULES([LZ4], [liblz4], [
> > > > +          AC_SEARCH_LIBS([LZ4_compress_default], [lz4],
> > > > [have_lz4="yes"],
> > > > [have_lz4="no"])],
> > > > +          [have_lz4="no"])
> > > >  
> > > 
> > > have_lz4 by default is defined no so you could remove the
> > > assignment
> > > to "no", but that's not a problem.
> > 
> > ok
> > 
> > > 
> > > Using AC_SEARCH_LIBS and [lz4] as library you are assuming you
> > > can
> > > link lz4 library with -llz4 (more or less) but this can be a
> > > problem
> > > if you are using MingW or another cross compiler that needs
> > > other
> > > options. Maybe this is hidden by the fact that we just require a
> > > library path and is already included by another library.
> > > Did you try to cross compile the client for Windows?
> > 
> > lz4 is not available for mingw so I haven't tried it
> > 
> 
> Mumble... but probably is worth adding in the future.
> As far as I know the code should be something like
> https://cgit.freedesktop.org/spice/spice-gtk/commit/?id=f697933524b1
> 9186cb078b5fca43f748fd68cf9f
> replacing LIBS and CFLAGS temporarily before doing the function
> check but it's quite ugly (well, lot of configure code looks ugly
> to me...)
>  
not a bad idea, i will do it

> > > 
> > > >        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])
> > > > +        AC_MSG_ERROR([lz4 support requested but liblz4 >= 129
> > > > could not be
> > > > found])
> > > >        fi
> > > >      fi
> > > >      AM_CONDITIONAL(HAVE_LZ4, test "x$have_lz4" = "xyes")
> > > 
> > > Beside the cross compilation issue patch works for me.
> > > 
> 
> Frediano
On Thu, Nov 24, 2016 at 09:57:39AM +0100, Pavel Grunt wrote:
> LZ4 changed versioning scheme from r131 to v1.7.3 making our configure
> fail with (1.7.3 < 129).
> 

Having versioning going backwards is a huge mistake from upstream imo.
Has there been a bug report or a discussion about this change in
lz4-land?

Christophe
On Thu, 2016-11-24 at 12:41 +0100, Christophe Fergeau wrote:
> On Thu, Nov 24, 2016 at 09:57:39AM +0100, Pavel Grunt wrote:
> > LZ4 changed versioning scheme from r131 to v1.7.3 making our
> > configure
> > fail with (1.7.3 < 129).
> > 
> 
> Having versioning going backwards is a huge mistake from upstream
> imo.
> Has there been a bug report or a discussion about this change in
> lz4-land?

Not really, the new scheme matches the cli version scheme.

https://github.com/lz4/lz4/issues/224#issuecomment-247760318

> 
> Christophe
On Thu, Nov 24, 2016 at 12:53:01PM +0100, Pavel Grunt wrote:
> On Thu, 2016-11-24 at 12:41 +0100, Christophe Fergeau wrote:
> > On Thu, Nov 24, 2016 at 09:57:39AM +0100, Pavel Grunt wrote:
> > > LZ4 changed versioning scheme from r131 to v1.7.3 making our
> > > configure
> > > fail with (1.7.3 < 129).
> > > 
> > 
> > Having versioning going backwards is a huge mistake from upstream
> > imo.
> > Has there been a bug report or a discussion about this change in
> > lz4-land?
> 
> Not really, the new scheme matches the cli version scheme.
> 
> https://github.com/lz4/lz4/issues/224#issuecomment-247760318

Well, that's not a very compelling reason imo ;) The fedora package
updates properly, but it seems it's more luck than by design (the
package version was r131, rather than 131, not sure this was
intentional).

Christophe
On 11/24/2016 10:57 AM, 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.

Ack.

Uri.

> ---
>  m4/spice-deps.m4 | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> index adedec4..43a7d4a 100644
> --- a/m4/spice-deps.m4
> +++ b/m4/spice-deps.m4
> @@ -185,12 +185,14 @@ 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"])
> +      PKG_CHECK_MODULES([LZ4], [liblz4], [
> +          AC_SEARCH_LIBS([LZ4_compress_default], [lz4], [have_lz4="yes"], [have_lz4="no"])],
> +          [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])
> +        AC_MSG_ERROR([lz4 support requested but liblz4 >= 129 could not be found])
>        fi
>      fi
>      AM_CONDITIONAL(HAVE_LZ4, test "x$have_lz4" = "xyes")
>