From: Matthew Ogilvie <mmogilvi_git@miniinfo•net>
To: Jeff King <peff@peff•net>
Cc: git@vger•kernel.org, gitster@pobox•com
Subject: Re: [PATCH] Makefile: Check for perl script errors with perl -c
Date: Sat, 17 Apr 2010 11:05:00 -0600 [thread overview]
Message-ID: <20100417170500.GA4587@comcast.net> (raw)
In-Reply-To: <20100417072721.GD10365@coredump.intra.peff.net>
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]
next prev parent reply other threads:[~2010-04-17 17:05 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2010-04-17 17:55 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100417170500.GA4587@comcast.net \
--to=mmogilvi_git@miniinfo$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox