Thread: Re: pgsql: Improve logging of TAP tests.

* Andrew Dunstan (andrew@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

On 09/08/2015 02:58 PM, Stephen Frost wrote:
> * Andrew Dunstan (andrew@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

* Andrew Dunstan (andrew@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

On Tue, Sep 08, 2015 at 02:58:36PM -0400, Stephen Frost wrote:
> * Andrew Dunstan (andrew@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 @@ endefdefine prove_checkrm -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/*.plendef
 

> +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.log2>&1
 
> +endif
> +    $(if $(EXTRA_INSTALL),for extra in $(EXTRA_INSTALL); do $(MAKE) -C '$(top_builddir)'/$$extra
DESTDIR='$(abs_top_builddir)'/tmp_installinstall >>'$(abs_top_builddir)'/tmp_install/log/install.log || exit; done)
 
> +endif

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

Thanks,
nm

Noah,

* Noah Misch (noah@leadboat.com) wrote:
> On Tue, Sep 08, 2015 at 02:58:36PM -0400, Stephen Frost wrote:
> > * Andrew Dunstan (andrew@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

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

Done.

Thanks!

Stephen