libpq_pipeline in tmp_install
The test program libpq_pipeline produced by the test suite in
src/test/modules/libpq_pipeline/ is installed into tmp_install as part
of make check. This isn't a real problem by itself, but I think it
creates a bit of an asymmetric situation that might be worth cleaning up.
Before, the contents of tmp_install exactly matched an actual
installation. There were no extra test programs installed.
Also, the test suite code doesn't actually use that installed version,
so it's not of any use, and it creates confusion about which copy is in use.
The reason this is there is that the test suite uses PGXS to build the
test program, and so things get installed automatically. I suggest that
we should either write out the build system by hand to avoid this, or
maybe extend PGXS to support building programs but not installing them.
The advantage of the former approach is that it would allow additional
test programs to be added later as well. (We should really collect the
libpq tests under src/interfaces/libpq/ anyway at some point.)
On 10.05.21 20:26, Peter Eisentraut wrote:
The test program libpq_pipeline produced by the test suite in
src/test/modules/libpq_pipeline/ is installed into tmp_install as part
of make check. This isn't a real problem by itself, but I think it
creates a bit of an asymmetric situation that might be worth cleaning up.Before, the contents of tmp_install exactly matched an actual
installation. There were no extra test programs installed.Also, the test suite code doesn't actually use that installed version,
so it's not of any use, and it creates confusion about which copy is in
use.The reason this is there is that the test suite uses PGXS to build the
test program, and so things get installed automatically. I suggest that
we should either write out the build system by hand to avoid this, or
maybe extend PGXS to support building programs but not installing them.
Here is a patch that implements the second solution, which turned out to
be very easy.
Attachments:
0001-Add-NO_INSTALL-option-to-pgxs.patchtext/plain; charset=UTF-8; name=0001-Add-NO_INSTALL-option-to-pgxs.patch; x-mac-creator=0; x-mac-type=0Download
From 0c2f11034d66c072ffd2141bc5724c211ffb6ae8 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 19 May 2021 08:12:15 +0200
Subject: [PATCH] Add NO_INSTALL option to pgxs
Apply in libpq_pipeline test makefile, so that the test file is not
installed into tmp_install.
Discussion: https://www.postgresql.org/message-id/flat/cb9d16a6-760f-cd44-28d6-b091d5fb6ca7%40enterprisedb.com
---
src/makefiles/pgxs.mk | 4 ++++
src/test/modules/libpq_pipeline/Makefile | 2 ++
2 files changed, 6 insertions(+)
diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk
index 271e7eaba8..95d0441e9b 100644
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -227,6 +227,8 @@ all: all-lib
endif # MODULE_big
+ifndef NO_INSTALL
+
install: all installdirs
ifneq (,$(EXTENSION))
$(INSTALL_DATA) $(addprefix $(srcdir)/, $(addsuffix .control, $(EXTENSION))) '$(DESTDIR)$(datadir)/extension/'
@@ -336,6 +338,8 @@ endif # with_llvm
uninstall: uninstall-lib
endif # MODULE_big
+endif # NO_INSTALL
+
clean:
ifdef MODULES
diff --git a/src/test/modules/libpq_pipeline/Makefile b/src/test/modules/libpq_pipeline/Makefile
index b798f5fbbc..c9c5ae1beb 100644
--- a/src/test/modules/libpq_pipeline/Makefile
+++ b/src/test/modules/libpq_pipeline/Makefile
@@ -3,6 +3,8 @@
PROGRAM = libpq_pipeline
OBJS = libpq_pipeline.o
+NO_INSTALL = 1
+
PG_CPPFLAGS = -I$(libpq_srcdir)
PG_LIBS_INTERNAL += $(libpq_pgport)
--
2.31.1
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
On 10.05.21 20:26, Peter Eisentraut wrote:
The reason this is there is that the test suite uses PGXS to build the
test program, and so things get installed automatically. I suggest that
we should either write out the build system by hand to avoid this, or
maybe extend PGXS to support building programs but not installing them.
Here is a patch that implements the second solution, which turned out to
be very easy.
+1, except that you should add documentation for NO_INSTALL to the
list of definable symbols at the head of pgxs.mk, and to the list
in extend.sgml (compare that for NO_INSTALLCHECK).
regards, tom lane
On 2021-May-19, Tom Lane wrote:
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
On 10.05.21 20:26, Peter Eisentraut wrote:
The reason this is there is that the test suite uses PGXS to build the
test program, and so things get installed automatically.� I suggest that
we should either write out the build system by hand to avoid this, or
maybe extend PGXS to support building programs but not installing them.Here is a patch that implements the second solution, which turned out to
be very easy.
Great, thank you.
+1, except that you should add documentation for NO_INSTALL to the
list of definable symbols at the head of pgxs.mk, and to the list
in extend.sgml (compare that for NO_INSTALLCHECK).
I propose this.
--
�lvaro Herrera Valdivia, Chile
Attachments:
noinstall-doc.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml
index ec95b4eb01..1a4f208692 100644
--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -1672,6 +1672,16 @@ include $(PGXS)
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><varname>NO_INSTALL</varname></term>
+ <listitem>
+ <para>
+ don't define an <literal>install</literal> target, useful for test
+ modules that don't need their build products to be installed.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><varname>NO_INSTALLCHECK</varname></term>
<listitem>
diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk
index 271e7eaba8..224eac069c 100644
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -49,6 +49,8 @@
# TAP_TESTS -- switch to enable TAP tests
# ISOLATION -- list of isolation test cases
# ISOLATION_OPTS -- additional switches to pass to pg_isolation_regress
+# NO_INSTALL -- don't define an install target, useful for test modules
+# that don't need their build products to be installed
# NO_INSTALLCHECK -- don't define an installcheck target, useful e.g. if
# tests require special configuration, or don't use pg_regress
# EXTRA_CLEAN -- extra files to remove in 'make clean'
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
On 2021-May-19, Tom Lane wrote:
+1, except that you should add documentation for NO_INSTALL to the
list of definable symbols at the head of pgxs.mk, and to the list
in extend.sgml (compare that for NO_INSTALLCHECK).
I propose this.
WFM.
regards, tom lane
On 19.05.21 19:35, Tom Lane wrote:
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
On 2021-May-19, Tom Lane wrote:
+1, except that you should add documentation for NO_INSTALL to the
list of definable symbols at the head of pgxs.mk, and to the list
in extend.sgml (compare that for NO_INSTALLCHECK).I propose this.
WFM.
Thanks for the feedback. I found that my proposal doesn't quite work,
because "check" doesn't depend on "all" (anymore; see dbf2ec1a1c0), so
running make check-world doesn't build the test program first. The
easiest workaround I found was to add an "install: all" line even for
the NO_INSTALL case. It's all a bit hackish, though.
Attachments:
v2-0001-Add-NO_INSTALL-option-to-pgxs.patchtext/plain; charset=UTF-8; name=v2-0001-Add-NO_INSTALL-option-to-pgxs.patch; x-mac-creator=0; x-mac-type=0Download
From 24946ef3c8118b88b8df04d6bab051d0b3b20420 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 25 May 2021 11:01:58 +0200
Subject: [PATCH v2] Add NO_INSTALL option to pgxs
Apply in libpq_pipeline test makefile, so that the test file is not
installed into tmp_install.
Reviewed-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/cb9d16a6-760f-cd44-28d6-b091d5fb6ca7%40enterprisedb.com
---
doc/src/sgml/extend.sgml | 10 ++++++++++
src/makefiles/pgxs.mk | 13 +++++++++++++
src/test/modules/libpq_pipeline/Makefile | 2 ++
3 files changed, 25 insertions(+)
diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml
index ec95b4eb01..dae5737574 100644
--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -1672,6 +1672,16 @@ <title>Extension Building Infrastructure</title>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><varname>NO_INSTALL</varname></term>
+ <listitem>
+ <para>
+ don't define an <literal>install</literal> target, useful for test
+ modules that don't need their build products to be installed
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><varname>NO_INSTALLCHECK</varname></term>
<listitem>
diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk
index 271e7eaba8..0f71fa293d 100644
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -49,6 +49,8 @@
# TAP_TESTS -- switch to enable TAP tests
# ISOLATION -- list of isolation test cases
# ISOLATION_OPTS -- additional switches to pass to pg_isolation_regress
+# NO_INSTALL -- don't define an install target, useful for test modules
+# that don't need their build products to be installed
# NO_INSTALLCHECK -- don't define an installcheck target, useful e.g. if
# tests require special configuration, or don't use pg_regress
# EXTRA_CLEAN -- extra files to remove in 'make clean'
@@ -227,6 +229,8 @@ all: all-lib
endif # MODULE_big
+ifndef NO_INSTALL
+
install: all installdirs
ifneq (,$(EXTENSION))
$(INSTALL_DATA) $(addprefix $(srcdir)/, $(addsuffix .control, $(EXTENSION))) '$(DESTDIR)$(datadir)/extension/'
@@ -336,6 +340,15 @@ endif # with_llvm
uninstall: uninstall-lib
endif # MODULE_big
+else # NO_INSTALL
+
+# Need this so that temp-install builds artifacts not meant for
+# installation (Normally, check should depend on all, but we don't do
+# that because of parallel make risk (dbf2ec1a1c0).)
+install: all
+
+endif # NO_INSTALL
+
clean:
ifdef MODULES
diff --git a/src/test/modules/libpq_pipeline/Makefile b/src/test/modules/libpq_pipeline/Makefile
index b798f5fbbc..c9c5ae1beb 100644
--- a/src/test/modules/libpq_pipeline/Makefile
+++ b/src/test/modules/libpq_pipeline/Makefile
@@ -3,6 +3,8 @@
PROGRAM = libpq_pipeline
OBJS = libpq_pipeline.o
+NO_INSTALL = 1
+
PG_CPPFLAGS = -I$(libpq_srcdir)
PG_LIBS_INTERNAL += $(libpq_pgport)
--
2.31.1