Обсуждение: pgsql: Require version 0.98 of Test::More for TAP tests

Require version 0.98 of Test::More for TAP tests

This means that the subtest feature will be available for use.

We expect that this change will make prairiedog go red until it is
updated, but other buildfarm animals should be fine.

Discussion: https://postgr.es/m/f5e1d308-4e33-37a7-bdf1-f6e0c75119de@dunslane.net

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/405f32fc49609eb94fa39e7b5e7c1fe2bb2b73aa

Modified Files
--------------
configure                              | 2 +-
configure.ac                           | 2 +-
src/test/perl/PostgreSQL/Test/Utils.pm | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)

Andrew Dunstan <andrew@dunslane.net> writes:
> Require version 0.98 of Test::More for TAP tests
> This means that the subtest feature will be available for use.

> We expect that this change will make prairiedog go red until it is
> updated, but other buildfarm animals should be fine.

Hah, looks like wrasse beat me to it [1].  I'd supposed that Noah
was using a manually-installed Perl there, but maybe not?

            regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=wrasse&dt=2021-11-20%2022%3A58%3A17

I wrote:
> Hah, looks like wrasse beat me to it [1].  I'd supposed that Noah
> was using a manually-installed Perl there, but maybe not?

No, wait, I *did* check wrasse.  Its configure run reports

checking for perl module Test::More 0.98... 1.302162

so everything looks fine there.  But now we have this
in test-decoding-check:

Test::More version 0.98 required--this is only version 0.92 at
/export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/../pgsql/src/test/perl/PostgreSQL/Test/Utils.pmline 63. 
BEGIN failed--compilation aborted at
/export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/../pgsql/src/test/perl/PostgreSQL/Test/Utils.pmline 63. 

So apparently the true issue is that this test is somehow failing to use
the same perl, or the same perl import path, as configure did.

            regards, tom lane

I wrote:
> So apparently the true issue is that this test is somehow failing to use
> the same perl, or the same perl import path, as configure did.

Oh, I see it: wrasse is configured to use a nondefault Perl:

'config_env' => {
                  'PERL' => '/home/nm/sw/nopath/perl64/bin/perl',

but that configuration is not sufficient to ensure the correct
choice of "prove":

checking for prove... /opt/csw/bin/prove

so the TAP tests are being run with some other, much older, Perl version.

I wonder if we ought to teach configure to try to find "prove" right
beside "perl", rather than expecting people to be careful to set PROVE
as well as PERL.

            regards, tom lane

Noah Misch <noah@leadboat.com> writes:
> Yep.  wrasse sets PERL to a manually-installed Perl, but PROVE still uses an
> old Perl.  I'll fix that somehow.

A quick workaround is to set PROVE in the animal's config_env, but
I don't think that's ideal, because configure then skips module
presence tests:

  # Check for necessary modules, unless user has specified the "prove" to use;
  # in that case it's her responsibility to have a working configuration.
  # (prove might be part of a different Perl installation than perl, eg on
  # MSys, so the result of AX_PROG_PERL_MODULES could be irrelevant anyway.)

What I'm inclined to do is temporarily push `dirname $PERL` onto the front
of PATH while running

  PGAC_PATH_PROGS(PROVE, prove)

            regards, tom lane

I wrote:
> What I'm inclined to do is temporarily push `dirname $PERL` onto the front
> of PATH while running
>   PGAC_PATH_PROGS(PROVE, prove)

The attached seems like it will work.  I think we want to backpatch
this, since the wrong-PROVE hazard is the same in all branches.

            regards, tom lane

diff --git a/configure b/configure
index 977b4d3df5..6789b09d03 100755
--- a/configure
+++ b/configure
@@ -19501,7 +19501,10 @@ else
 $as_echo "$as_me: WARNING: could not find perl" >&2;}
 fi
   fi
-  # Now make sure we know where prove is
+  # Now make sure we know where prove is.
+  # Prefer to find prove in the same directory as perl.
+  save_PATH="$PATH"
+  PATH="`dirname "$PERL"`:$PATH"
   if test -z "$PROVE"; then
   for ac_prog in prove
 do
@@ -19556,6 +19559,7 @@ $as_echo_n "checking for PROVE... " >&6; }
 $as_echo "$PROVE" >&6; }
 fi

+  PATH="$save_PATH"
   if test -z "$PROVE"; then
     as_fn_error $? "prove not found" "$LINENO" 5
   fi
