Re: [XEN PATCH v7 20/51] build: avoid re-executing the main Makefile by introducing build.mk

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.08.2021 12:50, Anthony PERARD wrote:
> Currently, the xen/Makefile is re-parsed several times: once to start
> the build process, and several more time with Rules.mk including it.
> This makes it difficult to reason with a Makefile used for several
> purpose, and it actually slow down the build process.

I'm struggling some with what you want to express here. What does
"to reason" refer to?

> So this patch introduce "build.mk" which Rules.mk will use when
> present instead of the "Makefile" of a directory. (Linux's Kbuild
> named that file "Kbuild".)
> 
> We have a few targets to move to "build.mk" identified by them been
> build via "make -f Rules.mk" without changing directory.
> 
> As for the main targets like "build", we can have them depends on
> there underscore-prefix targets like "_build" without having to use
> "Rules.mk" while still retaining the check for unsupported
> architecture. (Those main rules are changed to be single-colon as
> there should only be a single recipe for them.)
> 
> With nearly everything needed to move to "build.mk" moved, there is a
> single dependency left from "Rules.mk": $(TARGET), which is moved to
> the main Makefile.

I'm having trouble identifying what this describes. Searching for
$(TARGET) in the patch doesn't yield any obvious match. Thinking
about it, do you perhaps mean the setting of that variable? Is
moving that guaranteed to not leave the variable undefined? Or in
other words is there no scenario at all where xen/Makefile might
get bypassed? (Aiui building an individual .o, .i, or .s would
continue to be fine, but it feels like something along these lines
might get broken.)

> @@ -279,11 +281,13 @@ export CFLAGS_UBSAN
>  
>  endif # need-config
>  
> -.PHONY: build install uninstall clean distclean MAP
> -build install uninstall debug clean distclean MAP::
> +main-targets := build install uninstall clean distclean MAP
> +.PHONY: $(main-targets)
>  ifneq ($(XEN_TARGET_ARCH),x86_32)
> -     $(MAKE) -f Rules.mk _$@
> +$(main-targets): %: _%
> +     @:

Isn't the conventional way to express "no commands" via

$(main-targets): %: _% ;

?

> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -9,8 +9,6 @@ include $(XEN_ROOT)/Config.mk
>  include $(BASEDIR)/scripts/Kbuild.include
>  
>  
> -TARGET := $(BASEDIR)/xen
> -
>  # Note that link order matters!

Could I talk you into removing yet another blank line at this occasion?

> @@ -36,7 +34,9 @@ SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \
>                                              rodata.cst$(a)) \
>                           $(foreach r,rel rel.ro,data.$(r).local)
>  
> -include Makefile
> +# The filename build.mk has precedence over Makefile
> +mk-dir := .

What's the goal of this variable? All I can spot for now it that ...

> +include $(if $(wildcard 
> $(mk-dir)/build.mk),$(mk-dir)/build.mk,$(mk-dir)/Makefile)

... this is harder to read than

include $(if $(wildcard ./build.mk),./build.mk,./Makefile)

which could be further simplified to

include $(if $(wildcard build.mk),build.mk,Makefile)

and then maybe altered to

include $(firstword $(wildcard build.mk) Makefile)

> --- /dev/null
> +++ b/xen/build.mk
> @@ -0,0 +1,58 @@
> +quiet_cmd_banner = BANNER  $@
> +define cmd_banner
> +    if which figlet >/dev/null 2>&1 ; then \
> +     echo " Xen $(XEN_FULLVERSION)" | figlet -f $< > $@.tmp; \
> +    else \
> +     echo " Xen $(XEN_FULLVERSION)" > $@.tmp; \
> +    fi; \
> +    mv -f $@.tmp $@
> +endef
> +
> +.banner: tools/xen.flf FORCE
> +     $(call if_changed,banner)
> +targets += .banner

To make the end of the rule more easily recognizable, may I ask that
you either insert a blank line after the rule or that you move the +=
up immediately ahead of the construct?

Jan