| Message ID | 20210824105038.1257926-21-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Build system improvements, now with out-of-tree build! | expand |
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
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,
On 11.10.2021 16:58, Anthony PERARD wrote: > 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". Okay, thanks, this confirms the understanding I had developed; maybe you try to reword this some. What your reply doesn't address is my question, though. Jan
On Mon, Oct 11, 2021 at 05:31:08PM +0200, Jan Beulich wrote: > On 11.10.2021 16:58, Anthony PERARD wrote: > > On Mon, Oct 11, 2021 at 12:56:54PM +0200, Jan Beulich wrote: > >> On 24.08.2021 12:50, Anthony PERARD wrote: > >>> 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". > > Okay, thanks, this confirms the understanding I had developed; maybe > you try to reword this some. What your reply doesn't address is my > question, though. Which question, there seems to be 3 of them :-). Is it about xen/Makefile been bypassed? Building anything in xen/ requires to first execute xen/Makefile as this is where variables like CFLAGS or XEN_ROOT are defined, and this includes single builds or building an individual .o.
On 11.10.2021 19:07, Anthony PERARD wrote: > On Mon, Oct 11, 2021 at 05:31:08PM +0200, Jan Beulich wrote: >> On 11.10.2021 16:58, Anthony PERARD wrote: >>> On Mon, Oct 11, 2021 at 12:56:54PM +0200, Jan Beulich wrote: >>>> On 24.08.2021 12:50, Anthony PERARD wrote: >>>>> 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". >> >> Okay, thanks, this confirms the understanding I had developed; maybe >> you try to reword this some. What your reply doesn't address is my >> question, though. > > Which question, there seems to be 3 of them :-). Is it about > xen/Makefile been bypassed? Yes, sorry for being imprecise. > Building anything in xen/ requires to first execute xen/Makefile as this > is where variables like CFLAGS or XEN_ROOT are defined, and this > includes single builds or building an individual .o. Okay, thanks for confirming. Jan
diff --git a/xen/Makefile b/xen/Makefile index b408d35b4af0..7f100845cdd0 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -45,6 +45,8 @@ export KCONFIG_CONFIG ?= .config export CC CXX LD +export TARGET := $(BASEDIR)/xen + .PHONY: default default: build @@ -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): %: _% + @: else +$(main-targets): echo "*** Xen x86/32 target no longer supported!" endif @@ -391,65 +395,6 @@ $(TARGET): FORCE $(MAKE) -f $(BASEDIR)/Rules.mk arch/$(TARGET_ARCH)/include/asm/asm-offsets.h $(MAKE) -f $(BASEDIR)/Rules.mk -C arch/$(TARGET_ARCH) $@ -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 - -# Don't refresh this files during e.g., 'sudo make install' -quiet_cmd_compile.h = UPD $@ -define cmd_compile.h - if [ ! -r $@ -o -O $@ ]; then \ - sed -e 's/@@date@@/$(XEN_BUILD_DATE)/g' \ - -e 's/@@time@@/$(XEN_BUILD_TIME)/g' \ - -e 's/@@whoami@@/$(XEN_WHOAMI)/g' \ - -e 's/@@domain@@/$(XEN_DOMAIN)/g' \ - -e 's/@@hostname@@/$(XEN_BUILD_HOST)/g' \ - -e 's!@@compiler@@!$(shell $(CC) $(CFLAGS) --version 2>&1 | head -1)!g' \ - -e 's/@@version@@/$(XEN_VERSION)/g' \ - -e 's/@@subversion@@/$(XEN_SUBVERSION)/g' \ - -e 's/@@extraversion@@/$(XEN_EXTRAVERSION)/g' \ - -e 's!@@changeset@@!$(shell tools/scmversion $(XEN_ROOT) || echo "unavailable")!g' \ - < $< > $(dot-target).tmp; \ - sed -rf tools/process-banner.sed < .banner >> $(dot-target).tmp; \ - mv -f $(dot-target).tmp $@; \ - fi -endef - -include/xen/compile.h: include/xen/compile.h.in .banner FORCE - @cat .banner - $(call if_changed,compile.h) -targets += include/xen/compile.h - -asm-offsets.s: arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.c - $(CC) $(call cpp_flags,$(c_flags)) -S -g0 -o $@.new -MQ $@ $< - $(call move-if-changed,$@.new,$@) - -arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: asm-offsets.s - @(set -e; \ - echo "/*"; \ - echo " * DO NOT MODIFY."; \ - echo " *"; \ - echo " * This file was auto-generated from $<"; \ - echo " *"; \ - echo " */"; \ - echo ""; \ - echo "#ifndef __ASM_OFFSETS_H__"; \ - echo "#define __ASM_OFFSETS_H__"; \ - echo ""; \ - sed -rne "/^[^#].*==>/{s:.*==>(.*)<==.*:\1:; s: [\$$#]: :; p;}"; \ - echo ""; \ - echo "#endif") <$< >$@ - SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test define all_sources ( find arch/$(TARGET_ARCH)/include -name '*.h' -print; \ diff --git a/xen/Rules.mk b/xen/Rules.mk index f449669475bd..a49ca4ceca39 100644 --- 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! ALL_OBJS-y += $(BASEDIR)/common/built_in.o ALL_OBJS-y += $(BASEDIR)/drivers/built_in.o @@ -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 := . +include $(if $(wildcard $(mk-dir)/build.mk),$(mk-dir)/build.mk,$(mk-dir)/Makefile) # Linking # --------------------------------------------------------------------------- diff --git a/xen/build.mk b/xen/build.mk new file mode 100644 index 000000000000..369e1fe5c698 --- /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 + +# Don't refresh this files during e.g., 'sudo make install' +quiet_cmd_compile.h = UPD $@ +define cmd_compile.h + if [ ! -r $@ -o -O $@ ]; then \ + sed -e 's/@@date@@/$(XEN_BUILD_DATE)/g' \ + -e 's/@@time@@/$(XEN_BUILD_TIME)/g' \ + -e 's/@@whoami@@/$(XEN_WHOAMI)/g' \ + -e 's/@@domain@@/$(XEN_DOMAIN)/g' \ + -e 's/@@hostname@@/$(XEN_BUILD_HOST)/g' \ + -e 's!@@compiler@@!$(shell $(CC) $(CFLAGS) --version 2>&1 | head -1)!g' \ + -e 's/@@version@@/$(XEN_VERSION)/g' \ + -e 's/@@subversion@@/$(XEN_SUBVERSION)/g' \ + -e 's/@@extraversion@@/$(XEN_EXTRAVERSION)/g' \ + -e 's!@@changeset@@!$(shell tools/scmversion $(XEN_ROOT) || echo "unavailable")!g' \ + < $< > $(dot-target).tmp; \ + sed -rf tools/process-banner.sed < .banner >> $(dot-target).tmp; \ + mv -f $(dot-target).tmp $@; \ + fi +endef + +include/xen/compile.h: include/xen/compile.h.in .banner FORCE + @cat .banner + $(call if_changed,compile.h) +targets += include/xen/compile.h + +asm-offsets.s: arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.c + $(CC) $(call cpp_flags,$(c_flags)) -S -g0 -o $@.new -MQ $@ $< + $(call move-if-changed,$@.new,$@) + +arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: asm-offsets.s + @(set -e; \ + echo "/*"; \ + echo " * DO NOT MODIFY."; \ + echo " *"; \ + echo " * This file was auto-generated from $<"; \ + echo " *"; \ + echo " */"; \ + echo ""; \ + echo "#ifndef __ASM_OFFSETS_H__"; \ + echo "#define __ASM_OFFSETS_H__"; \ + echo ""; \ + sed -rne "/^[^#].*==>/{s:.*==>(.*)<==.*:\1:; s: [\$$#]: :; p;}"; \ + echo ""; \ + echo "#endif") <$< >$@
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. 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. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Notes: v7: - new patch xen/Makefile | 69 ++++++---------------------------------------------- xen/Rules.mk | 6 ++--- xen/build.mk | 58 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 65 deletions(-) create mode 100644 xen/build.mk