Обсуждение: meson vs make: missing/inconsistent ENV

I noticed warnings:
Use of uninitialized value $ENV{"with_icu"} in string eq at /home/pryzbyj/src/postgres/src/bin/pg_dump/t/002_pg_dump.pl
line56.
 

and looked through: git grep ^export '*/Makefile'

and found that:
src/bin/pg_dump/meson.build is missing with_icu since 396d348b0

Also, e6927270c added ZSTD to src/bin/pg_basebackup/meson.build, but
it's not in ./Makefile ??  Maybe that was for consistency with other
places, or pre-emptive in case the tap tests want to do tests involving
the ZSTD tool.  But it'd be better if ./Makefile had it too.

The rest I think are not errors:

src/test/meson.build is missing PG_TEST_EXTRA
src/bin/pg_upgrade/meson.build and ../src/test/recovery/meson.build
are missing REGRESS_SHLIB

Is there any consideration of promoting these or other warnings to
fatal?

-- 
Justin

Hi,

On 2023-02-26 16:52:39 -0600, Justin Pryzby wrote:
> I noticed warnings:
> Use of uninitialized value $ENV{"with_icu"} in string eq at
/home/pryzbyj/src/postgres/src/bin/pg_dump/t/002_pg_dump.plline 56.
 
> 
> and looked through: git grep ^export '*/Makefile'
> 
> and found that:
> src/bin/pg_dump/meson.build is missing with_icu since 396d348b0

Looks like it.


> Also, e6927270c added ZSTD to src/bin/pg_basebackup/meson.build, but
> it's not in ./Makefile ??  Maybe that was for consistency with other
> places, or pre-emptive in case the tap tests want to do tests involving
> the ZSTD tool.  But it'd be better if ./Makefile had it too.

I suspect I just over-eagerly added it when the pg_basebackup zstd support
went in, using the GZIP_PROGRAM/LZ4 cases as a template. And foolishly
assuming a newly added compression method would be tested.


> The rest I think are not errors:
> 
> src/test/meson.build is missing PG_TEST_EXTRA

> src/bin/pg_upgrade/meson.build and ../src/test/recovery/meson.build
> are missing REGRESS_SHLIB

Yep, these are added in the top-level meson.build.


> Is there any consideration of promoting these or other warnings to
> fatal?

You mean the perl warnings?

Greetings,

Andres Freund

On Sun, Feb 26, 2023 at 03:21:04PM -0800, Andres Freund wrote:
> > Is there any consideration of promoting these or other warnings to
> > fatal?
> 
> You mean the perl warnings?

Yes - it'd be nice if the warnings caused an obvious failure to allow
addressing the issue.  I noticed the icu warning while looking at a bug
in 0da243fed, and updating to add ZSTD.  

Justin Pryzby <pryzby@telsasoft.com> writes:

> On Sun, Feb 26, 2023 at 03:21:04PM -0800, Andres Freund wrote:
>> > Is there any consideration of promoting these or other warnings to
>> > fatal?
>> 
>> You mean the perl warnings?
>
> Yes - it'd be nice if the warnings caused an obvious failure to allow
> addressing the issue.  I noticed the icu warning while looking at a bug
> in 0da243fed, and updating to add ZSTD.  

Perl warnings can be made fatal with `use warnings FATAL =>
<categories>;`, but one should be careful about which categories to
fatalise, per <https://metacpan.org/pod/warnings#Fatal-Warnings>.

Some categories are inherently unsafe to fatalise, as documented in
<https://metacpan.org/pod/strictures#CATEGORY-SELECTIONS>.

- ilmari

Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:

> Justin Pryzby <pryzby@telsasoft.com> writes:
>
>> On Sun, Feb 26, 2023 at 03:21:04PM -0800, Andres Freund wrote:
>>> > Is there any consideration of promoting these or other warnings to
>>> > fatal?
>>> 
>>> You mean the perl warnings?
>>
>> Yes - it'd be nice if the warnings caused an obvious failure to allow
>> addressing the issue.  I noticed the icu warning while looking at a bug
>> in 0da243fed, and updating to add ZSTD.  
>
> Perl warnings can be made fatal with `use warnings FATAL =>
> <categories>;`, but one should be careful about which categories to
> fatalise, per <https://metacpan.org/pod/warnings#Fatal-Warnings>.
>
> Some categories are inherently unsafe to fatalise, as documented in
> <https://metacpan.org/pod/strictures#CATEGORY-SELECTIONS>.

