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

[Andre Przywara <andre.przywara@arm.com>]

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)
>