[libvirt] [PATCH] build: Require cppi for syntax-check
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
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
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/mappi... -- Andrea Bolognani / Red Hat / Virtualization
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/mappi...
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
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/mappi...
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
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
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
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 :|
participants (6)
-
Andrea Bolognani -
Daniel P. Berrangé -
Ján Tomko -
Martin Kletzander -
Michal Privoznik -
Pavel Hrdina