Re: [XEN PATCH v7 49/51] build: adding out-of-tree support to the xen build

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.08.2021 12:50, Anthony PERARD wrote:
> This implement out-of-tree support, there's two ways to create an
> out-of-tree build tree (after that, `make` in that new directory
> works):
>     make O=build
>     mkdir build; cd build; make -f ../Makefile
> also works with an absolute path for both.
> 
> This implementation only works if the source tree is clean, as we use
> VPATH.
> 
> This patch copies most new code with handling out-of-tree build from
> Linux v5.12.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
>  xen/Makefile                | 173 +++++++++++++++++++++++++++++-------
>  xen/Rules.mk                |  11 ++-
>  xen/arch/arm/efi/Makefile   |   3 +
>  xen/arch/x86/arch.mk        |   3 +
>  xen/arch/x86/boot/Makefile  |   5 +-
>  xen/arch/x86/efi/Makefile   |   3 +
>  xen/include/Makefile        |   9 +-
>  xen/scripts/mkmakefile      |  17 ++++
>  xen/test/livepatch/Makefile |   2 +
>  xen/xsm/flask/Makefile      |   3 +
>  xen/xsm/flask/ss/Makefile   |   3 +
>  11 files changed, 194 insertions(+), 38 deletions(-)
>  create mode 100755 xen/scripts/mkmakefile

Linux have done away with this script just recently; I don't think we
should introduce it.

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -1,3 +1,7 @@
> +# $(lastword,) for GNU Make older than 0.81

DYM 3.81?

> +lastword = $(word $(words $(1)),$(1))
> +this-makefile := $(call lastword,$(MAKEFILE_LIST))

Oh - is it documented somewhere that this would always be last?

> @@ -17,33 +21,18 @@ export XEN_BUILD_HOST     ?= $(shell hostname)
>  PYTHON_INTERPRETER   := $(word 1,$(shell which python3 python python2 
> 2>/dev/null) python)
>  export PYTHON                ?= $(PYTHON_INTERPRETER)
>  
> -export XEN_ROOT := $(CURDIR)/..
> +$(if $(filter __%, $(MAKECMDGOALS)), \
> +     $(error targets prefixed with '__' are only for internal use))
>  
> -srctree := .
> -objtree := .
> -export srctree objtree
> +# That's our default target when none is given on the command line
> +PHONY := __all
> +__all:
>  
>  # Do not use make's built-in rules and variables
>  MAKEFLAGS += -rR
>  
>  EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi
>  
> -ARCH=$(XEN_TARGET_ARCH)
> -SRCARCH=$(shell echo $(ARCH) | \
> -          sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
> -              -e s'/riscv.*/riscv/g')
> -export ARCH SRCARCH
> -
> -# Don't break if the build process wasn't called from the top level
> -# we need XEN_TARGET_ARCH to generate the proper config
> -include $(XEN_ROOT)/Config.mk
> -
> -# Set ARCH/SUBARCH appropriately.
> -export TARGET_SUBARCH  := $(XEN_TARGET_ARCH)
> -export TARGET_ARCH     := $(shell echo $(XEN_TARGET_ARCH) | \
> -                            sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' 
> \
> -                                -e s'/riscv.*/riscv/g')
> -
>  # Allow someone to change their config file
>  export KCONFIG_CONFIG ?= .config

Would it be possible to split off some of the pure movement of pieces,
to make it easier to see what (if anything) is actually changed at the
same time as getting moved?

> @@ -51,14 +40,9 @@ export CC CXX LD
>  
>  export TARGET := xen
>  
> -.PHONY: default
> -default: build
> -
>  .PHONY: dist
>  dist: install
>  
> -include scripts/Kbuild.include
> -
>  ifneq ($(root-make-done),y)
>  # section to run before calling Rules.mk, but only once.
>  
> @@ -130,14 +114,93 @@ endif
>  
>  ifneq ($(findstring s,$(filter-out --%,$(MAKEFLAGS))),)
>      quiet := silent_
> +    KBUILD_VERBOSE = 0
>  endif

