{"thread":{"id":"23499","subject":"[PATCH] Makefile: Check for perl script errors with perl -c","startedAt":"2010-04-17T02:29:40Z","lastAt":"2010-04-17T17:55:53Z","messageCount":4,"participants":["Matthew Ogilvie","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"139711","messageId":"1271471380-17701-1-git-send-email-mmogilvi_git@miniinfo.net","threadId":"23499","inReplyTo":null,"subject":"[PATCH] Makefile: Check for perl script errors with perl -c","fromName":"Matthew Ogilvie","fromEmail":"mmogilvi_git@miniinfo.net","sentAt":"2010-04-17T02:29:40Z","receivedAt":"2010-04-17T02:29:40Z","isPatch":true,"sender":{"key":"mmogilvi_git@miniinfo.net","avatar":null},"body":"This allows you to notice trivial syntax errors in perl scripts earlier,\nfor example before running t/* tests that generate a lot of\nseparate errors.\n\nYou have to set USE_PERL_CHECK to enable this, because it uses\nthe non-standard PIPESTATUS bashism to grep out \"{script} syntax OK\"\nuseless noise.\n\nSigned-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>\n---\n\nI'm not sure anyone will think this is worth including, but I'm\nused to \"make\" (and the compiler) detecting trivial errors\nin compiled langauges, and was getting annoyed that it wasn't\ndoing something similar for perl scripts (especially since in git you\nare really expected to \"make\" the scripts anyway).\n\nThe whole tradeoff between noise (\"{script} syntax OK\"), portability\n(PIPESTATUS is a bashism), or really ugly contortions with redirecting\nextra file descriptors (to avoid PIPESTATUS) seems to be the biggest\ndownside of the idea behind this patch.\n\n--\nMatthew Ogilvie   [mmogilvi_git@miniinfo.net]\n\n Makefile |   12 ++++++++++++\n 1 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 910f471..1e827bb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -168,6 +168,10 @@ all::\n #\n # Define NO_PERL if you do not want Perl scripts or libraries at all.\n #\n+# Define USE_PERL_CHECK if you want the makefile to run \"perl -cw\" to\n+# check perl scripts for basic errors.  This requires that your\n+# $SHELL_PATH supports the ${PIPESTATUS[0]} variable, like bash.\n+#\n # Define NO_PYTHON if you do not want Python scripts or libraries at all.\n #\n # Define NO_TCLTK if you do not want Tcl/Tk GUI.\n@@ -1553,6 +1557,14 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl\n \t    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \\\n \t    $@.perl >$@+ && \\\n \tchmod +x $@+ && \\\n+\tif test x\"$(USE_PERL_CHECK)\" != x\"\" ; then \\\n+\t    '$(PERL_PATH_SQ)' -cw $@+ 2>&1 | grep -v '^$@+ syntax OK$$' 1>&2 ; \\\n+\t    perlStat=\"$${PIPESTATUS[0]}\" && \\\n+\t    if test x\"$$perlStat\" != x\"0\" ; then \\\n+\t        echo '\"$(PERL_PATH_SQ) -c $@+\" failed' 1>&2 ; \\\n+\t        exit \"$$perlStat\" ; \\\n+\t    fi ; \\\n+\tfi && \\\n \tmv $@+ $@\n \n \n-- \n1.7.0.GIT\n"},{"id":"139722","messageId":"20100417072721.GD10365@coredump.intra.peff.net","threadId":"23499","inReplyTo":"1271471380-17701-1-git-send-email-mmogilvi_git@miniinfo.net","subject":"Re: [PATCH] Makefile: Check for perl script errors with perl -c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-17T07:27:21Z","receivedAt":"2010-04-17T07:27:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 16, 2010 at 08:29:40PM -0600, Matthew Ogilvie wrote:\n\n> I'm not sure anyone will think this is worth including, but I'm\n> used to \"make\" (and the compiler) detecting trivial errors\n> in compiled langauges, and was getting annoyed that it wasn't\n> doing something similar for perl scripts (especially since in git you\n> are really expected to \"make\" the scripts anyway).\n\nI usually do the same thing in my perl makefiles, so I would find it\nuseful.\n\n> The whole tradeoff between noise (\"{script} syntax OK\"), portability\n> (PIPESTATUS is a bashism), or really ugly contortions with redirecting\n> extra file descriptors (to avoid PIPESTATUS) seems to be the biggest\n> downside of the idea behind this patch.\n\nWhy do you need to run it through grep? Doesn't:\n\n  echo 'use strict; bogosity' >foo.pl\n  perl -wc foo.pl\n\nproperly set the exit code? I get:\n\n  $ perl -wc foo.pl\n  Bareword \"bogosity\" not allowed while \"strict subs\" in use at foo.pl line 1.\n  foo.pl had compilation errors.\n  $ echo $?\n  255\n\n> @@ -1553,6 +1557,14 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl\n>  \t    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \\\n>  \t    $@.perl >$@+ && \\\n>  \tchmod +x $@+ && \\\n> +\tif test x\"$(USE_PERL_CHECK)\" != x\"\" ; then \\\n> +\t    '$(PERL_PATH_SQ)' -cw $@+ 2>&1 | grep -v '^$@+ syntax OK$$' 1>&2 ; \\\n> +\t    perlStat=\"$${PIPESTATUS[0]}\" && \\\n> +\t    if test x\"$$perlStat\" != x\"0\" ; then \\\n> +\t        echo '\"$(PERL_PATH_SQ) -c $@+\" failed' 1>&2 ; \\\n> +\t        exit \"$$perlStat\" ; \\\n> +\t    fi ; \\\n> +\tfi && \\\n>  \tmv $@+ $@\n\nSo something like:\n\ndiff --git a/Makefile b/Makefile\nindex 87c90d6..d9b6613 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1545,6 +1545,10 @@ $(SCRIPT_LIB) : % : %.sh\n ifndef NO_PERL\n $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak\n \n+ifdef USE_PERL_CHECK\n+PERL_CHECK = perl -wc $@+ &&\n+endif\n+\n perl/perl.mak: GIT-CFLAGS perl/Makefile perl/Makefile.PL\n \t$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)\n \n@@ -1562,6 +1566,7 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl\n \t    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \\\n \t    $@.perl >$@+ && \\\n \tchmod +x $@+ && \\\n+\t$(PERL_CHECK) \\\n \tmv $@+ $@\n \n \n\nYou could even just make it unconditional. I don't know that we have an\nofficial policy, but we usually strive for strict, warnings-free perl.\n\n-Peff\n"},{"id":"139760","messageId":"20100417170500.GA4587@comcast.net","threadId":"23499","inReplyTo":"20100417072721.GD10365@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: Check for perl script errors with perl -c","fromName":"Matthew Ogilvie","fromEmail":"mmogilvi_git@miniinfo.net","sentAt":"2010-04-17T17:05:00Z","receivedAt":"2010-04-17T17:05:00Z","isPatch":true,"sender":{"key":"mmogilvi_git@miniinfo.net","avatar":null},"body":"On Sat, Apr 17, 2010 at 03:27:21AM -0400, Jeff King wrote:\n> On Fri, Apr 16, 2010 at 08:29:40PM -0600, Matthew Ogilvie wrote:\n> > The whole tradeoff between noise (\"{script} syntax OK\"), portability\n> > (PIPESTATUS is a bashism), or really ugly contortions with redirecting\n> > extra file descriptors (to avoid PIPESTATUS) seems to be the biggest\n> > downside of the idea behind this patch.\n> \n> Why do you need to run it through grep? Doesn't:\n> \n>   echo 'use strict; bogosity' >foo.pl\n>   perl -wc foo.pl\n> \n> properly set the exit code? I get:\n> \n>   $ perl -wc foo.pl\n>   Bareword \"bogosity\" not allowed while \"strict subs\" in use at foo.pl line 1.\n>   foo.pl had compilation errors.\n>   $ echo $?\n>   255\n\nYes, \"perl -cw\"'s exit code is always good, but the standard error is\nneedlessly noisy in the success case:\n\n  $ perl -cw -e 'print \"hi\\n\"'\n  -e syntax OK\n  $ echo $?\n  0\n\nWhich then leaves a choice among not-great options:\n\n1. Accept the noise output from make and perl.  If we are willing to\n   accept this, then a simpler and/or uncoditional patch would be fine.\n\n2. Filter out the \"{scriptName} syntax OK\" noise with grep (or sed),\n   but then $? is grep's status (not perl's), and you have to go\n   through contortions to properly test perl's status:\n\n    2a. Use PIPESTATUS, but this is a non-portable bashism.\n        My current version of the patch elects to do this, but\n        leaves the check disabled to (hopefully) avoid portability\n        issues.  (A second advantage of leaving it disabled [or at\n        least disablable] is if someone is in a cross-compile\n        environment and the target perl path is different \n        from the build perl path.)\n\n    2b. Use a portable technique that involves echoing the status\n        redirected to file descriptor 3, then pulling the status out\n        of file descriptor 3 outside the pipeline.  This is frankly\n        kind of complicated and hard to read.\n\nSo, I take it you would be happy with the noisy output?\nAnyone else have an opinion?  If someone knows a cleaner\nway to resolve this, or if the group consensus is that we like the\npatch's concept but would rather resolve the noisy output some\nother way (perhaps just accept it), I could change the patch.\n\n--\nMatthew Ogilvie   [mmogilvi_git@miniinfo.net]\n"},{"id":"139765","messageId":"20100417175553.GC23642@coredump.intra.peff.net","threadId":"23499","inReplyTo":"20100417170500.GA4587@comcast.net","subject":"Re: [PATCH] Makefile: Check for perl script errors with perl -c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-17T17:55:53Z","receivedAt":"2010-04-17T17:55:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 17, 2010 at 11:05:00AM -0600, Matthew Ogilvie wrote:\n\n> Yes, \"perl -cw\"'s exit code is always good, but the standard error is\n> needlessly noisy in the success case:\n> \n>   $ perl -cw -e 'print \"hi\\n\"'\n>   -e syntax OK\n>   $ echo $?\n>   0\n\nAh, OK. I misunderstood what you were trying to do before.\n\n> 1. Accept the noise output from make and perl.  If we are willing to\n>    accept this, then a simpler and/or uncoditional patch would be fine.\n\nThough I would prefer it silenced, I don't personally have a big problem\nwith this. I guess others might.\n\n> 2. Filter out the \"{scriptName} syntax OK\" noise with grep (or sed),\n>    but then $? is grep's status (not perl's), and you have to go\n>    through contortions to properly test perl's status:\n> \n>     2a. Use PIPESTATUS, but this is a non-portable bashism.\n>         My current version of the patch elects to do this, but\n>         leaves the check disabled to (hopefully) avoid portability\n>         issues.  (A second advantage of leaving it disabled [or at\n>         least disablable] is if someone is in a cross-compile\n>         environment and the target perl path is different \n>         from the build perl path.)\n\nHmm. The cross-compilation thing is interesting, but I'm not sure it\neven works now. We already are relying on generating perl.mak and using\nit as part of our build, I think. I haven't looked closely at the perl\nbuild stuff in git, though, so maybe there is a way to make it work.\n\n>     2b. Use a portable technique that involves echoing the status\n>         redirected to file descriptor 3, then pulling the status out\n>         of file descriptor 3 outside the pipeline.  This is frankly\n>         kind of complicated and hard to read.\n\nYeah, I have used that technique before, and it is unreadable. Maybe\nsimpler is to cheat with a tempfile:\n\n  if ! perl -wc $@+ 2>$@.stderr; \\\n    then cat >&2 $@.stderr; rm -f $@.stderr; exit 1; \\\n    else rm -f $@.stderr; fi && \\\n\nbut that is getting a bit unreadable, too. I dunno.\n\n-Peff\n"}]}