diff --git a/configure.ac b/configure.ac
index 95e5169c4f..8a70ca16ef 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2389,8 +2389,12 @@ if test "$enable_tap_tests" = yes; then
     AX_PROG_PERL_MODULES([IPC::Run=0.79 Test::More=0.98 Time::HiRes=1.52], ,
       [AC_MSG_ERROR([Additional Perl modules are required to run TAP tests])])
   fi
-  # Now make sure we know where prove is
+  # Now make sure we know where prove is.
+  # Prefer to find prove in the same directory as perl.
+  save_PATH="$PATH"
+  PATH="`dirname "$PERL"`:$PATH"
   PGAC_PATH_PROGS(PROVE, prove)
+  PATH="$save_PATH"
   if test -z "$PROVE"; then
     AC_MSG_ERROR([prove not found])
   fi

Noah Misch <noah@leadboat.com> writes:
> On Sat, Nov 20, 2021 at 07:50:02PM -0500, Tom Lane wrote:
>> What I'm inclined to do is temporarily push `dirname $PERL` onto the front
>> of PATH while running
>> PGAC_PATH_PROGS(PROVE, prove)

> Adding to PATH, even briefly, is way too brazen.  You'd need to be sure that
> PATH is never searched for anything other than "prove", which is hard to
> ensure in a shell script.

Hmm.  I kind of doubt that anyone would be selecting a perl in an
untrustworthy directory --- wouldn't that imply that $blackhat could
overwrite perl itself?

Still, it wouldn't be that much more trouble to write something like

    if [ -x "`dirname "$PERL"`/prove" ]; then
        PROVE="`dirname "$PERL"`/prove"
    else
        PGAC_PATH_PROGS(PROVE, prove)
    fi

(this lacks some infrastructure, but you get the point).

> I'd be -1 on a back-patch and -0.7 for HEAD.

I think we need a back-patch of *something*.  It's pure luck that wrasse
hasn't shown problems already.  I don't want to be rediscovering this
issue a year from now when somebody back-patches some test requiring
subtests.

            regards, tom lane

Noah Misch <noah@leadboat.com> writes:
> On Sat, Nov 20, 2021 at 08:22:14PM -0500, Tom Lane wrote:
>> I think we need a back-patch of *something*.  It's pure luck that wrasse
>> hasn't shown problems already.  I don't want to be rediscovering this
>> issue a year from now when somebody back-patches some test requiring
>> subtests.

> If you want to allow subtests in all branches, $SUBJECT is the thing needing a
> back-patch.

Agreed, but I think we're going to want to do that as soon as the dust
has settled.  We have plenty of precedent for back-patching test
infrastructure, and I don't see why this wouldn't qualify.

> By default, they remain banned in back-patches.

They're banned only in the sense that one or two old, slow buildfarm
animals will fail on them; the odds that anyone would notice the problem
before a patch reaches the buildfarm seem poor.  I'd just as soon not
set that trap for ourselves.  It's not like we expect testing with
obsolete Perl modules to be much of a concern in non-buildfarm usage,
even for consumers of back branches.

I also note that prairiedog will soon not be in the camp that fails,
even for back branches.  Neither will wrasse unless you invent some
truly strange configuration trick for it.  So the argument that there's
a back-branch ban on subtests doesn't seem like it will have much
enforcement behind it.

            regards, tom lane

On 11/20/21 20:10, Noah Misch wrote:
> On Sat, Nov 20, 2021 at 07:50:02PM -0500, Tom Lane wrote:
>> Noah Misch <noah@leadboat.com> writes:
>>> Yep.  wrasse sets PERL to a manually-installed Perl, but PROVE still uses an
>>> old Perl.  I'll fix that somehow.
>> A quick workaround is to set PROVE in the animal's config_env, but
>> I don't think that's ideal, because configure then skips module
>> presence tests:
>>
>>   # Check for necessary modules, unless user has specified the "prove" to use;
>>   # in that case it's her responsibility to have a working configuration.
>>   # (prove might be part of a different Perl installation than perl, eg on
>>   # MSys, so the result of AX_PROG_PERL_MODULES could be irrelevant anyway.)
> The skip would be unnecessary if configure just tested whether $PROVE can run
> a test requiring the module.  We're testing $PERL, but we're actually
> indifferent to $PERL's Test::More.



