Re: [HACKERS] pgsql: Improve logging of TAP tests.

2015-09-10 Thread Stephen Frost
* Stephen Frost (sfr...@snowman.net) wrote:
> Patch attached for review.  Barring objections, I'll commit this in a
> few hours.

Done.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pgsql: Improve logging of TAP tests.

2015-09-09 Thread Stephen Frost
Noah,

* Noah Misch (n...@leadboat.com) wrote:
> On Tue, Sep 08, 2015 at 02:58:36PM -0400, Stephen Frost wrote:
> > * Andrew Dunstan (and...@dunslane.net) wrote:
> > > Improve logging of TAP tests.
> > 
> > [...]
> > 
> > This broke 'make check' for REL9_4_STABLE with --enable-tap-tests
> > because it added a reference to 'with_temp_install' but didn't actually
> > define it.
> 
> The corresponding commits for HEAD (1ea0620) and 9.5 (fa4a4df) added just an
> "rm" invocation to that Makefile.  Commit ef57b98 had no occasion to do more;
> I suspect a merge accident.  Best to revert the extra change:
> 
> --- a/src/Makefile.global.in
> +++ b/src/Makefile.global.in
> @@ -357,5 +357,7 @@ endef
>  define prove_check
>  rm -rf $(CURDIR)/tmp_check/log
> -cd $(srcdir) && TESTDIR='$(CURDIR)' $(with_temp_install) 
> PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' $(PROVE) 
> $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
> +$(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)) 
> top_builddir='$(CURDIR)/$(top_builddir)' PGPORT='6$(DEF_PGPORT)' $(PROVE) 
> $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
>  endef

Yup, reverting mine and applying the above appears to work based on my
testing.

Patch attached for review.  Barring objections, I'll commit this in a
few hours.

Thanks!

Stephen
From 78a6aa0e7db6b8b4ac731119932b2878450972df Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Wed, 9 Sep 2015 08:31:35 -0400
Subject: [PATCH] Revert ed47666 and part of ef57b98

This reverts ed47666, which ended up adding a second tempoary
installation for all 'make check' runs, and reverts the part of
ef57b98 that changed the TAP 'prove_check' section, which appears to
have been unintentional to begin with on this branch.

Analysis and patch by Noah.
---
 src/Makefile.global.in | 22 +++---
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index bea05ad..eaa3ec4 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -59,7 +59,6 @@ endif
 endif
 else # not PGXS
 vpath_build = @vpath_build@
-abs_top_builddir = @abs_top_builddir@
 abs_top_srcdir = @abs_top_srcdir@
 
 ifneq ($(vpath_build),yes)
@@ -317,19 +316,6 @@ BZIP2	= bzip2
 
 # Testing
 
-check: temp-install
-
-.PHONY: temp-install
-temp-install:
-ifndef NO_TEMP_INSTALL
-ifeq ($(MAKELEVEL),0)
-	rm -rf '$(abs_top_builddir)'/tmp_install
-	$(MKDIR_P) '$(abs_top_builddir)'/tmp_install/log
-	$(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
-endif
-	$(if $(EXTRA_INSTALL),for extra in $(EXTRA_INSTALL); do $(MAKE) -C '$(top_builddir)'/$$extra DESTDIR='$(abs_top_builddir)'/tmp_install install >>'$(abs_top_builddir)'/tmp_install/log/install.log || exit; done)
-endif
-
 PROVE = @PROVE@
 PG_PROVE_FLAGS = -I $(top_srcdir)/src/test/perl/
 PROVE_FLAGS = --verbose
@@ -344,10 +330,6 @@ define ld_library_path_var
 $(if $(filter $(PORTNAME),darwin),DYLD_LIBRARY_PATH,$(if $(filter $(PORTNAME),aix),LIBPATH,LD_LIBRARY_PATH))
 endef
 
-define with_temp_install
-PATH="$(abs_top_builddir)/tmp_install$(bindir):$$PATH" $(call add_to_path,$(ld_library_path_var),$(abs_top_builddir)/tmp_install$(libdir))
-endef
-
 ifeq ($(enable_tap_tests),yes)
 
 define prove_installcheck
@@ -356,7 +338,9 @@ endef
 
 define prove_check
 rm -rf $(CURDIR)/tmp_check/log
-cd $(srcdir) && TESTDIR='$(CURDIR)' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
+$(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)) top_builddir='$(CURDIR)/$(top_builddir)' PGPORT='6$(DEF_PGPORT)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
 endef
 
 else
