Re: [Qemu-devel] [PATCH v3 1/8] make: move top level dir to end of include search path

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4082 bytes --]

On 01/24/2017 05:01 AM, Daniel P. Berrange wrote:
> Currently the search path is
> 
>   1. source dir corresponding to input file (implicit by compiler)
>   2. top level build dir
>   3. top level source dir
>   4. top level source include/ dir
>   5. source dir corresponding to input file
>   6. build dir corresponding to output file
> 
> This causes a semantic difference in behaviour for builds
> where srcdir == builddir vs srcdir != builddir, because
> item 5 moves from end to start, when srcdir == builddir.

Rather, item 5 is a no-op (because it duplicated 1), and item 6 moves
from the end to the beginning when srcdir == builddir

> 
> As a general rule we also want to move to have all shared
> headers in the include/ dir, so move that ahead of the
> top level dirs in the search order.

Wait - are you proposing that you swap 4 to occur earlier than 2/3?...

> 
> Thus we now have:
> 
>   1. source dir corresponding to input file
>   2. build dir corresponding to output file
>   3. top level build dir
>   4. top level source dir
>   5. top level source include/ dir

...because this doesn't match that swap, and I don't see it in the patch
body (but I may have missed it; I'm not as strong at reviewing make as I
am at C)

> 
> and items 1+2 and 4+5 collapse into a single dir when srcdir==builddir

Isn't that items 3+4 (not 4+5) that collapse?

> so overall order remains the same.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  rules.mak | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)


> 
> diff --git a/rules.mak b/rules.mak
> index d5c516c..e09aabe 100644
> --- a/rules.mak
> +++ b/rules.mak
> @@ -26,8 +26,10 @@ QEMU_CXXFLAGS = -D__STDC_LIMIT_MACROS $(filter-out -Wstrict-prototypes -Wmissing
>  # Flags for dependency generation
>  QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(@D)/$(*F).d
>  
> -# Same as -I$(SRC_PATH) -I., but for the nested source/object directories
> -QEMU_INCLUDES += -I$(<D) -I$(@D)

In particular, this is the old code for 5 and 6,

> +# Compiler searches the source file dir first, but in vpath builds
> +# we need to make it search the build dir too, before any other
> +# explicit search paths.
> +QEMU_LOCAL_INCLUDES = -I$(BUILD_DIR)/$(@D)

while this is the new code for 2, plus documentation that 1 is implicit.

>  
>  WL_U := -Wl,-u,
>  find-symbols = $(if $1, $(sort $(shell $(NM) -P -g $1 | $2)))
> @@ -61,7 +63,7 @@ expand-objs = $(strip $(sort $(filter %.o,$1)) \
>                    $(filter-out %.o %.mo,$1))
>  
>  %.o: %.c
> -	$(call quiet-command,$(CC) $(QEMU_INCLUDES) $(QEMU_CFLAGS) $(QEMU_DGFLAGS) $(CFLAGS) $($@-cflags) -c -o $@ $<,"CC","$(TARGET_DIR)$@")
> +	$(call quiet-command,$(CC) $(QEMU_LOCAL_INCLUDES) $(QEMU_INCLUDES) $(QEMU_CFLAGS) $(QEMU_DGFLAGS) $(CFLAGS) $($@-cflags) -c -o $@ $<,"CC","$(TARGET_DIR)$@")

And the pre-pending of QEMU_LOCAL_INCLUDES is what changes the position
of the local directory from last to first, thus delaying the top level
dir to the end, but I don't see top/include/ moving.

These are now some long lines; is it worth taking the time to add \ line
splitting for legibility, either in this patch or as an add-on?

> @@ -359,6 +361,7 @@ define unnest-vars
>                  $(eval $(o:%.mo=%$(DSOSUF)): module-common.o $($o-objs)),
>                  $(error $o added in $v but $o-objs is not set)))
>          $(shell mkdir -p ./ $(sort $(dir $($v))))
> +        $(shell cd $(BUILD_DIR) && mkdir -p ./ $(sort $(dir $($v))))
>          # Include all the .d files

Okay, this change makes sense (make sure all the build directories exist
in time; no-op for in-tree build, but helpful for VPATH), but seems
unrelated to the commit message.  Rebase snafu?

It looks like you're on the right track, but there's enough
discrepancies between the commit message and actual change that I'd
prefer a v4 before I grant R-b.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]