From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47137) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmQKT-0000ht-Jj for qemu-devel@nongnu.org; Tue, 20 Sep 2016 15:03:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmQKQ-00046Y-9A for qemu-devel@nongnu.org; Tue, 20 Sep 2016 15:03:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57236) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmQKP-00046G-W9 for qemu-devel@nongnu.org; Tue, 20 Sep 2016 15:03:38 -0400 References: <1474396477-25791-1-git-send-email-peter.maydell@linaro.org> From: Eric Blake Message-ID: <1e4e9527-1d22-5513-ad03-296c7c795dce@redhat.com> Date: Tue, 20 Sep 2016 14:03:35 -0500 MIME-Version: 1.0 In-Reply-To: <1474396477-25791-1-git-send-email-peter.maydell@linaro.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Tqk9nAkll2Um9Nsx4MX8naVW6djLFbM2j" Subject: Re: [Qemu-devel] [PATCH] rules.mak: quiet-command: Split command name and args to print List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell , qemu-devel@nongnu.org Cc: patches@linaro.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Tqk9nAkll2Um9Nsx4MX8naVW6djLFbM2j From: Eric Blake To: Peter Maydell , qemu-devel@nongnu.org Cc: patches@linaro.org Message-ID: <1e4e9527-1d22-5513-ad03-296c7c795dce@redhat.com> Subject: Re: [Qemu-devel] [PATCH] rules.mak: quiet-command: Split command name and args to print References: <1474396477-25791-1-git-send-email-peter.maydell@linaro.org> In-Reply-To: <1474396477-25791-1-git-send-email-peter.maydell@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > +++ b/Makefile > @@ -105,20 +105,20 @@ SUBDIR_DEVICES_MAK_DEP=3D$(patsubst %, %-config-d= evices.mak.d, $(TARGET_DIRS)) > =20 > 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|^\([^=3D]*\)=3D\(.*\)$$|\1:=3D$$(findstring y,$$(\1)\2= )|p' \ > $(SUBDIR_DEVICES_MAK) | sort -u > $@, \ > - " GEN $@") > + "GEN","$@") > endif > =20 > -include $(SUBDIR_DEVICES_MAK_DEP) > =20 > %/config-devices.mak: default-configs/%.mak $(SRC_PATH)/scripts/make_d= evice_config.sh > $(call quiet-command, \ > - $(SHELL) $(SRC_PATH)/scripts/make_device_config.sh $< $*-c= onfig-devices.mak.d $@ > $@.tmp, " GEN $@.tmp") > + $(SHELL) $(SRC_PATH)/scripts/make_device_config.sh $< $*-c= onfig-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/s= haderinclude.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=3D8 --center=3D" " --release=3D" " 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_dm= a.bin kvmvapic.bin > =20 > =20 > %.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)$@") > =20 > %.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? > =20 > %.raw: %.img > - $(call quiet-command,$(OBJCOPY) -O binary -j .text $< $@," Building = $(TARGET_DIR)$@") > + $(call quiet-command,$(OBJCOPY) -O binary -j .text $< $@,"Building","= $(TARGET_DIR)$@") > =20 > %.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? > =20 > 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 +=3D -Wl,-pie -nostdlib > build-all: s390-ccw.img > =20 > s390-ccw.elf: $(OBJECTS) > - $(call quiet-command,$(CC) $(LDFLAGS) -o $@ $(OBJECTS)," Building $(= TARGET_DIR)$@") > + $(call quiet-command,$(CC) $(LDFLAGS) -o $@ $(OBJECTS),"Building","$(= TARGET_DIR)$@") > =20 > s390-ccw.img: s390-ccw.elf > - $(call quiet-command,strip --strip-unneeded $< -o $@," Stripping $(T= ARGET_DIR)$@") > + $(call quiet-command,strip --strip-unneeded $< -o $@,"STRIP","$(TARGE= T_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? > =20 > .PHONY: modules > modules: > @@ -105,9 +105,15 @@ modules: > $(call LINK,$(filter %.o %.a %.mo, $^)) > =20 > %.a: > - $(call quiet-command,rm -f $@ && $(AR) rcs $@ $^," AR $(TARGET_DI= R)$@") > + $(call quiet-command,rm -f $@ && $(AR) rcs $@ $^,"AR","$(TARGET_DIR)$= @") > =20 > -quiet-command =3D $(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=3D1 just print the whole command and args > +# otherwise print the 'quiet' output in the format " NAME args t= o 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 m= ode. > +quiet-command =3D $(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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --Tqk9nAkll2Um9Nsx4MX8naVW6djLFbM2j Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJX4YgHAAoJEKeha0olJ0NqxmAH/25kyRk4nRYJBSK1qv9jfgJX RFqVDnhVl9c8k55xgliFE1GW6MRHn0QKz7r0yXyvkJJ/GZz3RHjvuV+BelO7gtIG +hJQ8MEEsT29HChVgk2P3x804HBlhNZdhqUDtuCqw3fvatSNLsM/w69PywYiZmHp 52331sraSPo07XwyyXLn6wqPKmaWwOFk47ys2v1VB2gUKCVzH1zJICZc8h+fAy9i C+K5ZDySV72hq8QwgUuyKEaaq7ceQiz5he2UTEURxfFSF95oLFpFx01QXHd02X8+ 3TrzCHoUZoBae2syUgLXb+x8jZm7STQT3ZyX8T7HGwmV4r5LeIOS3FdER7qeNyY= =+q04 -----END PGP SIGNATURE----- --Tqk9nAkll2Um9Nsx4MX8naVW6djLFbM2j--