Thread: 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.
Attachment
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 generalInputOutput::RequireCheckedSyscalls and add explicit error checking at the sites it points out. And then if we startusing 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.)
Attachment
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?
Attachment
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
> On 18 Mar 2024, at 14:18, Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote: > Daniel Gustafsson <daniel@yesql.se> writes: >> 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"). Thanks for the clarification! Looking over the || die() codepaths we have, and we'll add as part of this patchset, none are vulnerable to the above issue AFAICT. -- Daniel Gustafsson
On 18.03.24 09:17, Daniel Gustafsson wrote: >> 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. Committed. I was aware of the semantic difference between "||" and "or", and I had tried to keep it similar to surrounding code.