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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, Oct 11, 2021 at 12:56:54PM +0200, Jan Beulich wrote:
> 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?

I guess "to reason with something" isn't an expression. I mean that the
main Makefile is difficult to work with as it setup the build process
for the rest of the build. And it is difficult to understand what is
happening when it recursed into itself, and thus possibly re-executing
part of the build process setup.

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

I mean that "xen/Rules.mk" will never "include" "xen/Makefile" after
this patch, but the variable "TARGET" is only set in "xen/Rules.mk". But
"xen/Makefile" still needs "TARGET" to be set so I moved the assignment
of the variable from "xen/Rules.mk" into "xen/Makefile".

> > @@ -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): %: _% ;
> 
> ?

I guess, I'll change it.

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

Will do.

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

It's just me thinking ahead where a folling patch will just have to
write "$mk-dir := $(src)", instead of editing the following line.

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

I can try with this last one, there is less repeating of "build.mk" so
that sound like a good thing to do.

> > --- /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?

Will do.

Thanks,

-- 
Anthony PERARD