Re: [XEN PATCH v3] xen: rework `checkpolicy` detection when using "randconfig"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.09.2021 13:17, Anthony PERARD wrote:
> --- a/Config.mk
> +++ b/Config.mk
> @@ -137,12 +137,6 @@ export XEN_HAS_BUILD_ID=y
>  build_id_linker := --build-id=sha1
>  endif
>  
> -ifndef XEN_HAS_CHECKPOLICY
> -    CHECKPOLICY ?= checkpolicy
> -    XEN_HAS_CHECKPOLICY := $(shell $(CHECKPOLICY) -h 2>&1 | grep -q xen && 
> echo y || echo n)
> -    export XEN_HAS_CHECKPOLICY
> -endif

Is there a particular reason to go from XEN_HAS_CHECKPOLICY to ...

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -17,6 +17,8 @@ export XEN_BUILD_HOST       ?= $(shell hostname)
>  PYTHON_INTERPRETER   := $(word 1,$(shell which python3 python python2 
> 2>/dev/null) python)
>  export PYTHON                ?= $(PYTHON_INTERPRETER)
>  
> +export CHECKPOLICY   ?= checkpolicy
> +
>  export BASEDIR := $(CURDIR)
>  export XEN_ROOT := $(BASEDIR)/..
>  
> @@ -178,6 +180,8 @@ CFLAGS += $(CLANG_FLAGS)
>  export CLANG_FLAGS
>  endif
>  
> +export HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q 
> xen)

... HAS_CHECKPOLICY? As soon as things get put in the environment,
I'm always suspecting possible name collisions ...

> @@ -189,14 +193,24 @@ ifeq ($(config-build),y)
>  # *config targets only - make sure prerequisites are updated, and descend
>  # in tools/kconfig to make the *config target
>  
> +# Create a file for KCONFIG_ALLCONFIG which depends on the environment.
> +# This will be use by kconfig targets 
> allyesconfig/allmodconfig/allnoconfig/randconfig
> +filechk_kconfig_allconfig = \
> +    $(if $(findstring n,$(HAS_CHECKPOLICY)),echo 
> 'CONFIG_XSM_FLASK_POLICY=n';) \
> +    $(if $(KCONFIG_ALLCONFIG), cat $(KCONFIG_ALLCONFIG), :)

Nit: It would be nice if you were consistent with the blanks after
commas in $(if ...). Personally I'm also considering $(if ...)s the
more difficult to follow the longer they are. Hence for the 2nd one
I wonder whether

    $(if $(KCONFIG_ALLCONFIG),cat,:) $(KCONFIG_ALLCONFIG)

wouldn't be easier to read.

> +
> +

Nit: Please avoid double blank lines.

> +.allconfig.tmp: FORCE
> +     set -e; { $(call filechk_kconfig_allconfig); } > $@

Is there a particular reason for the .tmp suffix?

Jan