[libvirt] [PATCH] build: Require cppi for syntax-check

Michal Privoznik posted 1 patch 4 years, 9 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/e94a740431b2615470cbed763467f2a73ef02bfe.1560604642.git.mprivozn@redhat.com
cfg.mk | 32 ++++++++++++--------------------
1 file changed, 12 insertions(+), 20 deletions(-)
[libvirt] [PATCH] build: Require cppi for syntax-check
Posted by Michal Privoznik 4 years, 9 months ago
Since its introduction in v0.8.0~314 we allowed for cppi to be
not present for 'syntax-check' because the package did exist in
Fedora Core 12. Well, we don't care anymore ¯\_(ツ)_/¯
All recent distros have it. Moreover, it's fairly easy to miss
'cppi not installed' message in sea of syntax-check output (esp.
for newbies who probably benefit from it the most).

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 cfg.mk | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 5074ef611a..dd0a177fb3 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -697,30 +697,22 @@ sc_require_whitespace_in_translation:
 
 # Enforce recommended preprocessor indentation style.
 sc_preprocessor_indentation:
-	@if cppi --version >/dev/null 2>&1; then \
-	  $(VC_LIST_EXCEPT) | $(GREP) -E '\.[ch](\.in)?$$' | xargs cppi -a -c \
-	    || { echo '$(ME): incorrect preprocessor indentation' 1>&2; \
-		exit 1; }; \
-	else \
-	  echo '$(ME): skipping test $@: cppi not installed' 1>&2; \
-	fi
+	@$(VC_LIST_EXCEPT) | $(GREP) -E '\.[ch](\.in)?$$' | xargs cppi -a -c \
+	  || { echo '$(ME): incorrect preprocessor indentation' 1>&2; \
+	  exit 1; };
 
 # Enforce similar spec file indentation style, by running cppi on a
 # (comment-only) C file that mirrors the same layout as the spec file.
 sc_spec_indentation:
-	@if cppi --version >/dev/null 2>&1; then \
-	  for f in $$($(VC_LIST_EXCEPT) | $(GREP) '\.spec\.in$$'); do \
-	    $(SED) -e 's|#|// #|; s|%ifn*\(arch\)* |#if a // |' \
-		-e 's/%\(else\|endif\|define\)/#\1/' \
-		-e 's/^\( *\)\1\1\1#/#\1/' \
-		-e 's|^\( *[^#/ ]\)|// \1|; s|^\( */[^/]\)|// \1|' $$f \
-	    | cppi -a -c 2>&1 | $(SED) "s|standard input|$$f|"; \
-	  done | { if $(GREP) . >&2; then false; else :; fi; } \
-	    || { echo '$(ME): incorrect preprocessor indentation' 1>&2; \
-		exit 1; }; \
-	else \
-	  echo '$(ME): skipping test $@: cppi not installed' 1>&2; \
-	fi
+	@for f in $$($(VC_LIST_EXCEPT) | $(GREP) '\.spec\.in$$'); do \
+	  $(SED) -e 's|#|// #|; s|%ifn*\(arch\)* |#if a // |' \
+	  -e 's/%\(else\|endif\|define\)/#\1/' \
+	  -e 's/^\( *\)\1\1\1#/#\1/' \
+	  -e 's|^\( *[^#/ ]\)|// \1|; s|^\( */[^/]\)|// \1|' $$f \
+	  | cppi -a -c 2>&1 | $(SED) "s|standard input|$$f|"; \
+	done | { if $(GREP) . >&2; then false; else :; fi; } \
+	  || { echo '$(ME): incorrect preprocessor indentation' 1>&2; \
+	  exit 1; };
 
 # Nested conditionals are easier to understand if we enforce that endifs
 # can be paired back to the if
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: Require cppi for syntax-check
Posted by Ján Tomko 4 years, 9 months ago
On Sat, Jun 15, 2019 at 03:17:22PM +0200, Michal Privoznik wrote:
>Since its introduction in v0.8.0~314 we allowed for cppi to be
>not present for 'syntax-check' because the package did exist in
>Fedora Core 12. Well, we don't care anymore ¯\_(ツ)_/¯
>All recent distros have it. Moreover, it's fairly easy to miss
>'cppi not installed' message in sea of syntax-check output (esp.
>for newbies who probably benefit from it the most).
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> cfg.mk | 32 ++++++++++++--------------------
> 1 file changed, 12 insertions(+), 20 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: Require cppi for syntax-check
Posted by Andrea Bolognani 4 years, 9 months ago
On Mon, 2019-06-17 at 01:19 +0200, Ján Tomko wrote:
> On Sat, Jun 15, 2019 at 03:17:22PM +0200, Michal Privoznik wrote:
> > Since its introduction in v0.8.0~314 we allowed for cppi to be
> > not present for 'syntax-check' because the package did exist in
> > Fedora Core 12. Well, we don't care anymore ¯\_(ツ)_/¯
> > All recent distros have it. Moreover, it's fairly easy to miss
> > 'cppi not installed' message in sea of syntax-check output (esp.
> > for newbies who probably benefit from it the most).
> > 
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > ---
> > cfg.mk | 32 ++++++++++++--------------------
> > 1 file changed, 12 insertions(+), 20 deletions(-)
> > 
> 
> Reviewed-by: Ján Tomko <jtomko@redhat.com>

