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

[Anthony PERARD <anthony.perard@citrix.com>]

On Thu, Sep 16, 2021 at 05:34:00PM +0200, Jan Beulich wrote:
> 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,

Not really anymore, it's just left over from having put this in Kconfig
in previous version of the patch.

> I'm always suspecting possible name collisions ...

Yes, it's probably better to keep the XEN_ prefix.

> > @@ -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.

How about:

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

.. instead, as that would be more consistent with the previous line,
that is there would be only one branch to the $(if ) and no else, and
thus probably easier to read.

> > +.allconfig.tmp: FORCE
> > +	set -e; { $(call filechk_kconfig_allconfig); } > $@
> 
> Is there a particular reason for the .tmp suffix?

Yes, .*.tmp are already ignored via .gitignore.

Thanks,

-- 
Anthony PERARD