Re: [PATCH] Makefile: Check for perl script errors with perl -c

From: Jeff King <peff@peff.net>
Date: Apr 17, 2010, 07:27 UTC
Message-ID: <20100417072721.GD10365@coredump.intra.peff.net>
In-Reply-To: <1271471380-17701-1-git-send-email-mmogilvi_git@miniinfo.net>

On Fri, Apr 16, 2010 at 08:29:40PM -0600, Matthew Ogilvie wrote:

Show 5 quoted lines

> 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

Show 13 quoted lines

> @@ -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