Re: [HACKERS] pgsql: Improve logging of TAP tests.
* 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.
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.
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.
* 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.
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.
* 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
