[PATCH] kvmtool: Allow optional libraries to be forced off

[PATCH] kvmtool: Allow optional libraries to be forced off

[Chirantan Ekbote]

The makefile currently automatically detects if certain optional
libraries are present on the file system and links with them if they
are.

This isn't always desired though: users may not want to link against
those libraries even if they do exist.  Add new variables that when set
to 0 will force the corresponding optional library off.

Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>
---
 Makefile | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/Makefile b/Makefile
index eeb54a4..69daa1e 100644
--- a/Makefile
+++ b/Makefile
@@ -250,26 +250,33 @@ ifeq (y,$(ARCH_HAS_FRAMEBUFFER))
 	endif
 endif
 
-ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz),y)
-	CFLAGS_DYNOPT	+= -DCONFIG_HAS_ZLIB
-	LIBS_DYNOPT	+= -lz
-else
-	NOTFOUND	+= zlib
-endif
-ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz -static),y)
-	CFLAGS_STATOPT	+= -DCONFIG_HAS_ZLIB
-	LIBS_STATOPT	+= -lz
-endif
+# Define USE_ZLIB=0 to disable zlib.
+ifneq ($(USE_ZLIB),0)
+	ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz),y)
+		CFLAGS_DYNOPT	+= -DCONFIG_HAS_ZLIB
+		LIBS_DYNOPT	+= -lz
+	else
+		NOTFOUND	+= zlib
+	endif
 
-ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),$(LDFLAGS) -laio),y)
-	CFLAGS_DYNOPT	+= -DCONFIG_HAS_AIO
-	LIBS_DYNOPT	+= -laio
-else
-	NOTFOUND	+= aio
+	ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz -static),y)
+		CFLAGS_STATOPT	+= -DCONFIG_HAS_ZLIB
+		LIBS_STATOPT	+= -lz
+	endif
 endif
-ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),$(LDFLAGS) -laio -static),y)
-	CFLAGS_STATOPT	+= -DCONFIG_HAS_AIO
-	LIBS_STATOPT	+= -laio
+
+# Define USE_AIO=0 to disable libaio.
+ifneq ($(USE_AIO),0)
+	ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),$(LDFLAGS) -laio),y)
+		CFLAGS_DYNOPT	+= -DCONFIG_HAS_AIO
+		LIBS_DYNOPT	+= -laio
+	else
+		NOTFOUND	+= aio
+	endif
+	ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),$(LDFLAGS) -laio -static),y)
+		CFLAGS_STATOPT	+= -DCONFIG_HAS_AIO
+		LIBS_STATOPT	+= -laio
+	endif
 endif
 
 ifeq ($(LTO),1)
-- 
2.11.0.483.g087da7b7c-goog

Re: [PATCH] kvmtool: Allow optional libraries to be forced off

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>

Re: [PATCH] kvmtool: Allow optional libraries to be forced off

[Andre Przywara]

Hi Chirantan,

On 19/01/17 18:46, Chirantan Ekbote wrote:
> The makefile currently automatically detects if certain optional
> libraries are present on the file system and links with them if they
> are.
> 
> This isn't always desired though: users may not want to link against
> those libraries even if they do exist.  Add new variables that when set
> to 0 will force the corresponding optional library off.

thanks for the patch, and I support the idea of being able to turn off
libraries. But shouldn't that actually cover all of them? I am thinking
about gtk3 in particular here.

...

> 
> Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>
> ---
>  Makefile | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index eeb54a4..69daa1e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -250,26 +250,33 @@ ifeq (y,$(ARCH_HAS_FRAMEBUFFER))
>  	endif
>  endif
>  
> -ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz),y)
> -	CFLAGS_DYNOPT	+= -DCONFIG_HAS_ZLIB
> -	LIBS_DYNOPT	+= -lz
> -else
> -	NOTFOUND	+= zlib
> -endif
> -ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz -static),y)
> -	CFLAGS_STATOPT	+= -DCONFIG_HAS_ZLIB
> -	LIBS_STATOPT	+= -lz
> -endif
> +# Define USE_ZLIB=0 to disable zlib.
> +ifneq ($(USE_ZLIB),0)

