Revert "spec: Simplify setting features off by default"

[PATCH] Revert "spec: Simplify setting features off by default"

Posted by Neal Gompa 3 years, 6 months ago

As it turns out, the rather complicated structure that is
currently used for enabling or disabling features in the libvirt
build does not cleanly map well to RPM's bcond feature.

Consequently, we need these back in order to support trivially
activating these features through extra macros as build inputs.

This reverts commit 31d687a3218c9072d7050dd608e013e063ca180f.

Signed-off-by: Neal Gompa <ngompa13@gmail.com>
---
 libvirt.spec.in | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 2a4324b974..84515cc7de 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -98,14 +98,14 @@
 %define with_numactl          0%{!?_without_numactl:1}
 
 # A few optional bits off by default, we enable later
-%define with_fuse             0
-%define with_sanlock          0
-%define with_numad            0
-%define with_firewalld_zone   0
-%define with_libssh2          0
-%define with_wireshark        0
-%define with_libssh           0
-%define with_dmidecode        0
+%define with_fuse             0%{!?_without_fuse:0}
+%define with_sanlock          0%{!?_without_sanlock:0}
+%define with_numad            0%{!?_without_numad:0}
+%define with_firewalld_zone   0%{!?_without_firewalld:0}
+%define with_libssh2          0%{!?_without_libssh2:0}
+%define with_wireshark        0%{!?_without_wireshark:0}
+%define with_libssh           0%{!?_without_libssh:0}
+%define with_dmidecode        0%{!?_without_dmidecode:0}
 
 # Finally set the OS / architecture specific special cases
 
-- 
2.28.0

Re: [PATCH] Revert "spec: Simplify setting features off by default"

Posted by Michal Privoznik 3 years, 6 months ago

On 10/26/20 10:53 PM, Neal Gompa wrote:
> As it turns out, the rather complicated structure that is
> currently used for enabling or disabling features in the libvirt
> build does not cleanly map well to RPM's bcond feature.
> 
> Consequently, we need these back in order to support trivially
> activating these features through extra macros as build inputs.
> 
> This reverts commit 31d687a3218c9072d7050dd608e013e063ca180f.
> 
> Signed-off-by: Neal Gompa <ngompa13@gmail.com>
> ---
>   libvirt.spec.in | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

and pushed.

Michal

Re: [PATCH] Revert "spec: Simplify setting features off by default"

Posted by Andrea Bolognani 3 years, 6 months ago

On Tue, 2020-10-27 at 11:07 +0100, Michal Privoznik wrote:
> On 10/26/20 10:53 PM, Neal Gompa wrote:
> > As it turns out, the rather complicated structure that is
> > currently used for enabling or disabling features in the libvirt
> > build does not cleanly map well to RPM's bcond feature.
> > 
> > Consequently, we need these back in order to support trivially
> > activating these features through extra macros as build inputs.
> > 
> > This reverts commit 31d687a3218c9072d7050dd608e013e063ca180f.
> > 
> > Signed-off-by: Neal Gompa <ngompa13@gmail.com>
> > ---
> >   libvirt.spec.in | 16 ++++++++--------
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 
> and pushed.

I'm not convinced reverting this was the right call.

The way RPM conditional macros work is that, if you have

  %{!?macro:value}

that will expand to 'value' if 'macro' is *not* defined, and to
nothing otherwise. So if you have for example

  %define with_fuse          0%{!?_without_fuse:0}

that means that, if you pass

  --define '_without_fuse 1'

to rpmbuild the line will expand to

  %define with_fuse          0

and if you don't pass the extra option to rpmbuild it will instead
expand to

  %define with_fuse          00

The two are clearly equivalent, so there is no point in keeping the 
conditional macro - it merely obfuscates the logic.

Of course things would be different if you had

  %define with_fuse          0%{!?_without_fuse:1}

instead: in this case, the line would expand to

  %define with_fuse          0

and

  %define with_fuse          01

respectively, which means the feature would be enabled by default but
could optionally be disabled by passing the correct argument to
rpmbuild, as expected. We have plenty similar macros in libvirt.spec,
and since they work just as intended 31d687a3218c didn't touch them.

Note that in this case I've removed

  # fuse is used to provide virtualized /proc for LXC
  %if %{with_lxc}
      %define with_fuse      0%{!?_without_fuse:1}
  %endif

from the spec to make sure that the value for the 'fuse' option
passed to Meson depended solely on the value of the _without_fuse
macro, and then checked the rpmbuild output to compare.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] Revert "spec: Simplify setting features off by default"

Posted by Andrea Bolognani 3 years, 6 months ago

On Tue, 2020-10-27 at 14:05 +0100, Andrea Bolognani wrote:
> I'm not convinced reverting this was the right call.
> 
> The way RPM conditional macros work is that, if you have
> 
>   %{!?macro:value}
> 
> that will expand to 'value' if 'macro' is *not* defined, and to
> nothing otherwise. So if you have for example
> 
>   %define with_fuse          0%{!?_without_fuse:0}
> 
> that means that, if you pass
> 
>   --define '_without_fuse 1'
> 
> to rpmbuild the line will expand to
> 
>   %define with_fuse          0
> 
> and if you don't pass the extra option to rpmbuild it will instead
> expand to
> 
>   %define with_fuse          00
> 
> The two are clearly equivalent, so there is no point in keeping the 
> conditional macro - it merely obfuscates the logic.
> 
> Of course things would be different if you had
> 
>   %define with_fuse          0%{!?_without_fuse:1}
> 
> instead: in this case, the line would expand to
> 
>   %define with_fuse          0
> 
> and
> 
>   %define with_fuse          01
> 
> respectively, which means the feature would be enabled by default but
> could optionally be disabled by passing the correct argument to
> rpmbuild, as expected. We have plenty similar macros in libvirt.spec,
> and since they work just as intended 31d687a3218c didn't touch them.
> 
> Note that in this case I've removed
> 
>   # fuse is used to provide virtualized /proc for LXC
>   %if %{with_lxc}
>       %define with_fuse      0%{!?_without_fuse:1}
>   %endif
> 
> from the spec to make sure that the value for the 'fuse' option
> passed to Meson depended solely on the value of the _without_fuse
> macro, and then checked the rpmbuild output to compare.

