[Libvir] make syntax-check fails with bzr checkouts
I seem to be completely unable to get make syntax-checks to function properly with my bzr checkout of libvirt[1]. I've attached the output as as-is.txt. I tried adding hacking bzr support into vc-list-files (see vc-list-files-bzr.patch), but that didn't quite seem to do the trick, as you can see in in vc-list-files-maybe-fixed.log, which is the output after I patched vc-list-files. I tried CVS, too, and that also fails (see cvs-syntax-check.log). Is it only meant to work with git? [1]: http://bazaar.launchpad.net/~vcs-imports/libvirt/trunk -- Soren Hansen | Virtualisation specialist | Ubuntu Server Team Canonical Ltd. | http://www.ubuntu.com/
On Tue, Apr 29, 2008 at 05:11:59PM +0200, Soren Hansen wrote:
I tried CVS, too, and that also fails (see cvs-syntax-check.log).
weird
Is it only meant to work with git?
Really no, I only use CVS checkouts... Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Tue, Apr 29, 2008 at 05:11:59PM +0200, Soren Hansen wrote:
I seem to be completely unable to get make syntax-checks to function properly with my bzr checkout of libvirt[1]. I've attached the output as as-is.txt. I tried adding hacking bzr support into vc-list-files (see vc-list-files-bzr.patch), but that didn't quite seem to do the trick, as you can see in in vc-list-files-maybe-fixed.log, which is the output after I patched vc-list-files.
I tried CVS, too, and that also fails (see cvs-syntax-check.log).
Is it only meant to work with git?
It works with CVS - I use it with CVS most of the time. This....
--- po-check-1 2008-04-29 17:03:10.452343086 +0200 +++ po-check-2 2008-04-29 17:03:10.756338852 +0200 @@ -1,36 +1,36 @@ -gnulib/lib/gai_strerror.c -qemud/qemud.c -qemud/remote.c -src/conf.c -src/console.c -src/hash.c -src/iptables.c -src/libvirt.c -src/lxc_conf.c -src/lxc_container.c -src/lxc_driver.c -src/openvz_conf.c -src/openvz_driver.c -src/proxy_internal.c -src/qemu_conf.c -src/qemu_driver.c -src/remote_internal.c -src/sexpr.c -src/storage_backend.c -src/storage_backend_disk.c -src/storage_backend_fs.c -src/storage_backend_iscsi.c -src/storage_backend_logical.c -src/storage_conf.c -src/storage_driver.c -src/test.c -src/util.c -src/uuid.c -src/virsh.c -src/virterror.c -src/xen_internal.c -src/xend_internal.c -src/xm_internal.c -src/xml.c -src/xmlrpc.c -src/xs_internal.c +./gnulib/lib/gai_strerror.c +./qemud/qemud.c +./qemud/remote.c +./src/conf.c +./src/console.c +./src/hash.c +./src/iptables.c +./src/libvirt.c +./src/lxc_conf.c +./src/lxc_container.c +./src/lxc_driver.c +./src/openvz_conf.c +./src/openvz_driver.c +./src/proxy_internal.c +./src/qemu_conf.c +./src/qemu_driver.c +./src/remote_internal.c +./src/sexpr.c +./src/storage_backend.c +./src/storage_backend_disk.c +./src/storage_backend_fs.c +./src/storage_backend_iscsi.c +./src/storage_backend_logical.c +./src/storage_conf.c +./src/storage_driver.c +./src/test.c +./src/util.c +./src/uuid.c +./src/virsh.c +./src/virterror.c +./src/xen_internal.c +./src/xend_internal.c +./src/xm_internal.c +./src/xml.c +./src/xmlrpc.c +./src/xs_internal.c
...says that the vc-list-files script is getting a bogus leading ./ on all your filenames. So something must be different about Ubuntu versions of some program that causes this: awk -F/ '{ \ if (!$1 && $3 !~ /^-/) { \ f=FILENAME; \ sub(/CVS\/Entries/, "", f); \ print f $2; \ }}' \ $(find ${*-*} -name Entries -print) /dev/null; to prepend a ./ - it could be the content of CVS/Entries, or perhaps the find command - if you can figure out which, it ought to be easy enough to strip off the ./ Regards, Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
The bzr problem was obviously that vc-list-files didn't support bzr. Failing with CVS was caused by a bashism in vc-list-files. In Ubuntu, /bin/sh points to dash instead of bash, but vc-list-files had "#!/bin/sh". This patch fixes both issues: === modified file 'build-aux/vc-list-files' --- build-aux/vc-list-files 2008-02-01 19:47:07 +0000 +++ build-aux/vc-list-files 2008-04-30 07:43:06 +0000 @@ -39,6 +39,8 @@ exec git ls-files "$dir" elif test -d .hg; then exec hg locate "$dir/*" +elif test -d .bzr; then + exec bzr ls --versioned --kind=file ${*:---from-root} elif test -d CVS; then if test -x build-aux/cvsu; then build-aux/cvsu --find --types=AFGM "$dir" @@ -49,7 +51,7 @@ sub(/CVS\/Entries/, "", f); \ print f $2; \ }}' \ - $(find ${*-*} -name Entries -print) /dev/null; + $(find ${*:-*} -name Entries -print) /dev/null; fi else echo "$0: Failed to determine type of version control used in "`pwd` 1>&2 -- Soren Hansen | Virtualisation specialist | Ubuntu Server Team Canonical Ltd. | http://www.ubuntu.com/
Soren Hansen <soren@ubuntu.com> wrote:
The bzr problem was obviously that vc-list-files didn't support bzr.
Failing with CVS was caused by a bashism in vc-list-files. In Ubuntu, /bin/sh points to dash instead of bash, but vc-list-files had "#!/bin/sh".
This patch fixes both issues:
Hi Soren, Thanks for the patch.
=== modified file 'build-aux/vc-list-files' --- build-aux/vc-list-files 2008-02-01 19:47:07 +0000 +++ build-aux/vc-list-files 2008-04-30 07:43:06 +0000 @@ -39,6 +39,8 @@ exec git ls-files "$dir" elif test -d .hg; then exec hg locate "$dir/*" +elif test -d .bzr; then + exec bzr ls --versioned --kind=file ${*:---from-root}
Unfortunately, the above no longer applies, due to upstream (gnulib) changes to deal with non-srcdir (aka VPATH) builds. I updated libvirt from gnulib just yesterday, and will again, later today. Can you adapt your patch to make bzr work with the newer version?
}}' \ - $(find ${*-*} -name Entries -print) /dev/null; + $(find ${*:-*} -name Entries -print) /dev/null;
Thanks for reporting that. Note though that POSIX appears to require the behavior that bash exhibits, so calling this a bashism doesn't seem right. However, in the spec, just beneath the part that talks about ${parameter-word} there are escape clauses for e.g., ${#parameter} and ${parameter%word} etc.: If parameter is '*' or '@', the result of the expansion is unspecified. but there is no such exemption for the ${parameter-word} syntax. All that to say that this looks more like an infelicity in dash, and since Ubuntu relies on it, you may want to investigate further. However, back to vc-list-files, that awk-based code is so hard to reach for me -- I avoid CVS, and when I do use it, I have cvsu in my path -- that it is rarely tested. Anyhow the use of "*" there is unnecessary, and in addition, there was a subtle (but probably never problematic) quoting bug. Here's the patch I've pushed to gnulib: http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=173a9f0c48a16c3507... If you care about the CVS-oriented cases, there are two more patches. Note that the awk+find-based code *still* fails in some pathological cases, e.g., with a file named 'a b/CVS/Entries' in your hierarchy, but the response there is "just don't do that". Or depend on find -print0+xargs -0, but that would be counter-productive just to avoid a warning in such an unlikely scenario. Jim
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 According to Jim Meyering on 4/30/2008 4:19 AM: |> - $(find ${*-*} -name Entries -print) /dev/null; |> + $(find ${*:-*} -name Entries -print) /dev/null; | | Thanks for reporting that. | Note though that POSIX appears to require the behavior that bash exhibits, | so calling this a bashism doesn't seem right. However, in the spec, | just beneath the part that talks about ${parameter-word} there are escape | clauses for e.g., ${#parameter} and ${parameter%word} etc.: | | If parameter is '*' or '@', the result of the expansion is unspecified. | | but there is no such exemption for the ${parameter-word} syntax. I think POSIX is ambiguous here. For the ${a:-b} (and ${a-b}) form, it only states that if the parameter is null/unset, use the substitute; but it can be argued that * and @ are _always_ set. Maybe it's worth asking the Austin group. But Jim's workaround looked better anyway. - -- Don't work too hard, make some time for fun as well! Eric Blake ebb9@byu.net -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (Cygwin) Comment: Public key at home.comcast.net/~ericblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkgYYJ0ACgkQ84KuGfSFAYDEUwCfVbpDefbEG7jDvE1xb9QN3YQF d78AoK0uSOptJ23xiRSAmtFEXyl4IqhY =WMpq -----END PGP SIGNATURE-----
On Wed, Apr 30, 2008 at 12:19:51PM +0200, Jim Meyering wrote:
Unfortunately, the above no longer applies, due to upstream (gnulib) changes to deal with non-srcdir (aka VPATH) builds. I updated libvirt from gnulib just yesterday, and will again, later today.
Can you adapt your patch to make bzr work with the newer version?
I'll do it when you're done updating gnulib.
}}' \ - $(find ${*-*} -name Entries -print) /dev/null; + $(find ${*:-*} -name Entries -print) /dev/null;
Thanks for reporting that. Note though that POSIX appears to require the behavior that bash exhibits, so calling this a bashism doesn't seem right.
No. POSIX requires that ${*-*} should expand to * if $* is unset (and not if it's null). $* is defined to expand to the positional parameters starting from 1. The spec is not *entirely* clear, I'll give you that, but considering $* to be unset (instead of just null) messes with my sanity. At any rate, relying on bash's interpretation of the spec is a text book example of a bashism, if you ask me.
but there is no such exemption for the ${parameter-word} syntax. All that to say that this looks more like an infelicity in dash, and since Ubuntu relies on it, you may want to investigate further.
Well, if anyone insists on not changing something that only works with bash to something that works with any shell (${*:-*} vs. ${*-*}) , but also refuses to put /bin/bash explicitly as the interpreter is just being pointlessly difficult, IMO.
However, back to vc-list-files, that awk-based code is so hard to reach for me -- I avoid CVS, and when I do use it, I have cvsu in my path -- that it is rarely tested.
Anyhow the use of "*" there is unnecessary, and in addition, there was a subtle (but probably never problematic) quoting bug. Here's the patch I've pushed to gnulib:
http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=173a9f0c48a16c3507...
That patch will make the cvs case fail, too. It will prepend a ./ to all path names which will screw with the comparison, AFAICS.
If you care about the CVS-oriented cases, there are two more patches.
Not really. :) It just popped up when I was trying to determine why my bzr case was failing. -- Soren Hansen | Virtualisation specialist | Ubuntu Server Team Canonical Ltd. | http://www.ubuntu.com/
Soren Hansen <soren@ubuntu.com> wrote:
On Wed, Apr 30, 2008 at 12:19:51PM +0200, Jim Meyering wrote:
Unfortunately, the above no longer applies, due to upstream (gnulib) changes to deal with non-srcdir (aka VPATH) builds. I updated libvirt from gnulib just yesterday, and will again, later today.
Can you adapt your patch to make bzr work with the newer version?
I'll do it when you're done updating gnulib.
I'm done for now, and pulled the new version into libvirt. ...
Well, if anyone insists on not changing something that only works with bash to something that works with any shell (${*:-*} vs. ${*-*}) , but also refuses to put /bin/bash explicitly as the interpreter is just being pointlessly difficult, IMO.
Um... maybe you didn't notice that I removed the problematic syntax altogether? ...
Here's the patch I've pushed to gnulib:
http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=173a9f0c48a16c3507...
That patch will make the cvs case fail, too. It will prepend a ./ to all path names which will screw with the comparison, AFAICS.
It handled one case. The two subsequent patches addressed the others.
If you care about the CVS-oriented cases, there are two more patches.
On Wed, Apr 30, 2008 at 07:24:42PM +0200, Jim Meyering wrote:
Unfortunately, the above no longer applies, due to upstream (gnulib) changes to deal with non-srcdir (aka VPATH) builds. I updated libvirt from gnulib just yesterday, and will again, later today. Can you adapt your patch to make bzr work with the newer version? I'll do it when you're done updating gnulib. I'm done for now, and pulled the new version into libvirt.
Ok, I'll fix up the patch later tonight.
Well, if anyone insists on not changing something that only works with bash to something that works with any shell (${*:-*} vs. ${*-*}) , but also refuses to put /bin/bash explicitly as the interpreter is just being pointlessly difficult, IMO. Um... maybe you didn't notice that I removed the problematic syntax altogether?
Sure. I'm just pointing out that if *someone* were to insist that this is "an Ubuntu problem, because we don't use the same /bin/sh as most other distros", I'd be rather annoyed.
Here's the patch I've pushed to gnulib:
http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=173a9f0c48a16c3507...
That patch will make the cvs case fail, too. It will prepend a ./ to all path names which will screw with the comparison, AFAICS. It handled one case. The two subsequent patches addressed the others.
Oh, ok. Sorry. I thought they would adress different things. I can't see them in that git tree, I think? -- Soren Hansen | Virtualisation specialist | Ubuntu Server Team Canonical Ltd. | http://www.ubuntu.com/
Soren Hansen <soren@ubuntu.com> wrote: ...
Here's the patch I've pushed to gnulib:
http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=173a9f0c48a16c3507...
That patch will make the cvs case fail, too. It will prepend a ./ to all path names which will screw with the comparison, AFAICS. It handled one case. The two subsequent patches addressed the others.
Oh, ok. Sorry. I thought they would adress different things. I can't see them in that git tree, I think?
Looking at the summary, I see three change sets that mention vc-list-files: http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=summary
On Wed, Apr 30, 2008 at 12:19:51PM +0200, Jim Meyering wrote:
Unfortunately, the above no longer applies, due to upstream (gnulib) changes to deal with non-srcdir (aka VPATH) builds. I updated libvirt from gnulib just yesterday, and will again, later today.
Here's the new patch that seems to do the trick: === modified file 'build-aux/vc-list-files' --- old/build-aux/vc-list-files 2008-04-30 16:11:08 +0000 +++ new/build-aux/vc-list-files 2008-05-06 12:25:50 +0000 @@ -75,6 +75,9 @@ eval exec git ls-files '"$dir"' $postprocess elif test -d .hg; then eval exec hg locate '"$dir/*"' $postprocess +elif test -d .bzr; then + test "$postprocess" = '' && postprocess="| sed 's|^\./||'" + eval exec bzr ls --versioned '"$dir"' $postprocess elif test -d CVS; then test "$postprocess" = '' && postprocess="| sed 's|^\./||'" if test -x build-aux/cvsu; then -- Soren Hansen | Virtualisation specialist | Ubuntu Server Team Canonical Ltd. | http://www.ubuntu.com/
Soren Hansen <soren@ubuntu.com> wrote:
On Wed, Apr 30, 2008 at 12:19:51PM +0200, Jim Meyering wrote:
Unfortunately, the above no longer applies, due to upstream (gnulib) changes to deal with non-srcdir (aka VPATH) builds. I updated libvirt from gnulib just yesterday, and will again, later today.
Here's the new patch that seems to do the trick:
=== modified file 'build-aux/vc-list-files' --- old/build-aux/vc-list-files 2008-04-30 16:11:08 +0000 +++ new/build-aux/vc-list-files 2008-05-06 12:25:50 +0000 @@ -75,6 +75,9 @@ eval exec git ls-files '"$dir"' $postprocess elif test -d .hg; then eval exec hg locate '"$dir/*"' $postprocess +elif test -d .bzr; then + test "$postprocess" = '' && postprocess="| sed 's|^\./||'" + eval exec bzr ls --versioned '"$dir"' $postprocess elif test -d CVS; then test "$postprocess" = '' && postprocess="| sed 's|^\./||'" if test -x build-aux/cvsu; then
Thanks. That looks fine. I've pushed it into gnulib (upstream), http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=176956aa54 and will apply here in libvirt shortly.
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Jim Meyering -
Soren Hansen