Re: [Qemu-devel] [PATCH v4 26/26] qidl: unit tests and build infrastructure

[Michael Roth]

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    $@"))
>