Re: [Qemu-devel] [PATCH v3 22/22] qidl: unit tests and build infrastructure

[Michael Roth]

On Fri, Oct 05, 2012 at 10:24:30AM +0200, Paolo Bonzini wrote:
> Il 04/10/2012 19:33, 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)
> > +   $(call rm -f $(*D)/qidl-generated/$(*F).qidl.c)
> > +   $(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 \
> > +       --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    $@"))
> 
> 
> Because the .qidl.c files are not created for files without a QIDL_ENABLE()
> directive, all those files will be grepped on every invocation of the 
> makefile.
> 
> It is better to define the list of QIDL_ENABLEd files in an auxiliary makefile
> like this (untested):

Thanks for the suggestion / example.

FYI, I tried to get this working but didn't manage to get it in for v4. 
Everything
seems to be falling back to the non-qidl %o: target, and I haven't had time to
debug it.

FWIW, it doesn't seem to be a major factor performance-wise. Current build
times on my laptop are (for all-target builds):

QIDL v4:
 real    8m35.383s
 user    31m1.844s
 sys     1m33.998s

upstream:
 real    8m28.181s
 user    30m44.983s
 sys     1m29.926s

There's also a potential performance trade-off with the suggested approach in 
that
a smaller build might be slower due to us always grepping over an exhaustive 
list
of possibly QIDL-fied files. To be optimal we'd want to limit the search to 
objects
the target actually pulls in.

Maintainance-wise, there's also a nice benefit in the current approach in that
we don't need to maintain a list of files to scan through, so anything we add to
the tree will get picked up by QIDL automatically.

So hopefully the current approach is a reasonable start for now.

> 
> QIDL_FILES =
> -include qemu-idl-files.mak
> qemu-idl-files.mak: $(obj-y) $(common-obj-y) $(hw-obj-y)
>       (grep -l "QIDL_ENABLE()" $^ || :) | sed 's/^/QIDL_FILES += /' > $@
> 
> $(QIDL_FILES): %.qidl.c: %.c
>       $(call quiet-command, \
>           $(CC) $(QEMU_INCLUDES) $(QEMU_CFLAGS) $(CFLAGS) -E -c -DQIDL_GEN $< 
> | \
>           $(PYTHON) $(SRC_PATH)/scripts/qidl.py \
>           --output-filepath=$(*D)/qidl-generated/tmp-$(*F).qidl.c || [ "$$?" 
> -eq 2 ], \
>           "qidl PP $(*D)/$(*F).c"),)
>       mv $(*D)/qidl-generated/tmp-$(*F).qidl.c 
> $(*D)/qidl-generated/$(*F).qidl.c
> 
> $(QIDL_FILES): %.o: %.c %.qidl.c
>       $(call quiet-command, \
>           $(CC) $(QEMU_INCLUDES) $(QEMU_CFLAGS) $(QEMU_DGFLAGS) $(CFLAGS) -c \
>               -DQIDL_ENABLED -include $< -o $@ 
> $(*D)/qidl-generated/$(*F).qidl.c, \
>               "qidl CC $@"), \
> 
> %.o: %.c
>       $(call quiet-command, \
>           $(CC) $(QEMU_INCLUDES) $(QEMU_CFLAGS) $(QEMU_DGFLAGS) $(CFLAGS) -c \
>             -o $@ $<,"  CC    $@"))
> 
> Paolo
>