[libvirt PATCH v2 0/9] spec: Improve feature and architecture handling
Changes from [v1]: * address review feedback. [v1] https://www.redhat.com/archives/libvir-list/2020-October/msg00336.html Andrea Bolognani (9): spec: Simplify setting features off by default spec: firewalld is always enabled spec: bash completion actually defaults to on spec: Move with_numactl definition spec: Introduce with_dmidecode spec: Move _vpath_builddir definition spec: Drop s390 architecture from conditionals spec: Refactor qemu_kvm_arches definition spec: Introduce arches_* libvirt.spec.in | 116 ++++++++++++++++++++++++------------------------ 1 file changed, 57 insertions(+), 59 deletions(-) -- 2.26.2
The right-hand side of these expressions will always evaluate to zero. Stop obfuscating this fact. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 80563ce6ef..36a2cac2b2 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -85,15 +85,15 @@ %endif # A few optional bits off by default, we enable later -%define with_fuse 0%{!?_without_fuse:0} -%define with_sanlock 0%{!?_without_sanlock:0} -%define with_numad 0%{!?_without_numad:0} -%define with_firewalld 0%{!?_without_firewalld:0} -%define with_firewalld_zone 0%{!?_without_firewalld_zone:0} -%define with_libssh2 0%{!?_without_libssh2:0} -%define with_wireshark 0%{!?_without_wireshark:0} -%define with_libssh 0%{!?_without_libssh:0} -%define with_bash_completion 0%{!?_without_bash_completion:0} +%define with_fuse 0 +%define with_sanlock 0 +%define with_numad 0 +%define with_firewalld 0 +%define with_firewalld_zone 0 +%define with_libssh2 0 +%define with_wireshark 0 +%define with_libssh 0 +%define with_bash_completion 0 # Finally set the OS / architecture specific special cases -- 2.26.2
Knowing this, we can remove some code. Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Neal Gompa <ngompa13@gmail.com> --- libvirt.spec.in | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 36a2cac2b2..508b4c8dcc 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -88,7 +88,6 @@ %define with_fuse 0 %define with_sanlock 0 %define with_numad 0 -%define with_firewalld 0 %define with_firewalld_zone 0 %define with_libssh2 0 %define with_wireshark 0 @@ -138,8 +137,6 @@ %endif %endif -%define with_firewalld 1 - %if 0%{?fedora} || 0%{?rhel} > 7 %define with_firewalld_zone 0%{!?_without_firewalld_zone:1} %endif @@ -1089,12 +1086,6 @@ exit 1 %define arg_sanlock -Dsanlock=disabled %endif -%if %{with_firewalld} - %define arg_firewalld -Dfirewalld=enabled -%else - %define arg_firewalld -Dfirewalld=disabled -%endif - %if %{with_firewalld_zone} %define arg_firewalld_zone -Dfirewalld_zone=enabled %else @@ -1170,7 +1161,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/%{name}.spec) -Dlibpcap=enabled \ -Daudit=enabled \ -Ddtrace=enabled \ - %{?arg_firewalld} \ + -Dfirewalld=enabled \ %{?arg_firewalld_zone} \ %{?arg_wireshark} \ -Dpm_utils=disabled \ -- 2.26.2
Remove the red herring. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 508b4c8dcc..66f0560a5c 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -84,6 +84,9 @@ %define with_storage_iscsi_direct 0 %endif +# Other optional features +%define with_bash_completion 0%{!?_without_bash_completion:1} + # A few optional bits off by default, we enable later %define with_fuse 0 %define with_sanlock 0 @@ -92,7 +95,6 @@ %define with_libssh2 0 %define with_wireshark 0 %define with_libssh 0 -%define with_bash_completion 0 # Finally set the OS / architecture specific special cases @@ -174,8 +176,6 @@ %define with_libssh 0%{!?_without_libssh:1} %endif -%define with_bash_completion 0%{!?_without_bash_completion:1} - %if %{with_qemu} || %{with_lxc} # numad is used to manage the CPU and memory placement dynamically, # it's not available on many non-x86 architectures. -- 2.26.2
Keep it close to similar ones. Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Neal Gompa <ngompa13@gmail.com> --- libvirt.spec.in | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 66f0560a5c..d9fa27a5fe 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -68,8 +68,6 @@ %endif %endif -%define with_numactl 0%{!?_without_numactl:1} - # F25+ has zfs-fuse %if 0%{?fedora} %define with_storage_zfs 0%{!?_without_storage_zfs:1} @@ -86,6 +84,7 @@ # Other optional features %define with_bash_completion 0%{!?_without_bash_completion:1} +%define with_numactl 0%{!?_without_numactl:1} # A few optional bits off by default, we enable later %define with_fuse 0 -- 2.26.2
To keep things maintainable, we want to have architecture handling all in one spot instead of sprinkling %ifarch conditionals all over the place. Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Neal Gompa <ngompa13@gmail.com> --- libvirt.spec.in | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index d9fa27a5fe..ae6381fe3e 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -94,6 +94,7 @@ %define with_libssh2 0 %define with_wireshark 0 %define with_libssh 0 +%define with_dmidecode 0 # Finally set the OS / architecture specific special cases @@ -183,6 +184,10 @@ %endif %endif +%ifarch %{ix86} x86_64 + %define with_dmidecode 0%{!?_without_dmidecode:1} +%endif + # Force QEMU to run as non-root %define qemu_user qemu %define qemu_group qemu @@ -435,7 +440,7 @@ Requires: iproute-tc %endif Requires: polkit >= 0.112 -%ifarch %{ix86} x86_64 +%if %{with_dmidecode} # For virConnectGetSysinfo Requires: dmidecode %endif -- 2.26.2
It belongs before package-specific feature flags are defined. Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Neal Gompa <ngompa13@gmail.com> --- libvirt.spec.in | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index ae6381fe3e..dd2247d712 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -12,6 +12,11 @@ %define supported_platform 0 %endif +# On RHEL 7 and older macro _vpath_builddir is not defined. +%if 0%{?rhel} && 0%{?rhel} <= 7 + %define _vpath_builddir %{_target_platform} +%endif + # The hypervisor drivers that run in libvirtd %define with_qemu 0%{!?_without_qemu:1} %define with_lxc 0%{!?_without_lxc:1} @@ -31,11 +36,6 @@ %define qemu_kvm_arches x86_64 %{power64} aarch64 s390x %endif -# On RHEL 7 and older macro _vpath_builddir is not defined. -%if 0%{?rhel} && 0%{?rhel} <= 7 - %define _vpath_builddir %{_target_platform} -%endif - %ifarch %{qemu_kvm_arches} %define with_qemu_kvm %{with_qemu} %else -- 2.26.2
Neither Fedora nor RHEL build packages on this architecture. Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Neal Gompa <ngompa13@gmail.com> Reviewed-by: Thomas Huth <thuth@redhat.com> --- libvirt.spec.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index dd2247d712..7ab62ebd63 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -109,12 +109,12 @@ %endif # Numactl is not available on many non-x86 archs -%ifarch s390 s390x %{arm} riscv64 +%ifarch s390x %{arm} riscv64 %define with_numactl 0 %endif # zfs-fuse is not available on some architectures -%ifarch s390 s390x aarch64 riscv64 +%ifarch s390x aarch64 riscv64 %define with_storage_zfs 0 %endif @@ -179,7 +179,7 @@ %if %{with_qemu} || %{with_lxc} # numad is used to manage the CPU and memory placement dynamically, # it's not available on many non-x86 architectures. - %ifnarch s390 s390x %{arm} riscv64 + %ifnarch s390x %{arm} riscv64 %define with_numad 0%{!?_without_numad:1} %endif %endif -- 2.26.2
There's no need to set a default for it if we're going to override it immediately afterwards anyway, and setting with_qemu_tcg at the same time only makes things more confusing. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 7ab62ebd63..a0cd5ce5af 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -17,31 +17,30 @@ %define _vpath_builddir %{_target_platform} %endif +%define qemu_kvm_arches %{ix86} x86_64 %{power64} %{arm} aarch64 s390x +%if 0%{?rhel} + %define qemu_kvm_arches x86_64 %{power64} aarch64 s390x +%endif + # The hypervisor drivers that run in libvirtd %define with_qemu 0%{!?_without_qemu:1} %define with_lxc 0%{!?_without_lxc:1} %define with_libxl 0%{!?_without_libxl:1} %define with_vbox 0%{!?_without_vbox:1} -%define with_qemu_tcg %{with_qemu} - -%define qemu_kvm_arches %{ix86} x86_64 - -%if 0%{?fedora} - %define qemu_kvm_arches %{ix86} x86_64 %{power64} s390x %{arm} aarch64 -%endif - -%if 0%{?rhel} - %define with_qemu_tcg 0 - %define qemu_kvm_arches x86_64 %{power64} aarch64 s390x -%endif - %ifarch %{qemu_kvm_arches} %define with_qemu_kvm %{with_qemu} %else %define with_qemu_kvm 0 %endif +%define with_qemu_tcg %{with_qemu} + +# RHEL disables TCG on all architectures +%if 0%{?rhel} + %define with_qemu_tcg 0 +%endif + %if ! %{with_qemu_tcg} && ! %{with_qemu_kvm} %define with_qemu 0 %endif -- 2.26.2
With this commit, all architecture lists that we base feature enablement decisions on are defined within a few lines of each other, increasing maintainability. Additionally, generic architecture lists that appear in the conditions for multiple features are defined, so that repetition is reduced. Note that a few checks (numactl, zfs, ceph) have been changed from %ifarch to %ifnarch for consistency: while doing so, the corresponding list of architectures has also been replaced with the complement of the original one to ensure the overall behavior would be preserved. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 48 ++++++++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index a0cd5ce5af..cbcf7dcc60 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -17,18 +17,30 @@ %define _vpath_builddir %{_target_platform} %endif -%define qemu_kvm_arches %{ix86} x86_64 %{power64} %{arm} aarch64 s390x +%define arches_qemu_kvm %{ix86} x86_64 %{power64} %{arm} aarch64 s390x %if 0%{?rhel} - %define qemu_kvm_arches x86_64 %{power64} aarch64 s390x + %define arches_qemu_kvm x86_64 %{power64} aarch64 s390x %endif +%define arches_64bit x86_64 %{power64} aarch64 s390x riscv64 +%define arches_x86 %{ix86} x86_64 + +%define arches_systemtap_64bit %{arches_64bit} +%define arches_dmidecode %{arches_x86} +%define arches_xen %{arches_x86} aarch64 +%define arches_vbox %{arches_x86} +%define arches_ceph %{arches_64bit} +%define arches_zfs %{arches_x86} %{power64} %{arm} +%define arches_numactl %{arches_x86} %{power64} aarch64 +%define arches_numad %{arches_x86} %{power64} aarch64 + # The hypervisor drivers that run in libvirtd %define with_qemu 0%{!?_without_qemu:1} %define with_lxc 0%{!?_without_lxc:1} %define with_libxl 0%{!?_without_libxl:1} %define with_vbox 0%{!?_without_vbox:1} -%ifarch %{qemu_kvm_arches} +%ifarch %{arches_qemu_kvm} %define with_qemu_kvm %{with_qemu} %else %define with_qemu_kvm 0 @@ -60,7 +72,7 @@ %endif %define with_storage_gluster 0%{!?_without_storage_gluster:1} -%ifnarch %{qemu_kvm_arches} +%ifnarch %{arches_qemu_kvm} # gluster is only built where qemu driver is enabled on RHEL 8 %if 0%{?rhel} >= 8 %define with_storage_gluster 0 @@ -97,28 +109,20 @@ # Finally set the OS / architecture specific special cases -# Xen is available only on some architectures -%ifnarch %{ix86} x86_64 aarch64 +# Architecture-dependent features +%ifnarch %{arches_xen} %define with_libxl 0 %endif - -# vbox is available only on i386 x86_64 -%ifnarch %{ix86} x86_64 +%ifnarch %{arches_vbox} %define with_vbox 0 %endif - -# Numactl is not available on many non-x86 archs -%ifarch s390x %{arm} riscv64 +%ifnarch %{arches_numactl} %define with_numactl 0 %endif - -# zfs-fuse is not available on some architectures -%ifarch s390x aarch64 riscv64 +%ifnarch %{arches_zfs} %define with_storage_zfs 0 %endif - -# Ceph dropped support for 32-bit hosts -%ifarch %{arm} %{ix86} +%ifnarch %{arches_ceph} %define with_storage_rbd 0 %endif @@ -154,7 +158,7 @@ %define with_sanlock 0%{!?_without_sanlock:1} %endif %if 0%{?rhel} - %ifarch %{qemu_kvm_arches} + %ifarch %{arches_qemu_kvm} %define with_sanlock 0%{!?_without_sanlock:1} %endif %endif @@ -178,12 +182,12 @@ %if %{with_qemu} || %{with_lxc} # numad is used to manage the CPU and memory placement dynamically, # it's not available on many non-x86 architectures. - %ifnarch s390x %{arm} riscv64 + %ifnarch %{arches_numad} %define with_numad 0%{!?_without_numad:1} %endif %endif -%ifarch %{ix86} x86_64 +%ifarch %{arches_dmidecode} %define with_dmidecode 0%{!?_without_dmidecode:1} %endif @@ -1256,7 +1260,7 @@ rm -f $RPM_BUILD_ROOT%{_datadir}/augeas/lenses/tests/test_libvirtd_libxl.aug # Copied into libvirt-docs subpackage eventually mv $RPM_BUILD_ROOT%{_datadir}/doc/libvirt libvirt-docs -%ifarch %{power64} s390x x86_64 aarch64 riscv64 +%ifarch %{arches_systemtap_64bit} mv $RPM_BUILD_ROOT%{_datadir}/systemtap/tapset/libvirt_probes.stp \ $RPM_BUILD_ROOT%{_datadir}/systemtap/tapset/libvirt_probes-64.stp -- 2.26.2
On Fri, Oct 9, 2020 at 5:27 AM Andrea Bolognani <abologna@redhat.com> wrote:
Changes from [v1]:
* address review feedback.
[v1] https://www.redhat.com/archives/libvir-list/2020-October/msg00336.html
Andrea Bolognani (9): spec: Simplify setting features off by default spec: firewalld is always enabled spec: bash completion actually defaults to on spec: Move with_numactl definition spec: Introduce with_dmidecode spec: Move _vpath_builddir definition spec: Drop s390 architecture from conditionals spec: Refactor qemu_kvm_arches definition spec: Introduce arches_*
libvirt.spec.in | 116 ++++++++++++++++++++++++------------------------ 1 file changed, 57 insertions(+), 59 deletions(-)
-- 2.26.2
Series LGTM. Reviewed-by: Neal Gompa <ngompa13@gmail.com> -- 真実はいつも一つ！/ Always, there's only one truth!
participants (2)
-
Andrea Bolognani -
Neal Gompa