* [PATCH] Makefile: detect errors in running spatch
@ 2017-03-10 8:31 Jeff King
2017-03-10 17:03 ` René Scharfe
2017-03-10 20:16 ` Junio C Hamano
0 siblings, 2 replies; 5+ messages in thread
From: Jeff King @ 2017-03-10 8:31 UTC (permalink / raw)
To: René Scharfe; +Cc: git
The "make coccicheck" target runs spatch against each source
file. But it does so in a for loop, so "make" never sees the
exit code of spatch. Worse, it redirects stderr to a log
file, so the user has no indication of any failure. And then
to top it all off, because we touched the patch file's
mtime, make will refuse to repeat the command because it
think the target is up-to-date.
So for example:
$ make coccicheck SPATCH=does-not-exist
SPATCH contrib/coccinelle/free.cocci
SPATCH contrib/coccinelle/qsort.cocci
SPATCH contrib/coccinelle/xstrdup_or_null.cocci
SPATCH contrib/coccinelle/swap.cocci
SPATCH contrib/coccinelle/strbuf.cocci
SPATCH contrib/coccinelle/object_id.cocci
SPATCH contrib/coccinelle/array.cocci
$ make coccicheck SPATCH=does-not-exist
make: Nothing to be done for 'coccicheck'.
With this patch, you get:
$ make coccicheck SPATCH=does-not-exist
SPATCH contrib/coccinelle/free.cocci
/bin/sh: 4: does-not-exist: not found
Makefile:2338: recipe for target 'contrib/coccinelle/free.cocci.patch' failed
make: *** [contrib/coccinelle/free.cocci.patch] Error 1
It also dumps the log on failure, so any errors from spatch
itself (like syntax errors in our .cocci files) will be seen
by the user.
Signed-off-by: Jeff King <peff@peff.net>
---
This shell code is getting a bit unwieldy to stick inside the Makefile,
with all the line continuation and $-escaping. It might be worth moving
it into a helper script.
It also doesn't help that shells are awkward at passing status out of a
for-loop. I think the most "make-ish" way of doing this would actually
be to lose the for loop and have a per-cocci-per-source target.
I don't know if that would make the patches harder to apply. The results
aren't full patches, so I assume you usually do some kind of munging on
them? I resorted to:
make coccicheck SPATCH='spatch --in-place'
Makefile | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index 9ec6065cc..d97633892 100644
--- a/Makefile
+++ b/Makefile
@@ -2336,9 +2336,17 @@ check: common-cmds.h
C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))
%.cocci.patch: %.cocci $(C_SOURCES)
@echo ' ' SPATCH $<; \
+ ret=0; \
for f in $(C_SOURCES); do \
- $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS); \
- done >$@ 2>$@.log; \
+ $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
+ { ret=$$?; break; }; \
+ done >$@+ 2>$@.log; \
+ if test $$ret != 0; \
+ then \
+ cat $@.log; \
+ exit 1; \
+ fi; \
+ mv $@+ $@; \
if test -s $@; \
then \
echo ' ' SPATCH result: $@; \
--
2.12.0.450.gd7e60cc16
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Makefile: detect errors in running spatch
2017-03-10 8:31 [PATCH] Makefile: detect errors in running spatch Jeff King
@ 2017-03-10 17:03 ` René Scharfe
2017-03-10 18:12 ` Jeff King
2017-03-10 20:16 ` Junio C Hamano
1 sibling, 1 reply; 5+ messages in thread
From: René Scharfe @ 2017-03-10 17:03 UTC (permalink / raw)
To: Jeff King; +Cc: git
Am 10.03.2017 um 09:31 schrieb Jeff King:
> The "make coccicheck" target runs spatch against each source
> file. But it does so in a for loop, so "make" never sees the
> exit code of spatch. Worse, it redirects stderr to a log
> file, so the user has no indication of any failure. And then
> to top it all off, because we touched the patch file's
> mtime, make will refuse to repeat the command because it
> think the target is up-to-date.
>
> So for example:
>
> $ make coccicheck SPATCH=does-not-exist
> SPATCH contrib/coccinelle/free.cocci
> SPATCH contrib/coccinelle/qsort.cocci
> SPATCH contrib/coccinelle/xstrdup_or_null.cocci
> SPATCH contrib/coccinelle/swap.cocci
> SPATCH contrib/coccinelle/strbuf.cocci
> SPATCH contrib/coccinelle/object_id.cocci
> SPATCH contrib/coccinelle/array.cocci
> $ make coccicheck SPATCH=does-not-exist
> make: Nothing to be done for 'coccicheck'.
>
> With this patch, you get:
>
> $ make coccicheck SPATCH=does-not-exist
> SPATCH contrib/coccinelle/free.cocci
> /bin/sh: 4: does-not-exist: not found
> Makefile:2338: recipe for target 'contrib/coccinelle/free.cocci.patch' failed
> make: *** [contrib/coccinelle/free.cocci.patch] Error 1
That's nice. The current version is just a contraption that does the
bare minimum of work.
> It also dumps the log on failure, so any errors from spatch
> itself (like syntax errors in our .cocci files) will be seen
> by the user.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This shell code is getting a bit unwieldy to stick inside the Makefile,
> with all the line continuation and $-escaping. It might be worth moving
> it into a helper script.
There is one for the kernel
(https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/coccicheck).
It's quite big, though.
> It also doesn't help that shells are awkward at passing status out of a
> for-loop. I think the most "make-ish" way of doing this would actually
> be to lose the for loop and have a per-cocci-per-source target.
>
> I don't know if that would make the patches harder to apply. The results
> aren't full patches, so I assume you usually do some kind of munging on
> them?
They work with patch -p0.
We can get rid of the loop by using the spatch options --use-gitgrep and
--dir. I can't find the former one in the docs, though, so I'm not sure
if it only works with certain versions or what exactly it is even doing.
It seems to have the side effect of producing git-style patches
(applicable with patch -p1) at least.
> I resorted to:
>
> make coccicheck SPATCH='spatch --in-place'
Using SPATCH_FLAGS for adding an option in such case would be a bit simpler.
> Makefile | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 9ec6065cc..d97633892 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2336,9 +2336,17 @@ check: common-cmds.h
> C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))
> %.cocci.patch: %.cocci $(C_SOURCES)
> @echo ' ' SPATCH $<; \
> + ret=0; \
> for f in $(C_SOURCES); do \
> - $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS); \
> - done >$@ 2>$@.log; \
> + $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
> + { ret=$$?; break; }; \
> + done >$@+ 2>$@.log; \
> + if test $$ret != 0; \
> + then \
> + cat $@.log; \
> + exit 1; \
> + fi; \
> + mv $@+ $@; \
> if test -s $@; \
> then \
> echo ' ' SPATCH result: $@; \
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Makefile: detect errors in running spatch
2017-03-10 17:03 ` René Scharfe
@ 2017-03-10 18:12 ` Jeff King
0 siblings, 0 replies; 5+ messages in thread
From: Jeff King @ 2017-03-10 18:12 UTC (permalink / raw)
To: René Scharfe; +Cc: git
On Fri, Mar 10, 2017 at 06:03:47PM +0100, René Scharfe wrote:
> > This shell code is getting a bit unwieldy to stick inside the Makefile,
> > with all the line continuation and $-escaping. It might be worth moving
> > it into a helper script.
>
> There is one for the kernel (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/coccicheck).
> It's quite big, though.
Yeah, there's a lot going on there that I don't think we care about
(though I am new to coccinelle, so maybe I would grow to appreciate the
features).
I was thinking of just moving the current Makefile snippet into a
script. That lets us avoid the irritating quoting. And we can use things
like functions, which would make the $?-handling in the loop less
tedious (because we can return straight out of the loop).
> > I don't know if that would make the patches harder to apply. The results
> > aren't full patches, so I assume you usually do some kind of munging on
> > them?
>
> They work with patch -p0.
Hrm, you're right. I tried it earlier based on the commit message from
the original "coccicheck" commit, but I got "only garbage was found".
But now it works. I must have screwed it up (perhaps tab-completion
stopped at "copy.cocci" instead of "copy.cocci.patch").
> > make coccicheck SPATCH='spatch --in-place'
>
> Using SPATCH_FLAGS for adding an option in such case would be a bit simpler.
That too. :)
> We can get rid of the loop by using the spatch options --use-gitgrep and
> --dir. I can't find the former one in the docs, though, so I'm not sure if
> it only works with certain versions or what exactly it is even doing. It
> seems to have the side effect of producing git-style patches (applicable
> with patch -p1) at least.
I have no objections to pursuing that. But I think with my patch, it's
workable for now, so there's no rush.
-Peff
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Makefile: detect errors in running spatch
2017-03-10 8:31 [PATCH] Makefile: detect errors in running spatch Jeff King
2017-03-10 17:03 ` René Scharfe
@ 2017-03-10 20:16 ` Junio C Hamano
1 sibling, 0 replies; 5+ messages in thread
From: Junio C Hamano @ 2017-03-10 20:16 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, git
Jeff King <peff@peff.net> writes:
> It also doesn't help that shells are awkward at passing status out of a
> for-loop. I think the most "make-ish" way of doing this would actually
> be to lose the for loop and have a per-cocci-per-source target.
As we assume we can freely use GNUmake facilities, another option,
(i.e. the most "gnumake-ish" way) may be to have it unroll the loop
with $(foreach,...) so that the shell just sees a series of
commands.
> I don't know if that would make the patches harder to apply. The results
> aren't full patches, so I assume you usually do some kind of munging on
> them? I resorted to:
>
> make coccicheck SPATCH='spatch --in-place'
>
> Makefile | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 9ec6065cc..d97633892 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2336,9 +2336,17 @@ check: common-cmds.h
> C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))
> %.cocci.patch: %.cocci $(C_SOURCES)
> @echo ' ' SPATCH $<; \
> + ret=0; \
> for f in $(C_SOURCES); do \
> - $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS); \
> - done >$@ 2>$@.log; \
> + $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
> + { ret=$$?; break; }; \
> + done >$@+ 2>$@.log; \
> + if test $$ret != 0; \
> + then \
> + cat $@.log; \
> + exit 1; \
> + fi; \
> + mv $@+ $@; \
> if test -s $@; \
> then \
> echo ' ' SPATCH result: $@; \
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] Makefile: detect errors in running spatch
2017-03-28 19:42 [PATCH 0/18] snprintf cleanups Jeff King
@ 2017-03-29 7:10 ` Jeff King
0 siblings, 0 replies; 5+ messages in thread
From: Jeff King @ 2017-03-29 7:10 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Junio C Hamano
The "make coccicheck" target runs spatch against each source
file. But it does so in a for loop, so "make" never sees the
exit code of spatch. Worse, it redirects stderr to a log
file, so the user has no indication of any failure. And then
to top it all off, because we touched the patch file's
mtime, make will refuse to repeat the command because it
think the target is up-to-date.
So for example:
$ make coccicheck SPATCH=does-not-exist
SPATCH contrib/coccinelle/free.cocci
SPATCH contrib/coccinelle/qsort.cocci
SPATCH contrib/coccinelle/xstrdup_or_null.cocci
SPATCH contrib/coccinelle/swap.cocci
SPATCH contrib/coccinelle/strbuf.cocci
SPATCH contrib/coccinelle/object_id.cocci
SPATCH contrib/coccinelle/array.cocci
$ make coccicheck SPATCH=does-not-exist
make: Nothing to be done for 'coccicheck'.
With this patch, you get:
$ make coccicheck SPATCH=does-not-exist
SPATCH contrib/coccinelle/free.cocci
/bin/sh: 4: does-not-exist: not found
Makefile:2338: recipe for target 'contrib/coccinelle/free.cocci.patch' failed
make: *** [contrib/coccinelle/free.cocci.patch] Error 1
It also dumps the log on failure, so any errors from spatch
itself (like syntax errors in our .cocci files) will be seen
by the user.
Signed-off-by: Jeff King <peff@peff.net>
---
This is a verbatim repost of:
http://public-inbox.org/git/20170310083117.cbflqx7zbe4s7cqv@sigill.intra.peff.net/
I think this is a strict improvement over the status quo. The
conversation in that thread turned to possible refactorings, but since
those didn't materialize, I think we should apply this in the meantime.
Makefile | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index 9f8b35ad4..9b36068ac 100644
--- a/Makefile
+++ b/Makefile
@@ -2348,9 +2348,17 @@ check: common-cmds.h
C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))
%.cocci.patch: %.cocci $(C_SOURCES)
@echo ' ' SPATCH $<; \
+ ret=0; \
for f in $(C_SOURCES); do \
- $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS); \
- done >$@ 2>$@.log; \
+ $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
+ { ret=$$?; break; }; \
+ done >$@+ 2>$@.log; \
+ if test $$ret != 0; \
+ then \
+ cat $@.log; \
+ exit 1; \
+ fi; \
+ mv $@+ $@; \
if test -s $@; \
then \
echo ' ' SPATCH result: $@; \
--
2.12.2.920.gc31091ce4
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-29 7:11 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-10 8:31 [PATCH] Makefile: detect errors in running spatch Jeff King
2017-03-10 17:03 ` René Scharfe
2017-03-10 18:12 ` Jeff King
2017-03-10 20:16 ` Junio C Hamano
-- strict thread matches above, loose matches on Subject: below --
2017-03-28 19:42 [PATCH 0/18] snprintf cleanups Jeff King
2017-03-29 7:10 ` [PATCH] Makefile: detect errors in running spatch Jeff King
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).