Yeah, we could do something along these lines:

    andrew@emma:pg_head $ cat moduletest.pl 
    use IPC::Run 0.79;
    use Test::More 0.98; 
    use Time::HiRes 1.52;  
    diag("");
    diag("Test::More::VERSION: $Test::More::VERSION"); 
    diag("IPC::Run::VERSION: $IPC::Run::VERSION"); 
    diag("Tme::HiRes::VERSION: $Time::HiRes::VERSION"); 
    ok(1); 
    done_testing();
    andrew@emma:pg_head $ prove moduletest.pl
    moduletest.pl .. # 
    # Test::More::VERSION: 1.302183
    # IPC::Run::VERSION: 20200505.0
    # Tme::HiRes::VERSION: 1.9764
    moduletest.pl .. ok   
    All tests successful.
    Files=1, Tests=1,  0 wallclock secs ( 0.04 usr  0.01 sys +  0.13 cusr  0.02 csys =  0.20 CPU)
    Result: PASS

If I hack up the test to fail I get:

    andrew@emma:pg_head $ prove moduletest.pl
    moduletest.pl .. Test::More version 9.98 required--this is only version 1.302183 at moduletest.pl line 2.
    BEGIN failed--compilation aborted at moduletest.pl line 2.
    moduletest.pl .. Dubious, test returned 255 (wstat 65280, 0xff00)
    No subtests run 
    Test Summary Report
    -------------------
    moduletest.pl (Wstat: 65280 Tests: 0 Failed: 0)
      Non-zero exit status: 255
      Parse errors: No plan found in TAP output
    Files=1, Tests=0,  0 wallclock secs ( 0.03 usr  0.01 sys +  0.12 cusr  0.02 csys =  0.18 CPU)
    Result: FAIL

which is clear enough.


prove is pretty much always a script - if we want to know which perl is
invoked we could look at its shebang line.


cheers


andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:
> On 11/20/21 20:10, Noah Misch wrote:
>> The skip would be unnecessary if configure just tested whether $PROVE can run
>> a test requiring the module.  We're testing $PERL, but we're actually
>> indifferent to $PERL's Test::More.

> Yeah, we could do something along these lines:
> [ script ]

Seems reasonable to me, although it's not entirely clear how to hook
the output into configure's practices --- in particular, people using
"./configure -q" might not be pleased by unwanted diagnostic output.
But that could probably be dealt with.

> prove is pretty much always a script - if we want to know which perl is
> invoked we could look at its shebang line.

Do we care though?