In how far is this related here? Doesn't this belong in an earlier patch?

>  export quiet Q KBUILD_VERBOSE
>  
> +# $(realpath,) for GNU Make older than 0.81

Again - 3.81?

> +ifeq ($(abs_srctree),$(abs_objtree))
> +        # building in the source tree
> +        srctree := .
> +     building_out_of_srctree :=
> +else
> +        ifeq ($(abs_srctree)/,$(dir $(abs_objtree)))
> +                # building in a subdirectory of the source tree
> +                srctree := ..
> +        else
> +                srctree := $(abs_srctree)
> +        endif
> +     building_out_of_srctree := 1
> +endif
> +
> +objtree              := .
> +VPATH                := $(srctree)

Would you mind using blank padding here, and perhaps fewer blanks than
the tabs are worth?

> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -38,7 +38,7 @@ SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \
>                           $(foreach r,rel rel.ro,data.$(r).local)
>  
>  # The filename build.mk has precedence over Makefile
> -mk-dir := $(src)
> +mk-dir := $(srctree)/$(src)
>  include $(if $(wildcard 
> $(mk-dir)/build.mk),$(mk-dir)/build.mk,$(mk-dir)/Makefile)

Perhaps already when it was changed to $(src) the name has become
slightly misleading, at least imo: I would rather expect a variable
with this name to refer to the build dir/tree. Maybe "srcdir" or
even shorted "sd" right from the start? (Reaching here I can finally
see why having a shorthand is helpful.)

> --- a/xen/arch/x86/efi/Makefile
> +++ b/xen/arch/x86/efi/Makefile
> @@ -1,5 +1,8 @@
>  CFLAGS-y += -fshort-wchar
>  CFLAGS-y += -I$(srctree)/common/efi
> +ifdef building_out_of_srctree
> +CFLAGS-y += -I$(srctree)/$(src)
> +endif

At the example of this (where perhaps -iquote could be used again) - is
it strictly necessary to have the ifdef-s around such? I.e. would things
fail to work for an in-tree build without them?

> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -110,7 +110,7 @@ $(obj)/headers99.chk: $(PUBLIC_C99_HEADERS) 
> $(src)/Makefile
>       $(foreach i, $(filter %.h,$^),                                        \
>           echo "#include "\"$(i)\"                                          \
>           | $(CC) -x c -std=c99 -Wall -Werror                               \
> -           -include stdint.h $(foreach j, $($(i)-prereq), -include $(j).h) \
> +           -include stdint.h $(foreach j, $($(patsubst 
> $(srctree)/%,%,$i)-prereq), -include $(j).h) \

Please split the $(foreach ...) from the earlier -include.

> @@ -124,8 +124,8 @@ $(obj)/headers++.chk: $(PUBLIC_HEADERS) $(src)/Makefile
>       $(foreach i, $(filter %.h,$^),                                        \
>           echo "#include "\"$(i)\"                                          \
>           | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__        \
> -           -include stdint.h -include $(src)/public/xen.h                  \
> -           $(foreach j, $($(i)-prereq), -include c$(j)) -S -o /dev/null -  \
> +           -include stdint.h -include $(srctree)/$(src)/public/xen.h       \
> +           $(foreach j, $($(patsubst $(srctree)/%,%,$i)-prereq), -include 
> c$(j)) -S -o /dev/null -  \

Similarly here please split -S ... onto the next line.

> --- a/xen/xsm/flask/ss/Makefile
> +++ b/xen/xsm/flask/ss/Makefile
> @@ -9,3 +9,6 @@ obj-y += conditional.o
>  obj-y += mls.o
>  
>  CFLAGS-y += -I$(srctree)/xsm/flask/include
> +ifdef building_out_of_srctree
> +    CFLAGS-y += -I$(objtree)/xsm/flask/include

There's no header in $(srctree)/xsm/flask/include in this case, so if you
use "ifdef" here, shouldn't that other part move into an "else"?

Jan