| Message ID | 20200117105358.607910-5-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: Build system improvements | expand |
On 17.01.2020 11:53, Anthony PERARD wrote: > From: Anthony PERARD <anthony.perard@gmail.com> > > Most of the code executed by Rules.mk isn't necessary for the clean > target, especially not the CFLAGS. This make running make clean much > faster. > > This extract the code into a different Makefile. It doesn't want to > include Config.mk either so variables DEPS_RM and DEPS_INCLUDE are > extracted from Config.mk as well. DEPS_INCLUDE is put into > Kbuild.include so it could be use by other Makefiles. "extracted" makes it sound as if the intention was to move things, yet ... > --- > xen/Rules.mk | 13 ------------- > xen/scripts/Kbuild.include | 7 ++++++- > xen/scripts/Makefile.clean | 33 +++++++++++++++++++++++++++++++++ > 3 files changed, 39 insertions(+), 14 deletions(-) ... ./Config.mk doesn't get touched at all. I guess there are reasons for this, but I consider it dangerous to leave independent definitions of the same variables in disconnected places. What if one side gets updated without noticing the other? > --- /dev/null > +++ b/xen/scripts/Makefile.clean > @@ -0,0 +1,33 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# ========================================================================== > +# Cleaning up > +# ========================================================================== > + > +clean:: > + > +include $(BASEDIR)/scripts/Kbuild.include > + > +include Makefile > + > +# Figure out what we need to build from the various variables s/build/clean/ ? > +# ========================================================================== > +__subdir-y := $(filter %/, $(obj-y)) > +subdir-y += $(__subdir-y) > +subdir-n := $(subdir-n) $(subdir-) \ > + $(filter %/, $(obj-n) $(obj-)) > +subdir-all := $(subdir-y) $(subdir-n) Same remark as for the earlier patch regarding __subdir-y and the use of tabs. Additionally please use consistent indentation of := / += within this block. Jan
On Wed, Jan 29, 2020 at 03:30:19PM +0100, Jan Beulich wrote: > On 17.01.2020 11:53, Anthony PERARD wrote: > > From: Anthony PERARD <anthony.perard@gmail.com> > > > > Most of the code executed by Rules.mk isn't necessary for the clean > > target, especially not the CFLAGS. This make running make clean much > > faster. > > > > This extract the code into a different Makefile. It doesn't want to > > include Config.mk either so variables DEPS_RM and DEPS_INCLUDE are > > extracted from Config.mk as well. DEPS_INCLUDE is put into > > Kbuild.include so it could be use by other Makefiles. > > "extracted" makes it sound as if the intention was to move things, > yet ... > > > --- > > xen/Rules.mk | 13 ------------- > > xen/scripts/Kbuild.include | 7 ++++++- > > xen/scripts/Makefile.clean | 33 +++++++++++++++++++++++++++++++++ > > 3 files changed, 39 insertions(+), 14 deletions(-) > > ... ./Config.mk doesn't get touched at all. I guess there are reasons > for this, but I consider it dangerous to leave independent definitions > of the same variables in disconnected places. What if one side gets > updated without noticing the other? I guess the word "extracted" is the wrong one. I'll need to rewrite the patch commentary. As for why Config.mk isn't change, it's because it is used by both the hypervisor makefiles and the tools makefiles. I would like for recursive makefiles to not include Config.mk anymore, so having only xen/Makefile doing that include. (I would like to go further and not used Config.mk anymore, but that might not be necessary.) As for the last point, the variables DEPS_RM and DEPS_INCLUDE are copied because Makefile.clean doesn't have them and at some point Rules.mk (no patch yet) isn't going to have them either, so there will be a single location which is Kbuild.include. Currently with this patch, both variables from Kbuild.include are the one used by Rules.mk, so it doesn't matter if Config.mk is modified. Things doesn't look great yet, but it doesn't feel like there are better way to refactor the build system. > > --- /dev/null > > +++ b/xen/scripts/Makefile.clean > > +# Figure out what we need to build from the various variables > > s/build/clean/ ? Yep, I just copy the typo from Linux. But I can fix it in our repo. Thanks,
On 30.01.2020 19:10, Anthony PERARD wrote: > On Wed, Jan 29, 2020 at 03:30:19PM +0100, Jan Beulich wrote: >> On 17.01.2020 11:53, Anthony PERARD wrote: >>> From: Anthony PERARD <anthony.perard@gmail.com> >>> >>> Most of the code executed by Rules.mk isn't necessary for the clean >>> target, especially not the CFLAGS. This make running make clean much >>> faster. >>> >>> This extract the code into a different Makefile. It doesn't want to >>> include Config.mk either so variables DEPS_RM and DEPS_INCLUDE are >>> extracted from Config.mk as well. DEPS_INCLUDE is put into >>> Kbuild.include so it could be use by other Makefiles. >> >> "extracted" makes it sound as if the intention was to move things, >> yet ... >> >>> --- >>> xen/Rules.mk | 13 ------------- >>> xen/scripts/Kbuild.include | 7 ++++++- >>> xen/scripts/Makefile.clean | 33 +++++++++++++++++++++++++++++++++ >>> 3 files changed, 39 insertions(+), 14 deletions(-) >> >> ... ./Config.mk doesn't get touched at all. I guess there are reasons >> for this, but I consider it dangerous to leave independent definitions >> of the same variables in disconnected places. What if one side gets >> updated without noticing the other? > > I guess the word "extracted" is the wrong one. I'll need to rewrite the > patch commentary. > > As for why Config.mk isn't change, it's because it is used by both the > hypervisor makefiles and the tools makefiles. I would like for recursive > makefiles to not include Config.mk anymore, so having only xen/Makefile > doing that include. (I would like to go further and not used Config.mk > anymore, but that might not be necessary.) > > As for the last point, the variables DEPS_RM and DEPS_INCLUDE are copied > because Makefile.clean doesn't have them and at some point Rules.mk (no > patch yet) isn't going to have them either, so there will be a single > location which is Kbuild.include. Currently with this patch, both > variables from Kbuild.include are the one used by Rules.mk, so it > doesn't matter if Config.mk is modified. But with Config.mk still getting included from elsewhere underneath xen/, this is going to be confusing. Changing the Kbuild.include instances should really affect the entire xen/ tree then, at which point the Config.mk instances could be declared "for everything else" (and eventually be moved into the subtrees). I agree it's not very helpful that Config.mk contains not only common definitions, but also ones actually controlling how certain parts of the build process work (like the two DEPS_*; going through the file I wonder whether these two are actually the only outliers). > Things doesn't look great yet, but it doesn't feel like there are better > way to refactor the build system. Right, an incremental switch of the build machinery is going to run into oddities. Calling them out explicitly (including what the plan is to resolve them by the end of the transformation process) would be helpful, though. Jan
diff --git a/xen/Rules.mk b/xen/Rules.mk index 120323717d87..deab0abd63e1 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -94,8 +94,6 @@ LDFLAGS += $(LDFLAGS-y) include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk -DEPS = .*.d - include Makefile define gendep @@ -113,11 +111,6 @@ __subdir-y := $(filter %/, $(obj-y)) subdir-y += $(__subdir-y) obj-y := $(patsubst %/, %/built_in.o, $(obj-y)) -subdir-n := $(subdir-n) $(subdir-) \ - $(filter %/, $(obj-n) $(obj-)) - -subdir-all := $(subdir-y) $(subdir-n) - $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -DINIT_SECTIONS_ONLY ifeq ($(CONFIG_COVERAGE),y) @@ -181,12 +174,6 @@ FORCE: %/built_in_bin.o: FORCE $(MAKE) -f $(BASEDIR)/Rules.mk -C $* built_in_bin.o -.PHONY: clean -clean:: $(addprefix _clean_, $(subdir-all)) - rm -f *.o .*.o.tmp *~ core $(DEPS_RM) -_clean_%/: FORCE - $(MAKE) $(clean) $* - SRCPATH := $(patsubst $(BASEDIR)/%,%,$(CURDIR)) %.o: %.c Makefile diff --git a/xen/scripts/Kbuild.include b/xen/scripts/Kbuild.include index 2465cc4060c3..6a9b0c39da53 100644 --- a/xen/scripts/Kbuild.include +++ b/xen/scripts/Kbuild.include @@ -2,6 +2,11 @@ #### # kbuild: Generic definitions +### +# dependencies +DEPS = .*.d +DEPS_INCLUDE = $(addsuffix .d2, $(basename $(wildcard $(DEPS)))) + # cc-ifversion # Usage: EXTRA_CFLAGS += $(call cc-ifversion, -lt, 0402, -O1) cc-ifversion = $(shell [ $(CONFIG_GCC_VERSION)0 $(1) $(2)000 ] && echo $(3) || echo $(4)) @@ -9,4 +14,4 @@ cc-ifversion = $(shell [ $(CONFIG_GCC_VERSION)0 $(1) $(2)000 ] && echo $(3) || e # Shorthand for $(MAKE) clean # Usage: # $(MAKE) $(clean) dir -clean := -f $(BASEDIR)/Rules.mk clean -C +clean := -f $(BASEDIR)/scripts/Makefile.clean clean -C diff --git a/xen/scripts/Makefile.clean b/xen/scripts/Makefile.clean new file mode 100644 index 000000000000..31cf2b59594e --- /dev/null +++ b/xen/scripts/Makefile.clean @@ -0,0 +1,33 @@ +# SPDX-License-Identifier: GPL-2.0 +# ========================================================================== +# Cleaning up +# ========================================================================== + +clean:: + +include $(BASEDIR)/scripts/Kbuild.include + +include Makefile + +# Figure out what we need to build from the various variables +# ========================================================================== +__subdir-y := $(filter %/, $(obj-y)) +subdir-y += $(__subdir-y) +subdir-n := $(subdir-n) $(subdir-) \ + $(filter %/, $(obj-n) $(obj-)) +subdir-all := $(subdir-y) $(subdir-n) + +DEPS_RM = $(DEPS) $(DEPS_INCLUDE) +.PHONY: clean +clean:: $(addprefix _clean_, $(subdir-all)) + rm -f *.o .*.o.tmp *~ core $(DEPS_RM) + +# Descending +# --------------------------------------------------------------------------- + +_clean_%/: FORCE + $(MAKE) $(clean) $* + +# Force execution of pattern rules (for which PHONY cannot be directly used). +.PHONY: FORCE +FORCE: