travis-ci: fail if Coccinelle found something to transform

PATCH In review SZEDER Gábor 3 participants 9 messages Jul 23, 2018 – Jul 26, 2018

lore

Share & tools

lore

SZEDER Gábor

author

2018-07-23 13:02 UTC #0 lore raw

cover travis-ci: fail if Coccinelle found something to transform

The first patch makes the static analysis build job on Travis CI faster by running it with 'make -j2'.

The second patch makes it more more useful by failing the build job if Coccinelle finds something to transform, thereby drawing our attention to undesired code patterns trying to enter the codebase.

With these patches applied, the static analysis build job would fail on current 'pu' because of two small issues on two branches:

- js/range-diff: Dscho has sent out v4 of this series over the weekend, which already incorporates Coccinelle's suggestion, so it's basically done.

- pb/bisect-helper-2: this topic has not seen an update in about 9 months, so I'll send a followup patch 3/2 to be applied on top or squashed in, whichever is deemed better.

SZEDER Gábor (2):
travis-ci: run Coccinelle static analysis with two parallel jobs
travis-ci: fail if Coccinelle static analysis found something to
transform

 ci/run-static-analysis.sh | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

Signature · 1 line

2.18.0.408.g42635c01bc

SZEDER Gábor

author

2018-07-23 13:02 UTC #1 lore raw

PATCH 1/2 travis-ci: run Coccinelle static analysis with two parallel jobs

Currently the static analysis build job runs Coccinelle using a single 'make' job. Using two parallel jobs cuts down the build job's run time from around 10-12mins to 6-7mins, sometimes even under 6mins (there is quite large variation between build job runtimes). More than two parallel jobs don't seem to bring further runtime benefits.

Signed-off-by SZEDER Gábor <[email protected]>

---
 ci/run-static-analysis.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

ci/run-static-analysis.sh +1 -1

index fe4ee4e06b..fa719c9ef9 100755
@@ -5,6 +5,6 @@
 
 . ${0%/*}/lib-travisci.sh
 
-make coccicheck
+make --jobs=2 coccicheck
 
 save_good_tree

Signature · 1 line

2.18.0.408.g42635c01bc

SZEDER Gábor

author

2018-07-23 13:03 UTC #2 lore raw

PATCH 2/2 travis-ci: fail if Coccinelle static analysis found something to transform

In reply to SZEDER Gábor

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 <[email protected]>

---
 ci/run-static-analysis.sh | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

ci/run-static-analysis.sh +19 -0

index fa719c9ef9..5688f261d0 100755
@@ -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

Signature · 1 line

2.18.0.408.g42635c01bc

Derrick Stolee

2018-07-23 13:23 UTC #3 lore raw

Re: [PATCH 2/2] travis-ci: fail if Coccinelle static analysis found something to transform

Quoted 50 lines · SZEDER Gábor

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 <[email protected]>
---
  ci/run-static-analysis.sh | 19 +++++++++++++++++++
  1 file changed, 19 insertions(+)
ci/run-static-analysis.sh +10 -0
index fa719c9ef9..5688f261d0 100755
@@ -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

ci/run-static-analysis.sh line 29

Quoted 10 lines

ci/run-static-analysis.sh +9 -0

+	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

SZEDER Gábor

author

2018-07-26 09:08 UTC #4 lore raw

Re: [PATCH 2/2] travis-ci: fail if Coccinelle static analysis found something to transform

In reply to SZEDER Gábor

Quoted 6 lines · SZEDER Gábor

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.

And this is how it looks like "in action":

https://travis-ci.org/git/git/jobs/408269979#L570

Junio C Hamano

2018-07-26 16:49 UTC #5 lore raw

Re: [PATCH 2/2] travis-ci: fail if Coccinelle static analysis found something to transform

Quoted 9 lines · SZEDER Gábor

Quoted 6 lines · SZEDER Gábor
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.

Quoted 3 lines

And this is how it looks like "in action":
https://travis-ci.org/git/git/jobs/408269979#L570

;-)

SZEDER Gábor

author

2018-07-23 13:06 UTC #6 lore raw

[PATCH 3/2 for pb/bisect-helper-2] squash! bisect--helper: `bisect_start` shell function partially in C

In reply to SZEDER Gábor

bisect--helper: use oid_to_hex()

Signed-off-by SZEDER Gábor <[email protected]>

---
 builtin/bisect--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/bisect--helper.c +1 -1

index fc02f889e6..eac4c27787 100644
@@ -547,7 +547,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 		if (!get_oid(head, &head_oid) &&
 		    !starts_with(head, "refs/heads/")) {
 			strbuf_reset(&start_head);
-			strbuf_addstr(&start_head, sha1_to_hex(head_oid.hash));
+			strbuf_addstr(&start_head, oid_to_hex(&head_oid));
 		} else if (!get_oid(head, &head_oid) &&
 			   skip_prefix(head, "refs/heads/", &head)) {
 			/*

Signature · 1 line

2.18.0.408.g42635c01bc

Derrick Stolee

2018-07-23 13:24 UTC #7 lore raw

Re: [PATCH 3/2 for pb/bisect-helper-2] squash! bisect--helper: `bisect_start` shell function partially in C

Quoted 12 lines · SZEDER Gábor

bisect--helper: use oid_to_hex()
Signed-off-by SZEDER Gábor <[email protected]>
---
  builtin/bisect--helper.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
builtin/bisect--helper.c +0 -0
index fc02f889e6..eac4c27787 100644

builtin/bisect--helper.c static int bisect_start(struct bisect_terms *terms, int no_checkout, · line 553

Quoted 8 lines

builtin/bisect--helper.c +1 -1

@@ -547,7 +547,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
  		if (!get_oid(head, &head_oid) &&
  		    !starts_with(head, "refs/heads/")) {
  			strbuf_reset(&start_head);
-			strbuf_addstr(&start_head, sha1_to_hex(head_oid.hash));
+			strbuf_addstr(&start_head, oid_to_hex(&head_oid));
  		} else if (!get_oid(head, &head_oid) &&
  			   skip_prefix(head, "refs/heads/", &head)) {
  			/*

This patch looks obviously correct. Thanks!

-Stolee

Derrick Stolee

Reviewed-by on the v1 series

2018-07-23 13:25 UTC #8 lore raw

In reply to SZEDER Gábor

Quoted 20 lines · SZEDER Gábor

The first patch makes the static analysis build job on Travis CI faster by running it with 'make -j2'.
The second patch makes it more more useful by failing the build job if Coccinelle finds something to transform, thereby drawing our attention to undesired code patterns trying to enter the codebase.
With these patches applied, the static analysis build job would fail on current 'pu' because of two small issues on two branches:
- js/range-diff: Dscho has sent out v4 of this series over the weekend, which already incorporates Coccinelle's suggestion, so it's basically done.
- pb/bisect-helper-2: this topic has not seen an update in about 9 months, so I'll send a followup patch 3/2 to be applied on top or squashed in, whichever is deemed better.

Quoted 7 lines

SZEDER Gábor (2):
travis-ci: run Coccinelle static analysis with two parallel jobs
travis-ci: fail if Coccinelle static analysis found something to
transform
  ci/run-static-analysis.sh | 21 ++++++++++++++++++++-
  1 file changed, 20 insertions(+), 1 deletion(-)

Thanks, Szeder.

I agree that we should use all possible automation as part of CI to catch things early. I like the direction here.

Reviewed-by Derrick Stolee <[email protected]>