From: Derrick Stolee <stolee@gmail.com>
Date: Mon, 23 Jul 2018 13:23:34 GMT
Subject: Re: [PATCH 2/2] travis-ci: fail if Coccinelle static analysis found something to transform
Message-ID: <2266ffb0-b4b6-26bd-9ea8-76aaa325bcc6@gmail.com>
In-Reply-To: <20180723130230.22491-3-szeder.dev@gmail.com>

On 7/23/2018 9:02 AM, SZEDER Gábor wrote:
> Coccinelle's and in turn 'make coccicheck's exit code only indicates
> that Coccinelle managed to finish its analysis without any errors
> (e.g. no unknown --options, no missing files, no syntax errors in the
> semantic patches, etc.), but it doesn't indicate whether it found any
> undesired code patterns to transform or not.  To find out the latter,
> one has to look closer at 'make coccicheck's standard output and look
> for lines like:
>
>    SPATCH result: contrib/coccinelle/<something>.cocci.patch
>
> And this only indicates that there is something to transform, but to
> see what the suggested transformations are one has to actually look
> into those '*.cocci.patch' files.
>
> This makes the automated static analysis build job on Travis CI not
> particularly useful, because it neither draws our attention to
> Coccinelle's findings, nor shows the actual findings.  Consequently,
> new topics introducing undesired code patterns graduated to master
> on several occasions without anyone noticing.
>
> The only way to draw attention in such an automated setting is to fail
> the build job.  Therefore, modify the 'ci/run-static-analysis.sh'
> build script to check all the resulting '*.cocci.patch' files, and
> fail the build job if any of them turns out to be not empty.  Include
> those files' contents, i.e. Coccinelle's suggested transformations, in
> the build job's trace log, so we'll know why it failed.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>   ci/run-static-analysis.sh | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
>
> diff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh
> index fa719c9ef9..5688f261d0 100755
> --- a/ci/run-static-analysis.sh
> +++ b/ci/run-static-analysis.sh
> @@ -7,4 +7,23 @@
>   
>   make --jobs=2 coccicheck
>   
> +set +x
> +
> +fail=
> +for cocci_patch in contrib/coccinelle/*.patch
> +do
> +	if test -s "$cocci_patch"
> +	then
> +		echo "$(tput setaf 1)Coccinelle suggests the following changes in '$cocci_patch':$(tput sgr0)"
> +		cat "$cocci_patch"
> +		fail=UnfortunatelyYes
> +	fi
> +done
> +
> +if test -n "$fail"
> +then
> +	echo "$(tput setaf 1)error: Coccinelle suggested some changes$(tput sgr0)"
> +	exit 1
> +fi
> +
>   save_good_tree

This looks like the right way to report a failure. It provides as much 
information as possible. I agree that we should have this as part of CI 
so we find issues more quickly.

Thanks!

-Stolee