From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56549) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TNnfG-0004Yh-Jb for qemu-devel@nongnu.org; Mon, 15 Oct 2012 12:37:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TNnfA-00058r-Gg for qemu-devel@nongnu.org; Mon, 15 Oct 2012 12:37:14 -0400 Received: from mail-oa0-f45.google.com ([209.85.219.45]:50086) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TNnfA-00058m-BG for qemu-devel@nongnu.org; Mon, 15 Oct 2012 12:37:08 -0400 Received: by mail-oa0-f45.google.com with SMTP id i18so4990695oag.4 for ; Mon, 15 Oct 2012 09:37:07 -0700 (PDT) Sender: fluxion Date: Mon, 15 Oct 2012 11:37:01 -0500 From: Michael Roth Message-ID: <20121015163701.GU16157@illuin> References: <1350076268-18461-1-git-send-email-mdroth@linux.vnet.ibm.com> <1350076268-18461-27-git-send-email-mdroth@linux.vnet.ibm.com> <507BDFD5.80201@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <507BDFD5.80201@redhat.com> Subject: Re: [Qemu-devel] [PATCH v4 26/26] qidl: unit tests and build infrastructure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, peter.maydell@linaro.org, aliguori@us.ibm.com, qemu-devel@nongnu.org, blauwirbel@gmail.com 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 ... %.o: QIDL-PP-% 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 $@")) >