[libvirt] [PATCH] Use SED variable for `sed` binary in src/Makefile
Hi, I've been trying to get libvirt (client) to cleanly/easily compile on BSD-based systems (in this case OS X). "./configure" runs fine but the "make" caused an error with `sed` since BSD sed was reporting some sort of regex error. I realized that the "SED" variable was populated with "gsed" which worked properly. This made the make continue (failed again later, will address that when I can). This is my first contribution to a C-based project and I don't have much experience with autotools other than using them as a consumer. Let me know if anything can be improved. Thank you, Mitchell
Hm, the patch attached as binary data. Here is the attached patch as text. Apologies about that: --- src/Makefile.am | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index b321657..118c329 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1029,14 +1029,14 @@ libvirt.syms: libvirt_public.syms $(USED_SYM_FILES) libvirt.def: libvirt.syms $(AM_V_GEN)rm -f -- $@-tmp $@ ; \ printf 'EXPORTS\n' > $@-tmp && \ - sed -e '/^$$/d; /#/d; /:/d; /\}/d; /\*/d; /LIBVIRT_/d; s/[ \t]*\(.*\)\;/ \1/g' $^ >> $@-tmp && \ + $(SED) -e '/^$$/d; /#/d; /:/d; /\}/d; /\*/d; /LIBVIRT_/d; s/[ \t]*\(.*\)\;/ \1/g' $^ >> $@-tmp && \ chmod a-w $@-tmp && \ mv $@-tmp libvirt.def libvirt_qemu.def: $(srcdir)/libvirt_qemu.syms $(AM_V_GEN)rm -f -- $@-tmp $@ ; \ printf 'EXPORTS\n' > $@-tmp && \ - sed -e '/^$$/d; /#/d; /:/d; /\}/d; /\*/d; /LIBVIRT_/d; s/[ \t]*\(.*\)\;/ \1/g' $^ >> $@-tmp && \ + $(SED) -e '/^$$/d; /#/d; /:/d; /\}/d; /\*/d; /LIBVIRT_/d; s/[ \t]*\(.*\)\;/ \1/g' $^ >> $@-tmp && \ chmod a-w $@-tmp && \ mv $@-tmp libvirt_qemu.def -- 1.7.2.2 On Wed, Sep 8, 2010 at 11:39 PM, Mitchell Hashimoto < mitchell.hashimoto@gmail.com> wrote:
Hi,
I've been trying to get libvirt (client) to cleanly/easily compile on BSD-based systems (in this case OS X). "./configure" runs fine but the "make" caused an error with `sed` since BSD sed was reporting some sort of regex error. I realized that the "SED" variable was populated with "gsed" which worked properly. This made the make continue (failed again later, will address that when I can).
This is my first contribution to a C-based project and I don't have much experience with autotools other than using them as a consumer. Let me know if anything can be improved.
Thank you, Mitchell
On Wed, Sep 08, 2010 at 11:41:52PM -0700, Mitchell Hashimoto wrote:
Hm, the patch attached as binary data. Here is the attached patch as text. Apologies about that:
--- src/Makefile.am | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index b321657..118c329 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1029,14 +1029,14 @@ libvirt.syms: libvirt_public.syms $(USED_SYM_FILES) libvirt.def: libvirt.syms $(AM_V_GEN)rm -f -- $@-tmp $@ ; \ printf 'EXPORTS\n' > $@-tmp && \ - sed -e '/^$$/d; /#/d; /:/d; /\}/d; /\*/d; /LIBVIRT_/d; s/[ \t]*\(.*\)\;/ \1/g' $^ >> $@-tmp && \ + $(SED) -e '/^$$/d; /#/d; /:/d; /\}/d; /\*/d; /LIBVIRT_/d; s/[ \t]*\(.*\)\;/ \1/g' $^ >> $@-tmp && \ chmod a-w $@-tmp && \ mv $@-tmp libvirt.def
libvirt_qemu.def: $(srcdir)/libvirt_qemu.syms $(AM_V_GEN)rm -f -- $@-tmp $@ ; \ printf 'EXPORTS\n' > $@-tmp && \ - sed -e '/^$$/d; /#/d; /:/d; /\}/d; /\*/d; /LIBVIRT_/d; s/[ \t]*\(.*\)\;/ \1/g' $^ >> $@-tmp && \ + $(SED) -e '/^$$/d; /#/d; /:/d; /\}/d; /\*/d; /LIBVIRT_/d; s/[ \t]*\(.*\)\;/ \1/g' $^ >> $@-tmp && \ chmod a-w $@-tmp && \ mv $@-tmp libvirt_qemu.def
I think you also need to add AC_PROG_SED to configure.ac. At the moment it is being defined, but only incidentally because of some other macro. There are also uses of sed, at least one in configure.ac itself ... Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
On 09/09/2010 12:41 AM, Mitchell Hashimoto wrote:
Hi,
I've been trying to get libvirt (client) to cleanly/easily compile on BSD-based systems (in this case OS X). "./configure" runs fine but the "make" caused an error with `sed` since BSD sed was reporting some sort of regex error. I realized that the "SED" variable was populated with "gsed" which worked properly. This made the make continue (failed again later, will address that when I can).
--- src/Makefile.am | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index b321657..118c329 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1029,14 +1029,14 @@ libvirt.syms: libvirt_public.syms $(USED_SYM_FILES) libvirt.def: libvirt.syms $(AM_V_GEN)rm -f -- $@-tmp $@ ; \ printf 'EXPORTS\n'> $@-tmp&& \ - sed -e '/^$$/d; /#/d; /:/d; /\}/d; /\*/d; /LIBVIRT_/d; s/[ \t]*\(.*\)\;/ \1/g' $^>> $@-tmp&& \ + $(SED) -e '/^$$/d; /#/d; /:/d; /\}/d; /\*/d; /LIBVIRT_/d; s/[ \t]*\(.*\)\;/ \1/g' $^>> $@-tmp&& \
Thanks for the report. Hmm - the real problem here is that we are relying on a non-portable sed construct. Using $(SED) is a crutch that will let other users substitute gsed when needed, but the real fix is to correct the sed expression to be portable to POSIX rules in the first place. Neither \} nor \t are portable: $ printf 'a\tc\n}\n' | /usr/bin/sed '/\}/d; s/[\t]/ /' sed: 1: "/\}/d": RE error: parentheses not balanced $ printf 'abc\n}\n' | /usr/bin/sed '/}/d; s/[\t]/ /' a c $ printf 'abc\n}\n' | gsed '/\}/d; s/[\t]/ /' a c So, I'll propose a better patch that fixes the regex to be portable, at which point, I think we no longer need to worry about using $(SED) in the first place. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/Makefile.am (libvirt.def, libvirt_qemu.def): '\}' and '\t' are not required by POSIX. Use '}' and literal tab instead. (install-data-local): Avoid sed -i. * tests/read-bufsiz: Likewise. Reported by Mitchell Hashimoto. --- This should work for you, but I'd appreciate positive feedback from an actual build on OS X. I used cp/rm rather than mv when replacing sed -i, so as to preserve permissions on the just-installed file (in case creation of the temporary file via > gets different permissions due to umask). There was another use of sed -i in libvirt.spec.in, but as that is specific to rpm building, where we know GNU sed will be in use, I didn't see the point in changing it. src/Makefile.am | 12 ++++++++---- tests/read-bufsiz | 6 +++--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index b321657..9bc4287 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1029,14 +1029,14 @@ libvirt.syms: libvirt_public.syms $(USED_SYM_FILES) libvirt.def: libvirt.syms $(AM_V_GEN)rm -f -- $@-tmp $@ ; \ printf 'EXPORTS\n' > $@-tmp && \ - sed -e '/^$$/d; /#/d; /:/d; /\}/d; /\*/d; /LIBVIRT_/d; s/[ \t]*\(.*\)\;/ \1/g' $^ >> $@-tmp && \ + sed -e '/^$$/d; /#/d; /:/d; /}/d; /\*/d; /LIBVIRT_/d; s/[ ]*\(.*\)\;/ \1/g' $^ >> $@-tmp && \ chmod a-w $@-tmp && \ mv $@-tmp libvirt.def libvirt_qemu.def: $(srcdir)/libvirt_qemu.syms $(AM_V_GEN)rm -f -- $@-tmp $@ ; \ printf 'EXPORTS\n' > $@-tmp && \ - sed -e '/^$$/d; /#/d; /:/d; /\}/d; /\*/d; /LIBVIRT_/d; s/[ \t]*\(.*\)\;/ \1/g' $^ >> $@-tmp && \ + sed -e '/^$$/d; /#/d; /:/d; /}/d; /\*/d; /LIBVIRT_/d; s/[ ]*\(.*\)\;/ \1/g' $^ >> $@-tmp && \ chmod a-w $@-tmp && \ mv $@-tmp libvirt_qemu.def @@ -1177,8 +1177,12 @@ if WITH_NETWORK $(INSTALL_DATA) $(srcdir)/network/default.xml \ $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml test -z "$(UUID)" || \ - sed -i -e "s,</name>,</name>\n <uuid>$(UUID)</uuid>," \ - $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml + { sed -e "s,</name>,</name>\n <uuid>$(UUID)</uuid>," \ + $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml > \ + $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml.t && \ + cp $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml.t \ + $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml && \ + rm $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml.t; } test -e $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/autostart/default.xml || \ ln -s ../default.xml \ $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/autostart/default.xml diff --git a/tests/read-bufsiz b/tests/read-bufsiz index f0f03b9..2a91bcf 100755 --- a/tests/read-bufsiz +++ b/tests/read-bufsiz @@ -1,7 +1,7 @@ #!/bin/sh # ensure that reading a file larger than BUFSIZ works -# Copyright (C) 2008 Red Hat, Inc. +# Copyright (C) 2008, 2010 Red Hat, Inc. # This program is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -30,10 +30,10 @@ fi fail=0 # Output a valid definition, to be used as input. -$abs_top_builddir/tools/virsh -c test:///default dumpxml 1 > xml || fail=1 +$abs_top_builddir/tools/virsh -c test:///default dumpxml 1 > xml.t || fail=1 # Change the VM name -sed -i -e "s|<name>test</name>|<name>newtest</name>|g" xml +sed -e "s|<name>test</name>|<name>newtest</name>|g" xml.t > xml for i in before after; do # The largest BUFSIZ I've seen is 128K. This is slightly larger. -- 1.7.2.2
On 09/10/2010 06:43 PM, Eric Blake wrote:
* src/Makefile.am (libvirt.def, libvirt_qemu.def): '\}' and '\t' are not required by POSIX. Use '}' and literal tab instead. (install-data-local): Avoid sed -i. * tests/read-bufsiz: Likewise. Reported by Mitchell Hashimoto. ---
This should work for you, but I'd appreciate positive feedback from an actual build on OS X.
I used cp/rm rather than mv when replacing sed -i, so as to preserve permissions on the just-installed file (in case creation of the temporary file via> gets different permissions due to umask).
There was another use of sed -i in libvirt.spec.in, but as that is specific to rpm building, where we know GNU sed will be in use, I didn't see the point in changing it.
src/Makefile.am | 12 ++++++++---- tests/read-bufsiz | 6 +++--- 2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index b321657..9bc4287 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1029,14 +1029,14 @@ libvirt.syms: libvirt_public.syms $(USED_SYM_FILES) libvirt.def: libvirt.syms $(AM_V_GEN)rm -f -- $@-tmp $@ ; \ printf 'EXPORTS\n'> $@-tmp&& \ - sed -e '/^$$/d; /#/d; /:/d; /\}/d; /\*/d; /LIBVIRT_/d; s/[ \t]*\(.*\)\;/ \1/g' $^>> $@-tmp&& \ + sed -e '/^$$/d; /#/d; /:/d; /}/d; /\*/d; /LIBVIRT_/d; s/[ ]*\(.*\)\;/ \1/g' $^>> $@-tmp&& \ chmod a-w $@-tmp&& \ mv $@-tmp libvirt.def
libvirt_qemu.def: $(srcdir)/libvirt_qemu.syms $(AM_V_GEN)rm -f -- $@-tmp $@ ; \ printf 'EXPORTS\n'> $@-tmp&& \ - sed -e '/^$$/d; /#/d; /:/d; /\}/d; /\*/d; /LIBVIRT_/d; s/[ \t]*\(.*\)\;/ \1/g' $^>> $@-tmp&& \ + sed -e '/^$$/d; /#/d; /:/d; /}/d; /\*/d; /LIBVIRT_/d; s/[ ]*\(.*\)\;/ \1/g' $^>> $@-tmp&& \ chmod a-w $@-tmp&& \ mv $@-tmp libvirt_qemu.def
@@ -1177,8 +1177,12 @@ if WITH_NETWORK $(INSTALL_DATA) $(srcdir)/network/default.xml \ $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml test -z "$(UUID)" || \ - sed -i -e "s,</name>,</name>\n<uuid>$(UUID)</uuid>," \ - $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml + { sed -e "s,</name>,</name>\n<uuid>$(UUID)</uuid>," \ + $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml> \ + $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml.t&& \ + cp $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml.t \ + $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml&& \ + rm $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml.t; } test -e $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/autostart/default.xml || \ ln -s ../default.xml \ $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/autostart/default.xml diff --git a/tests/read-bufsiz b/tests/read-bufsiz index f0f03b9..2a91bcf 100755 --- a/tests/read-bufsiz +++ b/tests/read-bufsiz @@ -1,7 +1,7 @@ #!/bin/sh # ensure that reading a file larger than BUFSIZ works
-# Copyright (C) 2008 Red Hat, Inc. +# Copyright (C) 2008, 2010 Red Hat, Inc.
# This program is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -30,10 +30,10 @@ fi fail=0
# Output a valid definition, to be used as input. -$abs_top_builddir/tools/virsh -c test:///default dumpxml 1> xml || fail=1 +$abs_top_builddir/tools/virsh -c test:///default dumpxml 1> xml.t || fail=1
# Change the VM name -sed -i -e "s|<name>test</name>|<name>newtest</name>|g" xml +sed -e "s|<name>test</name>|<name>newtest</name>|g" xml.t> xml
for i in before after; do # The largest BUFSIZ I've seen is 128K. This is slightly larger.
ACK Paolo
On 09/14/2010 05:09 AM, Paolo Bonzini wrote:
On 09/10/2010 06:43 PM, Eric Blake wrote:
* src/Makefile.am (libvirt.def, libvirt_qemu.def): '\}' and '\t' are not required by POSIX. Use '}' and literal tab instead. (install-data-local): Avoid sed -i. * tests/read-bufsiz: Likewise. Reported by Mitchell Hashimoto.
ACK
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (4)
-
Eric Blake -
Mitchell Hashimoto -
Paolo Bonzini -
Richard W.M. Jones