Also note that I'm aware you want to eventually push for adoption of
the standard bcond macros, and I fully stand behind that desire! If
this patch had been the first in a series that introduced bcond
support and was clearing the path for that, I would have zero
problems with it. As it is, however, you're simply reintroducing some
of the obfuscation we had recently managed to get rid of, without
getting anything in return.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] Revert "spec: Simplify setting features off by default"

Posted by Neal Gompa 3 years, 6 months ago

On Tue, Oct 27, 2020 at 9:30 AM Andrea Bolognani <abologna@redhat.com> wrote:
>
> On Tue, 2020-10-27 at 14:05 +0100, Andrea Bolognani wrote:
> > I'm not convinced reverting this was the right call.
> >
> > The way RPM conditional macros work is that, if you have
> >
> >   %{!?macro:value}
> >
> > that will expand to 'value' if 'macro' is *not* defined, and to
> > nothing otherwise. So if you have for example
> >
> >   %define with_fuse          0%{!?_without_fuse:0}
> >
> > that means that, if you pass
> >
> >   --define '_without_fuse 1'
> >
> > to rpmbuild the line will expand to
> >
> >   %define with_fuse          0
> >
> > and if you don't pass the extra option to rpmbuild it will instead
> > expand to
> >
> >   %define with_fuse          00
> >
> > The two are clearly equivalent, so there is no point in keeping the
> > conditional macro - it merely obfuscates the logic.
> >
> > Of course things would be different if you had
> >
> >   %define with_fuse          0%{!?_without_fuse:1}
> >
> > instead: in this case, the line would expand to
> >
> >   %define with_fuse          0
> >
> > and
> >
> >   %define with_fuse          01
> >
> > respectively, which means the feature would be enabled by default but
> > could optionally be disabled by passing the correct argument to
> > rpmbuild, as expected. We have plenty similar macros in libvirt.spec,
> > and since they work just as intended 31d687a3218c didn't touch them.
> >
> > Note that in this case I've removed
> >
> >   # fuse is used to provide virtualized /proc for LXC
> >   %if %{with_lxc}
> >       %define with_fuse      0%{!?_without_fuse:1}
> >   %endif
> >
> > from the spec to make sure that the value for the 'fuse' option
> > passed to Meson depended solely on the value of the _without_fuse
> > macro, and then checked the rpmbuild output to compare.
>

Ugh, you're right, and those values need to be changed to 1.

> Also note that I'm aware you want to eventually push for adoption of
> the standard bcond macros, and I fully stand behind that desire! If
> this patch had been the first in a series that introduced bcond
> support and was clearing the path for that, I would have zero
> problems with it. As it is, however, you're simply reintroducing some
> of the obfuscation we had recently managed to get rid of, without
> getting anything in return.
>

Fixing this so that I can switch to bconds is going to be a massive
rewrite of how feature enablement works. That is not something I can
push for a 6.9.0 freeze break patch.

My in-progress rewrite is going to be a massive break in how this is managed...

-- 
真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH] Revert "spec: Simplify setting features off by default"

Posted by Andrea Bolognani 3 years, 6 months ago

On Wed, 2020-10-28 at 16:55 -0400, Neal Gompa wrote:
> On Tue, Oct 27, 2020 at 9:30 AM Andrea Bolognani <abologna@redhat.com> wrote:
> > On Tue, 2020-10-27 at 14:05 +0100, Andrea Bolognani wrote:
[...]
> > > Note that in this case I've removed
> > > 
> > >   # fuse is used to provide virtualized /proc for LXC
> > >   %if %{with_lxc}
> > >       %define with_fuse      0%{!?_without_fuse:1}
> > >   %endif
> > > 
> > > from the spec to make sure that the value for the 'fuse' option
> > > passed to Meson depended solely on the value of the _without_fuse
> > > macro, and then checked the rpmbuild output to compare.
> 
> Ugh, you're right, and those values need to be changed to 1.

Yeah, maybe we should reconsider whether the features in our spec
which are off by default should not be on by default instead. It's
not something that I've tried to do with my previous patches in the
area: I've limited myself to fixing logic issues and cleaning up the
implementation, leaving the semantics unmodified.

> > Also note that I'm aware you want to eventually push for adoption of
> > the standard bcond macros, and I fully stand behind that desire! If
> > this patch had been the first in a series that introduced bcond
> > support and was clearing the path for that, I would have zero
> > problems with it. As it is, however, you're simply reintroducing some
> > of the obfuscation we had recently managed to get rid of, without
> > getting anything in return.
> 
> Fixing this so that I can switch to bconds is going to be a massive
> rewrite of how feature enablement works. That is not something I can
> push for a 6.9.0 freeze break patch.
> 
> My in-progress rewrite is going to be a massive break in how this
> is managed...

The kind of work you're describing is definitely not freeze material!
It's much better to merge big series early in the development cycle
anyway.

In any case, since you agree with me that your revert left the logic
unchanged and merely reintroduced the old obfuscation, I'm going to
go ahead and... Revert it :)

Looking forward to your bcond patches!

-- 
Andrea Bolognani / Red Hat / Virtualization