I think the $64 question is whether it's a great idea to run the
TAP tests with a different Perl than is used for (a) build tooling
and (b) building plperl against.  I guess that it should work in
principle, and the msys animals apparently need it, but I'm
concerned that somebody will waste a lot of time being confused by
unexpected behavioral differences.  ("This works when I run a perl
script by hand, why doesn't it work in my TAP test?")  So I still
think that the best default behavior is to pick a prove associated
with the selected perl, and if you really want something other than
that then you have to set PROVE explicitly.

IOW, I think we should make (and back-patch) both of the changes
discussed in this thread.

            regards, tom lane

I wrote:
> Seems reasonable to me, although it's not entirely clear how to hook
> the output into configure's practices --- in particular, people using
> "./configure -q" might not be pleased by unwanted diagnostic output.
> But that could probably be dealt with.

Here's a draft patch based on Andrew's script.  I'm not entirely
satisfied with it, because I couldn't get the output to go where
I wanted.  I think that the reports of actual module versions should
go to config.log, and not appear in configure's user-visible output,
because (a) 99% of people won't care, and (b) otherwise we don't have
a good way to silence them under -q, where 100% of people won't care.

However, the only way I could get that to happen was to redirect
prove's stderr to config.log (&AS_MESSAGE_LOG_FD), which is not great
because it means that in the failure case the only place where any useful
info appears is config.log.  I tried printing the version reports to
STDOUT instead of using diag(), but that seems to get redirected to
/dev/null somewhere.  Anybody know how to get prove to not do that?

            regards, tom lane

diff --git a/config/check_modules.pl b/config/check_modules.pl
new file mode 100644
index 0000000000..55b522c6a7
--- /dev/null
+++ b/config/check_modules.pl
@@ -0,0 +1,20 @@
+#
+# Verify that required Perl modules are available,
+# in at least the required minimum versions.
+# (The required minimum versions are all quite ancient now,
+# but specify them anyway for documentation's sake.)
+#
+use IPC::Run 0.79;
+# While here, we might as well report exactly what versions we found.
+diag("IPC::Run::VERSION: $IPC::Run::VERSION");
+
+# Test::More and Time::HiRes are supposed to be part of core Perl,
+# but some distros omit them in a minimal installation.
+use Test::More 0.98;
+diag("Test::More::VERSION: $Test::More::VERSION");
+
+use Time::HiRes 1.52;
+diag("Time::HiRes::VERSION: $Time::HiRes::VERSION");
+
+ok(1);
+done_testing();
diff --git a/configure b/configure
index 977b4d3df5..c58c9ca051 100755
--- a/configure
+++ b/configure
@@ -19410,98 +19410,7 @@ fi
 # Check for test tools
 #
 if test "$enable_tap_tests" = yes; then
-  # Check for necessary modules, unless user has specified the "prove" to use;
-  # in that case it's her responsibility to have a working configuration.
-  # (prove might be part of a different Perl installation than perl, eg on
-  # MSys, so the result of AX_PROG_PERL_MODULES could be irrelevant anyway.)
-  if test -z "$PROVE"; then
-    # Test::More and Time::HiRes are supposed to be part of core Perl,
-    # but some distros omit them in a minimal installation.
-    # The required minimum versions are all quite ancient now, but specify
-    # them anyway for documentation's sake.
-
-
-
-
-
-
-
-
-
-
-# Make sure we have perl
-if test -z "$PERL"; then
-# Extract the first word of "perl", so it can be a program name with args.
-set dummy perl; ac_word=$2
-{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5
-$as_echo_n "checking for $ac_word... " >&6; }
-if ${ac_cv_prog_PERL+:} false; then :
-  $as_echo_n "(cached) " >&6
-else
-  if test -n "$PERL"; then
-  ac_cv_prog_PERL="$PERL" # Let the user override the test.
-else
-as_save_IFS=$IFS; IFS=$PATH_SEPARATOR
-for as_dir in $PATH
-do
-  IFS=$as_save_IFS
-  test -z "$as_dir" && as_dir=.
-    for ac_exec_ext in '' $ac_executable_extensions; do
-  if as_fn_executable_p "$as_dir/$ac_word$ac_exec_ext"; then
-    ac_cv_prog_PERL="perl"
-    $as_echo "$as_me:${as_lineno-$LINENO}: found $as_dir/$ac_word$ac_exec_ext" >&5
-    break 2
-  fi
-done
-  done
-IFS=$as_save_IFS
-
-fi
-fi
-PERL=$ac_cv_prog_PERL
-if test -n "$PERL"; then
-  { $as_echo "$as_me:${as_lineno-$LINENO}: result: $PERL" >&5
-$as_echo "$PERL" >&6; }
-else
-  { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
-$as_echo "no" >&6; }
-fi
-
-
-fi
-
-if test "x$PERL" != x; then
-  ax_perl_modules_failed=0
-  for ax_perl_module in 'IPC::Run 0.79' 'Test::More 0.98' 'Time::HiRes 1.52' ; do
-    { $as_echo "$as_me:${as_lineno-$LINENO}: checking for perl module $ax_perl_module" >&5
-$as_echo_n "checking for perl module $ax_perl_module... " >&6; }
-
-    # Would be nice to log result here, but can't rely on autoconf internals
-    modversion=`$PERL -e "use $ax_perl_module; my \\\$x=q($ax_perl_module); \\\$x =~ s/ .*//; \\\$x .= q(::VERSION);
evalqq{print \\\\$\\\$x\\n}; exit;" 2>/dev/null` 
-    if test $? -ne 0; then
-      { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
-$as_echo "no" >&6; };
-      ax_perl_modules_failed=1
-   else
-      { $as_echo "$as_me:${as_lineno-$LINENO}: result: $modversion" >&5
-$as_echo "$modversion" >&6; };
-    fi
-  done
-
-  # Run optional shell commands
-  if test "$ax_perl_modules_failed" = 0; then
-    :
-
-  else
-    :
-    as_fn_error $? "Additional Perl modules are required to run TAP tests" "$LINENO" 5
-  fi
-else
-  { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: could not find perl" >&5
-$as_echo "$as_me: WARNING: could not find perl" >&2;}
-fi
-  fi
-  # Now make sure we know where prove is
+  # Make sure we know where prove is.
   if test -z "$PROVE"; then
   for ac_prog in prove
 do
@@ -19559,6 +19468,20 @@ fi
   if test -z "$PROVE"; then
     as_fn_error $? "prove not found" "$LINENO" 5
   fi
+  # Check for necessary Perl modules.  You might think we should use
+  # AX_PROG_PERL_MODULES here, but prove might be part of a different Perl
+  # installation than perl, eg on MSys, so we have to check using prove.
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for Perl modules required for TAP tests" >&5
+$as_echo_n "checking for Perl modules required for TAP tests... " >&6; }
+  "$PROVE" "$srcdir/config/check_modules.pl" >&5 2>&1
+  if test $? -eq 0; then
+    { $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5
+$as_echo "yes" >&6; }
+  else
+    { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
+$as_echo "no" >&6; }
+    as_fn_error $? "Additional Perl modules are required to run TAP tests" "$LINENO" 5
+  fi
 fi

 # If compiler will take -Wl,--as-needed (or various platform-specific
