[Spice-devel,spice-common] Make LZ4 dependency check more robust

Submitted by Eduardo Lima (Etrunko) on June 15, 2016, 7:35 p.m.

Details

Message ID 1466019317-27820-1-git-send-email-etrunko@redhat.com
State New
Headers show
Series "Make LZ4 dependency check more robust" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Eduardo Lima (Etrunko) June 15, 2016, 7:35 p.m.
Add a new 'have_lz4' variable to really tell if we have the dependency
installed on the system. It will later be used in Makefile to decide
whether or not the specific files related to LZ4 should be built.

Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
---
 m4/spice-deps.m4 | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
index 2e2fcf5..a114e4f 100644
--- a/m4/spice-deps.m4
+++ b/m4/spice-deps.m4
@@ -185,14 +185,18 @@  AC_DEFUN([SPICE_CHECK_LZ4], [
 
     if test "x$enable_lz4" != "xno"; then
       PKG_CHECK_MODULES([LZ4], [liblz4],
-        [enable_lz4=yes
+        [have_lz4="yes"
          AC_DEFINE(USE_LZ4, [1], [Define to build with lz4 support])
         ],
-        [if test "x$enable_lz4" = "xyes"; then
-          AC_MSG_ERROR([lz4 support requested but liblz4 could not be found])
-        fi]
+        [have_lz4="no"
+         if test "x$enable_lz4" = "xyes"; then
+            AC_MSG_ERROR([lz4 support requested but liblz4 could not be found])
+         fi]
       )
+    else
+        have_lz4=no
     fi
+    AM_CONDITIONAL(HAVE_LZ4, test "x$have_lz4" = "xyes")
 ])
 
 

Comments

On Wed, Jun 15, 2016 at 04:35:14PM -0300, Eduardo Lima (Etrunko) wrote:
> Add a new 'have_lz4' variable to really tell if we have the dependency

"Add a new 'HAVE_LZ4' automake conditional"

> installed on the system. It will later be used in Makefile to decide
> whether or not the specific files related to LZ4 should be built.
> 
> Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
> ---
>  m4/spice-deps.m4 | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> index 2e2fcf5..a114e4f 100644
> --- a/m4/spice-deps.m4
> +++ b/m4/spice-deps.m4
> @@ -185,14 +185,18 @@ AC_DEFUN([SPICE_CHECK_LZ4], [
>  
>      if test "x$enable_lz4" != "xno"; then
>        PKG_CHECK_MODULES([LZ4], [liblz4],
> -        [enable_lz4=yes
> +        [have_lz4="yes"
>           AC_DEFINE(USE_LZ4, [1], [Define to build with lz4 support])
>          ],
> -        [if test "x$enable_lz4" = "xyes"; then
> -          AC_MSG_ERROR([lz4 support requested but liblz4 could not be found])
> -        fi]
> +        [have_lz4="no"
> +         if test "x$enable_lz4" = "xyes"; then
> +            AC_MSG_ERROR([lz4 support requested but liblz4 could not be found])
> +         fi]
>        )
> +    else
> +        have_lz4=no
>      fi
> +    AM_CONDITIONAL(HAVE_LZ4, test "x$have_lz4" = "xyes")
>  ])

I think this whole change could just be:

+   AM_CONDITIONAL(HAVE_LZ4, test "x$enable_lz4" = "xyes")

If you prefer to go with an additional have_lz4, I'd prefer to make it
closer to the SPICE_CHECK_SMARTCARD check for consistency reasons
(ie don't nest AC_DEFINE/AC_MSG_ERROR in PKG_CHECK_MODULES() but only
set have_lz4 in PKG_CHECK_MODULES() and do the rest after it).

Christophe
On 06/16/2016 07:03 AM, Christophe Fergeau wrote:
> On Wed, Jun 15, 2016 at 04:35:14PM -0300, Eduardo Lima (Etrunko) wrote:
>> Add a new 'have_lz4' variable to really tell if we have the dependency
> 
> "Add a new 'HAVE_LZ4' automake conditional"
> 
>> installed on the system. It will later be used in Makefile to decide
>> whether or not the specific files related to LZ4 should be built.
>>
>> Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
>> ---
>>  m4/spice-deps.m4 | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
>> index 2e2fcf5..a114e4f 100644
>> --- a/m4/spice-deps.m4
>> +++ b/m4/spice-deps.m4
>> @@ -185,14 +185,18 @@ AC_DEFUN([SPICE_CHECK_LZ4], [
>>  
>>      if test "x$enable_lz4" != "xno"; then
>>        PKG_CHECK_MODULES([LZ4], [liblz4],
>> -        [enable_lz4=yes
>> +        [have_lz4="yes"
>>           AC_DEFINE(USE_LZ4, [1], [Define to build with lz4 support])
>>          ],
>> -        [if test "x$enable_lz4" = "xyes"; then
>> -          AC_MSG_ERROR([lz4 support requested but liblz4 could not be found])
>> -        fi]
>> +        [have_lz4="no"
>> +         if test "x$enable_lz4" = "xyes"; then
>> +            AC_MSG_ERROR([lz4 support requested but liblz4 could not be found])
>> +         fi]
>>        )
>> +    else
>> +        have_lz4=no
>>      fi
>> +    AM_CONDITIONAL(HAVE_LZ4, test "x$have_lz4" = "xyes")
>>  ])
> 
> I think this whole change could just be:
> 
> +   AM_CONDITIONAL(HAVE_LZ4, test "x$enable_lz4" = "xyes")
> 
> If you prefer to go with an additional have_lz4, I'd prefer to make it
> closer to the SPICE_CHECK_SMARTCARD check for consistency reasons
> (ie don't nest AC_DEFINE/AC_MSG_ERROR in PKG_CHECK_MODULES() but only
> set have_lz4 in PKG_CHECK_MODULES() and do the rest after it).

I prefer it this way too. I will follow the smartcard check.
On Thu, Jun 16, 2016 at 10:02:24AM -0300, Eduardo Lima (Etrunko) wrote:
> On 06/16/2016 07:03 AM, Christophe Fergeau wrote:
> > On Wed, Jun 15, 2016 at 04:35:14PM -0300, Eduardo Lima (Etrunko) wrote:
> >> Add a new 'have_lz4' variable to really tell if we have the dependency
> > 
> > "Add a new 'HAVE_LZ4' automake conditional"
> > 
> >> installed on the system. It will later be used in Makefile to decide
> >> whether or not the specific files related to LZ4 should be built.
> >>
> >> Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
> >> ---
> >>  m4/spice-deps.m4 | 12 ++++++++----
> >>  1 file changed, 8 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> >> index 2e2fcf5..a114e4f 100644
> >> --- a/m4/spice-deps.m4
> >> +++ b/m4/spice-deps.m4
> >> @@ -185,14 +185,18 @@ AC_DEFUN([SPICE_CHECK_LZ4], [
> >>  
> >>      if test "x$enable_lz4" != "xno"; then
> >>        PKG_CHECK_MODULES([LZ4], [liblz4],
> >> -        [enable_lz4=yes
> >> +        [have_lz4="yes"
> >>           AC_DEFINE(USE_LZ4, [1], [Define to build with lz4 support])
> >>          ],
> >> -        [if test "x$enable_lz4" = "xyes"; then
> >> -          AC_MSG_ERROR([lz4 support requested but liblz4 could not be found])
> >> -        fi]
> >> +        [have_lz4="no"
> >> +         if test "x$enable_lz4" = "xyes"; then
> >> +            AC_MSG_ERROR([lz4 support requested but liblz4 could not be found])
> >> +         fi]
> >>        )
> >> +    else
> >> +        have_lz4=no
> >>      fi
> >> +    AM_CONDITIONAL(HAVE_LZ4, test "x$have_lz4" = "xyes")
> >>  ])
> > 
> > I think this whole change could just be:
> > 
> > +   AM_CONDITIONAL(HAVE_LZ4, test "x$enable_lz4" = "xyes")
> > 
> > If you prefer to go with an additional have_lz4, I'd prefer to make it
> > closer to the SPICE_CHECK_SMARTCARD check for consistency reasons
> > (ie don't nest AC_DEFINE/AC_MSG_ERROR in PKG_CHECK_MODULES() but only
> > set have_lz4 in PKG_CHECK_MODULES() and do the rest after it).
> 
> I prefer it this way too. I will follow the smartcard check.

One big advantage of making it one line is that you then don't need any
changes to spice-gtk configure.ac output, avoiding Pavel's concerns.

Christophe
On 06/16/2016 10:11 AM, Christophe Fergeau wrote:
> On Thu, Jun 16, 2016 at 10:02:24AM -0300, Eduardo Lima (Etrunko) wrote:
>> On 06/16/2016 07:03 AM, Christophe Fergeau wrote:
>>> On Wed, Jun 15, 2016 at 04:35:14PM -0300, Eduardo Lima (Etrunko) wrote:
>>>> Add a new 'have_lz4' variable to really tell if we have the dependency
>>>
>>> "Add a new 'HAVE_LZ4' automake conditional"
>>>
>>>> installed on the system. It will later be used in Makefile to decide
>>>> whether or not the specific files related to LZ4 should be built.
>>>>
>>>> Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
>>>> ---
>>>>  m4/spice-deps.m4 | 12 ++++++++----
>>>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
>>>> index 2e2fcf5..a114e4f 100644
>>>> --- a/m4/spice-deps.m4
>>>> +++ b/m4/spice-deps.m4
>>>> @@ -185,14 +185,18 @@ AC_DEFUN([SPICE_CHECK_LZ4], [
>>>>  
>>>>      if test "x$enable_lz4" != "xno"; then
>>>>        PKG_CHECK_MODULES([LZ4], [liblz4],
>>>> -        [enable_lz4=yes
>>>> +        [have_lz4="yes"
>>>>           AC_DEFINE(USE_LZ4, [1], [Define to build with lz4 support])
>>>>          ],
>>>> -        [if test "x$enable_lz4" = "xyes"; then
>>>> -          AC_MSG_ERROR([lz4 support requested but liblz4 could not be found])
>>>> -        fi]
>>>> +        [have_lz4="no"
>>>> +         if test "x$enable_lz4" = "xyes"; then
>>>> +            AC_MSG_ERROR([lz4 support requested but liblz4 could not be found])
>>>> +         fi]
>>>>        )
>>>> +    else
>>>> +        have_lz4=no
>>>>      fi
>>>> +    AM_CONDITIONAL(HAVE_LZ4, test "x$have_lz4" = "xyes")
>>>>  ])
>>>
>>> I think this whole change could just be:
>>>
>>> +   AM_CONDITIONAL(HAVE_LZ4, test "x$enable_lz4" = "xyes")
>>>
>>> If you prefer to go with an additional have_lz4, I'd prefer to make it
>>> closer to the SPICE_CHECK_SMARTCARD check for consistency reasons
>>> (ie don't nest AC_DEFINE/AC_MSG_ERROR in PKG_CHECK_MODULES() but only
>>> set have_lz4 in PKG_CHECK_MODULES() and do the rest after it).
>>
>> I prefer it this way too. I will follow the smartcard check.
> 
> One big advantage of making it one line is that you then don't need any
> changes to spice-gtk configure.ac output, avoiding Pavel's concerns.
> 

It would be indeed a big advantage if we did not need it for both
spice-gtk and spice server, but spice-gtk can live without that change
for a while.

By the way, what is the policy of updating the submodule? I thought
whenever we updated it for one repository it should also be updated for
the other one?

> Christophe
>
On Thu, Jun 16, 2016 at 10:22:32AM -0300, Eduardo Lima (Etrunko) wrote:
> By the way, what is the policy of updating the submodule? I thought
> whenever we updated it for one repository it should also be updated for
> the other one?

I don't think there is such a policy, we update it when needed, it's
good to check before making a release that important fixes are not
missing because the submodule is outdated, but apart from that, I don't
think we have a rule of updating both at the same time.

Christophe