# [PATCH 0/2] travis-ci: fail if Coccinelle found something to transform 9 messages from 2018-07-23 to 2018-07-26. Participants: SZEDER Gábor, Derrick Stolee, Junio C Hamano. Thread: https://gitlist.dev/t/48939 ## SZEDER Gábor, 2018-07-23 13:02 Subject: [PATCH 0/2] travis-ci: fail if Coccinelle found something to transform Message-ID: <20180723130230.22491-1-szeder.dev@gmail.com> URL: https://gitlist.dev/e/20180723130230.22491-1-szeder.dev%40gmail.com ``` 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(-) -- 2.18.0.408.g42635c01bc ``` ## SZEDER Gábor, 2018-07-23 13:02 Subject: [PATCH 1/2] travis-ci: run Coccinelle static analysis with two parallel jobs Message-ID: <20180723130230.22491-2-szeder.dev@gmail.com> URL: https://gitlist.dev/e/20180723130230.22491-2-szeder.dev%40gmail.com In-Reply-To: <20180723130230.22491-1-szeder.dev@gmail.com> ``` 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 --- ci/run-static-analysis.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh index fe4ee4e06b..fa719c9ef9 100755 --- a/ci/run-static-analysis.sh +++ b/ci/run-static-analysis.sh @@ -5,6 +5,6 @@ . ${0%/*}/lib-travisci.sh -make coccicheck +make --jobs=2 coccicheck save_good_tree -- 2.18.0.408.g42635c01bc ``` ## SZEDER Gábor, 2018-07-23 13:02 Subject: [PATCH 2/2] travis-ci: fail if Coccinelle static analysis found something to transform Message-ID: <20180723130230.22491-3-szeder.dev@gmail.com> URL: https://gitlist.dev/e/20180723130230.22491-3-szeder.dev%40gmail.com In-Reply-To: <20180723130230.22491-1-szeder.dev@gmail.com> ``` 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/.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 --- 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 -- 2.18.0.408.g42635c01bc ``` ## SZEDER Gábor, 2018-07-23 13:06 Subject: [PATCH 3/2 for pb/bisect-helper-2] squash! bisect--helper: `bisect_start` shell function partially in C Message-ID: <20180723130603.22703-1-szeder.dev@gmail.com> URL: https://gitlist.dev/e/20180723130603.22703-1-szeder.dev%40gmail.com In-Reply-To: <20180723130230.22491-1-szeder.dev@gmail.com> ``` bisect--helper: use oid_to_hex() Signed-off-by: SZEDER Gábor --- builtin/bisect--helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index fc02f889e6..eac4c27787 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -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)) { /* -- 2.18.0.408.g42635c01bc ``` ## Derrick Stolee, 2018-07-23 13:23 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> URL: https://gitlist.dev/e/2266ffb0-b4b6-26bd-9ea8-76aaa325bcc6%40gmail.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/.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 > --- > 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 ``` ## Derrick Stolee, 2018-07-23 13:24 Subject: Re: [PATCH 3/2 for pb/bisect-helper-2] squash! bisect--helper: `bisect_start` shell function partially in C Message-ID: URL: https://gitlist.dev/e/bcecc802-099e-7e7f-d977-e6f545ff2fee%40gmail.com In-Reply-To: <20180723130603.22703-1-szeder.dev@gmail.com> ``` On 7/23/2018 9:06 AM, SZEDER Gábor wrote: > bisect--helper: use oid_to_hex() > > Signed-off-by: SZEDER Gábor > --- > builtin/bisect--helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index fc02f889e6..eac4c27787 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -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, 2018-07-23 13:25 Subject: Re: [PATCH 0/2] travis-ci: fail if Coccinelle found something to transform Message-ID: <583d2499-c4f2-47a1-48b0-6beb2d4d9d9f@gmail.com> URL: https://gitlist.dev/e/583d2499-c4f2-47a1-48b0-6beb2d4d9d9f%40gmail.com In-Reply-To: <20180723130230.22491-1-szeder.dev@gmail.com> ``` On 7/23/2018 9:02 AM, SZEDER Gábor wrote: > 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(-) 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 ``` ## SZEDER Gábor, 2018-07-26 09:08 Subject: Re: [PATCH 2/2] travis-ci: fail if Coccinelle static analysis found something to transform Message-ID: URL: https://gitlist.dev/e/CAM0VKjmqwByERhPZLqZfg6JNEEeMkKQU1XuD93aW%2B3yfc2gucA%40mail.gmail.com In-Reply-To: <20180723130230.22491-3-szeder.dev@gmail.com> ``` On Mon, Jul 23, 2018 at 3:02 PM SZEDER Gábor wrote: > 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 Subject: Re: [PATCH 2/2] travis-ci: fail if Coccinelle static analysis found something to transform Message-ID: URL: https://gitlist.dev/e/xmqqy3dyq6w4.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` SZEDER Gábor writes: > On Mon, Jul 23, 2018 at 3:02 PM SZEDER Gábor wrote: > >> 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 ;-) ```