10:11 a.m.
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/
10:14 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
10:23 a.m.
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 :|
2:48 a.m.
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/
5:19 a.m.
Soren Hansen <soren(a)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=173a9f0c48a16c3...
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
7:05 a.m.
-----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(a)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-----
12:05 p.m.
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=173a9f0c48a16c3...
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/
12:24 p.m.
Soren Hansen <soren(a)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=173a9f0c48a16c3...
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.
12:30 p.m.
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=173a9f0c48a16c3...
>
> 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/
12:56 p.m.
Soren Hansen <soren(a)ubuntu.com> wrote:
...
> >> Here's the patch I've pushed to gnulib:
> >>
> >>
http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=173a9f0c48a16c3...
> >
> > 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
3:48 p.m.
New subject: [libvirt] Re: [Libvir] make syntax-check fails with bzr checkouts
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/
4:17 p.m.
New subject: [libvirt] Re: [Libvir] make syntax-check fails with bzr checkouts
Soren Hansen <soren(a)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.
6043
days inactive
6050
days old
11 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Jim Meyering -
Soren Hansen