According to [1], only Fedora and FreeBSD have cppi packaged, so
merging this patch will make 'make syntax-check' suddenly fail on
all other target platforms.

I agree that the current situation is suboptimal, though. How about
we keep cppi optional, but print a more visible message about it not
being available after going through all syntax-check rules? That way
it'd be definitely more difficult to miss.


[1] https://libvirt.org/git/?p=libvirt-jenkins-ci.git;a=blob;f=guests/vars/mappings.yml;h=98c0cc90c086d4b6d20c757062d679614e692ba4;hb=HEAD#l117
-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: Require cppi for syntax-check
Posted by Michal Privoznik 4 years, 9 months ago
On 6/17/19 9:15 AM, Andrea Bolognani wrote:
> On Mon, 2019-06-17 at 01:19 +0200, Ján Tomko wrote:
>> On Sat, Jun 15, 2019 at 03:17:22PM +0200, Michal Privoznik wrote:
>>> Since its introduction in v0.8.0~314 we allowed for cppi to be
>>> not present for 'syntax-check' because the package did exist in
>>> Fedora Core 12. Well, we don't care anymore ¯\_(ツ)_/¯
>>> All recent distros have it. Moreover, it's fairly easy to miss
>>> 'cppi not installed' message in sea of syntax-check output (esp.
>>> for newbies who probably benefit from it the most).
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>> cfg.mk | 32 ++++++++++++--------------------
>>> 1 file changed, 12 insertions(+), 20 deletions(-)
>>>
>>
>> Reviewed-by: Ján Tomko <jtomko@redhat.com>
> 
> According to [1], only Fedora and FreeBSD have cppi packaged, so
> merging this patch will make 'make syntax-check' suddenly fail on
> all other target platforms.
> 
> I agree that the current situation is suboptimal, though. How about
> we keep cppi optional, but print a more visible message about it not
> being available after going through all syntax-check rules? That way
> it'd be definitely more difficult to miss.
> 
> 
> [1] https://libvirt.org/git/?p=libvirt-jenkins-ci.git;a=blob;f=guests/vars/mappings.yml;h=98c0cc90c086d4b6d20c757062d679614e692ba4;hb=HEAD#l117
> 

Oh, you're right. I did not realize that cppi is not on CentOS and some 
other funky distros. So I guess our only option is to make the error 
message more visible, e.g. some banner?

**********************
* cppi not installed *
**********************


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: Require cppi for syntax-check
Posted by Martin Kletzander 4 years, 9 months ago
On Mon, Jun 17, 2019 at 10:46:33AM +0200, Michal Privoznik wrote:
>On 6/17/19 9:15 AM, Andrea Bolognani wrote:
>> On Mon, 2019-06-17 at 01:19 +0200, Ján Tomko wrote:
>>> On Sat, Jun 15, 2019 at 03:17:22PM +0200, Michal Privoznik wrote:
>>>> Since its introduction in v0.8.0~314 we allowed for cppi to be
>>>> not present for 'syntax-check' because the package did exist in
>>>> Fedora Core 12. Well, we don't care anymore ¯\_(ツ)_/¯
>>>> All recent distros have it. Moreover, it's fairly easy to miss
>>>> 'cppi not installed' message in sea of syntax-check output (esp.
>>>> for newbies who probably benefit from it the most).
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>> cfg.mk | 32 ++++++++++++--------------------
>>>> 1 file changed, 12 insertions(+), 20 deletions(-)
>>>>
>>>
>>> Reviewed-by: Ján Tomko <jtomko@redhat.com>
>>
>> According to [1], only Fedora and FreeBSD have cppi packaged, so
>> merging this patch will make 'make syntax-check' suddenly fail on
>> all other target platforms.
>>
>> I agree that the current situation is suboptimal, though. How about
>> we keep cppi optional, but print a more visible message about it not
>> being available after going through all syntax-check rules? That way
>> it'd be definitely more difficult to miss.
>>
>>
>> [1] https://libvirt.org/git/?p=libvirt-jenkins-ci.git;a=blob;f=guests/vars/mappings.yml;h=98c0cc90c086d4b6d20c757062d679614e692ba4;hb=HEAD#l117
>>
>
>Oh, you're right. I did not realize that cppi is not on CentOS and some
>other funky distros. So I guess our only option is to make the error
>message more visible, e.g. some banner?
>
>**********************
>* cppi not installed *
>**********************
>

Yeah, you can also add it to BuildRequires and check for it during configure as
warning there would be more visible, I guess.  You can also require it only where you know it is available, but it might be too harsh.

Just my $.02

