On Mon, Oct 15, 2012 at 12:05:09PM +0200, Paolo Bonzini wrote: > Il 12/10/2012 23:11, Michael Roth ha scritto: > > +%.qidl.c: %.c $(SRC_PATH)/qidl.h $(addprefix $(SRC_PATH)/scripts/,lexer.py > > qidl.py qidl_parser.py qapi.py qapi_visit.py) > > The rule here is wrong, because %.qidl.c is never produced by the > commands. The output is in the qidl-generated subdirectory, and that > cannot be expressed with pattern rules. > > I was worried that this causes many greps on every invocation of the > Makefile---even though the way you hid them in makefile functions would > hide them from the "make" output. > > However, this is not the case. What happens is quite complex. > hw/foo.qidl.c (as opposed to hw/qidl-generated/foo.qidl.c) will never > exist, but it is a dependency of hw/foo.o, so it is always rebuilt > before it. But it is not rebuilt on every invocation even though it > doesn't exist, because it only comes into play via implicit rules. > > I think this is quite clever and does exactly what you want. If you > want to keep the qidl-generated directory, I cannot imagine any other > way to do it. However, please rename %.qidl.c to something QIDL-PP-% so > that it is clear that it is not a "real" file name.
At the time I was in battle to the death with make to have it let me use qidl-generated/ directories to store the output, but I don't care too much anymore :P I think it's worth having if it doesn't cause any problems though. There is another way to do this (still using a "dummy" target) that is a bit less cryptic: QIDL-PP-%: %.c qidl.h ... <grep and create %.qidl.c> %.o: QIDL-PP-% <build normal or qidl CC> But make detects that QIDL-PP-% is an intermediate target and removes the *.qidl.c files after the build, which ends up forcing a re-build each time. I played around with .INTERMEDIATE/.PRECIOUS directives to have it keep the files but couldn't get it to cooperate, which is why I ended up with the current approach. > > If we decide this is too clever (and it's not all of it, it took me a > while to understand that Makefile functions are needed because > quiet-command expands to a @-prefixed command...), we can drop the > qidl-generated directory. > > > + $(call rm -f $(*D)/qidl-generated/$(*F).qidl.*) Yes, and in fact this doesn't work. I hadn't noticed it because the old files get clobbered by the code gen, so it only comes into play if you take a QIDL'd file and then un-QIDL it (since the %.o rule sees the .qidl.c file still there and tries to build it accordingly). I'll go ahead and do a v5 with these changes in place. > > I think this $(call) is not what you want, you need just "@rm -f ..." > > Paolo > > > + $(if $(strip $(shell grep "QIDL_ENABLE()" $< 1>/dev/null && echo > > "true")), \ > > + $(call quiet-command, \ > > + $(CC) $(QEMU_INCLUDES) $(QEMU_CFLAGS) $(CFLAGS) -E -c -DQIDL_GEN $< > > | \ > > + $(PYTHON) $(SRC_PATH)/scripts/qidl.py $(QIDL_FLAGS) \ > > + --output-filepath=$(*D)/qidl-generated/$(*F).qidl.c || [ "$$?" -eq > > 2 ], \ > > + "qidl PP $(*D)/$(*F).c"),) > > + > > +%.o: %.c %.qidl.c > > + $(if $(strip $(shell test -f $(*D)/qidl-generated/$(*F).qidl.c && echo > > "true")), \ > > + $(call quiet-command, \ > > + $(CC) $(QEMU_INCLUDES) $(QEMU_CFLAGS) $(QEMU_DGFLAGS) $(CFLAGS) -c \ > > + -DQIDL_ENABLED -include $< -o $@ > > $(*D)/qidl-generated/$(*F).qidl.c, \ > > + "qidl CC $@"), \ > > + $(call quiet-command, \ > > + $(CC) $(QEMU_INCLUDES) $(QEMU_CFLAGS) $(QEMU_DGFLAGS) $(CFLAGS) -c \ > > + -o $@ $<," CC $@")) >