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 :|
--
libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
