Include hdr-check in the CI builds
cover Include hdr-check in the CI builds
Our hdr-check target is now functional in more ways than before. Let's run it as part of the CI builds.
I offered this idea in a review of dl/honor-cflags-in-hdr-check
[https://public-inbox.org/git/[email protected]/]
but it was not picked up. So I offer it as a stand-alone patch on top of that series.
Johannes Schindelin (1):
ci: run hdr-check as part of the Static Analysis job
azure-pipelines.yml | 2 +- ci/install-dependencies.sh | 3 ++- ci/run-static-analysis.sh | 3 +++ 3 files changed, 6 insertions(+), 2 deletions(-)
base-commit: b503a2d515e6ac3d2cced7881791c12663c45d01
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-368%2Fdscho%2Fhdr-check-in-ci-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-368/dscho/hdr-check-in-ci-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/368
Signature · 1 line
gitgitgadget
PATCH 1/1 ci: run `hdr-check` as part of the `Static Analysis` job
From: Johannes Schindelin <[email protected]>
--- azure-pipelines.yml | 2 +- ci/install-dependencies.sh | 3 ++- ci/run-static-analysis.sh | 3 +++ 3 files changed, 6 insertions(+), 2 deletions(-)
azure-pipelines.yml +1 -1
@@ -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 && export jobname=StaticAnalysis &&
ci/install-dependencies.sh +2 -1
@@ -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
ci/run-static-analysis.sh +3 -0
@@ -26,4 +26,7 @@ then exit 1 fi +make hdr-check || +exit 1 + save_good_tree
Signature · 1 line
gitgitgadget
Re: [PATCH 1/1] ci: run `hdr-check` as part of the `Static Analysis` job
Hi Johannes,
Sorry for not replying or rolling this in earlier, I was waiting for my series to hit master before doing this change but I guess it doesn't really hurt to do it any earlier.
Quoted 14 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>--- azure-pipelines.yml | 2 +- ci/install-dependencies.sh | 3 ++- ci/run-static-analysis.sh | 3 +++ 3 files changed, 6 insertions(+), 2 deletions(-)azure-pipelines.yml +0
-0
azure-pipelines.yml jobs: · line 357
Quoted 5 lines
azure-pipelines.yml +1
-1@@ -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 &&
Seems like you accidentally included coccinelle twice.
Anyway, thanks for picking up where I left off.
-Denton
Quoted 31 lines
azure-pipelines.yml +0
-0export jobname=StaticAnalysis &&ci/install-dependencies.sh +2
-1@@ -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 updateci/run-static-analysis.sh +3
-0@@ -26,4 +26,7 @@ then exit 1 fi +make hdr-check || +exit 1 + save_good_treeSignature · 1 line
gitgitgadget
Re: [PATCH 1/1] ci: run `hdr-check` as part of the `Static Analysis` job
Hi Denton,
Quoted 3 lines · Denton Liu
Sorry for not replying or rolling this in earlier, I was waiting for my series to hit master before doing this change but I guess it doesn't really hurt to do it any earlier.
No worries, thank you for working on this!
I wonder whether it would make sense to add a hdr-check to at least one macOS and on Windows job, too, so that we get some confidence in the non-Linux contributions. But that I _will_ leave to others ;-)
Quoted 17 lines · Johannes Schindelin via GitGitGadget
Quoted 16 lines
From: Johannes Schindelin <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>--- azure-pipelines.yml | 2 +- ci/install-dependencies.sh | 3 ++- ci/run-static-analysis.sh | 3 +++ 3 files changed, 6 insertions(+), 2 deletions(-)azure-pipelines.yml +0
-0@@ -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
Quoted 5 lines
azure-pipelines.yml jobs: · line 357
Quoted 3 lines
azure-pipelines.yml +1
-1sudo apt-get update && - sudo apt-get install -y coccinelle && + sudo apt-get install -y coccinelle coccinelle libcurl4-openssl-dev libssl-dev libexpat-dev gettext &&Seems like you accidentally included coccinelle twice.
Good point (copy-edit fail!). I force-pushed a fixed version, and will probably send out a new version tomorrow or Friday.
Thanks,
Dscho
Quoted 37 lines
Anyway, thanks for picking up where I left off.
-Denton
Quoted 31 lines
azure-pipelines.yml +0
-0export jobname=StaticAnalysis &&ci/install-dependencies.sh +2
-1@@ -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 updateci/run-static-analysis.sh +3
-1@@ -26,4 +26,7 @@ then exit 1 fi +make hdr-check || +exit 1 + save_good_tree --gitgitgadget
Re: [PATCH 1/1] ci: run `hdr-check` as part of the `Static Analysis` job
In reply to Johannes Schindelin via GitGitGadget"Johannes Schindelin via GitGitGadget" <[email protected]>
azure-pipelines.yml jobs: · line 357
Quoted 2 lines · writes:
azure-pipelines.yml +1
-1- sudo apt-get install -y coccinelle && + sudo apt-get install -y coccinelle coccinelle libcurl4-openssl-dev libssl-dev libexpat-dev gettext &&
I think "s/coccinelle //" is necessary here (assuming that "apt-get install" is the same Debian thing I know about).
Will do so locally, so no need to resend, but "Yeah, that was a typo" or "no stupid, what I wrote is right" would be a nice response to see, especially if I have to undo the local fix.
Thanks.
Quoted 26 lines
ci/install-dependencies.sh +2
-1@@ -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 updateci/run-static-analysis.sh +3
-0@@ -26,4 +26,7 @@ then exit 1 fi +make hdr-check || +exit 1 + save_good_tree
Re: [PATCH 1/1] ci: run `hdr-check` as part of the `Static Analysis` job
Hi Junio,
Quoted 6 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
azure-pipelines.yml jobs: · line 357
Quoted 2 lines · writes:
azure-pipelines.yml +1
-1- sudo apt-get install -y coccinelle && + sudo apt-get install -y coccinelle coccinelle libcurl4-openssl-dev libssl-dev libexpat-dev gettext &&
Quoted 6 lines
I think "s/coccinelle //" is necessary here (assuming that "apt-get install" is the same Debian thing I know about).
Will do so locally, so no need to resend, but "Yeah, that was a typo" or "no stupid, what I wrote is right" would be a nice response to see, especially if I have to undo the local fix.
Yes, this was a copy-edit fail on my part.
Thanks,
Dscho
Quoted 31 lines
Thanks.
Quoted 26 lines
ci/install-dependencies.sh +2
-1@@ -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 updateci/run-static-analysis.sh +3
-0@@ -26,4 +26,7 @@ then exit 1 fi +make hdr-check || +exit 1 + save_good_tree
Re: [PATCH 1/1] ci: run `hdr-check` as part of the `Static Analysis` job
In reply to Johannes Schindelin via GitGitGadgetQuoted 1 line · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.
Quoted 12 lines
Signed-off-by Johannes Schindelin <[email protected]>--- azure-pipelines.yml | 2 +- ci/install-dependencies.sh | 3 ++- ci/run-static-analysis.sh | 3 +++ 3 files changed, 6 insertions(+), 2 deletions(-)azure-pipelines.yml +0
-0
azure-pipelines.yml jobs: · line 357
Quoted 5 lines
azure-pipelines.yml +1
-1@@ -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.)
Quoted 21 lines
azure-pipelines.yml +0
-0export jobname=StaticAnalysis &&ci/install-dependencies.sh +2
-1@@ -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 updateci/run-static-analysis.sh +0
-0
ci/run-static-analysis.sh then · line 30
Quoted 5 lines
ci/run-static-analysis.sh +2
-0@@ -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).)
Quoted 4 lines
ci/run-static-analysis.sh +1
-0+ save_good_treeSignature · 1 line
gitgitgadget