>
>Michal
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: Require cppi for syntax-check
Posted by Andrea Bolognani 4 years, 9 months ago
On Mon, 2019-06-17 at 11:37 +0200, Martin Kletzander wrote:
> On Mon, Jun 17, 2019 at 10:46:33AM +0200, Michal Privoznik wrote:
> > On 6/17/19 9:15 AM, Andrea Bolognani wrote:
> > > According to [1], only Fedora and FreeBSD have cppi packaged, so
> > > merging this patch will make 'make syntax-check' suddenly fail on
> > > all other target platforms.
> > > 
> > > I agree that the current situation is suboptimal, though. How about
> > > we keep cppi optional, but print a more visible message about it not
> > > being available after going through all syntax-check rules? That way
> > > it'd be definitely more difficult to miss.
> > 
> > Oh, you're right. I did not realize that cppi is not on CentOS and some
> > other funky distros. So I guess our only option is to make the error
> > message more visible, e.g. some banner?
> > 
> > **********************
> > * cppi not installed *
> > **********************

Maybe "cppi not installed, some checks have been skipped", but yeah,
that's pretty much exactly what I had in mind :)

> Yeah, you can also add it to BuildRequires and check for it during configure as
> warning there would be more visible, I guess.  You can also require it only where you know it is available, but it might be too harsh.

Since the spec file is only targeted at Fedora and RHEL/CentOS, we
can simply add

  %if 0%{?fedora}
  BuildRequires: cppi
  %endif

The configure time check wouldn't add a lot of value IMHO, since
there's basically no way you'll catch a warning among the deluge of
messages.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: Require cppi for syntax-check
Posted by Pavel Hrdina 4 years, 9 months ago
On Mon, Jun 17, 2019 at 11:50:01AM +0200, Andrea Bolognani wrote:
> On Mon, 2019-06-17 at 11:37 +0200, Martin Kletzander wrote:
> > On Mon, Jun 17, 2019 at 10:46:33AM +0200, Michal Privoznik wrote:
> > > On 6/17/19 9:15 AM, Andrea Bolognani wrote:
> > > > According to [1], only Fedora and FreeBSD have cppi packaged, so
> > > > merging this patch will make 'make syntax-check' suddenly fail on
> > > > all other target platforms.
> > > > 
> > > > I agree that the current situation is suboptimal, though. How about
> > > > we keep cppi optional, but print a more visible message about it not
> > > > being available after going through all syntax-check rules? That way
> > > > it'd be definitely more difficult to miss.
> > > 
> > > Oh, you're right. I did not realize that cppi is not on CentOS and some
> > > other funky distros. So I guess our only option is to make the error
> > > message more visible, e.g. some banner?
> > > 
> > > **********************
> > > * cppi not installed *
> > > **********************
> 
> Maybe "cppi not installed, some checks have been skipped", but yeah,
> that's pretty much exactly what I had in mind :)
> 
> > Yeah, you can also add it to BuildRequires and check for it during configure as
> > warning there would be more visible, I guess.  You can also require it only where you know it is available, but it might be too harsh.
> 
> Since the spec file is only targeted at Fedora and RHEL/CentOS, we
> can simply add
> 
>   %if 0%{?fedora}
>   BuildRequires: cppi
>   %endif

We don't actually run syntax-check when building RPMs so this should
not be in the spec file at all.

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: Require cppi for syntax-check
Posted by Daniel P. Berrangé 4 years, 9 months ago
On Mon, Jun 17, 2019 at 11:50:01AM +0200, Andrea Bolognani wrote:
> On Mon, 2019-06-17 at 11:37 +0200, Martin Kletzander wrote:
> > On Mon, Jun 17, 2019 at 10:46:33AM +0200, Michal Privoznik wrote:
> > > On 6/17/19 9:15 AM, Andrea Bolognani wrote:
> > > > According to [1], only Fedora and FreeBSD have cppi packaged, so
> > > > merging this patch will make 'make syntax-check' suddenly fail on
> > > > all other target platforms.
> > > > 
> > > > I agree that the current situation is suboptimal, though. How about
> > > > we keep cppi optional, but print a more visible message about it not
> > > > being available after going through all syntax-check rules? That way
> > > > it'd be definitely more difficult to miss.
> > > 
> > > Oh, you're right. I did not realize that cppi is not on CentOS and some
> > > other funky distros. So I guess our only option is to make the error
> > > message more visible, e.g. some banner?
> > > 
> > > **********************
> > > * cppi not installed *
> > > **********************
> 
> Maybe "cppi not installed, some checks have been skipped", but yeah,
> that's pretty much exactly what I had in mind :)
> 
> > Yeah, you can also add it to BuildRequires and check for it during configure as
> > warning there would be more visible, I guess.  You can also require it only where you know it is available, but it might be too harsh.
> 
> Since the spec file is only targeted at Fedora and RHEL/CentOS, we
> can simply add
> 
>   %if 0%{?fedora}
>   BuildRequires: cppi
>   %endif
> 
> The configure time check wouldn't add a lot of value IMHO, since
> there's basically no way you'll catch a warning among the deluge of
> messages.

The RPM spec addition is good, because it means when we tell people to
use "dnf builddep libvirt" to get pre-reqs, they'll get cppi easily.

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