Mmh, that sounds a bit backwards to me, since we don't really have a
USE_ZLIB=1 case. Wouldn't it be more intuitive to either use
DISABLE_ZLIB or WITHOUT_ZLIB?
Or what about using the variable instead of the "y" at the end of that
ifeq? Define USE_ZLIB=y by default, allow overriding on the cmd line,
then: ifeq ($(call try-build..., $(USE_ZLIB))
Wouldn't that work? That avoids the extra indentation and makes the
patch smaller.

Also: I know that I am getting a bit cheeky here, but this Makefile part
is a bit messy already and doesn't really get better with that patch.
Can't we use this opportunity to make this easier and more readable?
Maybe use some wrappers to consolidate the quite similar parts? Or/and
to cover static and non-static with one case instead of two separate calls?

Cheers,
Andre.

> +	ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz),y)
> +		CFLAGS_DYNOPT	+= -DCONFIG_HAS_ZLIB
> +		LIBS_DYNOPT	+= -lz
> +	else
> +		NOTFOUND	+= zlib
> +	endif
>  
> -ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),$(LDFLAGS) -laio),y)
> -	CFLAGS_DYNOPT	+= -DCONFIG_HAS_AIO
> -	LIBS_DYNOPT	+= -laio
> -else
> -	NOTFOUND	+= aio
> +	ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz -static),y)
> +		CFLAGS_STATOPT	+= -DCONFIG_HAS_ZLIB
> +		LIBS_STATOPT	+= -lz
> +	endif
>  endif
> -ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),$(LDFLAGS) -laio -static),y)
> -	CFLAGS_STATOPT	+= -DCONFIG_HAS_AIO
> -	LIBS_STATOPT	+= -laio
> +
> +# Define USE_AIO=0 to disable libaio.
> +ifneq ($(USE_AIO),0)
> +	ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),$(LDFLAGS) -laio),y)
> +		CFLAGS_DYNOPT	+= -DCONFIG_HAS_AIO
> +		LIBS_DYNOPT	+= -laio
> +	else
> +		NOTFOUND	+= aio
> +	endif
> +	ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),$(LDFLAGS) -laio -static),y)
> +		CFLAGS_STATOPT	+= -DCONFIG_HAS_AIO
> +		LIBS_STATOPT	+= -laio
> +	endif
>  endif
>  
>  ifeq ($(LTO),1)
> 

Re: [PATCH] kvmtool: Allow optional libraries to be forced off

[Mike Frysinger]

On Thu, Feb 2, 2017 at 8:17 AM, Andre Przywara wrote:
> On 19/01/17 18:46, Chirantan Ekbote wrote:
> > The makefile currently automatically detects if certain optional
> > libraries are present on the file system and links with them if they
> > are.
> >
> > This isn't always desired though: users may not want to link against
> > those libraries even if they do exist.  Add new variables that when set
> > to 0 will force the corresponding optional library off.
>
> thanks for the patch, and I support the idea of being able to turn off
> libraries. But shouldn't that actually cover all of them? I am thinking
> about gtk3 in particular here.
>
> ...
>
> >
> > Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>
> > ---
> >  Makefile | 43 +++++++++++++++++++++++++------------------
> >  1 file changed, 25 insertions(+), 18 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index eeb54a4..69daa1e 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -250,26 +250,33 @@ ifeq (y,$(ARCH_HAS_FRAMEBUFFER))
> >       endif
> >  endif
> >
> > -ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz),y)
> > -     CFLAGS_DYNOPT   += -DCONFIG_HAS_ZLIB
> > -     LIBS_DYNOPT     += -lz
> > -else
> > -     NOTFOUND        += zlib
> > -endif
> > -ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz -static),y)
> > -     CFLAGS_STATOPT  += -DCONFIG_HAS_ZLIB
> > -     LIBS_STATOPT    += -lz
> > -endif
> > +# Define USE_ZLIB=0 to disable zlib.
> > +ifneq ($(USE_ZLIB),0)
>
> Mmh, that sounds a bit backwards to me, since we don't really have a
> USE_ZLIB=1 case. Wouldn't it be more intuitive to either use
> DISABLE_ZLIB or WITHOUT_ZLIB?

personally, i find negative variables to be bad juju.  i spend more
time reasoning about things like "if (without_foo)" and "if
(!without_foo)" than i would with "if (foo)" and "if (!foo)".  so i'd
prefer to keep all the variable names positive.  if we want to retain
the defaults, that's easy with something like:
USE_ZLIB ?= y

> Or what about using the variable instead of the "y" at the end of that
> ifeq? Define USE_ZLIB=y by default, allow overriding on the cmd line,
> then: ifeq ($(call try-build..., $(USE_ZLIB))
> Wouldn't that work? That avoids the extra indentation and makes the
> patch smaller.

i think we were just going with something simple/straightforward due
to lack of precedent in this file.  if you have a preferred form,
switching to that shouldn't be a problem.

> Also: I know that I am getting a bit cheeky here, but this Makefile part
> is a bit messy already and doesn't really get better with that patch.
> Can't we use this opportunity to make this easier and more readable?
> Maybe use some wrappers to consolidate the quite similar parts? Or/and
> to cover static and non-static with one case instead of two separate calls?

or convert it to autotools :D
-mike