One disadvantage of making the warnings fatal is that it immediately
aborts the test.  Another option would be to to turn warnings into test
failures, à la https://metacpan.org/pod/Test::Warnings or
https://metacpan.org/pod/Test::FailWarnings.  Both those modules support
all the Perl versions we do, and have no non-core dependencies, but if
we don't want to add any more dependencies we can incorporate the logic
into one of our own testing modules.

- ilmari

Justin Pryzby <pryzby@telsasoft.com> writes:

On Sun, Feb 26, 2023 at 03:21:04PM -0800, Andres Freund wrote:

Is there any consideration of promoting these or other warnings to
fatal?

You mean the perl warnings?

Yes - it'd be nice if the warnings caused an obvious failure to allow
addressing the issue.  I noticed the icu warning while looking at a bug
in 0da243fed, and updating to add ZSTD.  

Perl warnings can be made fatal with `use warnings FATAL =>
<categories>;`, but one should be careful about which categories to
fatalise, per <https://metacpan.org/pod/warnings#Fatal-Warnings>.

Some categories are inherently unsafe to fatalise, as documented in
<https://metacpan.org/pod/strictures#CATEGORY-SELECTIONS>.

Yeah.

It would be nice if there were some fuller explanation of the various categories, but I don't know of one.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2023-02-27 Mo 06:17, Dagfinn Ilmari Mannsåker wrote:

Justin Pryzby <pryzby@telsasoft.com> writes:

On Sun, Feb 26, 2023 at 03:21:04PM -0800, Andres Freund wrote:

Is there any consideration of promoting these or other warnings to
fatal?

You mean the perl warnings?

Yes - it'd be nice if the warnings caused an obvious failure to allow
addressing the issue.  I noticed the icu warning while looking at a bug
in 0da243fed, and updating to add ZSTD.  

Perl warnings can be made fatal with `use warnings FATAL =>
<categories>;`, but one should be careful about which categories to
fatalise, per <https://metacpan.org/pod/warnings#Fatal-Warnings>.

Some categories are inherently unsafe to fatalise, as documented in
<https://metacpan.org/pod/strictures#CATEGORY-SELECTIONS>.

Yeah.

It would be nice if there were some fuller explanation of the various categories, but I don't know of one.

Looks like the explanations are in the perldiag manual page. <https://perldoc.perl.org/perldiag>

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Sun, Feb 26, 2023 at 03:21:04PM -0800, Andres Freund wrote:
> On 2023-02-26 16:52:39 -0600, Justin Pryzby wrote:
>> Also, e6927270c added ZSTD to src/bin/pg_basebackup/meson.build, but
>> it's not in ./Makefile ??  Maybe that was for consistency with other
>> places, or pre-emptive in case the tap tests want to do tests involving
>> the ZSTD tool.  But it'd be better if ./Makefile had it too.
>
> I suspect I just over-eagerly added it when the pg_basebackup zstd support
> went in, using the GZIP_PROGRAM/LZ4 cases as a template. And foolishly
> assuming a newly added compression method would be tested.

leaving the discussion with the perl warnings aside for the moment,
these still need to be adjusted.  Justin, would you like to write a
patch with everything you have found?
--
Michael

Hi,

On 2023-03-09 09:36:52 +0900, Michael Paquier wrote:
> On Sun, Feb 26, 2023 at 03:21:04PM -0800, Andres Freund wrote:
> > On 2023-02-26 16:52:39 -0600, Justin Pryzby wrote:
> >> Also, e6927270c added ZSTD to src/bin/pg_basebackup/meson.build, but
> >> it's not in ./Makefile ??  Maybe that was for consistency with other
> >> places, or pre-emptive in case the tap tests want to do tests involving
> >> the ZSTD tool.  But it'd be better if ./Makefile had it too.
> > 
> > I suspect I just over-eagerly added it when the pg_basebackup zstd support
> > went in, using the GZIP_PROGRAM/LZ4 cases as a template. And foolishly
> > assuming a newly added compression method would be tested.
> 
> leaving the discussion with the perl warnings aside for the moment,
> these still need to be adjusted.  Justin, would you like to write a
> patch with everything you have found?

I now pushed a fix for the two obvious cases pointed out by Justin.

Greetings,

Andres Freund

On Wed, Mar 08, 2023 at 05:59:13PM -0800, Andres Freund wrote:
> I now pushed a fix for the two obvious cases pointed out by Justin.

Thanks!
--
Michael