| Message ID | 20201023150514.2734046-1-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Makefile: separate meson rerun from the rest of the ninja invocation | expand |
On Fri, Oct 23, 2020 at 8:05 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > The rules to build Makefile.mtest are suffering from the "tunnel vision" > problem that is common with recursive makefiles. Makefile.mtest depends > on build.ninja, but Make does not know when build.ninja needs to be > rebuilt before creating Makefile.mtest. > > To fix this, separate the ninja invocation into the "regenerate build > files" phase and the QEMU build phase. Sentinel files such as > meson-private/coredata.dat or build.ninja are used to figure out the > phases that haven't run yet; however, because those files' timestamps > are not guaranteed to be touched, the usual makefile stamp-file trick > is used on top. > Thanks for the quick response. Unfortunately, it doesn't seem to work quite right for me. I think it should be possible to reproduce on unmodified upstream sources like this: $ cd bin/opt/arm $ ../../../configure --target-list=arm-softmmu $ make check-qtest # Feel free to interrupt after verifying that npcm7xx_timer-test is run $ cd - $ git revert 19d50149c857e50ccb1ee35dd4277f9d4954877f Removing tests/qtest/npcm7xx_timer-test.c [meson-test 3ce2b719aa] Revert "tests/qtest: Add npcm7xx timer test" 2 files changed, 563 deletions(-) delete mode 100644 tests/qtest/npcm7xx_timer-test.c $ cd - $ make check-qtest After the revert, check-qtest still runs npcm7xx_timer-test. Note that it doesn't fail because the patch I reverted only adds a new test, it doesn't touch the device being tested. If you run 'make check-qtest' again, npcm7xx_timer-test does not run. Interestingly, it looks like Makefile.mtest gets regenerated here -- I didn't notice that happening in the first run, nor does it happen if I run make check-qtest a third time. I pasted the output from the last two runs of make check-qtest here: https://gist.github.com/hskinnemoen/1773edafeb190773661076e00fdc14de Reported-by: Havard Skinnemoen <hskinnemoen@gmail.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > Makefile | 42 +++++++++++++++++++++++++++++++----------- > 1 file changed, 31 insertions(+), 11 deletions(-) > > diff --git a/Makefile b/Makefile > index 18f026eac3..007ad4d863 100644 > --- a/Makefile > +++ b/Makefile > @@ -92,6 +92,8 @@ endif > ifeq ($(NINJA),) > .PHONY: config-host.mak > x := $(shell rm -rf meson-private meson-info meson-logs) > +else > +export NINJA > endif > ifeq ($(wildcard build.ninja),) > .PHONY: config-host.mak > @@ -100,31 +102,46 @@ endif > > # 1. ensure config-host.mak is up-to-date > config-host.mak: $(SRC_PATH)/configure $(SRC_PATH)/pc-bios > $(SRC_PATH)/VERSION > - @echo $@ is out-of-date, running configure > + @echo config-host.mak is out-of-date, running configure > @if test -f meson-private/coredata.dat; then \ > ./config.status --skip-meson; \ > else \ > - ./config.status; \ > + ./config.status && touch build.ninja.stamp; \ > fi > > -# 2. ensure generated build files are up-to-date > +# 2. meson.stamp exists if meson has run at least once (so ninja > reconfigure > +# works), but otherwise never needs to be updated > +meson-private/coredata.dat: meson.stamp > +meson.stamp: config-host.mak > + @touch meson.stamp > + > +# 3. ensure generated build files are up-to-date > > -ifneq ($(NINJA),) > # A separate rule is needed for Makefile dependencies to avoid -n > -export NINJA > +ifneq ($(MESON),) > +build.ninja: build.ninja.stamp > +build.ninja.stamp: meson.stamp > + $(NINJA) $(if $V,-v,) reconfigure && touch $@ > +endif > + > +ifneq ($(NINJA),) > Makefile.ninja: build.ninja > - $(quiet-@){ echo 'ninja-targets = \'; $(NINJA) -t targets all | > sed 's/:.*//; $$!s/$$/ \\/'; } > $@ > + $(quiet-@){ \ > + echo 'ninja-targets = \'; \ > + $(NINJA) -t targets all | sed 's/:.*//; $$!s/$$/ \\/'; \ > + echo 'build-files = \'; \ > + $(NINJA) -t query build.ninja | sed -n '1,/^ input:/d; /^ > outputs:/q; s/$$/ \\/p'; \ > + } > $@.tmp && mv $@.tmp $@ > -include Makefile.ninja > endif > > ifneq ($(MESON),) > -# The dependency on config-host.mak ensures that meson has run > -Makefile.mtest: build.ninja scripts/mtest2make.py config-host.mak > +Makefile.mtest: build.ninja scripts/mtest2make.py > $(MESON) introspect --targets --tests --benchmarks | $(PYTHON) > scripts/mtest2make.py > $@ > -include Makefile.mtest > endif > > -# 3. Rules to bridge to other makefiles > +# 4. Rules to bridge to other makefiles > > ifneq ($(NINJA),) > NINJAFLAGS = $(if $V,-v,) \ > @@ -135,7 +152,10 @@ ninja-cmd-goals = $(or $(MAKECMDGOALS), all) > ninja-cmd-goals += $(foreach t, $(.tests), $(.test.deps.$t)) > > makefile-targets := build.ninja ctags TAGS cscope dist clean uninstall > -ninja-targets := $(filter-out $(makefile-targets), $(ninja-targets)) > +# "ninja -t targets" also lists all prerequisites. If build system > +# files are marked as PHONY, however, Make will always try to execute > +# "ninja reconfigure" to rebuild build.ninja. > +ninja-targets := $(filter-out $(build-files) $(makefile-targets), > $(ninja-targets)) > .PHONY: $(ninja-targets) run-ninja > $(ninja-targets): run-ninja > > @@ -214,7 +234,7 @@ distclean: clean > rm -f qemu-plugins-ld.symbols qemu-plugins-ld64.symbols > rm -f *-config-target.h *-config-devices.mak *-config-devices.h > rm -rf meson-private meson-logs meson-info compile_commands.json > - rm -f Makefile.ninja Makefile.mtest > + rm -f Makefile.ninja Makefile.mtest build.ninja.stamp meson.stamp > rm -f config.log > rm -f linux-headers/asm > rm -Rf .sdk > -- > 2.26.2 > > <div dir="ltr"><div dir="ltr">On Fri, Oct 23, 2020 at 8:05 AM Paolo Bonzini <<a href="mailto:pbonzini@redhat.com">pbonzini@redhat.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The rules to build Makefile.mtest are suffering from the "tunnel vision"<br> problem that is common with recursive makefiles. Makefile.mtest depends<br> on build.ninja, but Make does not know when build.ninja needs to be<br> rebuilt before creating Makefile.mtest.<br> <br> To fix this, separate the ninja invocation into the "regenerate build<br> files" phase and the QEMU build phase. Sentinel files such as<br> meson-private/coredata.dat or build.ninja are used to figure out the<br> phases that haven't run yet; however, because those files' timestamps<br> are not guaranteed to be touched, the usual makefile stamp-file trick<br> is used on top.<br></blockquote><div><br></div><div>Thanks for the quick response. Unfortunately, it doesn't seem to work quite right for me.</div><div><br></div><div>I think it should be possible to reproduce on unmodified upstream sources like this:</div><div><br></div><div>$ cd bin/opt/arm</div><div>$ ../../../configure --target-list=arm-softmmu</div><div>$ make check-qtest # Feel free to interrupt after verifying that npcm7xx_timer-test is run</div><div>$ cd -</div><div>$ git revert 19d50149c857e50ccb1ee35dd4277f9d4954877f<br></div><div>Removing tests/qtest/npcm7xx_timer-test.c<br>[meson-test 3ce2b719aa] Revert "tests/qtest: Add npcm7xx timer test"<br> 2 files changed, 563 deletions(-)<br> delete mode 100644 tests/qtest/npcm7xx_timer-test.c<br></div><div>$ cd -</div><div>$ make check-qtest</div><div><br></div><div>After the revert, check-qtest still runs npcm7xx_timer-test. Note that it doesn't fail because the patch I reverted only adds a new test, it doesn't touch the device being tested.</div><div><br></div><div>If you run 'make check-qtest' again, npcm7xx_timer-test does not run. Interestingly, it looks like Makefile.mtest gets regenerated here -- I didn't notice that happening in the first run, nor does it happen if I run make check-qtest a third time.</div><div><br></div><div>I pasted the output from the last two runs of make check-qtest here: <a href="https://gist.github.com/hskinnemoen/1773edafeb190773661076e00fdc14de">https://gist.github.com/hskinnemoen/1773edafeb190773661076e00fdc14de</a></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Reported-by: Havard Skinnemoen <<a href="mailto:hskinnemoen@gmail.com" target="_blank">hskinnemoen@gmail.com</a>><br> Signed-off-by: Paolo Bonzini <<a href="mailto:pbonzini@redhat.com" target="_blank">pbonzini@redhat.com</a>><br> ---<br> Makefile | 42 +++++++++++++++++++++++++++++++-----------<br> 1 file changed, 31 insertions(+), 11 deletions(-)<br> <br> diff --git a/Makefile b/Makefile<br> index 18f026eac3..007ad4d863 100644<br> --- a/Makefile<br> +++ b/Makefile<br> @@ -92,6 +92,8 @@ endif<br> ifeq ($(NINJA),)<br> .PHONY: config-host.mak<br> x := $(shell rm -rf meson-private meson-info meson-logs)<br> +else<br> +export NINJA<br> endif<br> ifeq ($(wildcard build.ninja),)<br> .PHONY: config-host.mak<br> @@ -100,31 +102,46 @@ endif<br> <br> # 1. ensure config-host.mak is up-to-date<br> config-host.mak: $(SRC_PATH)/configure $(SRC_PATH)/pc-bios $(SRC_PATH)/VERSION<br> - @echo $@ is out-of-date, running configure<br> + @echo config-host.mak is out-of-date, running configure<br> @if test -f meson-private/coredata.dat; then \<br> ./config.status --skip-meson; \<br> else \<br> - ./config.status; \<br> + ./config.status && touch build.ninja.stamp; \<br> fi<br> <br> -# 2. ensure generated build files are up-to-date<br> +# 2. meson.stamp exists if meson has run at least once (so ninja reconfigure<br> +# works), but otherwise never needs to be updated<br> +meson-private/coredata.dat: meson.stamp<br> +meson.stamp: config-host.mak<br> + @touch meson.stamp<br> +<br> +# 3. ensure generated build files are up-to-date<br> <br> -ifneq ($(NINJA),)<br> # A separate rule is needed for Makefile dependencies to avoid -n<br> -export NINJA<br> +ifneq ($(MESON),)<br> +build.ninja: build.ninja.stamp<br> +build.ninja.stamp: meson.stamp<br> + $(NINJA) $(if $V,-v,) reconfigure && touch $@<br> +endif<br> +<br> +ifneq ($(NINJA),)<br> Makefile.ninja: build.ninja<br> - $(quiet-@){ echo 'ninja-targets = \'; $(NINJA) -t targets all | sed 's/:.*//; $$!s/$$/ \\/'; } > $@<br> + $(quiet-@){ \<br> + echo 'ninja-targets = \'; \<br> + $(NINJA) -t targets all | sed 's/:.*//; $$!s/$$/ \\/'; \<br> + echo 'build-files = \'; \<br> + $(NINJA) -t query build.ninja | sed -n '1,/^ input:/d; /^ outputs:/q; s/$$/ \\/p'; \<br> + } > $@.tmp && mv $@.tmp $@<br> -include Makefile.ninja<br> endif<br> <br> ifneq ($(MESON),)<br> -# The dependency on config-host.mak ensures that meson has run<br> -Makefile.mtest: build.ninja scripts/mtest2make.py config-host.mak<br> +Makefile.mtest: build.ninja scripts/mtest2make.py<br> $(MESON) introspect --targets --tests --benchmarks | $(PYTHON) scripts/mtest2make.py > $@<br> -include Makefile.mtest<br> endif<br> <br> -# 3. Rules to bridge to other makefiles<br> +# 4. Rules to bridge to other makefiles<br> <br> ifneq ($(NINJA),)<br> NINJAFLAGS = $(if $V,-v,) \<br> @@ -135,7 +152,10 @@ ninja-cmd-goals = $(or $(MAKECMDGOALS), all)<br> ninja-cmd-goals += $(foreach t, $(.tests), $(.test.deps.$t))<br> <br> makefile-targets := build.ninja ctags TAGS cscope dist clean uninstall<br> -ninja-targets := $(filter-out $(makefile-targets), $(ninja-targets))<br> +# "ninja -t targets" also lists all prerequisites. If build system<br> +# files are marked as PHONY, however, Make will always try to execute<br> +# "ninja reconfigure" to rebuild build.ninja.<br> +ninja-targets := $(filter-out $(build-files) $(makefile-targets), $(ninja-targets))<br> .PHONY: $(ninja-targets) run-ninja<br> $(ninja-targets): run-ninja<br> <br> @@ -214,7 +234,7 @@ distclean: clean<br> rm -f qemu-plugins-ld.symbols qemu-plugins-ld64.symbols<br> rm -f *-config-target.h *-config-devices.mak *-config-devices.h<br> rm -rf meson-private meson-logs meson-info compile_commands.json<br> - rm -f Makefile.ninja Makefile.mtest<br> + rm -f Makefile.ninja Makefile.mtest build.ninja.stamp meson.stamp<br> rm -f config.log<br> rm -f linux-headers/asm<br> rm -Rf .sdk<br> -- <br> 2.26.2<br> <br> </blockquote></div></div>
diff --git a/Makefile b/Makefile index 18f026eac3..007ad4d863 100644 --- a/Makefile +++ b/Makefile @@ -92,6 +92,8 @@ endif ifeq ($(NINJA),) .PHONY: config-host.mak x := $(shell rm -rf meson-private meson-info meson-logs) +else +export NINJA endif ifeq ($(wildcard build.ninja),) .PHONY: config-host.mak @@ -100,31 +102,46 @@ endif # 1. ensure config-host.mak is up-to-date config-host.mak: $(SRC_PATH)/configure $(SRC_PATH)/pc-bios $(SRC_PATH)/VERSION - @echo $@ is out-of-date, running configure + @echo config-host.mak is out-of-date, running configure @if test -f meson-private/coredata.dat; then \ ./config.status --skip-meson; \ else \ - ./config.status; \ + ./config.status && touch build.ninja.stamp; \ fi -# 2. ensure generated build files are up-to-date +# 2. meson.stamp exists if meson has run at least once (so ninja reconfigure +# works), but otherwise never needs to be updated +meson-private/coredata.dat: meson.stamp +meson.stamp: config-host.mak + @touch meson.stamp + +# 3. ensure generated build files are up-to-date -ifneq ($(NINJA),) # A separate rule is needed for Makefile dependencies to avoid -n -export NINJA +ifneq ($(MESON),) +build.ninja: build.ninja.stamp +build.ninja.stamp: meson.stamp + $(NINJA) $(if $V,-v,) reconfigure && touch $@ +endif + +ifneq ($(NINJA),) Makefile.ninja: build.ninja - $(quiet-@){ echo 'ninja-targets = \'; $(NINJA) -t targets all | sed 's/:.*//; $$!s/$$/ \\/'; } > $@ + $(quiet-@){ \ + echo 'ninja-targets = \'; \ + $(NINJA) -t targets all | sed 's/:.*//; $$!s/$$/ \\/'; \ + echo 'build-files = \'; \ + $(NINJA) -t query build.ninja | sed -n '1,/^ input:/d; /^ outputs:/q; s/$$/ \\/p'; \ + } > $@.tmp && mv $@.tmp $@ -include Makefile.ninja endif ifneq ($(MESON),) -# The dependency on config-host.mak ensures that meson has run -Makefile.mtest: build.ninja scripts/mtest2make.py config-host.mak +Makefile.mtest: build.ninja scripts/mtest2make.py $(MESON) introspect --targets --tests --benchmarks | $(PYTHON) scripts/mtest2make.py > $@ -include Makefile.mtest endif -# 3. Rules to bridge to other makefiles +# 4. Rules to bridge to other makefiles ifneq ($(NINJA),) NINJAFLAGS = $(if $V,-v,) \ @@ -135,7 +152,10 @@ ninja-cmd-goals = $(or $(MAKECMDGOALS), all) ninja-cmd-goals += $(foreach t, $(.tests), $(.test.deps.$t)) makefile-targets := build.ninja ctags TAGS cscope dist clean uninstall -ninja-targets := $(filter-out $(makefile-targets), $(ninja-targets)) +# "ninja -t targets" also lists all prerequisites. If build system +# files are marked as PHONY, however, Make will always try to execute +# "ninja reconfigure" to rebuild build.ninja. +ninja-targets := $(filter-out $(build-files) $(makefile-targets), $(ninja-targets)) .PHONY: $(ninja-targets) run-ninja $(ninja-targets): run-ninja @@ -214,7 +234,7 @@ distclean: clean rm -f qemu-plugins-ld.symbols qemu-plugins-ld64.symbols rm -f *-config-target.h *-config-devices.mak *-config-devices.h rm -rf meson-private meson-logs meson-info compile_commands.json - rm -f Makefile.ninja Makefile.mtest + rm -f Makefile.ninja Makefile.mtest build.ninja.stamp meson.stamp rm -f config.log rm -f linux-headers/asm rm -Rf .sdk
The rules to build Makefile.mtest are suffering from the "tunnel vision" problem that is common with recursive makefiles. Makefile.mtest depends on build.ninja, but Make does not know when build.ninja needs to be rebuilt before creating Makefile.mtest. To fix this, separate the ninja invocation into the "regenerate build files" phase and the QEMU build phase. Sentinel files such as meson-private/coredata.dat or build.ninja are used to figure out the phases that haven't run yet; however, because those files' timestamps are not guaranteed to be touched, the usual makefile stamp-file trick is used on top. Reported-by: Havard Skinnemoen <hskinnemoen@gmail.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- Makefile | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-)