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