Re: [Qemu-devel] [PATCH] rules.mak: quiet-command: Split command name and args to print

[Eric Blake]

On 09/20/2016 01:34 PM, Peter Maydell wrote:
> The quiet-command make rule currently takes two arguments:
> the command and arguments to run, and a string to print if
> the V flag is not set (ie we are not being verbose).
> By convention, the string printed is of the form
> "  NAME   some args". Unfortunately to get nicely lined up
> output all the strings have to agree about what column the
> arguments should start in, which means that if we add a
> new quiet-command usage which wants a slightly longer CMD
> name then we either put up with misalignment or change
> every quiet-command string.
> 

> +++ b/Makefile
> @@ -105,20 +105,20 @@ SUBDIR_DEVICES_MAK_DEP=$(patsubst %, 
> %-config-devices.mak.d, $(TARGET_DIRS))
>  
>  ifeq ($(SUBDIR_DEVICES_MAK),)
>  config-all-devices.mak:
> -     $(call quiet-command,echo '# no devices' > $@,"  GEN   $@")
> +     $(call quiet-command,echo '# no devices' > $@,"GEN","$@")

Here, you have no whitespace;

>  else
>  config-all-devices.mak: $(SUBDIR_DEVICES_MAK)
>       $(call quiet-command, sed -n \
>               's|^\([^=]*\)=\(.*\)$$|\1:=$$(findstring y,$$(\1)\2)|p' \
>               $(SUBDIR_DEVICES_MAK) | sort -u > $@, \
> -             "  GEN   $@")
> +             "GEN","$@")
>  endif
>  
>  -include $(SUBDIR_DEVICES_MAK_DEP)
>  
>  %/config-devices.mak: default-configs/%.mak 
> $(SRC_PATH)/scripts/make_device_config.sh
>       $(call quiet-command, \
> -            $(SHELL) $(SRC_PATH)/scripts/make_device_config.sh $< 
> $*-config-devices.mak.d $@ > address@hidden, "  GEN   address@hidden")
> +            $(SHELL) $(SRC_PATH)/scripts/make_device_config.sh $< 
> $*-config-devices.mak.d $@ > address@hidden, "GEN","address@hidden")

But here, in addition to the long line (worth another \ separator?) you
have space before "GEN".  Either the whitespace doesn't hurt (in which
case we might want to use it in more places for legibility, or else
avoid it solely for consistency) or it matters (in which case the space
is wrong here).  Fortunately, the space was pre-existing, so it looks
like the former (did not matter); but as long as you are touching them
all, the consistency argument bears sway.

>  ui/shader/%-frag.h: $(SRC_PATH)/ui/shader/%.frag 
> $(SRC_PATH)/scripts/shaderinclude.pl
>       @mkdir -p $(dir $@)
>       $(call quiet-command,\
>               perl $(SRC_PATH)/scripts/shaderinclude.pl $< > $@,\
> -             "  FRAG  $@")
> +             "FRAG","$@")

Ah, more proof that the whitespace does not matter, but that you can use
\ to break up long $(call) lines.

>  qemu-ga.8: qemu-ga.texi
>       $(call quiet-command, \
>         perl -Ww -- $(SRC_PATH)/scripts/texi2pod.pl $< qemu-ga.pod && \
>         $(POD2MAN) --section=8 --center=" " --release=" " qemu-ga.pod > $@, \
> -       "  GEN   $@")
> +       "GEN","$@")

Hmm.  Thinking about it some more, I note that the old form HAD to use
spaces, as in "  FOO  $@", because the spaces after the first " were
special to the shell.  But now that we are using printf, we don't need
shell quoting on the first parameter.  Couldn't you just as easily write:

$(call, ..., GEN,"$@")

I'm less certain whether the $@ will need quotes, or whether that is
another place where (due to makefile rules) it will always expand to a
single shell word that didn't need quoting.

