What about Perl autodie?
I came across the Perl autodie pragma
(https://perldoc.perl.org/autodie). This seems pretty useful; is this
something we can use? Any drawbacks? Any minimum Perl version?
Attached is a sample patch of the kind of thing I'd be interested in.
The existing error handling of file operations in Perl is pretty
cumbersome, and this would simplify that.
Btw., here is a sample error message from autodie:
Can't open '../src/include/mb/pg_wchar.h' for reading: 'No such file or
directory' at ../src/include/catalog/../../backend/catalog/genbki.pl
line 391
which seems as good or better than the stuff we produce manually.
Attachments:
0001-WIP-Make-some-use-of-Perl-autodie-pragma.patchtext/plain; charset=UTF-8; name=0001-WIP-Make-some-use-of-Perl-autodie-pragma.patchDownload+15-21
On Wed, Feb 7, 2024 at 9:05 AM Peter Eisentraut <peter@eisentraut.org>
wrote:
I came across the Perl autodie pragma
(https://perldoc.perl.org/autodie). This seems pretty useful; is this
something we can use? Any drawbacks? Any minimum Perl version?
Big +1
No drawbacks. I've been using it heavily for many, many years. Came out in
5.10.1,
which should be available everywhere at this point (2009 was the year of
release)
Cheers,
Greg
On Wed, Feb 7, 2024 at 11:52 PM Greg Sabino Mullane <htamfids@gmail.com> wrote:
No drawbacks. I've been using it heavily for many, many years. Came out in 5.10.1,
which should be available everywhere at this point (2009 was the year of release)
We moved our minimum to 5.14 fairly recently, so we're good on that point.
John Naylor <johncnaylorls@gmail.com> writes:
On Wed, Feb 7, 2024 at 11:52 PM Greg Sabino Mullane <htamfids@gmail.com> wrote:
No drawbacks. I've been using it heavily for many, many years. Came out in 5.10.1,
which should be available everywhere at this point (2009 was the year of release)
We moved our minimum to 5.14 fairly recently, so we're good on that point.
Yeah, but only recently. I'm a little worried about the value of this
change relative to the amount of code churn involved, and more to the
point I worry about the risk of future back-patches injecting bad code
into back branches that don't use autodie.
(Back-patching the use of autodie doesn't seem feasible, since before
v16 we supported perl 5.8.something.)
regards, tom lane
On 08.02.24 07:03, Tom Lane wrote:
John Naylor <johncnaylorls@gmail.com> writes:
On Wed, Feb 7, 2024 at 11:52 PM Greg Sabino Mullane <htamfids@gmail.com> wrote:
No drawbacks. I've been using it heavily for many, many years. Came out in 5.10.1,
which should be available everywhere at this point (2009 was the year of release)We moved our minimum to 5.14 fairly recently, so we're good on that point.
Yeah, but only recently. I'm a little worried about the value of this
change relative to the amount of code churn involved, and more to the
point I worry about the risk of future back-patches injecting bad code
into back branches that don't use autodie.(Back-patching the use of autodie doesn't seem feasible, since before
v16 we supported perl 5.8.something.)
Yeah, good points. I suppose we could start using it for completely new
scripts.
On 8 Feb 2024, at 08:01, Peter Eisentraut <peter@eisentraut.org> wrote:
I suppose we could start using it for completely new scripts.
+1, it would be nice to eventually be able to move to it everywhere so starting
now with new scripts may make the eventual transition smoother.
--
Daniel Gustafsson
Daniel Gustafsson <daniel@yesql.se> writes:
On 8 Feb 2024, at 08:01, Peter Eisentraut <peter@eisentraut.org> wrote:
I suppose we could start using it for completely new scripts.
+1, it would be nice to eventually be able to move to it everywhere so starting
now with new scripts may make the eventual transition smoother.
I'm still concerned about people carelessly using autodie-reliant
code in places where they shouldn't. I offer two safer ways
forward:
1. Wait till v16 is the oldest supported branch, and then migrate
both HEAD and back branches to using autodie.
2. Don't wait, migrate them all now. This would mean requiring
Perl 5.10.1 or later to run the TAP tests, even in back branches.
I think #2 might not be all that radical. We have nothing older
than 5.14.0 in the buildfarm, so we don't really have much grounds
for claiming that 5.8.3 will work today. And 5.10.1 came out in
2009, so how likely is it that anyone cares anymore?
regards, tom lane
On 8 Feb 2024, at 16:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
2. Don't wait, migrate them all now. This would mean requiring
Perl 5.10.1 or later to run the TAP tests, even in back branches.I think #2 might not be all that radical. We have nothing older
than 5.14.0 in the buildfarm, so we don't really have much grounds
for claiming that 5.8.3 will work today. And 5.10.1 came out in
2009, so how likely is it that anyone cares anymore?
I would vote for this option, if we don't run the trailing edge anywhere where
breakage is visible to developers then it is like you say, far from guaranteed
to work.
--
Daniel Gustafsson
2. Don't wait, migrate them all now. This would mean requiring
Perl 5.10.1 or later to run the TAP tests, even in back branches.
#2 please. For context, meson did not even exist in 2009.
Cheers,
Greg
Daniel Gustafsson <daniel@yesql.se> writes:
On 8 Feb 2024, at 16:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
2. Don't wait, migrate them all now. This would mean requiring
Perl 5.10.1 or later to run the TAP tests, even in back branches.I think #2 might not be all that radical. We have nothing older
than 5.14.0 in the buildfarm, so we don't really have much grounds
for claiming that 5.8.3 will work today. And 5.10.1 came out in
2009, so how likely is it that anyone cares anymore?I would vote for this option, if we don't run the trailing edge anywhere where
breakage is visible to developers then it is like you say, far from guaranteed
to work.
The oldest Perl I'm aware of on a still-supported (fsvo) OS is RHEL 6,
which shipped 5.10.1 and has Extended Life-cycle Support until
2024-06-30.
For comparison, last year the at the Perl Toolchain Summit in Lyon we
decided that toolchain modules (the modules needed to build, test and
install CPAN distributions) are only required support versions of Perl
up to 10 years old, i.e. currently 5.18 (but there's a one-time
excemption to keep it to 5.16 until RHEL 7 goes out of maintenance
support on 2024-06-30).
- ilmari
On 2024-02-08 Th 11:08, Daniel Gustafsson wrote:
On 8 Feb 2024, at 16:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
2. Don't wait, migrate them all now. This would mean requiring
Perl 5.10.1 or later to run the TAP tests, even in back branches.I think #2 might not be all that radical. We have nothing older
than 5.14.0 in the buildfarm, so we don't really have much grounds
for claiming that 5.8.3 will work today. And 5.10.1 came out in
2009, so how likely is it that anyone cares anymore?I would vote for this option, if we don't run the trailing edge anywhere where
breakage is visible to developers then it is like you say, far from guaranteed
to work.
+1 from me too. We kept 5.8 going for a while because it was what the
Msys (v1) DTK perl was, but that doesn't matter any more I think.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
On 2024-02-08 Th 11:08, Daniel Gustafsson wrote:
On 8 Feb 2024, at 16:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
2. Don't wait, migrate them all now. This would mean requiring
Perl 5.10.1 or later to run the TAP tests, even in back branches.
I think #2 might not be all that radical. We have nothing older
than 5.14.0 in the buildfarm, so we don't really have much grounds
for claiming that 5.8.3 will work today. And 5.10.1 came out in
2009, so how likely is it that anyone cares anymore?
I would vote for this option, if we don't run the trailing edge anywhere where
breakage is visible to developers then it is like you say, far from guaranteed
to work.
+1 from me too. We kept 5.8 going for a while because it was what the
Msys (v1) DTK perl was, but that doesn't matter any more I think.
I've reconfigured longfin, which was using perl 5.14.0 on all
branches, to use 5.10.1 on the pre-v16 branches (and it did pass).
This seems like a good change even if we don't pull the trigger on
the above proposal --- although if we don't, maybe I should see
if I can get 5.8.3 to build on that machine.
regards, tom lane
On 08.02.24 16:53, Tom Lane wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
On 8 Feb 2024, at 08:01, Peter Eisentraut <peter@eisentraut.org> wrote:
I suppose we could start using it for completely new scripts.+1, it would be nice to eventually be able to move to it everywhere so starting
now with new scripts may make the eventual transition smoother.I'm still concerned about people carelessly using autodie-reliant
code in places where they shouldn't. I offer two safer ways
forward:1. Wait till v16 is the oldest supported branch, and then migrate
both HEAD and back branches to using autodie.2. Don't wait, migrate them all now. This would mean requiring
Perl 5.10.1 or later to run the TAP tests, even in back branches.I think #2 might not be all that radical. We have nothing older
than 5.14.0 in the buildfarm, so we don't really have much grounds
for claiming that 5.8.3 will work today. And 5.10.1 came out in
2009, so how likely is it that anyone cares anymore?
A gentler way might be to start using some perlcritic policies like
InputOutput::RequireCheckedOpen or the more general
InputOutput::RequireCheckedSyscalls and add explicit error checking at
the sites it points out. And then if we start using autodie in the
future, any inappropriate backpatching of calls lacking error checks
would be caught.
On 2024-02-14 We 11:52, Peter Eisentraut wrote:
On 08.02.24 16:53, Tom Lane wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
On 8 Feb 2024, at 08:01, Peter Eisentraut <peter@eisentraut.org>
wrote:
I suppose we could start using it for completely new scripts.+1, it would be nice to eventually be able to move to it everywhere
so starting
now with new scripts may make the eventual transition smoother.I'm still concerned about people carelessly using autodie-reliant
code in places where they shouldn't. I offer two safer ways
forward:1. Wait till v16 is the oldest supported branch, and then migrate
both HEAD and back branches to using autodie.2. Don't wait, migrate them all now. This would mean requiring
Perl 5.10.1 or later to run the TAP tests, even in back branches.I think #2 might not be all that radical. We have nothing older
than 5.14.0 in the buildfarm, so we don't really have much grounds
for claiming that 5.8.3 will work today. And 5.10.1 came out in
2009, so how likely is it that anyone cares anymore?A gentler way might be to start using some perlcritic policies like
InputOutput::RequireCheckedOpen or the more general
InputOutput::RequireCheckedSyscalls and add explicit error checking at
the sites it points out. And then if we start using autodie in the
future, any inappropriate backpatching of calls lacking error checks
would be caught.
Yeah, that should work.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On 19 Feb 2024, at 01:54, Andrew Dunstan <andrew@dunslane.net> wrote:
On 2024-02-14 We 11:52, Peter Eisentraut wrote:
A gentler way might be to start using some perlcritic policies like InputOutput::RequireCheckedOpen or the more general InputOutput::RequireCheckedSyscalls and add explicit error checking at the sites it points out. And then if we start using autodie in the future, any inappropriate backpatching of calls lacking error checks would be caught.
Yeah, that should work.
I didn't study the referenced rules but the concept seems sane, so definitely a
+1 on that.
--
Daniel Gustafsson
On 14.02.24 17:52, Peter Eisentraut wrote:
A gentler way might be to start using some perlcritic policies like
InputOutput::RequireCheckedOpen or the more general
InputOutput::RequireCheckedSyscalls and add explicit error checking at
the sites it points out.
Here is a start for that. I added the required stanza to perlcriticrc
and started with an explicit list of functions to check:
functions = chmod flock open read rename seek symlink system
and fixed all the issues it pointed out.
I picked those functions because most existing code already checked
those, so the omissions are probably unintended, or in some cases also
because I thought it would be important for test correctness (e.g., some
tests using chmod).
I didn't design any beautiful error messages, mostly just used "or die
$!", which mostly matches existing code, and also this is
developer-level code, so having the system error plus source code
reference should be ok.
In the second patch, I changed the perlcriticrc stanza to use an
exclusion list instead of an explicit inclusion list. That way, you can
see what we are currently *not* checking. I'm undecided which way
around is better, and exactly what functions we should be checking. (Of
course, in principle, all of them, but since this is test and build
support code, not production code, there are probably some reasonable
compromises to be made.)
Attachments:
v1-0001-perlcritic-InputOutput-RequireCheckedSyscalls.patchtext/plain; charset=UTF-8; name=v1-0001-perlcritic-InputOutput-RequireCheckedSyscalls.patchDownload+48-44
v1-0002-Write-perlcritic-InputOutput-RequireCheckedSyscal.patchtext/plain; charset=UTF-8; name=v1-0002-Write-perlcritic-InputOutput-RequireCheckedSyscal.patchDownload+2-2
On 21.02.24 08:26, Peter Eisentraut wrote:
On 14.02.24 17:52, Peter Eisentraut wrote:
A gentler way might be to start using some perlcritic policies like
InputOutput::RequireCheckedOpen or the more general
InputOutput::RequireCheckedSyscalls and add explicit error checking at
the sites it points out.Here is a start for that. I added the required stanza to perlcriticrc
and started with an explicit list of functions to check:functions = chmod flock open read rename seek symlink system
and fixed all the issues it pointed out.
I picked those functions because most existing code already checked
those, so the omissions are probably unintended, or in some cases also
because I thought it would be important for test correctness (e.g., some
tests using chmod).I didn't design any beautiful error messages, mostly just used "or die
$!", which mostly matches existing code, and also this is
developer-level code, so having the system error plus source code
reference should be ok.In the second patch, I changed the perlcriticrc stanza to use an
exclusion list instead of an explicit inclusion list. That way, you can
see what we are currently *not* checking. I'm undecided which way
around is better, and exactly what functions we should be checking. (Of
course, in principle, all of them, but since this is test and build
support code, not production code, there are probably some reasonable
compromises to be made.)
After some pondering, I figured the exclude list is better. So here is
a squashed patch, also with a complete commit message.
Btw., do we check perlcritic in an automated way, like on the buildfarm?
Attachments:
v2-0001-Activate-perlcritic-InputOutput-RequireCheckedSys.patchtext/plain; charset=UTF-8; name=v2-0001-Activate-perlcritic-InputOutput-RequireCheckedSys.patchDownload+52-44
On Mon, Mar 18, 2024 at 2:28 AM Peter Eisentraut <peter@eisentraut.org>
wrote:
On 21.02.24 08:26, Peter Eisentraut wrote:
On 14.02.24 17:52, Peter Eisentraut wrote:
A gentler way might be to start using some perlcritic policies like
InputOutput::RequireCheckedOpen or the more general
InputOutput::RequireCheckedSyscalls and add explicit error checking at
the sites it points out.Here is a start for that. I added the required stanza to perlcriticrc
and started with an explicit list of functions to check:functions = chmod flock open read rename seek symlink system
and fixed all the issues it pointed out.
I picked those functions because most existing code already checked
those, so the omissions are probably unintended, or in some cases also
because I thought it would be important for test correctness (e.g., some
tests using chmod).I didn't design any beautiful error messages, mostly just used "or die
$!", which mostly matches existing code, and also this is
developer-level code, so having the system error plus source code
reference should be ok.In the second patch, I changed the perlcriticrc stanza to use an
exclusion list instead of an explicit inclusion list. That way, you can
see what we are currently *not* checking. I'm undecided which way
around is better, and exactly what functions we should be checking. (Of
course, in principle, all of them, but since this is test and build
support code, not production code, there are probably some reasonable
compromises to be made.)After some pondering, I figured the exclude list is better. So here is
a squashed patch, also with a complete commit message.Btw., do we check perlcritic in an automated way, like on the buildfarm?
Yes. crake and koel do.
cheers
andrew
On 18 Mar 2024, at 07:27, Peter Eisentraut <peter@eisentraut.org> wrote:
After some pondering, I figured the exclude list is better.
Agreed.
So here is a squashed patch, also with a complete commit message.
Looks good from a read-through. It would have been nice to standardize on
using one of "|| die" and "or die" consistently but that's clearly not for this
body of work.
--
Daniel Gustafsson
Daniel Gustafsson <daniel@yesql.se> writes:
On 18 Mar 2024, at 07:27, Peter Eisentraut <peter@eisentraut.org> wrote:
After some pondering, I figured the exclude list is better.
Agreed.
So here is a squashed patch, also with a complete commit message.
Looks good from a read-through. It would have been nice to standardize on
using one of "|| die" and "or die" consistently but that's clearly not for this
body of work.
"or die" is generally the preferred form, since || has higher precedence
than comma, so it's easy to make mistakes if you don't parenthesise the
function args, like:
open my $fh, '>', $filname || die "can't open $filename: $!";
which will only fail if $filename is falsy (i.e. undef, "", or "0").
- ilmari