[v3] Produce more verbose error if cppi not found

[libvirt] [PATCH v3] Produce more verbose error if cppi not found

Posted by Michal Privoznik 4 years, 10 months ago

It's fairly easy (especially for new contributors) to not spot
the 'cppi not installed' line in the syntax-check output. Turn it
into a banner that is more visible and at the same time add it as
a build dependency. Unfortunately, RHEL doesn't ship cppi so we
can add the dependency only for Fedora.

Since it's v1 this has effectively became code copied over from
Andrea's review suggestions.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 cfg.mk          | 5 +++++
 libvirt.spec.in | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/cfg.mk b/cfg.mk
index 5074ef611a..c0c240b2c0 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -1145,6 +1145,11 @@ ifneq ($(_gl-Makefile),)
 syntax-check: spacing-check test-wrap-argv \
 	prohibit-duplicate-header mock-noinline group-qemu-caps \
         header-ifdef
+	@if ! cppi --version >/dev/null 2>&1; then \
+		echo "*****************************************************" >&2; \
+		echo "* cppi not installed, some checks have been skipped *" >&2; \
+		echo "*****************************************************" >&2; \
+	fi
 endif
 
 # Don't include duplicate header in the source (either *.c or *.h)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 3b5b4925fd..7019488711 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -271,6 +271,10 @@ BuildRequires: perl
 %endif
 BuildRequires: %{python}
 BuildRequires: systemd-units
+# For 'make syntax-check'
+%if 0%{?fedora}
+BuildRequires: cppi
+%endif
 %if %{with_libxl}
 BuildRequires: xen-devel
 %endif
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3] Produce more verbose error if cppi not found

Posted by Andrea Bolognani 4 years, 10 months ago

On Mon, 2019-06-17 at 18:21 +0200, Michal Privoznik wrote:
> It's fairly easy (especially for new contributors) to not spot
> the 'cppi not installed' line in the syntax-check output. Turn it
> into a banner that is more visible

Good so far.

> and at the same time add it as
> a build dependency. Unfortunately, RHEL doesn't ship cppi so we
> can add the dependency only for Fedora.

Depending on the outcome of the discussion below, this part might
need to be dropped.

> Since it's v1 this has effectively became code copied over from
> Andrea's review suggestions.

I don't feel like this remark belongs in the git history.

[...]
>  syntax-check: spacing-check test-wrap-argv \
>  	prohibit-duplicate-header mock-noinline group-qemu-caps \
>          header-ifdef
> +	@if ! cppi --version >/dev/null 2>&1; then \
> +		echo "*****************************************************" >&2; \
> +		echo "* cppi not installed, some checks have been skipped *" >&2; \
> +		echo "*****************************************************" >&2; \
> +	fi
>  endif

ACK to this hunk.

[...]
> +# For 'make syntax-check'
> +%if 0%{?fedora}
> +BuildRequires: cppi
> +%endif

CC'ing Dan and Martin, who argued for having this, and Pavel, who
argued against and made me switch sides. Let's see what ends up
being its fate :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3] Produce more verbose error if cppi not found

Posted by Daniel P. Berrangé 4 years, 10 months ago

On Mon, Jun 17, 2019 at 06:40:14PM +0200, Andrea Bolognani wrote:
> On Mon, 2019-06-17 at 18:21 +0200, Michal Privoznik wrote:
> > It's fairly easy (especially for new contributors) to not spot
> > the 'cppi not installed' line in the syntax-check output. Turn it
> > into a banner that is more visible
> 
> Good so far.
> 
> > and at the same time add it as
> > a build dependency. Unfortunately, RHEL doesn't ship cppi so we
> > can add the dependency only for Fedora.
> 
> Depending on the outcome of the discussion below, this part might
> need to be dropped.
> 
> > Since it's v1 this has effectively became code copied over from
> > Andrea's review suggestions.
> 
> I don't feel like this remark belongs in the git history.
> 
> [...]
> >  syntax-check: spacing-check test-wrap-argv \
> >  	prohibit-duplicate-header mock-noinline group-qemu-caps \
> >          header-ifdef
> > +	@if ! cppi --version >/dev/null 2>&1; then \
> > +		echo "*****************************************************" >&2; \
> > +		echo "* cppi not installed, some checks have been skipped *" >&2; \
> > +		echo "*****************************************************" >&2; \
> > +	fi
> >  endif
> 
> ACK to this hunk.
> 
> [...]
> > +# For 'make syntax-check'
> > +%if 0%{?fedora}
> > +BuildRequires: cppi
> > +%endif
> 
> CC'ing Dan and Martin, who argued for having this, and Pavel, who
> argued against and made me switch sides. Let's see what ends up
> being its fate :)

UI see Pavel objected because we don't run syntax-check in the RPM. This
is a reasonable objection IMHO. I had forgotten we didn't run syntax-check
when I suggested it was a good addition. IOW I'm with Pavel on this.

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3] Produce more verbose error if cppi not found

Posted by Michal Privoznik 4 years, 10 months ago

On 6/17/19 6:40 PM, Andrea Bolognani wrote:
> On Mon, 2019-06-17 at 18:21 +0200, Michal Privoznik wrote:
>> It's fairly easy (especially for new contributors) to not spot
>> the 'cppi not installed' line in the syntax-check output. Turn it
>> into a banner that is more visible
> 
> Good so far.
> 
>> and at the same time add it as
>> a build dependency. Unfortunately, RHEL doesn't ship cppi so we
>> can add the dependency only for Fedora.
> 
> Depending on the outcome of the discussion below, this part might
> need to be dropped.
> 
>> Since it's v1 this has effectively became code copied over from
>> Andrea's review suggestions.
> 
> I don't feel like this remark belongs in the git history.
> 
> [...]
>>   syntax-check: spacing-check test-wrap-argv \
>>   	prohibit-duplicate-header mock-noinline group-qemu-caps \
>>           header-ifdef
>> +	@if ! cppi --version >/dev/null 2>&1; then \
>> +		echo "*****************************************************" >&2; \
>> +		echo "* cppi not installed, some checks have been skipped *" >&2; \
>> +		echo "*****************************************************" >&2; \
>> +	fi
>>   endif
> 
> ACK to this hunk.
> 
> [...]
>> +# For 'make syntax-check'
>> +%if 0%{?fedora}
>> +BuildRequires: cppi
>> +%endif
> 
> CC'ing Dan and Martin, who argued for having this, and Pavel, who
> argued against and made me switch sides. Let's see what ends up
> being its fate :)
> 

Okay, I'll push only the first hunk after I've cleaned the commit 
message a bit.

Thanks,
Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3] Produce more verbose error if cppi not found

Posted by Andrea Bolognani 4 years, 10 months ago

On Tue, 2019-06-18 at 08:55 +0200, Michal Privoznik wrote:
> Okay, I'll push only the first hunk after I've cleaned the commit 
> message a bit.

Great! Thanks for putting up with me O:-)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list