-- 
1.9.1



signature.asc
Description: Digital signature


Re: [HACKERS] pgsql: Improve logging of TAP tests.

2015-09-08 Thread Noah Misch
On Tue, Sep 08, 2015 at 02:58:36PM -0400, Stephen Frost wrote:
> * Andrew Dunstan (and...@dunslane.net) wrote:
> > Improve logging of TAP tests.
> 
> [...]
> 
> This broke 'make check' for REL9_4_STABLE with --enable-tap-tests
> because it added a reference to 'with_temp_install' but didn't actually
> define it.

The corresponding commits for HEAD (1ea0620) and 9.5 (fa4a4df) added just an
"rm" invocation to that Makefile.  Commit ef57b98 had no occasion to do more;
I suspect a merge accident.  Best to revert the extra change:

--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -357,5 +357,7 @@ endef
 define prove_check
 rm -rf $(CURDIR)/tmp_check/log
-cd $(srcdir) && TESTDIR='$(CURDIR)' $(with_temp_install) 
PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' $(PROVE) 
$(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
+$(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)) 
top_builddir='$(CURDIR)/$(top_builddir)' PGPORT='6$(DEF_PGPORT)' $(PROVE) 
$(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
 endef

> +check: temp-install
> +
> +.PHONY: temp-install
> +temp-install:
> +ifndef NO_TEMP_INSTALL
> +ifeq ($(MAKELEVEL),0)
> + rm -rf '$(abs_top_builddir)'/tmp_install
> + $(MKDIR_P) '$(abs_top_builddir)'/tmp_install/log
> + $(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install 
> install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
> +endif
> + $(if $(EXTRA_INSTALL),for extra in $(EXTRA_INSTALL); do $(MAKE) -C 
> '$(top_builddir)'/$$extra DESTDIR='$(abs_top_builddir)'/tmp_install install 
> >>'$(abs_top_builddir)'/tmp_install/log/install.log || exit; done)
> +endif

This made non-TAP "check" targets create two temporary installations.

Thanks,
nm


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


Re: [HACKERS] pgsql: Improve logging of TAP tests.

2015-09-08 Thread Stephen Frost
* Andrew Dunstan (and...@dunslane.net) wrote:
> On 09/08/2015 02:58 PM, Stephen Frost wrote:
> >Would be great to get feedback on it as I'm no Makefile expert (it took
> >me far too long to run down what was happening and work out what seemed
> >like the right fix..).
> 
> 
> Seems OK to me from a quick look - I guess the proof of the pudding
> is in the eating. Does every "make check" target that was working
> still work? I assume so. It's interesting that nobody has noticed
> this for about 6 weeks.

This only impacted individuals running the TAP tests on REL9_4_STABLE
using 'make check', which I'm guessing is a pretty small set (possibly
a set of 1).

make check-world works for me with this patch, I'm guessing that covers
all of the 'make check' cases which use the TAP tests, but even if it
doesn't, it's probably more than enough to be representative. 

I'm doing one final run with the latest changes to REL9_4_STABLE and
once those complete, assuming all still looks good, I'll go ahead and
push this.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pgsql: Improve logging of TAP tests.

2015-09-08 Thread Andrew Dunstan



On 09/08/2015 02:58 PM, Stephen Frost wrote:

