Re: [Qemu-devel] [PATCH] rules.mak: quiet-command: Split command name and args to print
On 20 September 2016 at 20:03, Eric Blakewrote: > 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)$@") >> +
Re: [Qemu-devel] [PATCH] rules.mak: quiet-command: Split command name and args to print
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 >
[Qemu-devel] [PATCH] rules.mak: quiet-command: Split command name and args to print
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. Split the quiet-mode string into two, the "NAME" and the "same args" part, and use printf(1) to format the string automatically. This means we only need to change one place if we want to support a longer maximum name. In particular, we can now print 7-character names lined up properly (they are needed for the OSX "SETTOOL" invocation). Change all the uses of quiet-command to the new syntax. (Any which are missed or inadvertently reintroduced via later merges will result in slightly misformatted quiet output rather than disaster.) Signed-off-by: Peter Maydell--- I finally got irritated enough by the misaligned output doing an OSX build to fix it :-) We know that we always have POSIX printf because we already use it in configure. The bulk of this patch is all the changes of the quiet-command invocations; the interesting bit is the new definition which is buried in the middle somewhere. --- Makefile | 64 +-- Makefile.target | 18 ++-- pc-bios/optionrom/Makefile| 8 +++--- pc-bios/s390-ccw/Makefile | 4 +-- pc-bios/spapr-rtas/Makefile | 4 +-- po/Makefile | 6 ++-- qga/vss-win32/Makefile.objs | 6 ++-- rules.mak | 32 +- target-s390x/Makefile.objs| 4 +-- tests/Makefile.include| 26 +- tests/docker/Makefile.include | 8 +++--- trace/Makefile.objs | 24 12 files changed, 105 insertions(+), 99 deletions(-) diff --git a/Makefile b/Makefile index 444ae37..3369819 100644 --- a/Makefile +++ 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","$@") 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") $(call quiet-command, if test -f $@; then \ if cmp -s $@.old $@; then \ mv $@.tmp $@; \ @@ -135,7 +135,7 @@ endif else \ mv $@.tmp $@; \ cp -p $@ $@.old; \ -fi, " GEN $@"); +fi, "GEN","$@"); defconfig: rm -f config-all-devices.mak $(SUBDIR_DEVICES_MAK) @@ -189,7 +189,7 @@ qemu-version.h: FORCE config-host.h: config-host.h-timestamp config-host.h-timestamp: config-host.mak qemu-options.def: $(SRC_PATH)/qemu-options.hx $(SRC_PATH)/scripts/hxtool - $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > $@," GEN $@") + $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > $@,"GEN","$@") SUBDIR_RULES=$(patsubst %,subdir-%, $(TARGET_DIRS)) SOFTMMU_SUBDIR_RULES=$(filter %-softmmu,$(SUBDIR_RULES)) @@ -233,9 +233,9 @@ ALL_SUBDIRS=$(TARGET_DIRS) $(patsubst %,pc-bios/%, $(ROMS)) recurse-all: $(SUBDIR_RULES) $(ROMSUBDIR_RULES) $(BUILD_DIR)/version.o: $(SRC_PATH)/version.rc config-host.h | $(BUILD_DIR)/version.lo - $(call quiet-command,$(WINDRES) -I$(BUILD_DIR) -o $@ $<," RC version.o") + $(call quiet-command,$(WINDRES) -I$(BUILD_DIR) -o $@ $<,"RC","version.o") $(BUILD_DIR)/version.lo: $(SRC_PATH)/version.rc config-host.h - $(call quiet-command,$(WINDRES) -I$(BUILD_DIR) -o $@ $<," RC version.lo") + $(call quiet-command,$(WINDRES) -I$(BUILD_DIR) -o $@ $<,"RC","version.lo") Makefile: $(version-obj-y) $(version-lobj-y) @@ -262,7 +262,7 @@ fsdev/virtfs-proxy-helper$(EXESUF): fsdev/virtfs-proxy-helper.o fsdev/9p-marshal fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx $(SRC_PATH)/scripts/hxtool - $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > $@," GEN $@") +