build: fix specfile logic for disabling netcf

[PATCH] build: fix specfile logic for disabling netcf

Posted by Laine Stump 3 years, 2 months ago

I *thought* I had tested all the combinations of manually setting
--without netcf, different versions of Fedora, etc, but apparently
not.

The check in libvirt.spec.in to see if the target was an older Fedora
or older RHEL would alway resolve to true, because, e.g., if {?fedora}
is undefined, then "0%{?fedora} < 34" is "0 < 34", which is always
true. Since both {?fedora} and {?rhel} are never defined at the same
time, the result of the entire expression is always true.

Fix this by qualifying each subexpression.

Fixes: 35d5b26aa433bd33f4b33be3dbb67313357f97f9
Signed-off-by: Laine Stump <laine@redhat.com>
---
 libvirt.spec.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 34b481c69e..29b476184d 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -146,7 +146,7 @@
     %define with_firewalld_zone 0%{!?_without_firewalld_zone:1}
 %endif
 
-%if 0%{?fedora} < 34 || 0%{?rhel} < 9
+%if (0%{?fedora} && 0%{?fedora} < 34) || (0%{?rhel} && 0%{?rhel} < 9)
     %define with_netcf 0%{!?_without_netcf:1}
 %endif
 
-- 
2.29.2

Re: [PATCH] build: fix specfile logic for disabling netcf

Posted by Daniel P. Berrangé 3 years, 2 months ago

On Tue, Feb 02, 2021 at 01:05:04PM -0500, Laine Stump wrote:
> I *thought* I had tested all the combinations of manually setting
> --without netcf, different versions of Fedora, etc, but apparently
> not.
> 
> The check in libvirt.spec.in to see if the target was an older Fedora
> or older RHEL would alway resolve to true, because, e.g., if {?fedora}
> is undefined, then "0%{?fedora} < 34" is "0 < 34", which is always
> true. Since both {?fedora} and {?rhel} are never defined at the same
> time, the result of the entire expression is always true.
> 
> Fix this by qualifying each subexpression.
> 
> Fixes: 35d5b26aa433bd33f4b33be3dbb67313357f97f9
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
>  libvirt.spec.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] build: fix specfile logic for disabling netcf

Posted by Andrea Bolognani 3 years, 2 months ago

On Tue, 2021-02-02 at 13:05 -0500, Laine Stump wrote:
> -%if 0%{?fedora} < 34 || 0%{?rhel} < 9
> +%if (0%{?fedora} && 0%{?fedora} < 34) || (0%{?rhel} && 0%{?rhel} < 9)
>      %define with_netcf 0%{!?_without_netcf:1}
>  %endif

You could also replace the existing

  %define with_netcf 0

that appears earlier in the file with

  %define with_netcf 0%{!?_without_netcf:1}

and rewrite this version check with

  %if 0%{?fedora} > 33 || %{?rhel} > 8
      %define with_netcf 0
  %endif

instead. This would keep the version check simpler.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] build: fix specfile logic for disabling netcf

Posted by Laine Stump 3 years, 2 months ago

On 2/3/21 5:46 AM, Andrea Bolognani wrote:
> On Tue, 2021-02-02 at 13:05 -0500, Laine Stump wrote:
>> -%if 0%{?fedora} < 34 || 0%{?rhel} < 9
>> +%if (0%{?fedora} && 0%{?fedora} < 34) || (0%{?rhel} && 0%{?rhel} < 9)
>>       %define with_netcf 0%{!?_without_netcf:1}
>>   %endif
> You could also replace the existing
>
>    %define with_netcf 0
>
> that appears earlier in the file with
>
>    %define with_netcf 0%{!?_without_netcf:1}
>
> and rewrite this version check with
>
>    %if 0%{?fedora} > 33 || %{?rhel} > 8
>        %define with_netcf 0
>    %endif
>
> instead. This would keep the version check simpler.


I thought about that, but it would make the default be "enabled", and I 
want the default to be "disabled".

Re: [PATCH] build: fix specfile logic for disabling netcf

Posted by Andrea Bolognani 3 years, 2 months ago

On Wed, 2021-02-03 at 10:50 -0500, Laine Stump wrote:
> On 2/3/21 5:46 AM, Andrea Bolognani wrote:
> > You could also replace the existing
> > 
> >    %define with_netcf 0
> > 
> > that appears earlier in the file with
> > 
> >    %define with_netcf 0%{!?_without_netcf:1}
> > 
> > and rewrite this version check with
> > 
> >    %if 0%{?fedora} > 33 || %{?rhel} > 8
> >        %define with_netcf 0
> >    %endif
> > 
> > instead. This would keep the version check simpler.
> 
> I thought about that, but it would make the default be "enabled", and I 
> want the default to be "disabled".

In name only, given that you'd override it for most platforms later.

Alternatively, something like

  %if 0%{?fedora} > 33 || %{?rhel} > 8
      %define with_netcf 0
  %else
      %define with_netcf 0%{!?_without_netcf:1}
  %endif

would work too.

But, so does your current version, so feel free to just pick up Dan's
R-b and push the patch as-is :)

-- 
Andrea Bolognani / Red Hat / Virtualization