diff --git a/configure.ac b/configure.ac
index 95e5169c4f..90fed2f33b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2377,23 +2377,22 @@ PGAC_PATH_PROGS(DBTOEPUB, dbtoepub)
 # Check for test tools
 #
 if test "$enable_tap_tests" = yes; then
-  # Check for necessary modules, unless user has specified the "prove" to use;
-  # in that case it's her responsibility to have a working configuration.
-  # (prove might be part of a different Perl installation than perl, eg on
-  # MSys, so the result of AX_PROG_PERL_MODULES could be irrelevant anyway.)
-  if test -z "$PROVE"; then
-    # Test::More and Time::HiRes are supposed to be part of core Perl,
-    # but some distros omit them in a minimal installation.
-    # The required minimum versions are all quite ancient now, but specify
-    # them anyway for documentation's sake.
-    AX_PROG_PERL_MODULES([IPC::Run=0.79 Test::More=0.98 Time::HiRes=1.52], ,
-      [AC_MSG_ERROR([Additional Perl modules are required to run TAP tests])])
-  fi
-  # Now make sure we know where prove is
+  # Make sure we know where prove is.
   PGAC_PATH_PROGS(PROVE, prove)
   if test -z "$PROVE"; then
     AC_MSG_ERROR([prove not found])
   fi
+  # Check for necessary Perl modules.  You might think we should use
+  # AX_PROG_PERL_MODULES here, but prove might be part of a different Perl
+  # installation than perl, eg on MSys, so we have to check using prove.
+  AC_MSG_CHECKING(for Perl modules required for TAP tests)
+  "$PROVE" "$srcdir/config/check_modules.pl" >&AS_MESSAGE_LOG_FD 2>&1
+  if test $? -eq 0; then
+    AC_MSG_RESULT(yes)
+  else
+    AC_MSG_RESULT(no)
+    AC_MSG_ERROR([Additional Perl modules are required to run TAP tests])
+  fi
 fi

 # If compiler will take -Wl,--as-needed (or various platform-specific

On 11/21/21 12:46, Tom Lane wrote:
>
> However, the only way I could get that to happen was to redirect
> prove's stderr to config.log (&AS_MESSAGE_LOG_FD), which is not great
> because it means that in the failure case the only place where any useful
> info appears is config.log.  I tried printing the version reports to
> STDOUT instead of using diag(), but that seems to get redirected to
> /dev/null somewhere.  Anybody know how to get prove to not do that?
>
>             


No, but I think you can get what you want with the attached.


cheers


andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:
> On 11/21/21 12:46, Tom Lane wrote:
>> ... I tried printing the version reports to
>> STDOUT instead of using diag(), but that seems to get redirected to
>> /dev/null somewhere.  Anybody know how to get prove to not do that?

> No, but I think you can get what you want with the attached.

OK, it's a bit of a hack but it'll do.  (I feel some sympathy for
Andres trying to convert this to meson ... but that's his problem.)

            regards, tom lane

... btw, should we drop the now-unused config/ax_prog_perl_modules.m4 ?

I'm inclined to do so, on the grounds that we're not likely ever to
need it.  I doubt we'd agree to require a non-core Perl module as
part of either our build tooling or PL/Perl's dependencies.  Not
to mention that it'd go away anyway if we switch to meson.

            regards, tom lane