On 6/17/19 6:40 PM, 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 :)
Okay, I'll push only the first hunk after I've cleaned the commit message a bit. Thanks, Michal