how to handle missing "prove"

Started by Peter Eisentrautabout 11 years ago12 messages

Peter Eisentraut

peter_e@gmx.net

about 11 years ago

1 attachment(s)

Here is a patch to use "missing" to handle the case when "prove" is not
present.

Other ideas?

Attachments:

missing-prove.patchtext/x-diff; name=missing-prove.patchDownload

diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index b04d005..aff9af7 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -311,13 +311,13 @@ $(if $(filter $(PORTNAME),darwin),DYLD_LIBRARY_PATH,$(if $(filter $(PORTNAME),ai
 endef
 
 define prove_installcheck
-cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
+$(if $(PROVE),cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl,-$(missing) prove)
 endef
 
 define prove_check
 $(MKDIR_P) tmp_check/log
 $(MAKE) -C $(top_builddir) DESTDIR='$(CURDIR)'/tmp_check/install install >'$(CURDIR)'/tmp_check/log/install.log 2>&1
-cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(CURDIR)/tmp_check/install$(bindir):$$PATH" $(call add_to_path,$(ld_library_path_var),$(CURDIR)/tmp_check/install$(libdir)) PGPORT='6$(DEF_PGPORT)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
+$(if $(PROVE),cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(CURDIR)/tmp_check/install$(bindir):$$PATH" $(call add_to_path,$(ld_library_path_var),$(CURDIR)/tmp_check/install$(libdir)) PGPORT='6$(DEF_PGPORT)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl,-$(missing) prove)
 endef
 
 # Installation.

Tom Lane

tgl@sss.pgh.pa.us

about 11 years ago

In reply to: Peter Eisentraut (#1)

Re: how to handle missing "prove"

Peter Eisentraut <peter_e@gmx.net> writes:

Here is a patch to use "missing" to handle the case when "prove" is not
present.

Wouldn't it be easier to do what we do for Perl, viz in Makefile.global.in

ifneq (@PERL@,)
# quoted to protect pathname with spaces
PERL = '@PERL@'
else
PERL = $(missing) perl
endif

However, with either of these approaches, "make check-world" gets a hard
failure if you lack "prove". Is that what we want? It's certainly not
very consistent with what you've been doing to make the tests just slide
by (rather than fail on) missing/too old Perl modules.

ISTM that the project policy for external components like this has been
"don't rely on them unless user says to use them, in which case fail if
they aren't present". So perhaps what we ought to have is a configure
switch along the lines of "--enable-tap-tests". If you don't specify it,
prove_check expands to nothing. If you do specify it, we fail if we
lack any of the expected support, both "prove" and whatever the agreed-on
set of Perl modules is.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Eisentraut

peter_e@gmx.net

about 11 years ago

In reply to: Tom Lane (#2)

Re: how to handle missing "prove"

On 10/28/14 9:16 PM, Tom Lane wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

Here is a patch to use "missing" to handle the case when "prove" is not
present.

Wouldn't it be easier to do what we do for Perl, viz in Makefile.global.in

ifneq (@PERL@,)
# quoted to protect pathname with spaces
PERL = '@PERL@'
else
PERL = $(missing) perl
endif

Yeah, maybe.

However, with either of these approaches, "make check-world" gets a hard
failure if you lack "prove". Is that what we want? It's certainly not
very consistent with what you've been doing to make the tests just slide
by (rather than fail on) missing/too old Perl modules.

The patch has

-$(missing) prove

and the - will make make ignore failures. Admittedly, that is very well
hidden.

ISTM that the project policy for external components like this has been
"don't rely on them unless user says to use them, in which case fail if
they aren't present". So perhaps what we ought to have is a configure
switch along the lines of "--enable-tap-tests". If you don't specify it,
prove_check expands to nothing. If you do specify it, we fail if we
lack any of the expected support, both "prove" and whatever the agreed-on
set of Perl modules is.

That's also a good idea.

(I might think of a different option name, because "TAP" is an output
format, not a piece of software. pg_regress could output TAP as well,
for example.)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andrew Dunstan

andrew@dunslane.net

about 11 years ago

In reply to: Tom Lane (#2)

Re: how to handle missing "prove"

On 10/28/2014 09:16 PM, Tom Lane wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

Here is a patch to use "missing" to handle the case when "prove" is not
present.

Wouldn't it be easier to do what we do for Perl, viz in Makefile.global.in

ifneq (@PERL@,)
# quoted to protect pathname with spaces
PERL = '@PERL@'
else
PERL = $(missing) perl
endif

However, with either of these approaches, "make check-world" gets a hard
failure if you lack "prove". Is that what we want? It's certainly not
very consistent with what you've been doing to make the tests just slide
by (rather than fail on) missing/too old Perl modules.

ISTM that the project policy for external components like this has been
"don't rely on them unless user says to use them, in which case fail if
they aren't present". So perhaps what we ought to have is a configure
switch along the lines of "--enable-tap-tests". If you don't specify it,
prove_check expands to nothing. If you do specify it, we fail if we
lack any of the expected support, both "prove" and whatever the agreed-on
set of Perl modules is.

If we go this way I'll add a tap icon to the buildfarm so you can see
which animals are running the tests.

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Eisentraut

peter_e@gmx.net

about 11 years ago

In reply to: Peter Eisentraut (#3)

1 attachment(s)

Re: how to handle missing "prove"

On 10/28/14 10:01 PM, Peter Eisentraut wrote:

On 10/28/14 9:16 PM, Tom Lane wrote:

ISTM that the project policy for external components like this has been
"don't rely on them unless user says to use them, in which case fail if
they aren't present". So perhaps what we ought to have is a configure
switch along the lines of "--enable-tap-tests". If you don't specify it,
prove_check expands to nothing. If you do specify it, we fail if we
lack any of the expected support, both "prove" and whatever the agreed-on
set of Perl modules is.

That's also a good idea.

Here is a patch.

Attachments:

0001-Add-configure-enable-tap-tests-option.patchtext/x-diff; name=0001-Add-configure-enable-tap-tests-option.patchDownload

>From 27b303938e4472c1a28f216978c847aed5750ced Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Thu, 30 Oct 2014 20:08:45 -0400
Subject: [PATCH] Add configure --enable-tap-tests option

---
 configure                      | 41 ++++++++++++++++++++++++++++++++++++++++-
 configure.in                   | 17 ++++++++++++++++-
 doc/src/sgml/installation.sgml | 10 ++++++++++
 src/Makefile.global.in         |  8 ++++++++
 4 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 1248b06..1b29be6 100755
--- a/configure
+++ b/configure
@@ -729,6 +729,7 @@ CPPFLAGS
 LDFLAGS
 CFLAGS
 CC
+enable_tap_tests
 enable_dtrace
 DTRACEFLAGS
 DTRACE
@@ -808,6 +809,7 @@ enable_debug
 enable_profiling
 enable_coverage
 enable_dtrace
+enable_tap_tests
 with_blocksize
 with_segsize
 with_wal_blocksize
@@ -1477,6 +1479,7 @@ Optional Features:
   --enable-profiling      build with profiling enabled
   --enable-coverage       build with coverage testing instrumentation
   --enable-dtrace         build with DTrace support
+  --enable-tap-tests      enable TAP tests (requires Perl)
   --enable-depend         turn on automatic dependency tracking
   --enable-cassert        enable assertion checks (for debugging)
   --disable-thread-safety disable thread-safety in client libraries
@@ -3466,6 +3469,34 @@ fi
 
 
 #
+# TAP tests
+#
+
+
+# Check whether --enable-tap-tests was given.
+if test "${enable_tap_tests+set}" = set; then :
+  enableval=$enable_tap_tests;
+  case $enableval in
+    yes)
+      :
+      ;;
+    no)
+      :
+      ;;
+    *)
+      as_fn_error $? "no argument expected for --enable-tap-tests option" "$LINENO" 5
+      ;;
+  esac
+
+else
+  enable_tap_tests=no
+
+fi
+
+
+
+
+#
 # Block size
 #
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking for block size" >&5
@@ -14785,7 +14816,8 @@ done
 #
 # Check for test tools
 #
-for ac_prog in prove
+if test "$enable_tap_tests" = yes; then
+  for ac_prog in prove
 do
   # Extract the first word of "$ac_prog", so it can be a program name with args.
 set dummy $ac_prog; ac_word=$2
@@ -14827,6 +14859,13 @@ fi
   test -n "$PROVE" && break
 done
 
+  if test -z "$PROVE"; then
+    as_fn_error $? "prove not found" "$LINENO" 5
+  fi
+  if test -z "$PERL"; then
+    as_fn_error $? "Perl not found" "$LINENO" 5
+  fi
+fi
 
 # Thread testing
 
diff --git a/configure.in b/configure.in
index 0a3725f..fe6f735 100644
--- a/configure.in
+++ b/configure.in
@@ -229,6 +229,13 @@ AC_SUBST(DTRACEFLAGS)])
 AC_SUBST(enable_dtrace)
 
 #
+# TAP tests
+#
+PGAC_ARG_BOOL(enable, tap-tests, no,
+              [enable TAP tests (requires Perl)])
+AC_SUBST(enable_tap_tests)
+
+#
 # Block size
 #
 AC_MSG_CHECKING([for block size])
@@ -1876,7 +1883,15 @@ AC_CHECK_PROGS(OSX, [osx sgml2xml sx])
 #
 # Check for test tools
 #
-AC_CHECK_PROGS(PROVE, prove)
+if test "$enable_tap_tests" = yes; then
+  AC_CHECK_PROGS(PROVE, prove)
+  if test -z "$PROVE"; then
+    AC_MSG_ERROR([prove not found])
+  fi
+  if test -z "$PERL"; then
+    AC_MSG_ERROR([Perl not found])
+  fi
+fi
 
 # Thread testing
 
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index 68931d2..adde5b3 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -1271,6 +1271,16 @@ <title>Configuration</title>
        </listitem>
       </varlistentry>
 
+      <varlistentry>
+       <term><option>--enable-tap-tests</option></term>
+       <listitem>
+        <para>
+         Enable tests using the Perl TAP tools.  This requires a Perl
+         installation and the Perl module <literal>IPC::Run</literal>.
+         See <xref linkend="regress-tap"> for more information.
+        </para>
+       </listitem>
+      </varlistentry>
      </variablelist>
     </para>
 
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index b04d005..63ff50b 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -174,6 +174,7 @@ enable_nls	= @enable_nls@
 enable_debug	= @enable_debug@
 enable_dtrace	= @enable_dtrace@
 enable_coverage	= @enable_coverage@
+enable_tap_tests	= @enable_tap_tests@
 enable_thread_safety	= @enable_thread_safety@
 
 python_enable_shared	= @python_enable_shared@
@@ -310,6 +311,8 @@ define ld_library_path_var
 $(if $(filter $(PORTNAME),darwin),DYLD_LIBRARY_PATH,$(if $(filter $(PORTNAME),aix),LIBPATH,LD_LIBRARY_PATH))
 endef
 
+ifeq ($(enable_tap_tests),yes)
+
 define prove_installcheck
 cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
 endef
@@ -320,6 +323,11 @@ $(MAKE) -C $(top_builddir) DESTDIR='$(CURDIR)'/tmp_check/install install >'$(CUR
 cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(CURDIR)/tmp_check/install$(bindir):$$PATH" $(call add_to_path,$(ld_library_path_var),$(CURDIR)/tmp_check/install$(libdir)) PGPORT='6$(DEF_PGPORT)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
 endef
 
+else
+prove_installcheck = @echo "TAP tests not enabled"
+prove_check = $(prove_installcheck)
+endif
+
 # Installation.
 
 install_bin = @install_bin@
-- 
2.1.2

Tom Lane

tgl@sss.pgh.pa.us

about 11 years ago

In reply to: Peter Eisentraut (#5)

Re: how to handle missing "prove"

Peter Eisentraut <peter_e@gmx.net> writes:

On 10/28/14 10:01 PM, Peter Eisentraut wrote:

On 10/28/14 9:16 PM, Tom Lane wrote:

ISTM that the project policy for external components like this has been
"don't rely on them unless user says to use them, in which case fail if
they aren't present". So perhaps what we ought to have is a configure
switch along the lines of "--enable-tap-tests". If you don't specify it,
prove_check expands to nothing. If you do specify it, we fail if we
lack any of the expected support, both "prove" and whatever the agreed-on
set of Perl modules is.

That's also a good idea.

Here is a patch.

Looks generally reasonable, but I thought you were planning to choose a
different option name?

One minor nitpick: perhaps the --help description of the option should
read

+ --enable-tap-tests enable TAP tests (requires Perl and IPC::Run)

because in practice it'll be much more likely that people will be missing
IPC::Run than that they'll be missing Perl altogether.

Also, shouldn't we have it fail rather than just skipping tests if
IPC::Run is missing?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Eisentraut

peter_e@gmx.net

about 11 years ago

In reply to: Tom Lane (#6)

Re: how to handle missing "prove"

On 10/30/14 9:09 PM, Tom Lane wrote:

Looks generally reasonable, but I thought you were planning to choose a
different option name?

Yeah, but I couldn't think of a better one. (Anything involving,
"enable-perl-..." would have been confusing with regard to PL/Perl.)

One minor nitpick: perhaps the --help description of the option should
read

+ --enable-tap-tests enable TAP tests (requires Perl and IPC::Run)

because in practice it'll be much more likely that people will be missing
IPC::Run than that they'll be missing Perl altogether.

Done.

Also, shouldn't we have it fail rather than just skipping tests if
IPC::Run is missing?

Done.

I was holding back on that pending the discussion on IPC::Cmd, but I
don't think that will happen anytime soon.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane

tgl@sss.pgh.pa.us

about 11 years ago

In reply to: Peter Eisentraut (#7)

Re: how to handle missing "prove"

Peter Eisentraut <peter_e@gmx.net> writes:

On 10/30/14 9:09 PM, Tom Lane wrote:

Looks generally reasonable, but I thought you were planning to choose a
different option name?

Yeah, but I couldn't think of a better one. (Anything involving,
"enable-perl-..." would have been confusing with regard to PL/Perl.)

Committed patch looks good, but should we also add the stanza we discussed
in Makefile.global.in concerning defining $(prove) in terms of "missing"
if we didn't find it? I think the behavior of HEAD when you ask for
--enable-tap-tests but don't have "prove" might be less than ideal.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Eisentraut

peter_e@gmx.net

about 11 years ago

In reply to: Tom Lane (#8)

Re: how to handle missing "prove"

On 11/2/14 11:36 AM, Tom Lane wrote:

Committed patch looks good, but should we also add the stanza we discussed
in Makefile.global.in concerning defining $(prove) in terms of "missing"
if we didn't find it? I think the behavior of HEAD when you ask for
--enable-tap-tests but don't have "prove" might be less than ideal.

configure will now fail when "prove" is not found.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10

Tom Lane

tgl@sss.pgh.pa.us

about 11 years ago

In reply to: Peter Eisentraut (#9)

Re: how to handle missing "prove"

Peter Eisentraut <peter_e@gmx.net> writes:

On 11/2/14 11:36 AM, Tom Lane wrote:

Committed patch looks good, but should we also add the stanza we discussed
in Makefile.global.in concerning defining $(prove) in terms of "missing"
if we didn't find it? I think the behavior of HEAD when you ask for
--enable-tap-tests but don't have "prove" might be less than ideal.

configure will now fail when "prove" is not found.

If there's a commit that goes with this statement, you neglected to push it...

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11

Peter Eisentraut

peter_e@gmx.net

about 11 years ago

In reply to: Tom Lane (#10)

Re: how to handle missing "prove"

On 11/3/14 3:11 PM, Tom Lane wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

On 11/2/14 11:36 AM, Tom Lane wrote:

Committed patch looks good, but should we also add the stanza we discussed
in Makefile.global.in concerning defining $(prove) in terms of "missing"
if we didn't find it? I think the behavior of HEAD when you ask for
--enable-tap-tests but don't have "prove" might be less than ideal.

configure will now fail when "prove" is not found.

If there's a commit that goes with this statement, you neglected to push it...

Are you not seeing this in configure.in:

#
# Check for test tools
#
if test "$enable_tap_tests" = yes; then
AC_CHECK_PROGS(PROVE, prove)
if test -z "$PROVE"; then
AC_MSG_ERROR([prove not found])
fi
if test -z "$PERL"; then
AC_MSG_ERROR([Perl not found])
fi
fi

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12

Tom Lane

tgl@sss.pgh.pa.us

about 11 years ago

In reply to: Peter Eisentraut (#11)

Re: how to handle missing "prove"

Peter Eisentraut <peter_e@gmx.net> writes:

On 11/3/14 3:11 PM, Tom Lane wrote:

If there's a commit that goes with this statement, you neglected to push it...

Are you not seeing this in configure.in:

AC_CHECK_PROGS(PROVE, prove)
if test -z "$PROVE"; then
AC_MSG_ERROR([prove not found])

Oh, duh. I read your message to mean that you'd just pushed a change
for that, not that it already did it. Nevermind ...

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers