| Message ID | 1518629505-22480-1-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | configure: Add missing space when using --with-pkgversion | expand |
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
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
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.
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 ")".
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
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
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.
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" ;;
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(-)