[libvirt] [PATCH 0/3] spec file fixes
Several spec file patches that I've noticed by comparing against open BZs and existing RPMs. Probably all worth including in 0.9.8, but I'm not comfortable pushing them without a review. Eric Blake (3): spec: always autoreconf when building rpm spec: add dmidecode as prereq spec: fix sanlock dependency libvirt.spec.in | 30 +++++++++++++++++++++--------- 1 files changed, 21 insertions(+), 9 deletions(-) -- 1.7.7.3
Over time, Fedora and RHEL RPMs have often backported upstream patches that touched configure.ac and/or Makefile.am; this necessitates rerunning the autotools for the patch to be effective. Making this part of the spec file will make it easier for future backports to pull patches without thinking about this issue. * libvirt.spec.in (BuildRequires): Add autotools. (%build): Use them before configure. --- libvirt.spec.in | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 06c949b..462421a 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -334,6 +334,9 @@ Requires: libcgroup %endif # All build-time requirements +BuildRequires: autoconf +BuildRequires: automake +BuildRequires: libtool BuildRequires: python-devel %if %{with_systemd} BuildRequires: systemd-units @@ -721,6 +724,7 @@ of recent versions of Linux (and other OSes). %define init_scripts --with-init_script=redhat %endif +autoreconf -if %configure %{?_without_xen} \ %{?_without_qemu} \ %{?_without_openvz} \ -- 1.7.7.3
On Mon, Dec 05, 2011 at 10:38:49AM -0700, Eric Blake wrote:
Over time, Fedora and RHEL RPMs have often backported upstream patches that touched configure.ac and/or Makefile.am; this necessitates rerunning the autotools for the patch to be effective. Making this part of the spec file will make it easier for future backports to pull patches without thinking about this issue.
* libvirt.spec.in (BuildRequires): Add autotools. (%build): Use them before configure. --- libvirt.spec.in | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 06c949b..462421a 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -334,6 +334,9 @@ Requires: libcgroup %endif
# All build-time requirements +BuildRequires: autoconf +BuildRequires: automake +BuildRequires: libtool BuildRequires: python-devel %if %{with_systemd} BuildRequires: systemd-units @@ -721,6 +724,7 @@ of recent versions of Linux (and other OSes). %define init_scripts --with-init_script=redhat %endif
+autoreconf -if %configure %{?_without_xen} \ %{?_without_qemu} \ %{?_without_openvz} \
NACK, we really shouldn't do this by default IMHO - regenerating autotools has not always been foolproof when newer autotools are released. If we want to include this for help of downstream, then it should be protected by a conditional statement, so it is off by default and you can set %define enable_autoconf 1 at the top of the spec to turn it on. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Over time, Fedora and RHEL RPMs have often backported upstream patches that touched configure.ac and/or Makefile.am; this necessitates rerunning the autotools for the patch to be effective. Making this part of the spec file will make it easier for future backports to pull patches without thinking about this issue. However, there have been historical instances where an update in the autotools caused FTBFS situations; make it easy to avoid these by allowing the user to override our default. * libvirt.spec.in (BuildRequires): Add autotools. (%build): Conditionally use them before configure. --- v2: make the autotools default overridable, so that an rpm packager can bypass any changes caused by incompatibilities in an autotools upgrade. libvirt.spec.in | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 97b811d..806ff8f 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -8,6 +8,11 @@ sed -ne 's/^\.fc\?\([0-9]\+\).*/%%define fedora \1/p')} %endif +# Default to running autoreconf in case any downstream patches touched +# configure.ac or Makefile.am. Set enable_autotools to 0 if you want +# to skip this step, perhaps if an upgrade in the autotools causes problems. +%{?enable_autotools:%define enable_autotools 1} + # A client only build will create a libvirt.so only containing # the generic RPC driver, and test driver and no libvirtd # Default to a full server + client build @@ -349,6 +354,11 @@ Requires(postun): systemd-units %endif # All build-time requirements +%if 0%{?enable_autotools} +BuildRequires: autoconf +BuildRequires: automake +BuildRequires: libtool +%endif BuildRequires: python-devel %if %{with_systemd} BuildRequires: systemd-units @@ -729,6 +739,9 @@ of recent versions of Linux (and other OSes). %define init_scripts --with-init_script=redhat %endif +%if 0%{?enable_autotools} +autoreconf -if +%endif %configure %{?_without_xen} \ %{?_without_qemu} \ %{?_without_openvz} \ -- 1.7.7.3
On Mon, Dec 05, 2011 at 12:15:29PM -0700, Eric Blake wrote:
Over time, Fedora and RHEL RPMs have often backported upstream patches that touched configure.ac and/or Makefile.am; this necessitates rerunning the autotools for the patch to be effective. Making this part of the spec file will make it easier for future backports to pull patches without thinking about this issue. However, there have been historical instances where an update in the autotools caused FTBFS situations; make it easy to avoid these by allowing the user to override our default.
* libvirt.spec.in (BuildRequires): Add autotools. (%build): Conditionally use them before configure. ---
v2: make the autotools default overridable, so that an rpm packager can bypass any changes caused by incompatibilities in an autotools upgrade.
libvirt.spec.in | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 97b811d..806ff8f 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -8,6 +8,11 @@ sed -ne 's/^\.fc\?\([0-9]\+\).*/%%define fedora \1/p')} %endif
+# Default to running autoreconf in case any downstream patches touched +# configure.ac or Makefile.am. Set enable_autotools to 0 if you want +# to skip this step, perhaps if an upgrade in the autotools causes problems. +%{?enable_autotools:%define enable_autotools 1}
No, we should default to *not* running autotools - it just introduces a new failure point for no benefit in 99% of the time where we have no patches touching makefiles/configure. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
https://bugzilla.redhat.com/show_bug.cgi?id=754909 complains that because libvirt didn't require dmidecode, that the logs are noisy and virConnectGetSysinfo needlessly fails. Even 'virt-what' requires dmidecode, so it's not that onerous of a dependency. We may be able to drop this in the future when we move to parsing sysfs data, but for now, listing the dependency will help matters. * libvirt.spec.in (Requires): Sort Requires before BuildRequires. Add dmidecode. --- libvirt.spec.in | 16 +++++++++------- 1 files changed, 9 insertions(+), 7 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 462421a..35762a5 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -332,6 +332,15 @@ Requires: device-mapper %if %{with_cgconfig} Requires: libcgroup %endif +# For virConnectGetSysinfo +Requires: dmidecode +# For service management +%if %{with_systemd} +Requires(post): systemd-units +Requires(post): systemd-sysv +Requires(preun): systemd-units +Requires(postun): systemd-units +%endif # All build-time requirements BuildRequires: autoconf @@ -485,13 +494,6 @@ BuildRequires: nfs-utils # Fedora build root suckage BuildRequires: gawk -%if %{with_systemd} -Requires(post): systemd-units -Requires(post): systemd-sysv -Requires(preun): systemd-units -Requires(postun): systemd-units -%endif - %description Libvirt is a C toolkit to interact with the virtualization capabilities of recent versions of Linux (and other OSes). The main package includes -- 1.7.7.3
On Mon, Dec 05, 2011 at 10:38:50AM -0700, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=754909 complains that because libvirt didn't require dmidecode, that the logs are noisy and virConnectGetSysinfo needlessly fails. Even 'virt-what' requires dmidecode, so it's not that onerous of a dependency. We may be able to drop this in the future when we move to parsing sysfs data, but for now, listing the dependency will help matters.
* libvirt.spec.in (Requires): Sort Requires before BuildRequires. Add dmidecode. --- libvirt.spec.in | 16 +++++++++------- 1 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 462421a..35762a5 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -332,6 +332,15 @@ Requires: device-mapper %if %{with_cgconfig} Requires: libcgroup %endif +# For virConnectGetSysinfo +Requires: dmidecode +# For service management +%if %{with_systemd} +Requires(post): systemd-units +Requires(post): systemd-sysv +Requires(preun): systemd-units +Requires(postun): systemd-units +%endif
# All build-time requirements BuildRequires: autoconf @@ -485,13 +494,6 @@ BuildRequires: nfs-utils # Fedora build root suckage BuildRequires: gawk
-%if %{with_systemd} -Requires(post): systemd-units -Requires(post): systemd-sysv -Requires(preun): systemd-units -Requires(postun): systemd-units -%endif - %description Libvirt is a C toolkit to interact with the virtualization capabilities of recent versions of Linux (and other OSes). The main package includes
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12/05/2011 10:54 AM, Daniel P. Berrange wrote:
On Mon, Dec 05, 2011 at 10:38:50AM -0700, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=754909 complains that because libvirt didn't require dmidecode, that the logs are noisy and virConnectGetSysinfo needlessly fails. Even 'virt-what' requires dmidecode, so it's not that onerous of a dependency. We may be able to drop this in the future when we move to parsing sysfs data, but for now, listing the dependency will help matters.
* libvirt.spec.in (Requires): Sort Requires before BuildRequires. Add dmidecode.
ACK
Thanks; pushed 2/3 and 3/3. I'll post a v2 for patch 1/3 in a few minutes. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
* libvirt.spec.in (with_sanlock): On RHEL, don't force sanlock on architectures where it isn't available. --- libvirt.spec.in | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 35762a5..39c814a 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -174,8 +174,14 @@ %endif # Enable sanlock library for lock management with QEMU -%if 0%{?fedora} >= 16 || 0%{?rhel} >= 6 -%define with_sanlock 0%{!?_without_sanlock:%{server_drivers}} ++# Sanlock is available only on i686 x86_64 for RHEL +%if 0%{?fedora} >= 16 +%define with_sanlock 0%{!?_without_sanlock:%{server_drivers}} +%endif +%if 0%{?rhel} >= 6 +%ifnarch i386 i586 i686 x86_64 +%define with_sanlock 0%{!?_without_sanlock:%{server_drivers}} +%endif %endif # Disable some drivers when building without libvirt daemon. -- 1.7.7.3
On 12/05/2011 10:38 AM, Eric Blake wrote:
* libvirt.spec.in (with_sanlock): On RHEL, don't force sanlock on architectures where it isn't available. --- libvirt.spec.in | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 35762a5..39c814a 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -174,8 +174,14 @@ %endif
# Enable sanlock library for lock management with QEMU -%if 0%{?fedora} >= 16 || 0%{?rhel} >= 6 -%define with_sanlock 0%{!?_without_sanlock:%{server_drivers}} ++# Sanlock is available only on i686 x86_64 for RHEL ^
Aargh - I sent the mail before saving in my editor. Drop that extra '+' if you want to test this one.
+%if 0%{?fedora} >= 16 +%define with_sanlock 0%{!?_without_sanlock:%{server_drivers}} +%endif +%if 0%{?rhel} >= 6 +%ifnarch i386 i586 i686 x86_64 +%define with_sanlock 0%{!?_without_sanlock:%{server_drivers}} +%endif %endif
# Disable some drivers when building without libvirt daemon.
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Dec 05, 2011 at 10:38:51AM -0700, Eric Blake wrote:
* libvirt.spec.in (with_sanlock): On RHEL, don't force sanlock on architectures where it isn't available. --- libvirt.spec.in | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 35762a5..39c814a 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -174,8 +174,14 @@ %endif
# Enable sanlock library for lock management with QEMU -%if 0%{?fedora} >= 16 || 0%{?rhel} >= 6 -%define with_sanlock 0%{!?_without_sanlock:%{server_drivers}} ++# Sanlock is available only on i686 x86_64 for RHEL +%if 0%{?fedora} >= 16 +%define with_sanlock 0%{!?_without_sanlock:%{server_drivers}} +%endif +%if 0%{?rhel} >= 6 +%ifnarch i386 i586 i686 x86_64 +%define with_sanlock 0%{!?_without_sanlock:%{server_drivers}} +%endif %endif
# Disable some drivers when building without libvirt daemon.
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
At 12/06/2011 01:38 AM, Eric Blake Write:
* libvirt.spec.in (with_sanlock): On RHEL, don't force sanlock on architectures where it isn't available. --- libvirt.spec.in | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 35762a5..39c814a 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -174,8 +174,14 @@ %endif
# Enable sanlock library for lock management with QEMU -%if 0%{?fedora} >= 16 || 0%{?rhel} >= 6 -%define with_sanlock 0%{!?_without_sanlock:%{server_drivers}} ++# Sanlock is available only on i686 x86_64 for RHEL
According this comment, force sanlock only on i686 x86_64 for RHEL
+%if 0%{?fedora} >= 16 +%define with_sanlock 0%{!?_without_sanlock:%{server_drivers}} +%endif +%if 0%{?rhel} >= 6 +%ifnarch i386 i586 i686 x86_64
But here you force it only not on i686 x86_64 for RHEL. s/ifnarch/ifarch/ ? Thanks Wen Congyang
+%define with_sanlock 0%{!?_without_sanlock:%{server_drivers}} +%endif %endif
# Disable some drivers when building without libvirt daemon.
On 12/13/2011 06:52 PM, Wen Congyang wrote:
At 12/06/2011 01:38 AM, Eric Blake Write:
* libvirt.spec.in (with_sanlock): On RHEL, don't force sanlock on architectures where it isn't available. --- libvirt.spec.in | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
++# Sanlock is available only on i686 x86_64 for RHEL
According this comment, force sanlock only on i686 x86_64 for RHEL
+%if 0%{?fedora} >= 16 +%define with_sanlock 0%{!?_without_sanlock:%{server_drivers}} +%endif +%if 0%{?rhel} >= 6 +%ifnarch i386 i586 i686 x86_64
But here you force it only not on i686 x86_64 for RHEL.
s/ifnarch/ifarch/ ?
Yes, thanks for spotting that. I've pushed the trivial patch in your name. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Over time, Fedora and RHEL RPMs have often backported upstream patches that touched configure.ac and/or Makefile.am; this necessitates rerunning the autotools for the patch to be effective. Making this a one-liner spec tweak will make it easier for future backports to pull patches without having to find all the places to touch to properly use the autotools. Meanwhile, there have been historical instances where an update in the autotools caused FTBFS situations, so this is not on by default. * libvirt.spec.in (BuildRequires): Add autotools. (%build): Conditionally use them before configure. --- v2: switch default value of %enable_autotools libvirt.spec.in | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 97b811d..c2c926d 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -8,6 +8,11 @@ sed -ne 's/^\.fc\?\([0-9]\+\).*/%%define fedora \1/p')} %endif +# Default to skipping autoreconf. Distros can change just this one line +# (or provide a command-line override) if they backport any patches that +# touch configure.ac or Makefile.am. +%{?enable_autotools:%define enable_autotools 0} + # A client only build will create a libvirt.so only containing # the generic RPC driver, and test driver and no libvirtd # Default to a full server + client build @@ -349,6 +354,11 @@ Requires(postun): systemd-units %endif # All build-time requirements +%if 0%{?enable_autotools} +BuildRequires: autoconf +BuildRequires: automake +BuildRequires: libtool +%endif BuildRequires: python-devel %if %{with_systemd} BuildRequires: systemd-units @@ -729,6 +739,9 @@ of recent versions of Linux (and other OSes). %define init_scripts --with-init_script=redhat %endif +%if 0%{?enable_autotools} +autoreconf -if +%endif %configure %{?_without_xen} \ %{?_without_qemu} \ %{?_without_openvz} \ -- 1.7.7.3
https://bugzilla.redhat.com/show_bug.cgi?id=694403 reports that the specfile is incorrectly checking for a running libvirt-guests service. For example, $ LC_ALL=es_ES chkconfig --list libvirt-guests libvirt-guests 0:desactivado 1:desactivado 2:desactivado 3:activo 4:activo 5:activo 6:desactivado will fail to find 5:on, even though it is active. But chkconfig already has a mode where you can silently use the exit status to check for an active service. * libvirt.spec.in (%post): Use simpler chkconfig options, to avoid issues with localization. --- v3: new patch libvirt.spec.in | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index c2c926d..ecaeee4 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -887,8 +887,7 @@ getent passwd qemu >/dev/null || \ # We want to install the default network for initial RPM installs # or on the first upgrade from a non-network aware libvirt only. # We check this by looking to see if the daemon is already installed -/sbin/chkconfig --list libvirtd 1>/dev/null 2>&1 -if test $? != 0 && test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml +if ! /sbin/chkconfig && test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml then UUID=`/usr/bin/uuidgen` sed -e "s,</name>,</name>\n <uuid>$UUID</uuid>," \ @@ -1021,8 +1020,7 @@ fi /sbin/chkconfig --add libvirt-guests if [ $1 -ge 1 ]; then level=$(/sbin/runlevel | /bin/cut -d ' ' -f 2) - if /sbin/chkconfig --list libvirt-guests 2>/dev/null \ - | /bin/grep -q $level:on ; then + if /sbin/chkconfig --levels $level libvirt-guests; then # this doesn't do anything but allowing for libvirt-guests to be # stopped on the first shutdown /sbin/service libvirt-guests start > /dev/null 2>&1 || true -- 1.7.7.3
On 12/06/2011 06:47 PM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=694403 reports that the specfile is incorrectly checking for a running libvirt-guests service. For example,
$ LC_ALL=es_ES chkconfig --list libvirt-guests libvirt-guests 0:desactivado 1:desactivado 2:desactivado 3:activo 4:activo 5:activo 6:desactivado
will fail to find 5:on, even though it is active. But chkconfig already has a mode where you can silently use the exit status to check for an active service.
* libvirt.spec.in (%post): Use simpler chkconfig options, to avoid issues with localization. ---
v3: new patch
libvirt.spec.in | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index c2c926d..ecaeee4 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -887,8 +887,7 @@ getent passwd qemu>/dev/null || \ # We want to install the default network for initial RPM installs # or on the first upgrade from a non-network aware libvirt only. # We check this by looking to see if the daemon is already installed -/sbin/chkconfig --list libvirtd 1>/dev/null 2>&1 -if test $? != 0&& test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml +if ! /sbin/chkconfig&& test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml then UUID=`/usr/bin/uuidgen` sed -e "s,</name>,</name>\n<uuid>$UUID</uuid>," \ @@ -1021,8 +1020,7 @@ fi /sbin/chkconfig --add libvirt-guests if [ $1 -ge 1 ]; then level=$(/sbin/runlevel | /bin/cut -d ' ' -f 2) - if /sbin/chkconfig --list libvirt-guests 2>/dev/null \ - | /bin/grep -q $level:on ; then + if /sbin/chkconfig --levels $level libvirt-guests; then # this doesn't do anything but allowing for libvirt-guests to be # stopped on the first shutdown /sbin/service libvirt-guests start> /dev/null 2>&1 || true
ACK.
On 12/07/2011 05:50 PM, Laine Stump wrote:
On 12/06/2011 06:47 PM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=694403 reports that the specfile is incorrectly checking for a running libvirt-guests service. For example,
$ LC_ALL=es_ES chkconfig --list libvirt-guests libvirt-guests 0:desactivado 1:desactivado 2:desactivado 3:activo 4:activo 5:activo 6:desactivado
will fail to find 5:on, even though it is active. But chkconfig already has a mode where you can silently use the exit status to check for an active service.
# We check this by looking to see if the daemon is already installed -/sbin/chkconfig --list libvirtd 1>/dev/null 2>&1 -if test $? != 0&& test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml +if ! /sbin/chkconfig&& test ! -f
Actually, in my testing, I realized this has to be '/bin/chkconfig libvirtd', not '/sbin/chkconfig'.
ACK.
I'll push the corrected version. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Dec 07, 2011 at 05:56:26PM -0700, Eric Blake wrote:
On 12/07/2011 05:50 PM, Laine Stump wrote:
On 12/06/2011 06:47 PM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=694403 reports that the specfile is incorrectly checking for a running libvirt-guests service. For example,
$ LC_ALL=es_ES chkconfig --list libvirt-guests libvirt-guests 0:desactivado 1:desactivado 2:desactivado 3:activo 4:activo 5:activo 6:desactivado
will fail to find 5:on, even though it is active. But chkconfig already has a mode where you can silently use the exit status to check for an active service.
# We check this by looking to see if the daemon is already installed -/sbin/chkconfig --list libvirtd 1>/dev/null 2>&1 -if test $? != 0&& test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml +if ! /sbin/chkconfig&& test ! -f
Actually, in my testing, I realized this has to be '/bin/chkconfig libvirtd', not '/sbin/chkconfig'.
ACK.
I'll push the corrected version.
Humm ... here on Fedora 14 it's definitely /sbin/chkconfig there is no /bin/chkconfig, let's wait after the release to get a definitive fix, Daniel
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 12/07/2011 07:53 PM, Daniel Veillard wrote:
# We check this by looking to see if the daemon is already installed -/sbin/chkconfig --list libvirtd 1>/dev/null 2>&1 -if test $? != 0&& test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml +if ! /sbin/chkconfig&& test ! -f
Actually, in my testing, I realized this has to be '/bin/chkconfig libvirtd', not '/sbin/chkconfig'.
ACK.
I'll push the corrected version.
Humm ... here on Fedora 14 it's definitely /sbin/chkconfig there is no /bin/chkconfig,
Aargh - I typo'd my typo correction.
let's wait after the release to get a definitive fix,
/sbin/chkconfig is what is actually in the commit, and I see no reason to defer it. For the record, here's the patch that I actually tested (rather than fat-fingering my email): commit c4a387fdd8f3d86df7815d9a8bfca57baa3f14f1 Author: Eric Blake <eblake@redhat.com> Date: Tue Dec 6 16:16:34 2011 -0700 spec: don't use chkconfig --list https://bugzilla.redhat.com/show_bug.cgi?id=694403 reports that the specfile is incorrectly checking for a running libvirt-guests service. For example, $ LC_ALL=es_ES chkconfig --list libvirt-guests libvirt-guests 0:desactivado 1:desactivado 2:desactivado 3:activo 4:activo 5:activo 6:desactivado will fail to find 5:on, even though it is active. But chkconfig already has a mode where you can silently use the exit status to check for an active service. * libvirt.spec.in (%post): Use simpler chkconfig options, to avoid issues with localization. diff --git a/libvirt.spec.in b/libvirt.spec.in index c2c926d..3a5363d 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -887,8 +887,7 @@ getent passwd qemu >/dev/null || \ # We want to install the default network for initial RPM installs # or on the first upgrade from a non-network aware libvirt only. # We check this by looking to see if the daemon is already installed -/sbin/chkconfig --list libvirtd 1>/dev/null 2>&1 -if test $? != 0 && test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml +if ! /sbin/chkconfig libvirtd && test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml then UUID=`/usr/bin/uuidgen` sed -e "s,</name>,</name>\n <uuid>$UUID</uuid>," \ @@ -1021,8 +1020,7 @@ fi /sbin/chkconfig --add libvirt-guests if [ $1 -ge 1 ]; then level=$(/sbin/runlevel | /bin/cut -d ' ' -f 2) - if /sbin/chkconfig --list libvirt-guests 2>/dev/null \ - | /bin/grep -q $level:on ; then + if /sbin/chkconfig --levels $level libvirt-guests; then # this doesn't do anything but allowing for libvirt-guests to be # stopped on the first shutdown /sbin/service libvirt-guests start > /dev/null 2>&1 || true -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Dec 07, 2011 at 07:58:40PM -0700, Eric Blake wrote:
On 12/07/2011 07:53 PM, Daniel Veillard wrote:
# We check this by looking to see if the daemon is already installed -/sbin/chkconfig --list libvirtd 1>/dev/null 2>&1 -if test $? != 0&& test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml +if ! /sbin/chkconfig&& test ! -f
Actually, in my testing, I realized this has to be '/bin/chkconfig libvirtd', not '/sbin/chkconfig'.
ACK.
I'll push the corrected version.
Humm ... here on Fedora 14 it's definitely /sbin/chkconfig there is no /bin/chkconfig,
Aargh - I typo'd my typo correction.
let's wait after the release to get a definitive fix,
/sbin/chkconfig is what is actually in the commit, and I see no reason to defer it. For the record, here's the patch that I actually tested (rather than fat-fingering my email):
commit c4a387fdd8f3d86df7815d9a8bfca57baa3f14f1 Author: Eric Blake <eblake@redhat.com> Date: Tue Dec 6 16:16:34 2011 -0700
spec: don't use chkconfig --list
https://bugzilla.redhat.com/show_bug.cgi?id=694403 reports that the specfile is incorrectly checking for a running libvirt-guests service. For example,
$ LC_ALL=es_ES chkconfig --list libvirt-guests libvirt-guests 0:desactivado 1:desactivado 2:desactivado 3:activo 4:activo 5:activo 6:desactivado
will fail to find 5:on, even though it is active. But chkconfig already has a mode where you can silently use the exit status to check for an active service.
* libvirt.spec.in (%post): Use simpler chkconfig options, to avoid issues with localization.
diff --git a/libvirt.spec.in b/libvirt.spec.in index c2c926d..3a5363d 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -887,8 +887,7 @@ getent passwd qemu >/dev/null || \ # We want to install the default network for initial RPM installs # or on the first upgrade from a non-network aware libvirt only. # We check this by looking to see if the daemon is already installed -/sbin/chkconfig --list libvirtd 1>/dev/null 2>&1 -if test $? != 0 && test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml +if ! /sbin/chkconfig libvirtd && test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml then UUID=`/usr/bin/uuidgen` sed -e "s,</name>,</name>\n <uuid>$UUID</uuid>," \ @@ -1021,8 +1020,7 @@ fi /sbin/chkconfig --add libvirt-guests if [ $1 -ge 1 ]; then level=$(/sbin/runlevel | /bin/cut -d ' ' -f 2) - if /sbin/chkconfig --list libvirt-guests 2>/dev/null \ - | /bin/grep -q $level:on ; then + if /sbin/chkconfig --levels $level libvirt-guests; then # this doesn't do anything but allowing for libvirt-guests to be # stopped on the first shutdown /sbin/service libvirt-guests start > /dev/null 2>&1 || true
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 12/07/2011 09:02 PM, Daniel Veillard wrote:
/sbin/chkconfig is what is actually in the commit, and I see no reason to defer it. For the record, here's the patch that I actually tested (rather than fat-fingering my email):
commit c4a387fdd8f3d86df7815d9a8bfca57baa3f14f1 Author: Eric Blake <eblake@redhat.com> Date: Tue Dec 6 16:16:34 2011 -0700
spec: don't use chkconfig --list
ACK,
I pushed this; the spec-file change for autoreconf can wait until post-release (as in v3, it is a no-op unless the distro tweaks it anyways). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/06/2011 06:47 PM, Eric Blake wrote:
Over time, Fedora and RHEL RPMs have often backported upstream patches that touched configure.ac and/or Makefile.am; this necessitates rerunning the autotools for the patch to be effective. Making this a one-liner spec tweak will make it easier for future backports to pull patches without having to find all the places to touch to properly use the autotools. Meanwhile, there have been historical instances where an update in the autotools caused FTBFS situations, so this is not on by default.
* libvirt.spec.in (BuildRequires): Add autotools. (%build): Conditionally use them before configure. ---
v2: switch default value of %enable_autotools
libvirt.spec.in | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 97b811d..c2c926d 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -8,6 +8,11 @@ sed -ne 's/^\.fc\?\([0-9]\+\).*/%%define fedora \1/p')} %endif
+# Default to skipping autoreconf. Distros can change just this one line +# (or provide a command-line override) if they backport any patches that +# touch configure.ac or Makefile.am. +%{?enable_autotools:%define enable_autotools 0} + # A client only build will create a libvirt.so only containing # the generic RPC driver, and test driver and no libvirtd # Default to a full server + client build @@ -349,6 +354,11 @@ Requires(postun): systemd-units %endif
# All build-time requirements +%if 0%{?enable_autotools} +BuildRequires: autoconf +BuildRequires: automake +BuildRequires: libtool +%endif BuildRequires: python-devel %if %{with_systemd} BuildRequires: systemd-units @@ -729,6 +739,9 @@ of recent versions of Linux (and other OSes). %define init_scripts --with-init_script=redhat %endif
+%if 0%{?enable_autotools} +autoreconf -if +%endif %configure %{?_without_xen} \ %{?_without_qemu} \ %{?_without_openvz} \
This follows danpb's suggestion in the previous review. ACK.
On Tue, Dec 06, 2011 at 04:47:27PM -0700, Eric Blake wrote:
Over time, Fedora and RHEL RPMs have often backported upstream patches that touched configure.ac and/or Makefile.am; this necessitates rerunning the autotools for the patch to be effective. Making this a one-liner spec tweak will make it easier for future backports to pull patches without having to find all the places to touch to properly use the autotools. Meanwhile, there have been historical instances where an update in the autotools caused FTBFS situations, so this is not on by default.
* libvirt.spec.in (BuildRequires): Add autotools. (%build): Conditionally use them before configure. ---
v2: switch default value of %enable_autotools
libvirt.spec.in | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 97b811d..c2c926d 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -8,6 +8,11 @@ sed -ne 's/^\.fc\?\([0-9]\+\).*/%%define fedora \1/p')} %endif
+# Default to skipping autoreconf. Distros can change just this one line +# (or provide a command-line override) if they backport any patches that +# touch configure.ac or Makefile.am. +%{?enable_autotools:%define enable_autotools 0} + # A client only build will create a libvirt.so only containing # the generic RPC driver, and test driver and no libvirtd # Default to a full server + client build @@ -349,6 +354,11 @@ Requires(postun): systemd-units %endif
# All build-time requirements +%if 0%{?enable_autotools} +BuildRequires: autoconf +BuildRequires: automake +BuildRequires: libtool +%endif BuildRequires: python-devel %if %{with_systemd} BuildRequires: systemd-units @@ -729,6 +739,9 @@ of recent versions of Linux (and other OSes). %define init_scripts --with-init_script=redhat %endif
+%if 0%{?enable_autotools} +autoreconf -if +%endif %configure %{?_without_xen} \ %{?_without_qemu} \ %{?_without_openvz} \
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Dec 08, 2011 at 01:04:27PM +0000, Daniel P. Berrange wrote:
On Tue, Dec 06, 2011 at 04:47:27PM -0700, Eric Blake wrote:
Over time, Fedora and RHEL RPMs have often backported upstream patches that touched configure.ac and/or Makefile.am; this necessitates rerunning the autotools for the patch to be effective. Making this a one-liner spec tweak will make it easier for future backports to pull patches without having to find all the places to touch to properly use the autotools. Meanwhile, there have been historical instances where an update in the autotools caused FTBFS situations, so this is not on by default.
* libvirt.spec.in (BuildRequires): Add autotools. (%build): Conditionally use them before configure. ---
v2: switch default value of %enable_autotools
libvirt.spec.in | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 97b811d..c2c926d 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -8,6 +8,11 @@ sed -ne 's/^\.fc\?\([0-9]\+\).*/%%define fedora \1/p')} %endif
+# Default to skipping autoreconf. Distros can change just this one line +# (or provide a command-line override) if they backport any patches that +# touch configure.ac or Makefile.am. +%{?enable_autotools:%define enable_autotools 0}
I've just tried this and it doesn't actually work. You need %define enable_autotools %{?enable_autotools:0} Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Dec 08, 2011 at 01:27:37PM +0000, Daniel P. Berrange wrote:
On Thu, Dec 08, 2011 at 01:04:27PM +0000, Daniel P. Berrange wrote:
On Tue, Dec 06, 2011 at 04:47:27PM -0700, Eric Blake wrote:
Over time, Fedora and RHEL RPMs have often backported upstream patches that touched configure.ac and/or Makefile.am; this necessitates rerunning the autotools for the patch to be effective. Making this a one-liner spec tweak will make it easier for future backports to pull patches without having to find all the places to touch to properly use the autotools. Meanwhile, there have been historical instances where an update in the autotools caused FTBFS situations, so this is not on by default.
* libvirt.spec.in (BuildRequires): Add autotools. (%build): Conditionally use them before configure. ---
v2: switch default value of %enable_autotools
libvirt.spec.in | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 97b811d..c2c926d 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -8,6 +8,11 @@ sed -ne 's/^\.fc\?\([0-9]\+\).*/%%define fedora \1/p')} %endif
+# Default to skipping autoreconf. Distros can change just this one line +# (or provide a command-line override) if they backport any patches that +# touch configure.ac or Makefile.am. +%{?enable_autotools:%define enable_autotools 0}
I've just tried this and it doesn't actually work. You need
%define enable_autotools %{?enable_autotools:0}
There also needs to be a BuildRequires: gettext-devel since autoreconf runs autopoint which isn't in the build root Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12/08/2011 08:12 AM, Daniel P. Berrange wrote:
On Thu, Dec 08, 2011 at 01:27:37PM +0000, Daniel P. Berrange wrote:
On Thu, Dec 08, 2011 at 01:04:27PM +0000, Daniel P. Berrange wrote:
On Tue, Dec 06, 2011 at 04:47:27PM -0700, Eric Blake wrote:
Over time, Fedora and RHEL RPMs have often backported upstream patches that touched configure.ac and/or Makefile.am; this necessitates rerunning the autotools for the patch to be effective. Making this a one-liner spec tweak will make it easier for future backports to pull patches without having to find all the places to touch to properly use the autotools. Meanwhile, there have been historical instances where an update in the autotools caused FTBFS situations, so this is not on by default.
* libvirt.spec.in (BuildRequires): Add autotools. (%build): Conditionally use them before configure.
+%{?enable_autotools:%define enable_autotools 0}
I've just tried this and it doesn't actually work. You need
Ah, you're right. It says: if enable_autotools was defined, redefine it to 0 if enable_autotools was not defined, do nothing
%define enable_autotools %{?enable_autotools:0}
That's not quite right, either. It says: if enable_autotools was defined, redefine it to 0 if enable_autotools was undefined, define it to an empty string I did more testing, and one correct answer is: %{!?enable_autotools:%define enable_autotools 0} read as: if enable_autotools is not defined, define it to 0 if enable_autotools is defined, leave it alone (whether 0 or 1) Another correct answer is: %define enable_autotools 0%{?enable_autotools} read as: if enable_autotools is defined, redefine it to the concatenation of 0 and its previous value if enable_autotools is undefined, define it to 0
There also needs to be a
BuildRequires: gettext-devel
since autoreconf runs autopoint which isn't in the build root
Glad we deferred this to post-0.9.8, then. Pushed with those fixes, as well as copying to the mingw spec file: diff --git i/libvirt.spec.in w/libvirt.spec.in index 0584b0c..0a3b00d 100644 --- i/libvirt.spec.in +++ w/libvirt.spec.in @@ -11,7 +11,7 @@ # Default to skipping autoreconf. Distros can change just this one line # (or provide a command-line override) if they backport any patches that # touch configure.ac or Makefile.am. -%{?enable_autotools:%define enable_autotools 0} +%{!?enable_autotools:%define enable_autotools 0} # A client only build will create a libvirt.so only containing # the generic RPC driver, and test driver and no libvirtd @@ -357,6 +357,7 @@ Requires(postun): systemd-units %if 0%{?enable_autotools} BuildRequires: autoconf BuildRequires: automake +BuildRequires: gettext-devel BuildRequires: libtool %endif BuildRequires: python-devel diff --git i/mingw32-libvirt.spec.in w/mingw32-libvirt.spec.in index 89d1d7f..06ff601 100644 --- i/mingw32-libvirt.spec.in +++ w/mingw32-libvirt.spec.in @@ -5,6 +5,11 @@ %define __find_provides %{_mingw32_findprovides} %define __debug_install_post %{_mingw32_debug_install_post} +# Default to skipping autoreconf. Distros can change just this one line +# (or provide a command-line override) if they backport any patches that +# touch configure.ac or Makefile.am. +%{!?enable_autotools:%define enable_autotools 0} + # The mingw build is client only. Set up defaults for hypervisor drivers # that talk via a native remote protocol, and for which prereq mingw # libraries exist. @@ -44,6 +49,12 @@ BuildRequires: mingw32-portablexdr BuildRequires: pkgconfig # Need native version for msgfmt BuildRequires: gettext +%if 0%{?enable_autotools} +BuildRequires: autoconf +BuildRequires: automake +BuildRequires: gettext-devel +BuildRequires: libtool +%endif %if %{with_phyp} BuildRequires: mingw32-libssh2 @@ -81,6 +92,9 @@ MinGW Windows libvirt virtualization library. %define _without_xenapi --without-xenapi %endif +%if 0%{?enable_autotools} +autoreconf -if +%endif # XXX enable SASL in future %{_mingw32_configure} \ --without-xen \ -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Laine Stump -
Wen Congyang