* Andrew Dunstan (and...@dunslane.net) wrote:

Improve logging of TAP tests.

[...]

This broke 'make check' for REL9_4_STABLE with --enable-tap-tests
because it added a reference to 'with_temp_install' but didn't actually
define it.

The attached seems to fix it.

Would be great to get feedback on it as I'm no Makefile expert (it took
me far too long to run down what was happening and work out what seemed
like the right fix..).




Seems OK to me from a quick look - I guess the proof of the pudding is 
in the eating. Does every "make check" target that was working still 
work? I assume so. It's interesting that nobody has noticed this for 
about 6 weeks.


Our Makefiles do seem to have become fairly esoteric.

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


Re: [HACKERS] pgsql: Improve logging of TAP tests.

2015-09-08 Thread Stephen Frost
* Andrew Dunstan (and...@dunslane.net) wrote:
> Improve logging of TAP tests.

[...]

This broke 'make check' for REL9_4_STABLE with --enable-tap-tests
because it added a reference to 'with_temp_install' but didn't actually
define it.

The attached seems to fix it.

Would be great to get feedback on it as I'm no Makefile expert (it took
me far too long to run down what was happening and work out what seemed
like the right fix..).

Thanks!

Stephen
From 79630d53ff16499651a10ade78372c301052306c Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Tue, 8 Sep 2015 14:18:49 -0400
Subject: [PATCH] Add temp-check, with_temp_install definition - 9.4

Commit fa4a4df93c8c28d5684cacb1677fbd13f58bb9f2 attempted to backpatch
to 9.4 a change to improve the logging of TAP tests.  Unfortunately, due
to how 9.4 and 9.5 had diverged, the backpatch to 9.4 depended on a few
things which didn't exist in 9.4 (but did in 9.5), specifically, the
'temp-check' production and the 'with_temp_install' definition (which
also required 'abs_top_builddir').

Add these definitions into REL9_4_STABLE to allow the TAP tests to run
correctly under 'make check'.
---
 src/Makefile.global.in | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 13d4aa3..bea05ad 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -59,6 +59,7 @@ endif
 endif
 else # not PGXS
 vpath_build = @vpath_build@
+abs_top_builddir = @abs_top_builddir@
 abs_top_srcdir = @abs_top_srcdir@
 
 ifneq ($(vpath_build),yes)
@@ -316,6 +317,19 @@ BZIP2	= bzip2
 
 # Testing
 
+check: temp-install
+
+.PHONY: temp-install
+temp-install:
+ifndef NO_TEMP_INSTALL
+ifeq ($(MAKELEVEL),0)
+	rm -rf '$(abs_top_builddir)'/tmp_install
+	$(MKDIR_P) '$(abs_top_builddir)'/tmp_install/log
+	$(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
+endif
+	$(if $(EXTRA_INSTALL),for extra in $(EXTRA_INSTALL); do $(MAKE) -C '$(top_builddir)'/$$extra DESTDIR='$(abs_top_builddir)'/tmp_install install >>'$(abs_top_builddir)'/tmp_install/log/install.log || exit; done)
+endif
+
 PROVE = @PROVE@
 PG_PROVE_FLAGS = -I $(top_srcdir)/src/test/perl/
 PROVE_FLAGS = --verbose
@@ -330,6 +344,10 @@ define ld_library_path_var
 $(if $(filter $(PORTNAME),darwin),DYLD_LIBRARY_PATH,$(if $(filter $(PORTNAME),aix),LIBPATH,LD_LIBRARY_PATH))
 endef
 
+define with_temp_install
+PATH="$(abs_top_builddir)/tmp_install$(bindir):$$PATH" $(call add_to_path,$(ld_library_path_var),$(abs_top_builddir)/tmp_install$(libdir))
+endef
+
 ifeq ($(enable_tap_tests),yes)
 
 define prove_installcheck
-- 
1.9.1



signature.asc
Description: Digital signature