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

[Peter Maydell]

On 20 September 2016 at 20:03, Eric Blake <address@hidden> wrote:
> 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.

The spacing doesn't matter, because the shell fragment we end up running
is printf "format string" "NAME" "stuff stuff stuff"; but I tend to
favour for makefiles using the no-spaces approach, because sometimes
it does matter.

> 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.

I think you do want quotes for the third word. You don't need
quotes around the NAME part, but I felt it was clearer in that
it hints "these are both strings"; plus it means both arguments
to the macro are being treated the same way.

>> +++ 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?

If we're going to change, then it ought to be BUILD. I left it as-is
since I didn't really want to get into the weeds on that kind of thing
(though as you note below I did fix up one really obvious case).
Arguably we should fix this to BUILD to get it below the 7 char limit.

>>
>>  %.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?

This one's 7 chars (but could be SIGN I guess).

>> +++ 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?

I left it as-is on the assumption the original author had a
reason for wanting to say '-r' in the output.

>>
>>  .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).

If this macro supplies the quotes then suddenly spaces are important
in the callsites.

>> +# 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.

I'd rather leave both of those as-is.

thanks
-- PMM