Thread: Make all Perl warnings fatal
We have a lot of Perl scripts in the tree, mostly code generation and TAP tests. Occasionally, these scripts produce warnings. These are AFAICT always mistakes on the developer side (true positives). Typical examples are warnings from genbki.pl or related when you make a mess in the catalog files during development, or warnings from tests when they massage a config file that looks different on different hosts, or mistakes during merges (e.g., duplicate subroutine definitions), or just mistakes that weren't noticed, because, you know, there is a lot of output in a verbose build. I wanted to figure put if we can catch these more reliably, in the style of -Werror. AFAICT, there is no way to automatically turn all warnings into fatal errors. But there is a way to do it per script, by replacing use warnings; by use warnings FATAL => 'all'; See attached patch to try it out. The documentation at <https://perldoc.perl.org/warnings#Fatal-Warnings> appears to sort of hand-wave against doing that. Their argument appears to be something like, the modules you use might in the future produce additional warnings, thus breaking your scripts. On balance, I'd take that risk, if it means I would notice the warnings in a more timely and robust way. But that's just me at a moment in time. Thoughts? Now to some funny business. If you apply this patch, the Cirrus CI Linux tasks will fail, because they get an undefined return from getprotobyname() in PostgreSQL/Test/Cluster.pm. Huh? I need this patch to get past that: diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm index 3fa679ff97..dfe7bc7b1a 100644 --- a/src/test/perl/PostgreSQL/Test/Cluster.pm +++ b/src/test/perl/PostgreSQL/Test/Cluster.pm @@ -1570,7 +1570,7 @@ sub can_bind my ($host, $port) = @_; my $iaddr = inet_aton($host); my $paddr = sockaddr_in($port, $iaddr); - my $proto = getprotobyname("tcp"); + my $proto = getprotobyname("tcp") || 6; socket(SOCK, PF_INET, SOCK_STREAM, $proto) or die "socket failed: $!"; What is going on there? Does this host not have /etc/protocols, or something like that? There are also a couple of issues in the MSVC legacy build system that would need to be tightened up in order to survive with fatal Perl warnings. Obviously, there is a question whether it's worth spending any time on that anymore.
Attachment
To avoid a complete bloodbath on cfbot, here is an updated patch set that includes a workaround for the getprotobyname() issue mentioned below. On 10.08.23 07:58, Peter Eisentraut wrote: > We have a lot of Perl scripts in the tree, mostly code generation and > TAP tests. Occasionally, these scripts produce warnings. These are > AFAICT always mistakes on the developer side (true positives). Typical > examples are warnings from genbki.pl or related when you make a mess in > the catalog files during development, or warnings from tests when they > massage a config file that looks different on different hosts, or > mistakes during merges (e.g., duplicate subroutine definitions), or just > mistakes that weren't noticed, because, you know, there is a lot of > output in a verbose build. > > I wanted to figure put if we can catch these more reliably, in the style > of -Werror. AFAICT, there is no way to automatically turn all warnings > into fatal errors. But there is a way to do it per script, by replacing > > use warnings; > > by > > use warnings FATAL => 'all'; > > See attached patch to try it out. > > The documentation at <https://perldoc.perl.org/warnings#Fatal-Warnings> > appears to sort of hand-wave against doing that. Their argument appears > to be something like, the modules you use might in the future produce > additional warnings, thus breaking your scripts. On balance, I'd take > that risk, if it means I would notice the warnings in a more timely and > robust way. But that's just me at a moment in time. > > Thoughts? > > > Now to some funny business. If you apply this patch, the Cirrus CI > Linux tasks will fail, because they get an undefined return from > getprotobyname() in PostgreSQL/Test/Cluster.pm. Huh? I need this patch > to get past that: > > diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm > b/src/test/perl/PostgreSQL/Test/Cluster.pm > index 3fa679ff97..dfe7bc7b1a 100644 > --- a/src/test/perl/PostgreSQL/Test/Cluster.pm > +++ b/src/test/perl/PostgreSQL/Test/Cluster.pm > @@ -1570,7 +1570,7 @@ sub can_bind > my ($host, $port) = @_; > my $iaddr = inet_aton($host); > my $paddr = sockaddr_in($port, $iaddr); > - my $proto = getprotobyname("tcp"); > + my $proto = getprotobyname("tcp") || 6; > > socket(SOCK, PF_INET, SOCK_STREAM, $proto) > or die "socket failed: $!"; > > What is going on there? Does this host not have /etc/protocols, or > something like that? > > > There are also a couple of issues in the MSVC legacy build system that > would need to be tightened up in order to survive with fatal Perl > warnings. Obviously, there is a question whether it's worth spending > any time on that anymore.
Attachment
To avoid a complete bloodbath on cfbot, here is an updated patch set that includes a workaround for the getprotobyname() issue mentioned below.
On 10.08.23 07:58, Peter Eisentraut wrote:We have a lot of Perl scripts in the tree, mostly code generation and TAP tests. Occasionally, these scripts produce warnings. These are AFAICT always mistakes on the developer side (true positives). Typical examples are warnings from genbki.pl or related when you make a mess in the catalog files during development, or warnings from tests when they massage a config file that looks different on different hosts, or mistakes during merges (e.g., duplicate subroutine definitions), or just mistakes that weren't noticed, because, you know, there is a lot of output in a verbose build.
I wanted to figure put if we can catch these more reliably, in the style of -Werror. AFAICT, there is no way to automatically turn all warnings into fatal errors. But there is a way to do it per script, by replacing
use warnings;
by
use warnings FATAL => 'all';
See attached patch to try it out.
The documentation at <https://perldoc.perl.org/warnings#Fatal-Warnings> appears to sort of hand-wave against doing that. Their argument appears to be something like, the modules you use might in the future produce additional warnings, thus breaking your scripts. On balance, I'd take that risk, if it means I would notice the warnings in a more timely and robust way. But that's just me at a moment in time.
Thoughts?
It's not really the same as -Werror, because many warnings can be generated at runtime rather than compile-time.
Still, I guess that might not matter too much since apart from plperl we only use perl for building / testing.
Regarding the dangers mentioned, I guess we can undo it if it proves a nuisance.
+1 to getting rid if the unnecessary call to getprotobyname().
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On Mon, Aug 21, 2023 at 11:51:24AM -0400, Andrew Dunstan wrote: > It's not really the same as -Werror, because many warnings can be generated > at runtime rather than compile-time. > > Still, I guess that might not matter too much since apart from plperl we > only use perl for building / testing. However, is it possible to trust the out-of-core perl modules posted on CPAN, assuming that these will never produce warnings? I've never seen any issues with IPC::Run in these last years, so perhaps that's OK in the long-run. > Regarding the dangers mentioned, I guess we can undo it if it proves a > nuisance. Yeah. I am wondering what the buildfarm would say with this change. > +1 to getting rid if the unnecessary call to getprotobyname(). Looking around here.. https://perldoc.perl.org/perlipc#Sockets%3A-Client%2FServer-Communication Hmm. Are you sure that this is OK even in the case where the TAP tests run on Windows without unix-domain socket support? The CI runs on Windows, but always with unix domain sockets around as far as I know. -- Michael
Attachment
On Mon, Aug 21, 2023 at 11:51:24AM -0400, Andrew Dunstan wrote:It's not really the same as -Werror, because many warnings can be generated at runtime rather than compile-time. Still, I guess that might not matter too much since apart from plperl we only use perl for building / testing.However, is it possible to trust the out-of-core perl modules posted on CPAN, assuming that these will never produce warnings? I've never seen any issues with IPC::Run in these last years, so perhaps that's OK in the long-run.
If we do find any such issues then warnings can be turned off locally. We already do that in several places.
Regarding the dangers mentioned, I guess we can undo it if it proves a nuisance.Yeah. I am wondering what the buildfarm would say with this change.+1 to getting rid if the unnecessary call to getprotobyname().Looking around here.. https://perldoc.perl.org/perlipc#Sockets%3A-Client%2FServer-Communication Hmm. Are you sure that this is OK even in the case where the TAP tests run on Windows without unix-domain socket support? The CI runs on Windows, but always with unix domain sockets around as far as I know.
The socket call in question is for a PF_INET socket, so this has nothing at all to do with unix domain sockets. See the man page for socket() (2) for an explanation of why 0 is ok in this case. (There's only one protocol that matches the rest of the parameters).
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On 2023-Aug-10, Peter Eisentraut wrote: > I wanted to figure put if we can catch these more reliably, in the style of > -Werror. AFAICT, there is no way to automatically turn all warnings into > fatal errors. But there is a way to do it per script, by replacing > > use warnings; > > by > > use warnings FATAL => 'all'; > > See attached patch to try it out. BTW in case we do find that there's some unforeseen problem and we want to roll back, it would be great to have a way to disable this without having to edit every single Perl file again later. However, I didn't find a way to do it -- I thought about creating a separate PgWarnings.pm file that would do the "use warnings FATAL => 'all'" dance and which every other Perl file would use or include; but couldn't make it work. Maybe some Perl expert knows a good answer to this. Maybe the BEGIN block of each file can `eval` a new PgWarnings.pm that emits the "use warnings" line? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "No renuncies a nada. No te aferres a nada."
On 2023-Aug-10, Peter Eisentraut wrote:I wanted to figure put if we can catch these more reliably, in the style of -Werror. AFAICT, there is no way to automatically turn all warnings into fatal errors. But there is a way to do it per script, by replacing use warnings; by use warnings FATAL => 'all'; See attached patch to try it out.BTW in case we do find that there's some unforeseen problem and we want to roll back, it would be great to have a way to disable this without having to edit every single Perl file again later. However, I didn't find a way to do it -- I thought about creating a separate PgWarnings.pm file that would do the "use warnings FATAL => 'all'" dance and which every other Perl file would use or include; but couldn't make it work. Maybe some Perl expert knows a good answer to this. Maybe the BEGIN block of each file can `eval` a new PgWarnings.pm that emits the "use warnings" line?
Once we try it, I doubt we would want to revoke it globally, and if we did I'd rather not be left with a wart like this. As I mentioned upthread, it is possible to override the setting locally. The manual page for the warnings pragma contains details.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On 21.08.23 17:51, Andrew Dunstan wrote: > Still, I guess that might not matter too much since apart from plperl we > only use perl for building / testing. > > Regarding the dangers mentioned, I guess we can undo it if it proves a > nuisance. > > +1 to getting rid if the unnecessary call to getprotobyname(). I have committed the latter part. The rest would still depend on some fixes for the MSVC build system, so I'll hold that until we decide what to do with that.
Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > On 2023-Aug-10, Peter Eisentraut wrote: > >> I wanted to figure put if we can catch these more reliably, in the style of >> -Werror. AFAICT, there is no way to automatically turn all warnings into >> fatal errors. But there is a way to do it per script, by replacing >> >> use warnings; >> >> by >> >> use warnings FATAL => 'all'; >> >> See attached patch to try it out. > > BTW in case we do find that there's some unforeseen problem and we want > to roll back, it would be great to have a way to disable this without > having to edit every single Perl file again later. However, I didn't > find a way to do it -- I thought about creating a separate PgWarnings.pm > file that would do the "use warnings FATAL => 'all'" dance and which > every other Perl file would use or include; but couldn't make it work. > Maybe some Perl expert knows a good answer to this. Like most pragmas (all-lower-case module names), `warnings` affects the currently-compiling lexical scope, so to have a module like PgWarnings inject it into the module that uses it, you'd call warnings->import in its import method (which gets called when the `use PgWarnings;`` statement is compiled, e.g.: package PgWarnings; sub import { warnings->import(FATAL => 'all'); } I wouldn't bother with a whole module just for that, but if we have a group of pragmas or modules we always want to enable/import and have the ability to change this set without having to edit all the files, it's quite common to have a ProjectName::Policy module that does that. For example, to exclude warnings that are unsafe, pointless, or impossible to fatalise (c.f. https://metacpan.org/pod/strictures#CATEGORY-SELECTIONS): package PostgreSQL::Policy; sub import { strict->import; warnings->import( FATAL => 'all', NONFATAL => qw(exec internal malloc recursion), ); warnings->uniport(qw(once)); } Now that we require Perl 5.14, we might want to consider enabling its feature bundle as well, with: feature->import(':5.14') Although the only features of note that adds are: - say: the `say` function, like `print` but appends a newline - state: `state` variables, like `my` but only initialised the first time the function they're in is called, and the value persists between calls (like function-scoped `static` variables in C) - unicode_strings: use unicode semantics for characters in the 128-255 range, regardless of internal representation > Maybe the BEGIN block of each file can `eval` a new PgWarnings.pm that > emits the "use warnings" line? That's ugly as sin, and thankfully not necessary. -ilmari
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:On 2023-Aug-10, Peter Eisentraut wrote:I wanted to figure put if we can catch these more reliably, in the style of -Werror. AFAICT, there is no way to automatically turn all warnings into fatal errors. But there is a way to do it per script, by replacing use warnings; by use warnings FATAL => 'all'; See attached patch to try it out.BTW in case we do find that there's some unforeseen problem and we want to roll back, it would be great to have a way to disable this without having to edit every single Perl file again later. However, I didn't find a way to do it -- I thought about creating a separate PgWarnings.pm file that would do the "use warnings FATAL => 'all'" dance and which every other Perl file would use or include; but couldn't make it work. Maybe some Perl expert knows a good answer to this.Like most pragmas (all-lower-case module names), `warnings` affects the currently-compiling lexical scope, so to have a module like PgWarnings inject it into the module that uses it, you'd call warnings->import in its import method (which gets called when the `use PgWarnings;`` statement is compiled, e.g.: package PgWarnings; sub import { warnings->import(FATAL => 'all'); } I wouldn't bother with a whole module just for that, but if we have a group of pragmas or modules we always want to enable/import and have the ability to change this set without having to edit all the files, it's quite common to have a ProjectName::Policy module that does that. For example, to exclude warnings that are unsafe, pointless, or impossible to fatalise (c.f. https://metacpan.org/pod/strictures#CATEGORY-SELECTIONS): package PostgreSQL::Policy; sub import { strict->import; warnings->import( FATAL => 'all', NONFATAL => qw(exec internal malloc recursion), ); warnings->uniport(qw(once)); } Now that we require Perl 5.14, we might want to consider enabling its feature bundle as well, with: feature->import(':5.14') Although the only features of note that adds are: - say: the `say` function, like `print` but appends a newline - state: `state` variables, like `my` but only initialised the first time the function they're in is called, and the value persists between calls (like function-scoped `static` variables in C) - unicode_strings: use unicode semantics for characters in the 128-255 range, regardless of internal representation
We'd probably have to modify the perlcritic rules to account for it. See <https://metacpan.org/pod/Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict> and similarly for RequireUseWarnings. In any case, it seems a bit like overkill.
Maybe the BEGIN block of each file can `eval` a new PgWarnings.pm that emits the "use warnings" line?That's ugly as sin, and thankfully not necessary.
Agreed.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On 10.08.23 07:58, Peter Eisentraut wrote: > There are also a couple of issues in the MSVC legacy build system that > would need to be tightened up in order to survive with fatal Perl > warnings. Obviously, there is a question whether it's worth spending > any time on that anymore. It looks like there are no principled objections to the overall idea here, but given this dependency on the MSVC build system removal, I'm going to set this patch to Returned with feedback for now.
On 12.09.23 07:42, Peter Eisentraut wrote: > On 10.08.23 07:58, Peter Eisentraut wrote: >> There are also a couple of issues in the MSVC legacy build system that >> would need to be tightened up in order to survive with fatal Perl >> warnings. Obviously, there is a question whether it's worth spending >> any time on that anymore. > > It looks like there are no principled objections to the overall idea > here, but given this dependency on the MSVC build system removal, I'm > going to set this patch to Returned with feedback for now. Now that that is done, here is an updated patch for this. I found one more bug in the Perl code because of this, a fix for which is included here. With this fix, this passes all the CI tests on all platforms.
Attachment
On 22.12.23 22:33, Peter Eisentraut wrote: > On 12.09.23 07:42, Peter Eisentraut wrote: >> On 10.08.23 07:58, Peter Eisentraut wrote: >>> There are also a couple of issues in the MSVC legacy build system >>> that would need to be tightened up in order to survive with fatal >>> Perl warnings. Obviously, there is a question whether it's worth >>> spending any time on that anymore. >> >> It looks like there are no principled objections to the overall idea >> here, but given this dependency on the MSVC build system removal, I'm >> going to set this patch to Returned with feedback for now. > > Now that that is done, here is an updated patch for this. > > I found one more bug in the Perl code because of this, a fix for which > is included here. > > With this fix, this passes all the CI tests on all platforms. committed
On Sat, Dec 30, 2023 at 12:57 AM Peter Eisentraut <peter@eisentraut.org> wrote: > > committed With the commit c5385929 converting perl warnings to FATAL, use of psql/safe_psql with timeout parameters [1] fail with the following error: Use of uninitialized value $ret in bitwise and (&) at /home/ubuntu/postgres/src/test/recovery/../../../src/test/perl/PostgreSQL/Test/Cluster.pm line 2015. Perhaps assigning a default error code to $ret instead of undef in PostgreSQL::Test::Cluster - psql() function is the solution. [1] use strict; use warnings FATAL => 'all'; use PostgreSQL::Test::Cluster; use PostgreSQL::Test::Utils; use Test::More; my $node = PostgreSQL::Test::Cluster->new('test'); $node->init; $node->start; my ($stdout, $stderr, $timed_out); my $cmdret = $node->psql('postgres', q[SELECT pg_sleep(600)], stdout => \$stdout, stderr => \$stderr, timeout => 5, timed_out => \$timed_out, extra_params => ['--single-transaction'], on_error_die => 1); print "pg_sleep timed out" if $timed_out; done_testing(); -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On 11.01.24 12:29, Bharath Rupireddy wrote: > On Sat, Dec 30, 2023 at 12:57 AM Peter Eisentraut <peter@eisentraut.org> wrote: >> >> committed > > With the commit c5385929 converting perl warnings to FATAL, use of > psql/safe_psql with timeout parameters [1] fail with the following > error: > > Use of uninitialized value $ret in bitwise and (&) at > /home/ubuntu/postgres/src/test/recovery/../../../src/test/perl/PostgreSQL/Test/Cluster.pm > line 2015. I think what is actually relevant is the timed_out parameter, otherwise the psql/safe_psql function ends up calling "die" and you don't get any further. > Perhaps assigning a default error code to $ret instead of undef in > PostgreSQL::Test::Cluster - psql() function is the solution. I would put this code my $core = $ret & 128 ? " (core dumped)" : ""; die "psql exited with signal " . ($ret & 127) . "$core: '$$stderr' while running '@psql_params'" if $ret & 127; $ret = $ret >> 8; inside a if (defined $ret) block. Then the behavior would be that the whole function returns undef on timeout, which is usefully different from returning 0 (and matches previous behavior).
On Fri, Jan 12, 2024 at 9:03 PM Peter Eisentraut <peter@eisentraut.org> wrote: > > On 11.01.24 12:29, Bharath Rupireddy wrote: > > On Sat, Dec 30, 2023 at 12:57 AM Peter Eisentraut <peter@eisentraut.org> wrote: > >> > >> committed > > > > With the commit c5385929 converting perl warnings to FATAL, use of > > psql/safe_psql with timeout parameters [1] fail with the following > > error: > > > > Use of uninitialized value $ret in bitwise and (&) at > > /home/ubuntu/postgres/src/test/recovery/../../../src/test/perl/PostgreSQL/Test/Cluster.pm > > line 2015. > > I think what is actually relevant is the timed_out parameter, otherwise > the psql/safe_psql function ends up calling "die" and you don't get any > further. > > > Perhaps assigning a default error code to $ret instead of undef in > > PostgreSQL::Test::Cluster - psql() function is the solution. > > I would put this code > > my $core = $ret & 128 ? " (core dumped)" : ""; > die "psql exited with signal " > . ($ret & 127) > . "$core: '$$stderr' while running '@psql_params'" > if $ret & 127; > $ret = $ret >> 8; > > inside a if (defined $ret) block. > > Then the behavior would be that the whole function returns undef on > timeout, which is usefully different from returning 0 (and matches > previous behavior). WFM. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Fri, Jan 12, 2024 at 9:21 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Fri, Jan 12, 2024 at 9:03 PM Peter Eisentraut <peter@eisentraut.org> wrote: > > > > I would put this code > > > > my $core = $ret & 128 ? " (core dumped)" : ""; > > die "psql exited with signal " > > . ($ret & 127) > > . "$core: '$$stderr' while running '@psql_params'" > > if $ret & 127; > > $ret = $ret >> 8; > > > > inside a if (defined $ret) block. > > > > Then the behavior would be that the whole function returns undef on > > timeout, which is usefully different from returning 0 (and matches > > previous behavior). > > WFM. I've attached a patch for the above change. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On 16.01.24 12:08, Bharath Rupireddy wrote: > On Fri, Jan 12, 2024 at 9:21 PM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> wrote: >> >> On Fri, Jan 12, 2024 at 9:03 PM Peter Eisentraut <peter@eisentraut.org> wrote: >>> >>> I would put this code >>> >>> my $core = $ret & 128 ? " (core dumped)" : ""; >>> die "psql exited with signal " >>> . ($ret & 127) >>> . "$core: '$$stderr' while running '@psql_params'" >>> if $ret & 127; >>> $ret = $ret >> 8; >>> >>> inside a if (defined $ret) block. >>> >>> Then the behavior would be that the whole function returns undef on >>> timeout, which is usefully different from returning 0 (and matches >>> previous behavior). >> >> WFM. > > I've attached a patch for the above change. Committed, thanks.
Hi! On Tue, Oct 22, 2024 at 12:26 PM Anton Voloshin <a.voloshin@postgrespro.ru> wrote: > On 18/01/2024 10:52, Peter Eisentraut wrote: > > Committed, thanks. > > since this patch two .pl files without FATAL in "use warnings" have been > committed to master: > src/test/recovery/t/043_wal_replay_wait.pl > src/test/modules/test_misc/t/006_signal_autovacuum.pl Thank you for spotting this. I have just changed relevant line in 043_wal_replay_wait.pl. ------ Regards, Alexander Korotkov Supabase
On 22.10.24 11:25, Anton Voloshin wrote: > Hello, > > On 18/01/2024 10:52, Peter Eisentraut wrote: > > Committed, thanks. > > since this patch two .pl files without FATAL in "use warnings" have been > committed to master: > src/test/recovery/t/043_wal_replay_wait.pl > src/test/modules/test_misc/t/006_signal_autovacuum.pl > > They come from > commit 06c418e163e913966e17cb2d3fb1c5f8a8d58308 > Author: Alexander Korotkov <akorotkov@postgresql.org> > Date: Tue Apr 2 22:48:03 2024 > > Implement pg_wal_replay_wait() stored procedure > > and > > commit d2b74882cab84b9f4fdce0f2f32e892ba9164f5c > Author: Michael Paquier <michael@paquier.xyz> > Date: Tue Jul 16 04:05:46 2024 > > Add tap test for pg_signal_autovacuum role > > An obvious patch adding FATAL => 'all' is attached. Thanks, I committed the rest of this.