> +++ b/pc-bios/optionrom/Makefile
> @@ -43,16 +43,16 @@ build-all: multiboot.bin linuxboot.bin linuxboot_dma.bin 
> kvmvapic.bin
>  
>  
>  %.o: %.S
> -     $(call quiet-command,$(CPP) $(QEMU_INCLUDES) $(QEMU_DGFLAGS) -c -o - $< 
> | $(AS) $(ASFLAGS) -o $@,"  AS    $(TARGET_DIR)$@")
> +     $(call quiet-command,$(CPP) $(QEMU_INCLUDES) $(QEMU_DGFLAGS) -c -o - $< 
> | $(AS) $(ASFLAGS) -o $@,"AS","$(TARGET_DIR)$@")
>  
>  %.img: %.o
> -     $(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m $(LD_I386_EMULATION) -T 
> $(SRC_PATH)/pc-bios/optionrom/flat.lds -s -o $@ $<,"  Building 
> $(TARGET_DIR)$@")
> +     $(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m $(LD_I386_EMULATION) -T 
> $(SRC_PATH)/pc-bios/optionrom/flat.lds -s -o $@ 
> $<,"Building","$(TARGET_DIR)$@")

Should we s/Building/BUILDING/ as part of this cleanup?

>  
>  %.raw: %.img
> -     $(call quiet-command,$(OBJCOPY) -O binary -j .text $< $@,"  Building 
> $(TARGET_DIR)$@")
> +     $(call quiet-command,$(OBJCOPY) -O binary -j .text $< 
> $@,"Building","$(TARGET_DIR)$@")
>  
>  %.bin: %.raw
> -     $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/signrom.py $< $@,"  
> Signing $(TARGET_DIR)$@")
> +     $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/signrom.py $< 
> $@,"Signing","$(TARGET_DIR)$@")

same for SIGNING?

>  
>  clean:
>       rm -f *.o *.d *.raw *.img *.bin *~
> diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
> index 0ab2538..35cf38c 100644
> --- a/pc-bios/s390-ccw/Makefile
> +++ b/pc-bios/s390-ccw/Makefile
> @@ -19,10 +19,10 @@ LDFLAGS += -Wl,-pie -nostdlib
>  build-all: s390-ccw.img
>  
>  s390-ccw.elf: $(OBJECTS)
> -     $(call quiet-command,$(CC) $(LDFLAGS) -o $@ $(OBJECTS),"  Building 
> $(TARGET_DIR)$@")
> +     $(call quiet-command,$(CC) $(LDFLAGS) -o $@ 
> $(OBJECTS),"Building","$(TARGET_DIR)$@")
>  
>  s390-ccw.img: s390-ccw.elf
> -     $(call quiet-command,strip --strip-unneeded $< -o $@,"  Stripping 
> $(TARGET_DIR)$@")
> +     $(call quiet-command,strip --strip-unneeded $< -o 
> $@,"STRIP","$(TARGET_DIR)$@")

And I see you changed Stripping to STRIP here.  Not quite mentioned in
the commit message; worth splitting out as a separate fix?

> +++ b/rules.mak

>  %.mo:
> -     $(call quiet-command,$(LD_REL) -o $@ $^,"  LD -r $(TARGET_DIR)$@")
> +     $(call quiet-command,$(LD_REL) -o $@ $^,"LD -r","$(TARGET_DIR)$@")

Should we 's/LD -r/LD/' here?

>  
>  .PHONY: modules
>  modules:
> @@ -105,9 +105,15 @@ modules:
>       $(call LINK,$(filter %.o %.a %.mo, $^))
>  
>  %.a:
> -     $(call quiet-command,rm -f $@ && $(AR) rcs $@ $^,"  AR    
> $(TARGET_DIR)$@")
> +     $(call quiet-command,rm -f $@ && $(AR) rcs $@ $^,"AR","$(TARGET_DIR)$@")
>  
> -quiet-command = $(if $(V),$1,$(if $(2),@echo $2 && $1, @$1))

And the real meat of the patch.  You know, you can use git
format-patch's -O/path/to/file (or git config diff.orderFile) with
appropriate contents in /path/to/file in order to hoist this file to the
top of the patch, to make review easier than hunting for "somewhere in
the middle".  :)

> +# Usage: $(call quiet-command,command and args,"NAME","args to print")

Again, I'm not convinced that we need "NAME", it should be sufficient
for just NAME.  "args to print" still matters if there are spaces or
shell metacharacters, though (on the other hand, we could refactor it so
that this expansion supplies the "" instead of making every caller
duplicate the work).

> +# This will run "command and args", and either:
> +#  if V=1 just print the whole command and args
> +#  otherwise print the 'quiet' output in the format "  NAME     args to 
> print"
> +# NAME should be a short name of the command, 7 letters or fewer.
> +# If called with only a single argument, will print nothing in quiet mode.
> +quiet-command = $(if $(V),$1,$(if $(2),@printf "  %-7s %s\n" $2 $3 && $1, 
> @$1))

Looks sane to me.  And confirms what I guessed at above; as written
here, any whitespace passed in either $2 or $3 by make is
inconsequential to their use as arguments to printf (and callers that
have whitespace or shell metacharacters in $3 have to provide their own
quotes).

I'll let you decide if dropping the now-useless quotes on $2, or moving
the burden of quotes on $3 from the callsites into the main workhorse,
makes enough sense to respin this patch.

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

signature.asc
Description: OpenPGP digital signature