[bugfix]"make installcheck" could not work in PGXS

Started by matsumura.ryo@fujitsu.comover 5 years ago3 messages
#1matsumura.ryo@fujitsu.com
matsumura.ryo@fujitsu.com
1 attachment(s)

Hello,

I found that "make installcheck" could not work in PGXS.

--[/src/foo_project/Makefile]--
SUBDIRS = foo

TAP_TESTS = 1

PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)

include $(PGXS)

$(recurse)
$(recurse_always)

--[/src/foo_project/t/001_foo_test.pl]
use strict;
use warnings;
use PostgresNode;
use TestLib;

# Replace with the number of tests to execute:
use Test::More tests => 1;

my $node = PostgresNode->get_new_node('primary');
$node->init; ## --> Bailout called in PostgresNode.pm that refers PG_REGRESS environment variable.

--[log]--
cd /src/foo_project
make installcheck
:
:
rm -rf '/src/foo_project'/tmp_check
/bin/mkdir -p '/src/foo_project'/tmp_check
cd ./ && TESTDIR='/src/foo_project' PATH="/installdir/bin:$PATH" PGPORT='65432' top_builddir='/src/foo_project//installdir/lib/pgxs/src/makefiles/../..' PG_REGRESS='/src/foo_project//installdir/lib/pgxs/src/makefiles/../../src/test/regress/pg_regress' REGRESS_SHLIB='/src/test/regress/regress.so' /bin/prove -I /installdir/lib/pgxs/src/makefiles/../../src/test/perl/ -I ./ t/*.pl
t/001_foo_test.pl .... Bailout called. Further testing stopped: system /src/foo_project//installdir/lib/pgxs/src/makefiles/../../src/test/regress/pg_regress failed
FAILED--Further testing stopped: system /src/foo_project//installdir/lib/pgxs/src/makefiles/../../src/test/regress/pg_regress failed
make: *** [installcheck] Error 255

The cause is in [Makefile.global.in].
Althogh $(CURDIR) is '/src/foo_project' and $(top_builddir) is '/installdir/lib/pgxs',
the code concatenates them for setting PG_REGRESS.

``
define prove_installcheck
rm -rf '$(CURDIR)'/tmp_check
$(MKDIR_P) '$(CURDIR)'/tmp_check
cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' REGRESS_SHLIB='$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
endef
``

In non-PGXS environment, top_builddir is a relative path against the top of postgresql source tree.
But, in PGXS, top_builddir is a absolute path like /installdir/lib/pgxs/src/makefiles intentionally.
The existing code of [Makefile.global.in] does not consider it.

I make a patch. (It may not to be smart.)
Please your comments.

Regards
Ryo Matsumura

Attachments:

bug_pgxs_installcheck.v1.patchapplication/octet-stream; name=bug_pgxs_installcheck.v1.patchDownload
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 9a6265b..274f90c 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -444,11 +444,18 @@ with_temp_install = \

 ifeq ($(enable_tap_tests),yes)

+ifdef PGXS
+define prove_installcheck
+rm -rf '$(CURDIR)'/tmp_check
+cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(top_builddir)' PG_REGRESS='$(top_builddir)/src/test/regr
+endef
+else
 define prove_installcheck
 rm -rf '$(CURDIR)'/tmp_check
 $(MKDIR_P) '$(CURDIR)'/tmp_check
 cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='$(CURDIR)/$(top_bui
 endef
+endif

 define prove_check
 rm -rf '$(CURDIR)'/tmp_check
#2Michael Paquier
michael@paquier.xyz
In reply to: matsumura.ryo@fujitsu.com (#1)
Re: [bugfix]"make installcheck" could not work in PGXS

On Fri, Jul 31, 2020 at 08:31:56AM +0000, matsumura.ryo@fujitsu.com wrote:

I found that "make installcheck" could not work in PGXS.

Yeah, that's a known problem. One way to counter that is for example
to grab the path of pg_regress from pg_config --libdir and set
$ENV{PG_REGRESS} to it, but that's hacky. So I agree that it would be
good to do something.

In non-PGXS environment, top_builddir is a relative path against the top of postgresql source tree.
But, in PGXS, top_builddir is a absolute path like /installdir/lib/pgxs/src/makefiles intentionally.
The existing code of [Makefile.global.in] does not consider it.

I make a patch. (It may not to be smart.)
Please your comments.

Not sure that this goes completely to the right direction. It seems
to me that we should have room to set and use PG_REGRESS also for
pg_regress_check and pg_regress_installcheck.
--
Michael

#3matsumura.ryo@fujitsu.com
matsumura.ryo@fujitsu.com
In reply to: Michael Paquier (#2)
1 attachment(s)
RE: [bugfix]"make installcheck" could not work in PGXS

Hello,

On Fri, Aug 5, 2020 at 10:57:56 +0000, Michael Paquier <michael(at)paquier(dot)xyz> wrote:

Yeah, that's a known problem. One way to counter that is for example
to grab the path of pg_regress from pg_config --libdir and set
$ENV{PG_REGRESS} to it, but that's hacky. So I agree that it would be
good to do something.

Thank you.
I attach a new patch.

Not sure that this goes completely to the right direction. It seems
to me that we should have room to set and use PG_REGRESS also for
pg_regress_check and pg_regress_installcheck.

I understand that PG_REGRESS is an environment variable for each test program.
So I add a gmake variable PG_REGRESS_PATH.

The followings are other changings.
- Change REGRESS_SHLIB like as PG_REGRESS.
- Replace $(CURDIR)/$(top_builddir) to $(abs_top_builddir).
- Remove setting of environment variable 'top_builddir' in command line for prove_installcheck.
I wonder what I should set to it and then I remove it.
Because top_builddir is used for gmake genellaly not for test programs and PostgreSQL's test framework doesn't use it.
Is it going too far?

Regards
Ryo Matsumura

Attachments:

bug_pgxs_installcheck.v1.1.patchapplication/octet-stream; name=bug_pgxs_installcheck.v1.1.patchDownload
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 9a6265b..09ab8b5 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -446,14 +446,13 @@ ifeq ($(enable_tap_tests),yes)
 
 define prove_installcheck
 rm -rf '$(CURDIR)'/tmp_check
-$(MKDIR_P) '$(CURDIR)'/tmp_check
-cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' REGRESS_SHLIB='$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
+cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' PG_REGRESS='$(PG_REGRESS_PATH)' REGRESS_SHLIB='$(REGRESS_SHLIB_PATH)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
 endef
 
 define prove_check
 rm -rf '$(CURDIR)'/tmp_check
 $(MKDIR_P) '$(CURDIR)'/tmp_check
-cd $(srcdir) && TESTDIR='$(CURDIR)' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' REGRESS_SHLIB='$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
+cd $(srcdir) && TESTDIR='$(CURDIR)' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' PG_REGRESS='$(PG_REGRESS_PATH)' REGRESS_SHLIB='$(REGRESS_SHLIB_PATH)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
 endef
 
 else
@@ -636,6 +635,16 @@ ifdef TEMP_CONFIG
 TEMP_CONF += --temp-config=$(TEMP_CONFIG)
 endif
 
+# Environment variables for tap test.
+# top_builddir is an absolute path of root directory in PGXS.
+ifdef PGXS
+	PG_REGRESS_PATH = '$(top_builddir)/src/test/regress/pg_regress'
+	REGRESS_SHLIB_PATH = '$(top_builddir)/src/test/regress/regress$(DLSUFFIX)'
+else
+	PG_REGRESS_PATH = '$(abs_top_builddir)/src/test/regress/pg_regress'
+	REGRESS_SHLIB_PATH = '$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)'
+endif
+
 pg_regress_locale_flags = $(if $(ENCODING),--encoding=$(ENCODING)) $(NOLOCALE)
 pg_regress_clean_files = results/ regression.diffs regression.out tmp_check/ tmp_check_iso/ log/ output_iso/