Re: [PATCH 1/1] ci: run `hdr-check` as part of the `Static Analysis` job

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Tue, Oct 01, 2019 at 04:16:26AM -0700, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>

Good idea, now that 'make hdr-check' runs clean this will help to keep
it that way.

I think adding it to the static analysis job makes sense, as opposed
to the additional VM startup, git clone, and dependency installation
overhead of a dedicated job.  However, this way we won't run 'make
hdr-check' if Coccinelle were to find something undesired, but I think
that's acceptable.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  azure-pipelines.yml        | 2 +-
>  ci/install-dependencies.sh | 3 ++-
>  ci/run-static-analysis.sh  | 3 +++
>  3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/azure-pipelines.yml b/azure-pipelines.yml
> index c329b7218b..15831f6006 100644
> --- a/azure-pipelines.yml
> +++ b/azure-pipelines.yml
> @@ -354,7 +354,7 @@ jobs:
>         test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1
>  
>         sudo apt-get update &&
> -       sudo apt-get install -y coccinelle &&
> +       sudo apt-get install -y coccinelle  coccinelle libcurl4-openssl-dev libssl-dev libexpat-dev gettext &&

These are the same additional packages that the hunk below adds to
'ci/install-dependencies.sh'.

So... why not just run 'ci/install-dependencies.sh' in the first place?

(On a related note, I noticed that the regular Linux and OSX jobs on
Azure Pipelines do run 'ci/install-dependencies.sh', but even in those
jobs 'azure-pipelines.yml' runs its own 'apt-get update && install'
anyway.)

>         export jobname=StaticAnalysis &&
>  
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 8cc72503cb..8ce9ce276e 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -49,7 +49,8 @@ osx-clang|osx-gcc)
>  	;;
>  StaticAnalysis)
>  	sudo apt-get -q update
> -	sudo apt-get -q -y install coccinelle
> +	sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \
> +		libexpat-dev gettext
>  	;;
>  Documentation)
>  	sudo apt-get -q update
> diff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh
> index a19aa7ebbc..65bcebda41 100755
> --- a/ci/run-static-analysis.sh
> +++ b/ci/run-static-analysis.sh
> @@ -26,4 +26,7 @@ then
>  	exit 1
>  fi
>  
> +make hdr-check ||
> +exit 1

This '|| exit 1' is unnecessary: our CI scripts are run with 'set -e',
so the non-zero exit code from a failing 'make hdr-check' alone would
fail the build.

(Yeah, there is an other 'exit 1' in the patch context above, but
that's a whole different ballpark and we do need that one.  Unlike
'make hdr-check', 'make coccicheck' (or even COccinelle itself for
that matter) doesn't exit with error when it found something to
transform, and it doesn't print those transformations either.
Consequently, we had to take a few extra steps to fail the build if it
found something, and that 'exit 1' is part of that.  More details in
0860a7641b (travis-ci: fail if Coccinelle static analysis found
something to transform, 2018-07-23).)


> +
>  save_good_tree
> -- 
> gitgitgadget