From: Alexander Lakhin <exclusion@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: make installcheck-world in a clean environment
Date: Thu, 2 Aug 2018 00:08:57 +0300
Message-ID: <39b7758a-f31a-b4f8-011a-446cc005b99a@gmail.com> (raw)
In-Reply-To: <5205.1533063699@sss.pgh.pa.us>
References: <bfee872b-ab16-ec7c-8f8a-b3949add1c1b@gmail.com>
<CA+TgmoafXiYMj6ROd09PxLFQ_dpurP1FJOL4ZwDonCkPeSubWg@mail.gmail.com>
<aff02bd6-20c9-0b6e-4e3c-a3428ce6568b@gmail.com>
<CA+TgmoZV08_6SXmag0hFkOurgWPjks6Jwyh+DeFXsCxLWLVjfw@mail.gmail.com>
<24834.1525712835@sss.pgh.pa.us>
<ec02a440-0346-4bdc-5118-0f7970d33c16@gmail.com>
<b36c96e5-05cf-aa0f-90a5-2276887170ce@2ndquadrant.com>
<beb142a3-aaf6-f6d7-2aae-69f7d65fb3d4@gmail.com>
<d50a2101-c8cc-1456-fd4c-c12edfa2549b@2ndquadrant.com>
<0401efec-68f1-679d-3ea3-21d4e8dd11af@gmail.com>
<15981.1532988967@sss.pgh.pa.us>
<26960.1532994120@sss.pgh.pa.us>
<8d6aebaa-d119-b702-f8a3-e0261e4cb586@gmail.com>
<5205.1533063699@sss.pgh.pa.us>
Hello Tom,
31.07.2018 22:01, Tom Lane wrote:
> Well, there's a lot of moving parts here, and it's not clear to me what
> makes the most sense. We potentially have the following things that
> ECPG "make check" generates or references in the build tree, but we might
> wish for "make installcheck" to use preinstalled versions of instead:
>
> 1. Server executables and other "installed" server support files.
> 2. PG cluster (datadir and running server).
> 3. pg_regress and libraries it depends on, such as libpq.
> 4. ecpg preprocessor.
> 5. ecpg's exported C header files, needed to compile test programs.
> 6. ecpg's libraries, used by test programs; also libpq.
>
> Currently, installcheck references an existing running cluster (#2)
> and therefore a fortiori #1 as well. That's fine. I believe we
> reference a local pg_regress and libraries (#3) in both cases,
> and that's also fine, because we don't install pg_regress at all.
> (Well, PGXS does, but ecpg tests shouldn't rely on that.)
> So it boils down to what to do about #4/#5/#6.
I would split 3 to:
3. client libraries, such as libpq.
7. pg_regress.
I missed the fact that pg_regress is installed in
lib/pgxs/src/test/regress/, and now it raises a question for me - why
this pg_regress instance should be used only for PGXS tests?
Leaving the question aside, I propose the fix to use the preinstalled
versions of 3-6.
> I suppose we could define "make installcheck" as referring only
> to using an installed server, not any ecpg support, in which
> case we don't need any big surgery to the ecpg makefiles.
> I'm not sure how consistent a definition that is, but maybe it
> makes sense by analogy to pg_regress.
>
> Another point here is that if you did "make check" and then
> "make installcheck", it presumably would not rebuild the test
> programs, meaning they'd still have been built using the local
> resources not the installed ones (or vice versa for the other
> order). So there's also a difference between "build" and
> "test execution", which seems relevant, and it's one we don't
> really have any make-target nomenclature to distinguish.
Yes, this is important question and I have no complete answer for now,
but maybe these test programs could be built as temporary?
> Given that you'd expect "make all" to use local copies of the
> build resources, perhaps there should be a separate target
> named along the lines of "make tests-from-install" that uses
> installed ecpg+other resources to compile the test programs.
If I understand you correctly, the attached patch does just this.
To illustrate it I also attached the log of "make -C
src/interfaces/ecpg; make installcheck" and the log of "make
installcheck" with patch applied (both procedures executed immediately
after "make clean").
> Anyway, as long as the installcheck target is dependent on "all" not
> something else, maybe it's fine as is. I'm definitely not sure what
> would be a consistent design for doing it differently.
I would propose the following design for "make installcheck":
1. For every thing that has installed version, use it.
2. For every thing that has no installed version and is not a subject to
build (e.g. .../t/*.sql), use it's version in the source tree.
3. For every thing that has no installed version and should be built
(e.g. src/interfaces/ecpg/test/connect/*.pgc), build one-time version
and use it.
Regarding to ecpg tests it means that we should use installed ecpg,
installed libs, installed *.h, and compile
src/interfaces/ecpg/test/connect/*.pgc into a temporary binary.
Maybe I miss some use cases, where existing design suits better, but in
general I think that the more test coverage (more installed things
checked), the better.
Best regards,
------
Alexander Lakhin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
[text/x-patch] make-checkinstall-master.patch (8.6K, ../39b7758a-f31a-b4f8-011a-446cc005b99a@gmail.com/3-make-checkinstall-master.patch)
download | inline diff:diff --git a/src/Makefile.global.in b/src/Makefile.global.inindex 91d7cb8..e23d95b 100644--- a/src/Makefile.global.in+++ b/src/Makefile.global.in@@ -244,7 +244,13 @@ CPPFLAGS = @CPPFLAGS@
override CPPFLAGS := $(ICU_CFLAGS) $(CPPFLAGS)
+ifdef USE_INSTALLED_ASSETS+USE_INCLUDEDIR = 1+endif
ifdef PGXS
+USE_INCLUDEDIR = 1+endif+ifdef USE_INCLUDEDIR
override CPPFLAGS := -I$(includedir_server) -I$(includedir_internal) $(CPPFLAGS)
else # not PGXS
override CPPFLAGS := -I$(top_srcdir)/src/include $(CPPFLAGS)
@@ -305,7 +311,13 @@ with_gnu_ld = @with_gnu_ld@
# "LDFLAGS := something" anywhere, ditto for LDFLAGS_INTERNAL.
# These initial assignments must be "=" type, and elsewhere we must only do
# "LDFLAGS += something" or "LDFLAGS_INTERNAL += something".
+ifdef USE_INSTALLED_ASSETS+ USE_LIBDIR = 1+endif
ifdef PGXS
+ USE_LIBDIR = 1+endif+ifdef USE_LIBDIR
LDFLAGS_INTERNAL = -L$(libdir)
else
LDFLAGS_INTERNAL = -L$(top_builddir)/src/port -L$(top_builddir)/src/common
diff --git a/src/interfaces/ecpg/Makefile b/src/interfaces/ecpg/Makefileindex 41460a1..d235059 100644--- a/src/interfaces/ecpg/Makefile+++ b/src/interfaces/ecpg/Makefile@@ -26,5 +26,8 @@ install-ecpglib-recurse: install-pgtypeslib-recurse
clean distclean maintainer-clean:
$(MAKE) -C test clean
-check checktcp installcheck:+check checktcp:
$(MAKE) -C test $@
++installcheck:+ USE_INSTALLED_ASSETS=1 $(MAKE) -C test $@diff --git a/src/interfaces/ecpg/test/Makefile.regress b/src/interfaces/ecpg/test/Makefile.regressindex 06c0461..1b803bc 100644--- a/src/interfaces/ecpg/test/Makefile.regress+++ b/src/interfaces/ecpg/test/Makefile.regress@@ -1,19 +1,26 @@
# This file is included into the Makefiles of subdirectories of ecpg/test/,
# so the file references have one more level of .. than you might expect.
-override CPPFLAGS := -I../../include -I$(top_srcdir)/src/interfaces/ecpg/include \- -I$(libpq_srcdir) $(CPPFLAGS)
override CFLAGS += $(PTHREAD_CFLAGS)
-LDFLAGS_INTERNAL += -L../../ecpglib -lecpg -L../../pgtypeslib -lpgtypes $(libpq)-
override LIBS += $(PTHREAD_LIBS)
-# Standard way to invoke the ecpg preprocessor+ifdef USE_INSTALLED_ASSETS+ECPG = '$(DESTDIR)$(bindir)/ecpg' --regression -I'$(DESTDIR)$(includedir)' \+ -I'$(DESTDIR)$(pkgincludedir)/informix/esql'+override CPPFLAGS := -I'$(DESTDIR)$(includedir)' \+ -I'$(DESTDIR)$(pkgincludedir)/informix/esql' $(CPPFLAGS)+LDFLAGS_INTERNAL += -lecpg -lpgtypes $(libpq)+else
ECPG = ../../preproc/ecpg --regression -I$(srcdir)/../../include -I$(srcdir)
+ECPG_DEP = ../../preproc/ecpg$(X)+override CPPFLAGS := -I../../include -I$(top_srcdir)/src/interfaces/ecpg/include \+ -I$(libpq_srcdir) $(CPPFLAGS)+LDFLAGS_INTERNAL += -L../../ecpglib -lecpg -L../../pgtypeslib -lpgtypes $(libpq)+endif
# Files that most or all ecpg preprocessor test outputs depend on
-ECPG_TEST_DEPENDENCIES = ../../preproc/ecpg$(X) \+ECPG_TEST_DEPENDENCIES = $(ECPG_DEP) \
$(srcdir)/../regression.h \
$(srcdir)/../../include/sqlca.h \
$(srcdir)/../../include/sqlda.h \
diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mkindex 158581b..3467841 100644--- a/src/makefiles/pgxs.mk+++ b/src/makefiles/pgxs.mk@@ -348,6 +348,7 @@ endif
# against installed postmaster
ifndef NO_INSTALLCHECK
+installcheck: export USE_INSTALLED_ASSETS=1
installcheck: submake $(REGRESS_PREP)
$(pg_regress_installcheck) $(REGRESS_OPTS) $(REGRESS)
endif
diff --git a/src/pl/plpgsql/src/Makefile b/src/pl/plpgsql/src/Makefileindex 25a5a9d..c3e87b4 100644--- a/src/pl/plpgsql/src/Makefile+++ b/src/pl/plpgsql/src/Makefile@@ -76,13 +76,16 @@ plerrcodes.h: $(top_srcdir)/src/backend/utils/errcodes.txt generate-plerrcodes.p
check: submake
$(pg_regress_check) $(REGRESS_OPTS) $(REGRESS)
-installcheck: submake+installcheck: installed_submake
$(pg_regress_installcheck) $(REGRESS_OPTS) $(REGRESS)
.PHONY: submake
submake:
$(MAKE) -C $(top_builddir)/src/test/regress pg_regress$(X)
+.PHONY: installed_submake+installed_submake:+ USE_INSTALLED_ASSETS=1 $(MAKE) -C $(top_builddir)/src/test/regress pg_regress$(X)
distprep: pl_gram.h pl_gram.c plerrcodes.h
diff --git a/src/test/isolation/Makefile b/src/test/isolation/Makefileindex c3c8280..ef90762 100644--- a/src/test/isolation/Makefile+++ b/src/test/isolation/Makefile@@ -9,12 +9,21 @@ subdir = src/test/isolation
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+# we need to build these libs only when we don't use installed ones+ifndef USE_INSTALLED_ASSETS+SUBMAKE_LIBPGPORT = submake-libpgport+SUBMAKE_LIBPQ = submake-libpq+endif+
override CPPFLAGS := -I$(srcdir) -I$(libpq_srcdir) -I$(srcdir)/../regress $(CPPFLAGS)
OBJS = specparse.o isolationtester.o $(WIN32RES)
all: isolationtester$(X) pg_isolation_regress$(X)
+installed_all:+ USE_INSTALLED_ASSETS=1 $(MAKE) all+
# Though we don't install these binaries, build them during installation
# (including temp-install). Otherwise, "make -j check-world" and "make -j
# installcheck-world" would spawn multiple, concurrent builds in this
@@ -31,7 +40,7 @@ pg_regress.o: | submake-regress
pg_isolation_regress$(X): isolation_main.o pg_regress.o $(WIN32RES)
$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
-isolationtester$(X): $(OBJS) | submake-libpq submake-libpgport+isolationtester$(X): $(OBJS) | $(SUBMAKE_LIBPQ) $(SUBMAKE_LIBPGPORT)
$(CC) $(CFLAGS) $^ $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
distprep: specparse.c specscanner.c
@@ -49,7 +58,7 @@ clean distclean:
maintainer-clean: distclean
rm -f specparse.c specscanner.c
-installcheck: all+installcheck: installed_all
$(pg_isolation_regress_installcheck) --schedule=$(srcdir)/isolation_schedule
check: all
diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefileindex 378bd01..ab24ecd 100644--- a/src/test/regress/GNUmakefile+++ b/src/test/regress/GNUmakefile@@ -28,6 +28,11 @@ EXTRADEFS = '-DHOST_TUPLE="$(host_tuple)"' \
'-DSHELLPROG="$(SHELL)"' \
'-DDLSUFFIX="$(DLSUFFIX)"'
+# we need to build libpgport only when we don't use installed libs+ifndef USE_INSTALLED_ASSETS+SUBMAKE_LIBPGPORT = submake-libpgport+endif+
##
## Prepare for tests
##
@@ -36,15 +41,19 @@ EXTRADEFS = '-DHOST_TUPLE="$(host_tuple)"' \
all: pg_regress$(X)
-pg_regress$(X): pg_regress.o pg_regress_main.o $(WIN32RES) | submake-libpgport+pg_regress$(X): pg_regress.o pg_regress_main.o $(WIN32RES) | $(SUBMAKE_LIBPGPORT)
$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
+.PHONY: installed_all+installed_all:+ USE_INSTALLED_ASSETS=1 $(MAKE) all+
# dependencies ensure that path changes propagate
pg_regress.o: pg_regress.c $(top_builddir)/src/port/pg_config_paths.h
pg_regress.o: override CPPFLAGS += -I$(top_builddir)/src/port $(EXTRADEFS)
# note: because of the submake dependency, this rule's action is really a no-op
-$(top_builddir)/src/port/pg_config_paths.h: | submake-libpgport+$(top_builddir)/src/port/pg_config_paths.h: | $(SUBMAKE_LIBPGPORT)
$(MAKE) -C $(top_builddir)/src/port pg_config_paths.h
install: all installdirs
@@ -67,7 +76,7 @@ include $(top_srcdir)/src/Makefile.shlib
all: all-lib
# Ensure parallel safety if a build is started in this directory
-$(OBJS): | submake-libpgport submake-generated-headers+$(OBJS): | $(SUBMAKE_LIBPGPORT) submake-generated-headers
# Test input and expected files. These are created by pg_regress itself, so we
# don't have a rule to create them. We do need rules to clean them however.
@@ -109,7 +118,7 @@ $(top_builddir)/contrib/spi/refint$(DLSUFFIX): | submake-contrib-spi ;
$(top_builddir)/contrib/spi/autoinc$(DLSUFFIX): | submake-contrib-spi ;
-submake-contrib-spi: | submake-libpgport submake-generated-headers+submake-contrib-spi: | $(SUBMAKE_LIBPGPORT) submake-generated-headers
$(MAKE) -C $(top_builddir)/contrib/spi
.PHONY: submake-contrib-spi
@@ -134,16 +143,16 @@ check: all tablespace-setup
check-tests: all tablespace-setup | temp-install
$(pg_regress_check) $(REGRESS_OPTS) $(MAXCONNOPT) $(TESTS) $(EXTRA_TESTS)
-installcheck: all tablespace-setup+installcheck: installed_all tablespace-setup
$(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/serial_schedule $(EXTRA_TESTS)
-installcheck-parallel: all tablespace-setup+installcheck-parallel: installed_all tablespace-setup
$(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
-installcheck-tests: all tablespace-setup+installcheck-tests: installed_all tablespace-setup
$(pg_regress_installcheck) $(REGRESS_OPTS) $(TESTS) $(EXTRA_TESTS)
-standbycheck: all+standbycheck: installed_all
$(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/standby_schedule --use-existing
# old interfaces follow...
[text/x-log] installcheck-patched.log (144.4K, ../39b7758a-f31a-b4f8-011a-446cc005b99a@gmail.com/4-installcheck-patched.log)
download
[text/x-log] installcheck.log (244.4K, ../39b7758a-f31a-b4f8-011a-446cc005b99a@gmail.com/5-installcheck.log)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: exclusion@gmail.com, tgl@sss.pgh.pa.us, peter.eisentraut@2ndquadrant.com, robertmhaas@gmail.com
Subject: Re: make installcheck-world in a clean environment
In-Reply-To: <39b7758a-f31a-b4f8-011a-446cc005b99a@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox