cfg.mk | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-)
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
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
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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.