1. Patchew
  2. Libvirt
  3. View series

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

Michal Privoznik posted 1 patch 4 years, 10 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/d23619de4c2645aee1bf5150e90b2a767c419a7c.1560773541.git.mprivozn@redhat.com
There is a newer version of this series
cfg.mk          | 9 +++++++--
libvirt.spec.in | 4 ++++
2 files changed, 11 insertions(+), 2 deletions(-)
[libvirt] [PATCH v2] 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.

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

Diff to v1:
- Instead of requiring cppi, produce more visible error
- Require cppi in specfile

 cfg.mk          | 9 +++++++--
 libvirt.spec.in | 4 ++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 5074ef611a..2ee860a2c5 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -695,6 +695,11 @@ sc_require_whitespace_in_translation:
 	  { echo '$(ME): missing whitespace at line split' 1>&2; \
 	    exit 1; } || :
 
+cppi_banner = \
+	" *****************************************************\n" \
+	"* cppi not installed, some checks have been skipped *\n" \
+	"*****************************************************"
+
 # Enforce recommended preprocessor indentation style.
 sc_preprocessor_indentation:
 	@if cppi --version >/dev/null 2>&1; then \
@@ -702,7 +707,7 @@ sc_preprocessor_indentation:
 	    || { echo '$(ME): incorrect preprocessor indentation' 1>&2; \
 		exit 1; }; \
 	else \
-	  echo '$(ME): skipping test $@: cppi not installed' 1>&2; \
+	  echo -e "$(ME): skipping test $@:\n"$(cppi_banner) 1>&2; \
 	fi
 
 # Enforce similar spec file indentation style, by running cppi on a
@@ -719,7 +724,7 @@ sc_spec_indentation:
 	    || { echo '$(ME): incorrect preprocessor indentation' 1>&2; \
 		exit 1; }; \
 	else \
-	  echo '$(ME): skipping test $@: cppi not installed' 1>&2; \
+	  echo -e "$(ME): skipping test $@:\n"$(cppi_banner) 1>&2; \
 	fi
 
 # Nested conditionals are easier to understand if we enforce that endifs
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 59a2a0cb24..1a9e92c4bc 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 v2] Produce more verbose error if cppi not found
Posted by Andrea Bolognani 4 years, 10 months ago 
On Mon, 2019-06-17 at 14:13 +0200, Michal Privoznik wrote:
[...]
> @@ -695,6 +695,11 @@ sc_require_whitespace_in_translation:
>  	  { echo '$(ME): missing whitespace at line split' 1>&2; \
>  	    exit 1; } || :
>  
> +cppi_banner = \
> +	" *****************************************************\n" \
> +	"* cppi not installed, some checks have been skipped *\n" \
> +	"*****************************************************"
> +
>  # Enforce recommended preprocessor indentation style.
>  sc_preprocessor_indentation:
>  	@if cppi --version >/dev/null 2>&1; then \
> @@ -702,7 +707,7 @@ sc_preprocessor_indentation:
>  	    || { echo '$(ME): incorrect preprocessor indentation' 1>&2; \
>  		exit 1; }; \
>  	else \
> -	  echo '$(ME): skipping test $@: cppi not installed' 1>&2; \
> +	  echo -e "$(ME): skipping test $@:\n"$(cppi_banner) 1>&2; \
>  	fi
>  
>  # Enforce similar spec file indentation style, by running cppi on a
> @@ -719,7 +724,7 @@ sc_spec_indentation:
>  	    || { echo '$(ME): incorrect preprocessor indentation' 1>&2; \
>  		exit 1; }; \
>  	else \
> -	  echo '$(ME): skipping test $@: cppi not installed' 1>&2; \
> +	  echo -e "$(ME): skipping test $@:\n"$(cppi_banner) 1>&2; \
>  	fi
>  
>  # Nested conditionals are easier to understand if we enforce that endifs

How about this instead of all of the above?

----- 8< ----- 8< ----- 8< ----- 8< ----- 8< ----- 8< ----- 8< -----
@@ -1150,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)
----- >8 ----- >8 ----- >8 ----- >8 ----- >8 ----- >8 ----- >8 -----

That is, keep briefly mentioning that a specific check has been
skipped because cppi is unavailable as we're already doing, but also
print a big fat warning *at the very end* so that you can't possibly
miss it even if you have been paying no attention while the command
was running.

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

Pavel almost convinced me this is not appropriate for the spec file
by pointing out that we're not actually running 'make syntax-check'
when building the RPM, but Dan pointed out that it can be useful
for people using 'dnf builddep' so it's kind of a toss-up...

I'd probably slightly prefer dropping the hunk, especially once we
have the big fat warning.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] Produce more verbose error if cppi not found
Posted by Michal Privoznik 4 years, 10 months ago 
On 6/17/19 3:27 PM, Andrea Bolognani wrote:
> On Mon, 2019-06-17 at 14:13 +0200, Michal Privoznik wrote:
> [...]
>> @@ -695,6 +695,11 @@ sc_require_whitespace_in_translation:
>>   	  { echo '$(ME): missing whitespace at line split' 1>&2; \
>>   	    exit 1; } || :
>>   
>> +cppi_banner = \
>> +	" *****************************************************\n" \
>> +	"* cppi not installed, some checks have been skipped *\n" \
>> +	"*****************************************************"
>> +
>>   # Enforce recommended preprocessor indentation style.
>>   sc_preprocessor_indentation:
>>   	@if cppi --version >/dev/null 2>&1; then \
>> @@ -702,7 +707,7 @@ sc_preprocessor_indentation:
>>   	    || { echo '$(ME): incorrect preprocessor indentation' 1>&2; \
>>   		exit 1; }; \
>>   	else \
>> -	  echo '$(ME): skipping test $@: cppi not installed' 1>&2; \
>> +	  echo -e "$(ME): skipping test $@:\n"$(cppi_banner) 1>&2; \
>>   	fi
>>   
>>   # Enforce similar spec file indentation style, by running cppi on a
>> @@ -719,7 +724,7 @@ sc_spec_indentation:
>>   	    || { echo '$(ME): incorrect preprocessor indentation' 1>&2; \
>>   		exit 1; }; \
>>   	else \
>> -	  echo '$(ME): skipping test $@: cppi not installed' 1>&2; \
>> +	  echo -e "$(ME): skipping test $@:\n"$(cppi_banner) 1>&2; \
>>   	fi
>>   
>>   # Nested conditionals are easier to understand if we enforce that endifs
> 
> How about this instead of all of the above?

Okay, let me post a v3.

Michal

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.