On Mon, Oct 5, 2020 at 2:40 PM Andrea Bolognani <abologna(a)redhat.com&gt; wrote: This crazy setup is an attempt to implement rpm %bcond behavior without actually using %bcond statements. Instead of removing the right-hand-side conditionals, perhaps it'd be better to just finally make the switch to %bcond statements. -- 真実はいつも一つ！/ Always, there's only one truth!

Knowing this, we can remove some code. Signed-off-by: Andrea Bolognani <abologna(a)redhat.com&gt; --- libvirt.spec.in | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 815ab246e9..4572044d2d 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 @@ -1088,12 +1085,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 @@ -1169,7 +1160,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(a)redhat.com&gt; --- libvirt.spec.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 4572044d2d..9e4c5d2b81 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(a)redhat.com&gt; --- libvirt.spec.in | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 9e4c5d2b81..65df7dc79f 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(a)redhat.com&gt; --- libvirt.spec.in | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 65df7dc79f..2401404008 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 @@ -434,7 +439,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(a)redhat.com&gt; --- libvirt.spec.in | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 2401404008..d8f689e651 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

On Mon, 2020-10-05 at 20:46 -0400, Neal Gompa wrote: We're installing Meson from PyPi instead of EPEL on CentOS as part of our CI setup, but indeed it looks like that's not necessary and we could simply use the EPEL package. I'll look into that. Anyway, at least on my Fedora machine _vpath_builddir is defined in /usr/lib/rpm/macros.d/macros.vpath, which is part of the redhat-rpm-config package, and the various %meson macros defined in /usr/lib/rpm/macros.d/macros.meson use it, so it doesn't look like even installing Meson from EPEL would remove the need to define that value? Note I haven't actually tried O:-) -- Andrea Bolognani / Red Hat / Virtualization

On Mon, Oct 5, 2020 at 2:41 PM Andrea Bolognani <abologna(a)redhat.com&gt; wrote: Good riddance to bad architectures... Reviewed-by: Neal Gompa <ngompa13(a)gmail.com&gt; -- 真実はいつも一つ！/ Always, there's only one truth!

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(a)redhat.com&gt; --- libvirt.spec.in | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index e036307d30..b62b17ee80 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -17,31 +17,31 @@ %define _vpath_builddir %{_target_platform} %endif +%if 0%{?fedora} + %define qemu_kvm_arches %{ix86} x86_64 %{power64} s390x %{arm} aarch64 +%else + %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(a)redhat.com&gt; --- libvirt.spec.in | 48 ++++++++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index b62b17ee80..32bc51b33c 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -17,10 +17,22 @@ %define _vpath_builddir %{_target_platform} %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} +%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 + %if 0%{?fedora} - %define qemu_kvm_arches %{ix86} x86_64 %{power64} s390x %{arm} aarch64 + %define arches_qemu_kvm %{arches_x86} %{power64} s390x %{arm} aarch64 %else - %define qemu_kvm_arches x86_64 %{power64} aarch64 s390x + %define arches_qemu_kvm x86_64 %{power64} aarch64 s390x %endif # The hypervisor drivers that run in libvirtd @@ -29,7 +41,7 @@ %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 @@ -61,7 +73,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 @@ -98,28 +110,20 @@ # Finally set the OS / architecture specific special cases -# Xen is available only on i386 x86_64 -%ifnarch %{ix86} x86_64 +# 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 @@ -155,7 +159,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 @@ -179,12 +183,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 Mon, Oct 5, 2020 at 2:41 PM Andrea Bolognani <abologna(a)redhat.com&gt; wrote: This conditional is functionally irrelevant. The superset defined for Fedora does not change how things work for RHEL, and it'd be easier to just use the one architecture set. -- 真実はいつも一つ！/ Always, there's only one truth!