[libvirt] [PATCH v3] Produce more verbose error if cppi not found
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
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
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 :|
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
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Michal Privoznik