* [PATCH] Makefile: Check for perl script errors with perl -c
@ 2010-04-17 2:29 Matthew Ogilvie
2010-04-17 7:27 ` Jeff King
0 siblings, 1 reply; 4+ messages in thread
From: Matthew Ogilvie @ 2010-04-17 2:29 UTC (permalink / raw)
To: git, gitster; +Cc: Matthew Ogilvie
This allows you to notice trivial syntax errors in perl scripts earlier,
for example before running t/* tests that generate a lot of
separate errors.
You have to set USE_PERL_CHECK to enable this, because it uses
the non-standard PIPESTATUS bashism to grep out "{script} syntax OK"
useless noise.
Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo•net>
---
I'm not sure anyone will think this is worth including, but I'm
used to "make" (and the compiler) detecting trivial errors
in compiled langauges, and was getting annoyed that it wasn't
doing something similar for perl scripts (especially since in git you
are really expected to "make" the scripts anyway).
The whole tradeoff between noise ("{script} syntax OK"), portability
(PIPESTATUS is a bashism), or really ugly contortions with redirecting
extra file descriptors (to avoid PIPESTATUS) seems to be the biggest
downside of the idea behind this patch.
--
Matthew Ogilvie [mmogilvi_git@miniinfo•net]
Makefile | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/Makefile b/Makefile
index 910f471..1e827bb 100644
--- a/Makefile
+++ b/Makefile
@@ -168,6 +168,10 @@ all::
#
# Define NO_PERL if you do not want Perl scripts or libraries at all.
#
+# Define USE_PERL_CHECK if you want the makefile to run "perl -cw" to
+# check perl scripts for basic errors. This requires that your
+# $SHELL_PATH supports the ${PIPESTATUS[0]} variable, like bash.
+#
# Define NO_PYTHON if you do not want Python scripts or libraries at all.
#
# Define NO_TCLTK if you do not want Tcl/Tk GUI.
@@ -1553,6 +1557,14 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
-e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
$@.perl >$@+ && \
chmod +x $@+ && \
+ if test x"$(USE_PERL_CHECK)" != x"" ; then \
+ '$(PERL_PATH_SQ)' -cw $@+ 2>&1 | grep -v '^$@+ syntax OK$$' 1>&2 ; \
+ perlStat="$${PIPESTATUS[0]}" && \
+ if test x"$$perlStat" != x"0" ; then \
+ echo '"$(PERL_PATH_SQ) -c $@+" failed' 1>&2 ; \
+ exit "$$perlStat" ; \
+ fi ; \
+ fi && \
mv $@+ $@
--
1.7.0.GIT
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] Makefile: Check for perl script errors with perl -c
2010-04-17 2:29 [PATCH] Makefile: Check for perl script errors with perl -c Matthew Ogilvie
@ 2010-04-17 7:27 ` Jeff King
2010-04-17 17:05 ` Matthew Ogilvie
0 siblings, 1 reply; 4+ messages in thread
From: Jeff King @ 2010-04-17 7:27 UTC (permalink / raw)
To: Matthew Ogilvie; +Cc: git, gitster
On Fri, Apr 16, 2010 at 08:29:40PM -0600, Matthew Ogilvie wrote:
> I'm not sure anyone will think this is worth including, but I'm
> used to "make" (and the compiler) detecting trivial errors
> in compiled langauges, and was getting annoyed that it wasn't
> doing something similar for perl scripts (especially since in git you
> are really expected to "make" the scripts anyway).
I usually do the same thing in my perl makefiles, so I would find it
useful.
> The whole tradeoff between noise ("{script} syntax OK"), portability
> (PIPESTATUS is a bashism), or really ugly contortions with redirecting
> extra file descriptors (to avoid PIPESTATUS) seems to be the biggest
> downside of the idea behind this patch.
Why do you need to run it through grep? Doesn't:
echo 'use strict; bogosity' >foo.pl
perl -wc foo.pl
properly set the exit code? I get:
$ perl -wc foo.pl
Bareword "bogosity" not allowed while "strict subs" in use at foo.pl line 1.
foo.pl had compilation errors.
$ echo $?
255
> @@ -1553,6 +1557,14 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
> -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
> $@.perl >$@+ && \
> chmod +x $@+ && \
> + if test x"$(USE_PERL_CHECK)" != x"" ; then \
> + '$(PERL_PATH_SQ)' -cw $@+ 2>&1 | grep -v '^$@+ syntax OK$$' 1>&2 ; \
> + perlStat="$${PIPESTATUS[0]}" && \
> + if test x"$$perlStat" != x"0" ; then \
> + echo '"$(PERL_PATH_SQ) -c $@+" failed' 1>&2 ; \
> + exit "$$perlStat" ; \
> + fi ; \
> + fi && \
> mv $@+ $@
So something like:
diff --git a/Makefile b/Makefile
index 87c90d6..d9b6613 100644
--- a/Makefile
+++ b/Makefile
@@ -1545,6 +1545,10 @@ $(SCRIPT_LIB) : % : %.sh
ifndef NO_PERL
$(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
+ifdef USE_PERL_CHECK
+PERL_CHECK = perl -wc $@+ &&
+endif
+
perl/perl.mak: GIT-CFLAGS perl/Makefile perl/Makefile.PL
$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
@@ -1562,6 +1566,7 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
-e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
$@.perl >$@+ && \
chmod +x $@+ && \
+ $(PERL_CHECK) \
mv $@+ $@
You could even just make it unconditional. I don't know that we have an
official policy, but we usually strive for strict, warnings-free perl.
-Peff
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] Makefile: Check for perl script errors with perl -c
2010-04-17 7:27 ` Jeff King
@ 2010-04-17 17:05 ` Matthew Ogilvie
2010-04-17 17:55 ` Jeff King
0 siblings, 1 reply; 4+ messages in thread
From: Matthew Ogilvie @ 2010-04-17 17:05 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster
On Sat, Apr 17, 2010 at 03:27:21AM -0400, Jeff King wrote:
> On Fri, Apr 16, 2010 at 08:29:40PM -0600, Matthew Ogilvie wrote:
> > The whole tradeoff between noise ("{script} syntax OK"), portability
> > (PIPESTATUS is a bashism), or really ugly contortions with redirecting
> > extra file descriptors (to avoid PIPESTATUS) seems to be the biggest
> > downside of the idea behind this patch.
>
> Why do you need to run it through grep? Doesn't:
>
> echo 'use strict; bogosity' >foo.pl
> perl -wc foo.pl
>
> properly set the exit code? I get:
>
> $ perl -wc foo.pl
> Bareword "bogosity" not allowed while "strict subs" in use at foo.pl line 1.
> foo.pl had compilation errors.
> $ echo $?
> 255
Yes, "perl -cw"'s exit code is always good, but the standard error is
needlessly noisy in the success case:
$ perl -cw -e 'print "hi\n"'
-e syntax OK
$ echo $?
0
Which then leaves a choice among not-great options:
1. Accept the noise output from make and perl. If we are willing to
accept this, then a simpler and/or uncoditional patch would be fine.
2. Filter out the "{scriptName} syntax OK" noise with grep (or sed),
but then $? is grep's status (not perl's), and you have to go
through contortions to properly test perl's status:
2a. Use PIPESTATUS, but this is a non-portable bashism.
My current version of the patch elects to do this, but
leaves the check disabled to (hopefully) avoid portability
issues. (A second advantage of leaving it disabled [or at
least disablable] is if someone is in a cross-compile
environment and the target perl path is different
from the build perl path.)
2b. Use a portable technique that involves echoing the status
redirected to file descriptor 3, then pulling the status out
of file descriptor 3 outside the pipeline. This is frankly
kind of complicated and hard to read.
So, I take it you would be happy with the noisy output?
Anyone else have an opinion? If someone knows a cleaner
way to resolve this, or if the group consensus is that we like the
patch's concept but would rather resolve the noisy output some
other way (perhaps just accept it), I could change the patch.
--
Matthew Ogilvie [mmogilvi_git@miniinfo•net]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Makefile: Check for perl script errors with perl -c
2010-04-17 17:05 ` Matthew Ogilvie
@ 2010-04-17 17:55 ` Jeff King
0 siblings, 0 replies; 4+ messages in thread
From: Jeff King @ 2010-04-17 17:55 UTC (permalink / raw)
To: Matthew Ogilvie; +Cc: git, gitster
On Sat, Apr 17, 2010 at 11:05:00AM -0600, Matthew Ogilvie wrote:
> Yes, "perl -cw"'s exit code is always good, but the standard error is
> needlessly noisy in the success case:
>
> $ perl -cw -e 'print "hi\n"'
> -e syntax OK
> $ echo $?
> 0
Ah, OK. I misunderstood what you were trying to do before.
> 1. Accept the noise output from make and perl. If we are willing to
> accept this, then a simpler and/or uncoditional patch would be fine.
Though I would prefer it silenced, I don't personally have a big problem
with this. I guess others might.
> 2. Filter out the "{scriptName} syntax OK" noise with grep (or sed),
> but then $? is grep's status (not perl's), and you have to go
> through contortions to properly test perl's status:
>
> 2a. Use PIPESTATUS, but this is a non-portable bashism.
> My current version of the patch elects to do this, but
> leaves the check disabled to (hopefully) avoid portability
> issues. (A second advantage of leaving it disabled [or at
> least disablable] is if someone is in a cross-compile
> environment and the target perl path is different
> from the build perl path.)
Hmm. The cross-compilation thing is interesting, but I'm not sure it
even works now. We already are relying on generating perl.mak and using
it as part of our build, I think. I haven't looked closely at the perl
build stuff in git, though, so maybe there is a way to make it work.
> 2b. Use a portable technique that involves echoing the status
> redirected to file descriptor 3, then pulling the status out
> of file descriptor 3 outside the pipeline. This is frankly
> kind of complicated and hard to read.
Yeah, I have used that technique before, and it is unreadable. Maybe
simpler is to cheat with a tempfile:
if ! perl -wc $@+ 2>$@.stderr; \
then cat >&2 $@.stderr; rm -f $@.stderr; exit 1; \
else rm -f $@.stderr; fi && \
but that is getting a bit unreadable, too. I dunno.
-Peff
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-04-17 17:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-04-17 2:29 [PATCH] Makefile: Check for perl script errors with perl -c Matthew Ogilvie
2010-04-17 7:27 ` Jeff King
2010-04-17 17:05 ` Matthew Ogilvie
2010-04-17 17:55 ` Jeff King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox