Re: [libvirt] [PATCH 2/2] spec: indent %if to make it easier to see conditions
On Thu, Jan 10, 2013 at 02:07:59PM +0100, Viktor Mihajlovski wrote: > On 01/10/2013 12:50 PM, Daniel P. Berrange wrote: > > >IIUC, RPM will expand macros *anywhere* in the spec file, even > >in places you don't want it to like comments ! So I don't think > >adding some whitespace will harm things > > > Technically absolutely true. I was thinking more about potential > formatting conventions used by distribution packagers. The > packaging rules I've seen don't explicitly mention indentation, so one > could infer it doesn't matter and it is merely an esthetic question. > But then beauty is in the eye of the beholder... Fedora has a rule: https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Spec_Legibility "All Fedora Package Spec Files must be legible. If the reviewer is unable to read the spec file, it will be impossible to perform a review. Fedora Spec files are not the place for entries into the Obfuscated Code Contest. " so I think this change helps meet that requirement 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] spec: indent %if to make it easier to see conditions
On 01/10/2013 12:50 PM, Daniel P. Berrange wrote: IIUC, RPM will expand macros *anywhere* in the spec file, even in places you don't want it to like comments ! So I don't think adding some whitespace will harm things Technically absolutely true. I was thinking more about potential formatting conventions used by distribution packagers. The packaging rules I've seen don't explicitly mention indentation, so one could infer it doesn't matter and it is merely an esthetic question. But then beauty is in the eye of the beholder... -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] spec: indent %if to make it easier to see conditions
On Thu, Jan 10, 2013 at 11:57:28AM +0100, Viktor Mihajlovski wrote: > On 01/10/2013 12:45 AM, Eric Blake wrote: > >* libvirt.spec.in: Add some indentation to make it easier to follow > >various conditionals. > >--- > > libvirt.spec.in | 858 > > > > 1 file changed, 428 insertions(+), 430 deletions(-) > > > Painful experience makes me agree that the unindented nested %ifs > decrease the readability of the spec file and thus would be in favor > of this patch. > However, indentation seems to be uncustomary. On the other hand, it > doesn't seem to be explicitly forbidden, looking at e.g. > http://fedoraproject.org/wiki/Packaging:Guidelines > Any packager here that wants to throw in 2 cents? IIUC, RPM will expand macros *anywhere* in the spec file, even in places you don't want it to like comments ! So I don't think adding some whitespace will harm things 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] spec: indent %if to make it easier to see conditions
On 01/10/2013 12:45 AM, Eric Blake wrote:
* libvirt.spec.in: Add some indentation to make it easier to follow
various conditionals.
---
libvirt.spec.in | 858
1 file changed, 428 insertions(+), 430 deletions(-)
Painful experience makes me agree that the unindented nested %ifs
decrease the readability of the spec file and thus would be in favor of
this patch.
However, indentation seems to be uncustomary. On the other hand, it
doesn't seem to be explicitly forbidden, looking at e.g.
http://fedoraproject.org/wiki/Packaging:Guidelines
Any packager here that wants to throw in 2 cents?
# Fedora 18 / RHEL-7 are first where firewalld support is enabled
%if 0%{?fedora} >= 17 || 0%{?rhel} >= 7
None of your doing, but the contradiction between comment (F18) and
directive (F17) should be resolved.
--
Mit freundlichen Grüßen/Kind Regards
Viktor Mihajlovski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] spec: indent %if to make it easier to see conditions
On Wed, Jan 09, 2013 at 16:45:12 -0700, Eric Blake wrote:
> * libvirt.spec.in: Add some indentation to make it easier to follow
> various conditionals.
> ---
> libvirt.spec.in | 858
>
> 1 file changed, 428 insertions(+), 430 deletions(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 2eba7c7..a567870 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -22,16 +22,16 @@
>
> # RHEL-5 builds are client-only for s390, ppc
> %if 0%{?rhel} == 5
> -%ifnarch %{ix86} x86_64 ia64
> -%define client_only1
> -%endif
> + %ifnarch %{ix86} x86_64 ia64
> + %define client_only1
> + %endif
> %endif
I like the idea but I'm a bit concerned about compatibility with various
RPM versions. Is this documented as being supported somewhere?
> @@ -2505,7 +2503,7 @@ rm -f
> $RPM_BUILD_ROOT%{_prefix}/lib/sysctl.d/libvirtd.conf
> - need rebuild since libxenstore is now versionned
>
> * Mon Jul 24 2006 Mark McLoughlin - 0.1.3-2
> -- Add BuildRequires: xen-devel
> +- AddBuildRequires: xen-devel
>
> * Wed Jul 12 2006 Jesse Keating - 0.1.3-1.1
> - rebuild
However, you certainly didn't want to do this last change :-)
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] spec: indent %if to make it easier to see conditions
* libvirt.spec.in: Add some indentation to make it easier to follow
various conditionals.
---
libvirt.spec.in | 858
1 file changed, 428 insertions(+), 430 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 2eba7c7..a567870 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -22,16 +22,16 @@
# RHEL-5 builds are client-only for s390, ppc
%if 0%{?rhel} == 5
-%ifnarch %{ix86} x86_64 ia64
-%define client_only1
-%endif
+ %ifnarch %{ix86} x86_64 ia64
+ %define client_only1
+ %endif
%endif
# Disable all server side drivers if client only build requested
%if %{client_only}
-%define server_drivers 0
+ %define server_drivers 0
%else
-%define server_drivers 1
+ %define server_drivers 1
%endif
# Always build with dlopen'd modules
@@ -54,15 +54,15 @@
%define with_qemu_tcg %{with_qemu}
# Change if we ever provide qemu-kvm binaries on non-x86 hosts
%if 0%{?fedora} >= 18
-%define qemu_kvm_arches%{ix86} x86_64 ppc64 s390x
+ %define qemu_kvm_arches%{ix86} x86_64 ppc64 s390x
%else
-%define qemu_kvm_arches%{ix86} x86_64
+ %define qemu_kvm_arches%{ix86} x86_64
%endif
%ifarch %{qemu_kvm_arches}
-%define with_qemu_kvm %{with_qemu}
+ %define with_qemu_kvm %{with_qemu}
%else
-%define with_qemu_kvm 0
+ %define with_qemu_kvm 0
%endif
# Then the hypervisor drivers that run outside libvirtd, in libvirt.so
@@ -84,14 +84,14 @@
%define with_storage_disk 0%{!?_without_storage_disk:%{server_drivers}}
%define with_storage_mpath0%{!?_without_storage_mpath:%{server_drivers}}
%if 0%{?fedora} >= 16
-%define with_storage_rbd 0%{!?_without_storage_rbd:%{server_drivers}}
+ %define with_storage_rbd 0%{!?_without_storage_rbd:%{server_drivers}}
%else
-%define with_storage_rbd 0
+ %define with_storage_rbd 0
%endif
%if 0%{?fedora} >= 17
-%define with_storage_sheepdog 0%{!?_without_storage_sheepdog:%{server_drivers}}
+ %define with_storage_sheepdog
0%{!?_without_storage_sheepdog:%{server_drivers}}
%else
-%define with_storage_sheepdog 0
+ %define with_storage_sheepdog 0
%endif
%define with_numactl 0%{!?_without_numactl:%{server_drivers}}
%define with_selinux 0%{!?_without_selinux:%{server_drivers}}
@@ -126,215 +126,215 @@
# Xen is available only on i386 x86_64 ia64
%ifnarch %{ix86} x86_64 ia64
-%define with_xen 0
-%define with_libxl 0
+ %define with_xen 0
+ %define with_libxl 0
%endif
# Numactl is not available on s390[x] and ARM
%ifarch s390 s390x %{arm}
-%define with_numactl 0
+ %define with_numactl 0
%endif
# RHEL doesn't ship OpenVZ, VBox, UML, PowerHypervisor,
# VMWare, libxenserver (xenapi), libxenlight (Xen 4.1 and newer),
# or HyperV.
%if 0%{?rhel}
-%define with_openvz 0
-%define with_vbox 0
-%define with_uml 0
-%define with_phyp 0
-%define with_vmware 0
-%define with_xenapi 0
-%define with_libxl 0
-%define with_hyperv 0
-%define with_parallels 0
+ %define with_openvz 0
+ %define with_vbox 0
+ %define with_uml 0
+ %define with_phyp 0
+ %define with_vmware 0
+ %define with_xenapi 0
+ %define with_libxl 0
+ %define with_hyperv 0
+ %define with_parallels 0
%endif
# Fedora 17 / RHEL-7 are first where we use systemd. Although earlier
# Fedora has systemd, libvirt still used sysvinit there.
%if 0%{?fedora} >= 17 || 0%{?rhel} >= 7
-%define with_systemd 1
+ %define with_systemd 1
%endif
# Fedora 18 / RHEL-7 are first where firewalld support is enabled
%if 0%{?fedora} >= 17 || 0%{?rhel} >= 7
-%define with_firewalld 1
+ %define with_firewalld 1
%endif
# RHEL-5 has restricted QEMU to x86_64 only and is too old for LXC
%if 0%{?rhel} == 5
-%define with_qemu_tcg 0
-%ifnarch x86_64
-%define with_qemu 0
-%define with_qemu_kvm 0
-%endif
-%define with_lxc 0
+ %define with_qemu_tcg 0
+ %ifnarch x86_64
+ %define with_qemu 0
+ %define with_qemu_kvm 0
+ %endif
+ %define with_lxc 0
%endif
# RHEL-6 has restricted QEMU to x86_64 only, stopped including Xen
# on all archs. Other archs all have LXC available though
%if 0%{?rhel} >= 6
-%define with_qemu_tcg 0
-%ifnarch x86_64
-%define with_qemu 0
-%define with_qemu_kvm 0
-%endif
-%define with_xen 0
+ %define with_qemu_tcg 0
+ %ifnarch x86_64
+ %define with_qemu 0
+ %define with_qemu_kvm 0
+ %endif
+ %define with_xen 0
%endif
# Fedora doesn't have any QEMU on ppc64 until FC16 - only ppc
%if 0%{?fedora} && 0%{?fedora} < 16
-%ifarch ppc64
-%define with_qemu 0
-%endif
+ %ifarch ppc64
+ %define with_qemu 0
+ %endif
%endif
# Fedora doesn't have new enough Xen for libxl until F18
%if 0%{?fedora} && 0%{?fedora} < 18
-%define with_libxl 0
+ %define with_libxl 0
%endif
# PolicyKit was introduced in Fedora 8 / RHEL-6 or newer
%if 0%{?fedora} >= 8 || 0%{?rhel} >= 6
-%define with_polkit0%{!?_without_polkit:1}
+ %define with_polkit0%{!?_without_polkit:1}
%endif
# libcapng is used to manage capabilities in Fedora 12 / RHEL-6 or newer
%if 0%{?fedora} >= 12
