On 20 September 2016 at 20:03, Eric Blake <ebl...@redhat.com> 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 $@ > $@.tmp, " GEN $@.tmp") >> + $(SHELL) $(SRC_PATH)/scripts/make_device_config.sh $< >> $*-config-devices.mak.d $@ > $@.tmp, "GEN","$@.tmp") > > 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