* [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion @ 2018-02-14 17:31 Thomas Huth 2018-02-14 18:33 ` Peter Maydell 2018-02-14 20:23 ` Eric Blake 0 siblings, 2 replies; 8+ messages in thread From: Thomas Huth @ 2018-02-14 17:31 UTC (permalink / raw) To: qemu-devel; +Cc: qemu-trivial, Paolo Bonzini When running configure with --with-pkgversion=foo there is no space anymore between the version number and the parentheses: $ m68k-softmmu/qemu-system-m68k -version QEMU emulator version 2.11.50(foo) Fix it by moving the space from the configure script to the Makefile. Fixes: 67a1de0d195a6185c39b436159c9ffc7720bf979 Buglink: https://bugs.launchpad.net/qemu/+bug/1673373 Signed-off-by: Thomas Huth <thuth@redhat.com> --- Makefile | 2 +- configure | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 4ec7a3c..41adbc9 100644 --- a/Makefile +++ b/Makefile @@ -369,7 +369,7 @@ qemu-version.h: FORCE (cd $(SRC_PATH); \ printf '#define QEMU_PKGVERSION '; \ if test -n "$(PKGVERSION)"; then \ - printf '"$(PKGVERSION)"\n'; \ + printf '" ($(PKGVERSION))"\n'; \ else \ if test -d .git; then \ printf '" ('; \ diff --git a/configure b/configure index fe9eea9..1fc687c 100755 --- a/configure +++ b/configure @@ -1162,7 +1162,7 @@ for opt do ;; --disable-blobs) blobs="no" ;; - --with-pkgversion=*) pkgversion=" ($optarg)" + --with-pkgversion=*) pkgversion="$optarg" ;; --with-coroutine=*) coroutine="$optarg" ;; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion 2018-02-14 17:31 [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion Thomas Huth @ 2018-02-14 18:33 ` Peter Maydell 2018-02-14 20:00 ` Thomas Huth 2018-02-14 20:23 ` Eric Blake 1 sibling, 1 reply; 8+ messages in thread From: Peter Maydell @ 2018-02-14 18:33 UTC (permalink / raw) To: Thomas Huth; +Cc: QEMU Developers, QEMU Trivial, Paolo Bonzini On 14 February 2018 at 17:31, Thomas Huth <thuth@redhat.com> wrote: > When running configure with --with-pkgversion=foo there is no > space anymore between the version number and the parentheses: > > $ m68k-softmmu/qemu-system-m68k -version > QEMU emulator version 2.11.50(foo) > > Fix it by moving the space from the configure script to the Makefile. > > Fixes: 67a1de0d195a6185c39b436159c9ffc7720bf979 > Buglink: https://bugs.launchpad.net/qemu/+bug/1673373 > Signed-off-by: Thomas Huth <thuth@redhat.com> I see that in the bug report I wrote "Also it looks like we return QEMU_PKGVERSION as part of the QMP qmp_query_version() code, so we should check to see what the expected behaviour there is regarding having the space or not." -- does this patch change the QMP reported string unexpectedly? thanks -- PMM ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion 2018-02-14 18:33 ` Peter Maydell @ 2018-02-14 20:00 ` Thomas Huth 2018-02-14 20:15 ` Eric Blake 0 siblings, 1 reply; 8+ messages in thread From: Thomas Huth @ 2018-02-14 20:00 UTC (permalink / raw) To: Peter Maydell; +Cc: QEMU Trivial, Paolo Bonzini, QEMU Developers On 14.02.2018 19:33, Peter Maydell wrote: > On 14 February 2018 at 17:31, Thomas Huth <thuth@redhat.com> wrote: >> When running configure with --with-pkgversion=foo there is no >> space anymore between the version number and the parentheses: >> >> $ m68k-softmmu/qemu-system-m68k -version >> QEMU emulator version 2.11.50(foo) >> >> Fix it by moving the space from the configure script to the Makefile. >> >> Fixes: 67a1de0d195a6185c39b436159c9ffc7720bf979 >> Buglink: https://bugs.launchpad.net/qemu/+bug/1673373 >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > I see that in the bug report I wrote > "Also it looks like we return QEMU_PKGVERSION as part of the > QMP qmp_query_version() code, so we should check to see what > the expected behaviour there is regarding having the space > or not." > > -- does this patch change the QMP reported string > unexpectedly? Without my patch and with --with-pkgversion=foo : { "execute": "query-version" } {"return": {"qemu": {"micro": 50, "minor": 11, "major": 2}, "package": "(foo)"}} With my patch and with --with-pkgversion=foo : { "execute": "query-version" } {"return": {"qemu": {"micro": 50, "minor": 11, "major": 2}, "package": " (foo)"}} Without my patch and without --with-pkgversion : { "execute": "query-version" } {"return": {"qemu": {"micro": 50, "minor": 11, "major": 2}, "package": " (v2.11.0-1512-g02f4fbe)"}} Using the old QEMU version 67a1de0d19~1 with --with-pkgversion=foo : { "execute": "query-version" } {"return": {"qemu": {"micro": 50, "minor": 6, "major": 2}, "package": " (foo)"}} So yes, this patch changes the behavior of query-version, but the new behavior is now the same behavior that you get without --with-pkgversion (i.e. a space is included) and it is the same as the behavior that we had in the past, before commit 67a1de0d19 has been merged. So I think this is the right way to go. Alternatively, we could maybe change query-version to always skip the initial space? Thomas ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion 2018-02-14 20:00 ` Thomas Huth @ 2018-02-14 20:15 ` Eric Blake 0 siblings, 0 replies; 8+ messages in thread From: Eric Blake @ 2018-02-14 20:15 UTC (permalink / raw) To: Thomas Huth, Peter Maydell; +Cc: QEMU Trivial, Paolo Bonzini, QEMU Developers On 02/14/2018 02:00 PM, Thomas Huth wrote: > On 14.02.2018 19:33, Peter Maydell wrote: >> On 14 February 2018 at 17:31, Thomas Huth <thuth@redhat.com> wrote: >>> When running configure with --with-pkgversion=foo there is no >>> space anymore between the version number and the parentheses: >>> >>> $ m68k-softmmu/qemu-system-m68k -version >>> QEMU emulator version 2.11.50(foo) It would be nice to document this as '--version' rather than '-version' (both work; but see our BiteSized task: https://wiki.qemu.org/BiteSizedTasks#Consistent_option_usage_in_documentation) >> >> -- does this patch change the QMP reported string >> unexpectedly? > > Without my patch and with --with-pkgversion=foo : > > { "execute": "query-version" } > {"return": {"qemu": {"micro": 50, "minor": 11, "major": 2}, "package": "(foo)"}} Looks nice. And the version info is ALSO passed as part of the initial handshake, even before you call query-version. > > With my patch and with --with-pkgversion=foo : > > { "execute": "query-version" } > {"return": {"qemu": {"micro": 50, "minor": 11, "major": 2}, "package": " (foo)"}} Potential regression (arguably cosmetic, though) > > Without my patch and without --with-pkgversion : > > { "execute": "query-version" } > {"return": {"qemu": {"micro": 50, "minor": 11, "major": 2}, "package": " (v2.11.0-1512-g02f4fbe)"}} And that means we're already inconsistent, so your patch at least made things consistent, > > Using the old QEMU version 67a1de0d19~1 with --with-pkgversion=foo : > > { "execute": "query-version" } > {"return": {"qemu": {"micro": 50, "minor": 6, "major": 2}, "package": " (foo)"}} and that means we've already "regressed", which further means: - it definitely is cosmetic, if no one is complaining - changing it to look nicer won't break anyone > > So yes, this patch changes the behavior of query-version, but the new > behavior is now the same behavior that you get without --with-pkgversion > (i.e. a space is included) and it is the same as the behavior that we had > in the past, before commit 67a1de0d19 has been merged. So I think this > is the right way to go. > Alternatively, we could maybe change query-version to always skip the > initial space? Yes, I like that option. So, if I'm summarizing correctly, your v2 patch will have: $ qemu --version QEMU emulator version 2.11.50 (foo) QMP query-version {"return": {"qemu": {"micro": 50, "minor": 6, "major": 2}, "package": "(foo)"}} regardless of whether --with-pkgversion was used during configure. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion 2018-02-14 17:31 [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion Thomas Huth 2018-02-14 18:33 ` Peter Maydell @ 2018-02-14 20:23 ` Eric Blake 2018-02-15 6:02 ` Thomas Huth 1 sibling, 1 reply; 8+ messages in thread From: Eric Blake @ 2018-02-14 20:23 UTC (permalink / raw) To: Thomas Huth, qemu-devel; +Cc: qemu-trivial, Paolo Bonzini On 02/14/2018 11:31 AM, Thomas Huth wrote: > When running configure with --with-pkgversion=foo there is no > space anymore between the version number and the parentheses: > > $ m68k-softmmu/qemu-system-m68k -version > QEMU emulator version 2.11.50(foo) > > Fix it by moving the space from the configure script to the Makefile. > > Fixes: 67a1de0d195a6185c39b436159c9ffc7720bf979 > Buglink: https://bugs.launchpad.net/qemu/+bug/1673373 > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > Makefile | 2 +- > configure | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index 4ec7a3c..41adbc9 100644 > --- a/Makefile > +++ b/Makefile > @@ -369,7 +369,7 @@ qemu-version.h: FORCE > (cd $(SRC_PATH); \ > printf '#define QEMU_PKGVERSION '; \ > if test -n "$(PKGVERSION)"; then \ > - printf '"$(PKGVERSION)"\n'; \ > + printf '" ($(PKGVERSION))"\n'; \ I would argue that putting a space here is awkward; wouldn't it instead be easier to have all CLIENTS of QEMU_PKGVERSION in the source code assume that the macro does NOT have a leading space, and to supply a space themselves? That is, change THESE locations: bsd-user/main.c: printf("qemu-" TARGET_NAME " version " QEMU_VERSION QEMU_PKGVERSION linux-user/main.c: printf("qemu-" TARGET_NAME " version " QEMU_VERSION QEMU_PKGVERSION qemu-img.c:#define QEMU_IMG_VERSION "qemu-img version " QEMU_VERSION QEMU_PKGVERSION \ qemu-io.c: printf("%s version " QEMU_VERSION QEMU_PKGVERSION "\n" qemu-nbd.c:"%s " QEMU_VERSION QEMU_PKGVERSION "\n" qga/main.c:"QEMU Guest Agent " QEMU_VERSION QEMU_PKGVERSION "\n" scsi/qemu-pr-helper.c:"%s " QEMU_VERSION QEMU_PKGVERSION "\n" ui/cocoa.m: @"QEMU emulator version %s%s", QEMU_VERSION, QEMU_PKGVERSION]; vl.c: printf("QEMU emulator version " QEMU_VERSION QEMU_PKGVERSION "\n" to instead supply the missing space, and have configure/Makefile always generate without a leading space. > +++ b/configure > @@ -1162,7 +1162,7 @@ for opt do > ;; > --disable-blobs) blobs="no" > ;; > - --with-pkgversion=*) pkgversion=" ($optarg)" > + --with-pkgversion=*) pkgversion="$optarg" Hmm - here you're changing who supplies the (). But that argues that maybe the callsites should supply " (" and ")" themselves. Here's how coreutils does it, by using gnulib's version-etc module: src/local.mk: $(AM_V_at)printf 'char const *Version = "$(PACKAGE_VERSION)";\n' >> $@t src/system.h: version_etc (stdout, Program_name, PACKAGE_NAME, Version, Authors, version_etc_arn (FILE *stream, const char *command_name, const char *package, const char *version, const char * const * authors, size_t n_authors) { if (command_name) fprintf (stream, "%s (%s) %s\n", command_name, package, version); which means that the Makefile magic outputs JUST the text that goes inside the (), and the callsite that outputs version information is what supplies the " (" and ")". -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion 2018-02-14 20:23 ` Eric Blake @ 2018-02-15 6:02 ` Thomas Huth 2018-02-15 10:08 ` Daniel P. Berrangé 0 siblings, 1 reply; 8+ messages in thread From: Thomas Huth @ 2018-02-15 6:02 UTC (permalink / raw) To: Eric Blake, qemu-devel Cc: qemu-trivial, Paolo Bonzini, Peter Maydell, Markus Armbruster On 14.02.2018 21:23, Eric Blake wrote: > On 02/14/2018 11:31 AM, Thomas Huth wrote: >> When running configure with --with-pkgversion=foo there is no >> space anymore between the version number and the parentheses: >> >> $ m68k-softmmu/qemu-system-m68k -version >> QEMU emulator version 2.11.50(foo) >> >> Fix it by moving the space from the configure script to the Makefile. >> >> Fixes: 67a1de0d195a6185c39b436159c9ffc7720bf979 >> Buglink: https://bugs.launchpad.net/qemu/+bug/1673373 >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> Makefile | 2 +- >> configure | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/Makefile b/Makefile >> index 4ec7a3c..41adbc9 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -369,7 +369,7 @@ qemu-version.h: FORCE >> (cd $(SRC_PATH); \ >> printf '#define QEMU_PKGVERSION '; \ >> if test -n "$(PKGVERSION)"; then \ >> - printf '"$(PKGVERSION)"\n'; \ >> + printf '" ($(PKGVERSION))"\n'; \ > > I would argue that putting a space here is awkward; wouldn't it instead > be easier to have all CLIENTS of QEMU_PKGVERSION in the source code > assume that the macro does NOT have a leading space, and to supply a > space themselves? > > That is, change THESE locations: > > bsd-user/main.c: printf("qemu-" TARGET_NAME " version " QEMU_VERSION > QEMU_PKGVERSION > linux-user/main.c: printf("qemu-" TARGET_NAME " version " > QEMU_VERSION QEMU_PKGVERSION > qemu-img.c:#define QEMU_IMG_VERSION "qemu-img version " QEMU_VERSION > QEMU_PKGVERSION \ > qemu-io.c: printf("%s version " QEMU_VERSION QEMU_PKGVERSION > "\n" > qemu-nbd.c:"%s " QEMU_VERSION QEMU_PKGVERSION "\n" > qga/main.c:"QEMU Guest Agent " QEMU_VERSION QEMU_PKGVERSION "\n" > scsi/qemu-pr-helper.c:"%s " QEMU_VERSION QEMU_PKGVERSION "\n" > ui/cocoa.m: @"QEMU emulator version %s%s", QEMU_VERSION, > QEMU_PKGVERSION]; > vl.c: printf("QEMU emulator version " QEMU_VERSION QEMU_PKGVERSION "\n" > > to instead supply the missing space, and have configure/Makefile always > generate without a leading space. > >> +++ b/configure >> @@ -1162,7 +1162,7 @@ for opt do >> ;; >> --disable-blobs) blobs="no" >> ;; >> - --with-pkgversion=*) pkgversion=" ($optarg)" >> + --with-pkgversion=*) pkgversion="$optarg" > > Hmm - here you're changing who supplies the (). But that argues that > maybe the callsites should supply " (" and ")" themselves. Yeah, that's likely the saner way to do this. The question is: What about the query-version QMP command? Should it report parentheses or not? I think I'd look nicer if it reports "package": "foo" instead of "package": "(foo)" - but we maybe could break some users who expect parentheses there (no matter whether there is a preceding space or not)? Thomas ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion 2018-02-15 6:02 ` Thomas Huth @ 2018-02-15 10:08 ` Daniel P. Berrangé 2018-02-15 14:11 ` Eric Blake 0 siblings, 1 reply; 8+ messages in thread From: Daniel P. Berrangé @ 2018-02-15 10:08 UTC (permalink / raw) To: Thomas Huth Cc: Eric Blake, qemu-devel, qemu-trivial, Paolo Bonzini, Markus Armbruster, Peter Maydell On Thu, Feb 15, 2018 at 07:02:40AM +0100, Thomas Huth wrote: > On 14.02.2018 21:23, Eric Blake wrote: > > On 02/14/2018 11:31 AM, Thomas Huth wrote: > >> When running configure with --with-pkgversion=foo there is no > >> space anymore between the version number and the parentheses: > >> > >> $ m68k-softmmu/qemu-system-m68k -version > >> QEMU emulator version 2.11.50(foo) > >> > >> Fix it by moving the space from the configure script to the Makefile. > >> > >> Fixes: 67a1de0d195a6185c39b436159c9ffc7720bf979 > >> Buglink: https://bugs.launchpad.net/qemu/+bug/1673373 > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > >> --- > >> Makefile | 2 +- > >> configure | 2 +- > >> 2 files changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/Makefile b/Makefile > >> index 4ec7a3c..41adbc9 100644 > >> --- a/Makefile > >> +++ b/Makefile > >> @@ -369,7 +369,7 @@ qemu-version.h: FORCE > >> (cd $(SRC_PATH); \ > >> printf '#define QEMU_PKGVERSION '; \ > >> if test -n "$(PKGVERSION)"; then \ > >> - printf '"$(PKGVERSION)"\n'; \ > >> + printf '" ($(PKGVERSION))"\n'; \ > > > > I would argue that putting a space here is awkward; wouldn't it instead > > be easier to have all CLIENTS of QEMU_PKGVERSION in the source code > > assume that the macro does NOT have a leading space, and to supply a > > space themselves? > > > > That is, change THESE locations: > > > > bsd-user/main.c: printf("qemu-" TARGET_NAME " version " QEMU_VERSION > > QEMU_PKGVERSION > > linux-user/main.c: printf("qemu-" TARGET_NAME " version " > > QEMU_VERSION QEMU_PKGVERSION > > qemu-img.c:#define QEMU_IMG_VERSION "qemu-img version " QEMU_VERSION > > QEMU_PKGVERSION \ > > qemu-io.c: printf("%s version " QEMU_VERSION QEMU_PKGVERSION > > "\n" > > qemu-nbd.c:"%s " QEMU_VERSION QEMU_PKGVERSION "\n" > > qga/main.c:"QEMU Guest Agent " QEMU_VERSION QEMU_PKGVERSION "\n" > > scsi/qemu-pr-helper.c:"%s " QEMU_VERSION QEMU_PKGVERSION "\n" > > ui/cocoa.m: @"QEMU emulator version %s%s", QEMU_VERSION, > > QEMU_PKGVERSION]; > > vl.c: printf("QEMU emulator version " QEMU_VERSION QEMU_PKGVERSION "\n" > > > > to instead supply the missing space, and have configure/Makefile always > > generate without a leading space. > > > >> +++ b/configure > >> @@ -1162,7 +1162,7 @@ for opt do > >> ;; > >> --disable-blobs) blobs="no" > >> ;; > >> - --with-pkgversion=*) pkgversion=" ($optarg)" > >> + --with-pkgversion=*) pkgversion="$optarg" > > > > Hmm - here you're changing who supplies the (). But that argues that > > maybe the callsites should supply " (" and ")" themselves. > > Yeah, that's likely the saner way to do this. The question is: What > about the query-version QMP command? Should it report parentheses or > not? I think I'd look nicer if it reports "package": "foo" instead of > "package": "(foo)" - but we maybe could break some users who expect > parentheses there (no matter whether there is a preceding space or not)? The pkgversion is an opaque string - users/apps should never try to interpret its contents, because its format can vary arbitrarily between distros. It is merely intended as an informative string to help the package maintainer identify which version of QEMU was used when someone submits a bug reoprt. IOW it is totally valid to change the command to omit '()', and if anything breaks that is their own fault for trying to interpret an opaque blob of data. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion 2018-02-15 10:08 ` Daniel P. Berrangé @ 2018-02-15 14:11 ` Eric Blake 0 siblings, 0 replies; 8+ messages in thread From: Eric Blake @ 2018-02-15 14:11 UTC (permalink / raw) To: Daniel P. Berrangé, Thomas Huth Cc: qemu-devel, qemu-trivial, Paolo Bonzini, Markus Armbruster, Peter Maydell On 02/15/2018 04:08 AM, Daniel P. Berrangé wrote: >>> Hmm - here you're changing who supplies the (). But that argues that >>> maybe the callsites should supply " (" and ")" themselves. >> >> Yeah, that's likely the saner way to do this. The question is: What >> about the query-version QMP command? Should it report parentheses or >> not? I think I'd look nicer if it reports "package": "foo" instead of >> "package": "(foo)" - but we maybe could break some users who expect >> parentheses there (no matter whether there is a preceding space or not)? > > The pkgversion is an opaque string - users/apps should never try to > interpret its contents, because its format can vary arbitrarily between > distros. It is merely intended as an informative string to help the > package maintainer identify which version of QEMU was used when someone > submits a bug reoprt. > > IOW it is totally valid to change the command to omit '()', and if anything > breaks that is their own fault for trying to interpret an opaque blob of > data. Agreed - the fact that we had a leading space in the QMP output for a long time (and even inconsistent at whether it was there), with no one noticing, means that dropping the () from QMP won't hurt anyone. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-02-15 14:12 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-02-14 17:31 [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion Thomas Huth 2018-02-14 18:33 ` Peter Maydell 2018-02-14 20:00 ` Thomas Huth 2018-02-14 20:15 ` Eric Blake 2018-02-14 20:23 ` Eric Blake 2018-02-15 6:02 ` Thomas Huth 2018-02-15 10:08 ` Daniel P. Berrangé 2018-02-15 14:11 ` Eric Blake
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).