| Message ID | 20191129095927.17382-1-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: Module fixes and cleanups | expand |
On 11/29/19 3:59 AM, Markus Armbruster wrote: > Generated .h need to be generated before compiling any .c using them. > To know which .h a .c uses, we need to compile it. > > Since commit 4115852bb0 "build: do not sprinkle around > GENERATED_HEADERS dependencies", we break this circular dependency the > simple & stupid way: the generated headers are a prerequisite of > Makefile, which causes Make to generate them first, then start over. which is a pain when using 'make --dry-run' to see what would get built (a dependency of Makefile _is_ rebuilt if Makefile itself has to be updated), but not the fault of this patch. > > Except for qga we still use the older method of making all its .o > summarily depend on all its generated .h (commit 016c77ad62 "Makefile: > add missing deps on $(GENERATED_HEADERS)"). > > Add qga's generated files to generated-files-y to get rid of this > exception. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > Makefile | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> > +++ b/Makefile > @@ -130,6 +130,15 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi > > generated-files-y += $(GENERATED_QAPI_FILES) > > +GENERATED_QGA_FILES := qga-qapi-types.c qga-qapi-types.h > +GENERATED_QGA_FILES += qga-qapi-visit.c qga-qapi-visit.h > +GENERATED_QGA_FILES += qga-qapi-commands.h qga-qapi-commands.c > +GENERATED_QGA_FILES += qga-qapi-init-commands.h qga-qapi-init-commands.c > +GENERATED_QGA_FILES += qga-qapi-doc.texi > +GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES)) Would it be worth using two separate variable names (maybe GENERATED_QGA_BASEFILES for the first list) rather than exploiting the arcane knowledge that consecutive use of := causes GNU make to rewrite an existing variable with new contents?
Eric Blake <eblake@redhat.com> writes: > On 11/29/19 3:59 AM, Markus Armbruster wrote: >> Generated .h need to be generated before compiling any .c using them. >> To know which .h a .c uses, we need to compile it. >> >> Since commit 4115852bb0 "build: do not sprinkle around >> GENERATED_HEADERS dependencies", we break this circular dependency the >> simple & stupid way: the generated headers are a prerequisite of >> Makefile, which causes Make to generate them first, then start over. > > which is a pain when using 'make --dry-run' to see what would get > built (a dependency of Makefile _is_ rebuilt if Makefile itself has to > be updated), but not the fault of this patch. Yes. >> Except for qga we still use the older method of making all its .o >> summarily depend on all its generated .h (commit 016c77ad62 "Makefile: >> add missing deps on $(GENERATED_HEADERS)"). >> >> Add qga's generated files to generated-files-y to get rid of this >> exception. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> Makefile | 21 +++++++++++---------- >> 1 file changed, 11 insertions(+), 10 deletions(-) > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks! >> +++ b/Makefile >> @@ -130,6 +130,15 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi >> generated-files-y += $(GENERATED_QAPI_FILES) >> +GENERATED_QGA_FILES := qga-qapi-types.c qga-qapi-types.h >> +GENERATED_QGA_FILES += qga-qapi-visit.c qga-qapi-visit.h >> +GENERATED_QGA_FILES += qga-qapi-commands.h qga-qapi-commands.c >> +GENERATED_QGA_FILES += qga-qapi-init-commands.h qga-qapi-init-commands.c >> +GENERATED_QGA_FILES += qga-qapi-doc.texi >> +GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES)) > > Would it be worth using two separate variable names (maybe > GENERATED_QGA_BASEFILES for the first list) rather than exploiting the > arcane knowledge that consecutive use of := causes GNU make to rewrite > an existing variable with new contents? Our rules.mak relies on this already. It's full of magic, which admittedly diminishes its suitability to serve as a good example. Your worry might be rooted in old "=" burns. "=" makes the variable recursively expanded, and GENERATED_QGA_FILES = $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES)) would be an infinite loop. ":=" makes it simply expanded; there's not even a loop, let alone an infinite one. The GNU Make manual explains this clearly at https://www.gnu.org/software/make/manual/html_node/Flavors.html Aside: there's a reason one of the two flavors is called "simple". It could additionally be called "not as slow". One of my pet makefile peeves: unthinking use of recursively expanded variables, complicating semantics and slowing down builds. Back to this patch. I had started to write the thing in longhand, but got tired of repeating qga/qapi-generated/, so I factored that out. Would longhand be easier to understand?
On 12/4/19 12:56 AM, Markus Armbruster wrote: >>> +++ b/Makefile >>> @@ -130,6 +130,15 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi >>> generated-files-y += $(GENERATED_QAPI_FILES) >>> +GENERATED_QGA_FILES := qga-qapi-types.c qga-qapi-types.h >>> +GENERATED_QGA_FILES += qga-qapi-visit.c qga-qapi-visit.h >>> +GENERATED_QGA_FILES += qga-qapi-commands.h qga-qapi-commands.c >>> +GENERATED_QGA_FILES += qga-qapi-init-commands.h qga-qapi-init-commands.c >>> +GENERATED_QGA_FILES += qga-qapi-doc.texi >>> +GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES)) >> >> Would it be worth using two separate variable names (maybe >> GENERATED_QGA_BASEFILES for the first list) rather than exploiting the >> arcane knowledge that consecutive use of := causes GNU make to rewrite >> an existing variable with new contents? > > Our rules.mak relies on this already. It's full of magic, which > admittedly diminishes its suitability to serve as a good example. > > Your worry might be rooted in old "=" burns. "=" makes the variable > recursively expanded, and > > GENERATED_QGA_FILES = $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES)) Indeed, but I have to refer to the manual to remind myself of whether = or := is what I want in a given situation. > > would be an infinite loop. ":=" makes it simply expanded; there's not > even a loop, let alone an infinite one. The GNU Make manual explains > this clearly at > https://www.gnu.org/software/make/manual/html_node/Flavors.html > > Aside: there's a reason one of the two flavors is called "simple". It > could additionally be called "not as slow". One of my pet makefile > peeves: unthinking use of recursively expanded variables, complicating > semantics and slowing down builds. > > Back to this patch. I had started to write the thing in longhand, but > got tired of repeating qga/qapi-generated/, so I factored that out. > Would longhand be easier to understand? It's more verbose. My suggestion was more: GENERATED_QGA_BASENAMES := qga-qapi-types.c qga-qapi-types.h GENERATED_QGA_BASENAMES += qga-qapi-visit.c qga-qapi-visit.h ... GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, $(GENERATED_QGA_BASENAMES)) to avoid the reassignment-to-self issue altogether, while still remaining concise compared to longhand.
Eric Blake <eblake@redhat.com> writes: > On 12/4/19 12:56 AM, Markus Armbruster wrote: > >>>> +++ b/Makefile >>>> @@ -130,6 +130,15 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi >>>> generated-files-y += $(GENERATED_QAPI_FILES) >>>> +GENERATED_QGA_FILES := qga-qapi-types.c qga-qapi-types.h >>>> +GENERATED_QGA_FILES += qga-qapi-visit.c qga-qapi-visit.h >>>> +GENERATED_QGA_FILES += qga-qapi-commands.h qga-qapi-commands.c >>>> +GENERATED_QGA_FILES += qga-qapi-init-commands.h qga-qapi-init-commands.c >>>> +GENERATED_QGA_FILES += qga-qapi-doc.texi >>>> +GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES)) >>> >>> Would it be worth using two separate variable names (maybe >>> GENERATED_QGA_BASEFILES for the first list) rather than exploiting the >>> arcane knowledge that consecutive use of := causes GNU make to rewrite >>> an existing variable with new contents? >> >> Our rules.mak relies on this already. It's full of magic, which >> admittedly diminishes its suitability to serve as a good example. >> >> Your worry might be rooted in old "=" burns. "=" makes the variable >> recursively expanded, and >> >> GENERATED_QGA_FILES = $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES)) > > Indeed, but I have to refer to the manual to remind myself of whether > = or := is what I want in a given situation. Trust me, you're either sure you want "=", or you want ":=". On a green field, I recommend a hard rule "no = without a comment explaining why". >> would be an infinite loop. ":=" makes it simply expanded; there's not >> even a loop, let alone an infinite one. The GNU Make manual explains >> this clearly at >> https://www.gnu.org/software/make/manual/html_node/Flavors.html >> >> Aside: there's a reason one of the two flavors is called "simple". It >> could additionally be called "not as slow". One of my pet makefile >> peeves: unthinking use of recursively expanded variables, complicating >> semantics and slowing down builds. >> >> Back to this patch. I had started to write the thing in longhand, but >> got tired of repeating qga/qapi-generated/, so I factored that out. >> Would longhand be easier to understand? > > It's more verbose. My suggestion was more: > > GENERATED_QGA_BASENAMES := qga-qapi-types.c qga-qapi-types.h > GENERATED_QGA_BASENAMES += qga-qapi-visit.c qga-qapi-visit.h > ... > GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, > $(GENERATED_QGA_BASENAMES)) > > to avoid the reassignment-to-self issue altogether, while still > remaining concise compared to longhand. Either way, we use multiple assignments to build GENERATED_QGA_FILES. The only difference is that the version using two variables would also work with recursive expansion, due to the magic of +=.
diff --git a/Makefile b/Makefile index 8dad949483..d4138343cd 100644 --- a/Makefile +++ b/Makefile @@ -130,6 +130,15 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi generated-files-y += $(GENERATED_QAPI_FILES) +GENERATED_QGA_FILES := qga-qapi-types.c qga-qapi-types.h +GENERATED_QGA_FILES += qga-qapi-visit.c qga-qapi-visit.h +GENERATED_QGA_FILES += qga-qapi-commands.h qga-qapi-commands.c +GENERATED_QGA_FILES += qga-qapi-init-commands.h qga-qapi-init-commands.c +GENERATED_QGA_FILES += qga-qapi-doc.texi +GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES)) + +generated-files-y += $(GENERATED_QGA_FILES) + generated-files-y += trace/generated-tcg-tracers.h generated-files-y += trace/generated-helpers-wrappers.h @@ -608,12 +617,7 @@ $(SRC_PATH)/scripts/qapi/types.py \ $(SRC_PATH)/scripts/qapi/visit.py \ $(SRC_PATH)/scripts/qapi-gen.py -qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h \ -qga/qapi-generated/qga-qapi-visit.c qga/qapi-generated/qga-qapi-visit.h \ -qga/qapi-generated/qga-qapi-commands.h qga/qapi-generated/qga-qapi-commands.c \ -qga/qapi-generated/qga-qapi-init-commands.h qga/qapi-generated/qga-qapi-init-commands.c \ -qga/qapi-generated/qga-qapi-doc.texi: \ -qga/qapi-generated/qapi-gen-timestamp ; +$(GENERATED_QGA_FILES): qga/qapi-generated/qapi-gen-timestamp ; qga/qapi-generated/qapi-gen-timestamp: $(SRC_PATH)/qga/qapi-schema.json $(qapi-py) $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-gen.py \ -o qga/qapi-generated -p "qga-" $<, \ @@ -630,9 +634,6 @@ qapi-gen-timestamp: $(qapi-modules) $(qapi-py) "GEN","$(@:%-timestamp=%)") @>$@ -QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qapi-commands.h qga-qapi-init-commands.h) -$(qga-obj-y): $(QGALIB_GEN) - qemu-ga$(EXESUF): $(qga-obj-y) $(COMMON_LDADDS) $(call LINK, $^) @@ -722,7 +723,7 @@ clean: recurse-clean rm -f trace/generated-tracers-dtrace.h* rm -f $(foreach f,$(generated-files-y),$(f) $(f)-timestamp) rm -f qapi-gen-timestamp - rm -rf qga/qapi-generated + rm -f qga/qapi-generated/qapi-gen-timestamp rm -f config-all-devices.mak VERSION ?= $(shell cat VERSION)
Generated .h need to be generated before compiling any .c using them. To know which .h a .c uses, we need to compile it. Since commit 4115852bb0 "build: do not sprinkle around GENERATED_HEADERS dependencies", we break this circular dependency the simple & stupid way: the generated headers are a prerequisite of Makefile, which causes Make to generate them first, then start over. Except for qga we still use the older method of making all its .o summarily depend on all its generated .h (commit 016c77ad62 "Makefile: add missing deps on $(GENERATED_HEADERS)"). Add qga's generated files to generated-files-y to get rid of this exception. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- Makefile | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)