Thread: Should contrib modules install .h files?
So I have this immediate problem: a PGXS build of a module, specifically an hstore transform for a non-core PL, is much harder than it should be because it has no way to get at hstore.h since that file is never installed anywhere. Should that be changed? -- Andrew (irc:RhodiumToad)
Hi, On 2018-07-01 19:23:03 +0100, Andrew Gierth wrote: > So I have this immediate problem: a PGXS build of a module, specifically > an hstore transform for a non-core PL, is much harder than it should be > because it has no way to get at hstore.h since that file is never > installed anywhere. > > Should that be changed? I've hit this before, and the more capable our extension framework and more complex individual extensions get, the more we'll hit this. One question is where to install them - the extensions with headers often just have them in the source code itself, which isn't a great solution. contrib/$extension/ or $extension/ both seems ok, but i'd be somewhat against just $extension.h Greetings, Andres Freund
On 01.07.18 20:23, Andrew Gierth wrote: > So I have this immediate problem: a PGXS build of a module, specifically > an hstore transform for a non-core PL, is much harder than it should be > because it has no way to get at hstore.h since that file is never > installed anywhere. > > Should that be changed? Yes, just install it, I think. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> So I have this immediate problem: a PGXS build of a module,
>> specifically an hstore transform for a non-core PL, is much harder
>> than it should be because it has no way to get at hstore.h since
>> that file is never installed anywhere.
>>
>> Should that be changed?
Peter> Yes, just install it, I think.
I'm thinking that $(includedir_server)/$(MODULEDIR) would be a
reasonable place? MODULEDIR defaults to either "contrib" or "extension"
depending on whether EXTENSION is set.
Something like the attached patch seem reasonable?
--
Andrew (irc:RhodiumToad)
diff --git a/contrib/hstore/Makefile b/contrib/hstore/Makefile
index ab7fef3979..470474b062 100644
--- a/contrib/hstore/Makefile
+++ b/contrib/hstore/Makefile
@@ -11,6 +11,8 @@ DATA = hstore--1.4.sql hstore--1.4--1.5.sql \
hstore--unpackaged--1.0.sql
PGFILEDESC = "hstore - key/value pair data type"
+INCLUDES = hstore.h
+
REGRESS = hstore
ifdef USE_PGXS
diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk
index bfae02f90c..f6662be2b8 100644
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -38,6 +38,7 @@
# SCRIPTS -- script files (not binaries) to install into $PREFIX/bin
# SCRIPTS_built -- script files (not binaries) to install into $PREFIX/bin,
# which need to be built first
+# INCLUDES -- files to install into $PREFIX/include/server/$MODULEDIR
# REGRESS -- list of regression test cases (without suffix)
# REGRESS_OPTS -- additional switches to pass to pg_regress
# NO_INSTALLCHECK -- don't define an installcheck target, useful e.g. if
@@ -94,13 +95,16 @@ endif
ifdef MODULEDIR
datamoduledir := $(MODULEDIR)
docmoduledir := $(MODULEDIR)
+incmoduledir := $(MODULEDIR)
else
ifdef EXTENSION
datamoduledir := extension
docmoduledir := extension
+incmoduledir := extension
else
datamoduledir := contrib
docmoduledir := contrib
+incmoduledir := contrib
endif
endif
@@ -154,6 +158,9 @@ endif # SCRIPTS
ifdef SCRIPTS_built
$(INSTALL_SCRIPT) $(SCRIPTS_built) '$(DESTDIR)$(bindir)/'
endif # SCRIPTS_built
+ifdef INCLUDES
+ $(INSTALL_DATA) $(addprefix $(srcdir)/, $(INCLUDES)) '$(DESTDIR)$(includedir_server)/$(incmoduledir)/'
+endif # INCLUDES
ifdef MODULE_big
ifeq ($(with_llvm), yes)
$(call install_llvm_module,$(MODULE_big),$(OBJS))
@@ -184,6 +191,9 @@ endif # DOCS
ifneq (,$(PROGRAM)$(SCRIPTS)$(SCRIPTS_built))
$(MKDIR_P) '$(DESTDIR)$(bindir)'
endif
+ifdef INCLUDES
+ $(MKDIR_P) '$(DESTDIR)$(includedir_server)/$(incmoduledir)'
+endif
ifdef MODULE_big
installdirs: installdirs-lib
@@ -218,6 +228,9 @@ endif
ifdef SCRIPTS_built
rm -f $(addprefix '$(DESTDIR)$(bindir)'/, $(SCRIPTS_built))
endif
+ifdef INCLUDES
+ rm -f $(addprefix '$(DESTDIR)$(includedir_server)/$(incmoduledir)'/, $(INCLUDES))
+endif
ifdef MODULE_big
ifeq ($(with_llvm), yes)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> I'm thinking that $(includedir_server)/$(MODULEDIR) would be a
> reasonable place? MODULEDIR defaults to either "contrib" or "extension"
> depending on whether EXTENSION is set.
> Something like the attached patch seem reasonable?
FWIW, I agree with Andres' thought that each contrib module should have
its own subdirectory under $(includedir_server). Otherwise we're going
to be faced with questions about whether .h files need to be renamed
because they're not globally unique enough. There are already some that
are pretty shaky from this standpoint:
contrib$ ls */*.h
bloom/bloom.h pg_trgm/trgm.h
btree_gist/btree_gist.h pgcrypto/blf.h
btree_gist/btree_utils_num.h pgcrypto/imath.h
btree_gist/btree_utils_var.h pgcrypto/mbuf.h
cube/cubedata.h pgcrypto/md5.h
hstore/hstore.h pgcrypto/pgcrypto.h
intarray/_int.h pgcrypto/pgp.h
isn/EAN13.h pgcrypto/px-crypt.h
isn/ISBN.h pgcrypto/px.h
isn/ISMN.h pgcrypto/rijndael.h
isn/ISSN.h pgcrypto/sha1.h
isn/UPC.h postgres_fdw/postgres_fdw.h
isn/isn.h seg/segdata.h
ltree/crc32.h sepgsql/sepgsql.h
ltree/ltree.h tablefunc/tablefunc.h
pageinspect/pageinspect.h
Not sure about whether the MODULEDIR part is useful. Seems like
making a distinction between extensions and other contrib is
likely to create more headaches than it avoids.
BTW, it's somewhat interesting to think about whether we ought to
change the coding conventions so that extensions refer to their
own headers with a subdirectory, e.g., #include "bloom/bloom.h".
Having done that, all of contrib could build with a single
centrally-provided -I switch pointing at BUILDDIR/contrib/,
and there would be a path to allowing the code to build out of
tree by pointing that common -I at $(includedir_server)/ or
$(includedir_server)/MODULEDIR. This seems like it could be
a lot less messy as we accrete more cross-module references.
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: > Andrew Gierth <andrew@tao11.riddles.org.uk> writes: >> I'm thinking that $(includedir_server)/$(MODULEDIR) would be a >> reasonable place? MODULEDIR defaults to either "contrib" or >> "extension" depending on whether EXTENSION is set. Something like >> the attached patch seem reasonable? Tom> FWIW, I agree with Andres' thought that each contrib module should Tom> have its own subdirectory under $(includedir_server). Otherwise Tom> we're going to be faced with questions about whether .h files need Tom> to be renamed because they're not globally unique enough. There Tom> are already some that are pretty shaky from this standpoint: I'm not suggesting that all modules should install a .h file or that all of a module's .h files should be installed. A slight snag in trying to use a subdir for each module is that there is not in fact anywhere in the existing makefiles that uses or assigns such a name. Indeed some contrib subdirs install multiple modules. Tom> Not sure about whether the MODULEDIR part is useful. Seems like Tom> making a distinction between extensions and other contrib is Tom> likely to create more headaches than it avoids. Sure, but that's just copied from DATA and DOCS which already do it that way. For DATA there seems some justification based on CREATE EXTENSION, but for docs? -- Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> Tom> FWIW, I agree with Andres' thought that each contrib module should
> Tom> have its own subdirectory under $(includedir_server). Otherwise
> Tom> we're going to be faced with questions about whether .h files need
> Tom> to be renamed because they're not globally unique enough. There
> Tom> are already some that are pretty shaky from this standpoint:
> I'm not suggesting that all modules should install a .h file or that all
> of a module's .h files should be installed.
I agree with that, which implies the need for a new macro comparable to
DATA and DOCS that lists the .h files to be installed.
> A slight snag in trying to use a subdir for each module is that there is
> not in fact anywhere in the existing makefiles that uses or assigns such
> a name. Indeed some contrib subdirs install multiple modules.
So, given that we have to add something to the module makefiles anyway,
we could also add a macro specifying the subdirectory name to use.
(Although in practice this should always be equal to the contrib/
subdirectory name, so maybe we could extract it on that basis?)
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> A slight snag in trying to use a subdir for each module is that >> there is not in fact anywhere in the existing makefiles that uses or >> assigns such a name. Indeed some contrib subdirs install multiple >> modules. Tom> So, given that we have to add something to the module makefiles Tom> anyway, we could also add a macro specifying the subdirectory name Tom> to use. (Although in practice this should always be equal to the Tom> contrib/ subdirectory name, so maybe we could extract it on that Tom> basis?) Using the subdir name may work for in-tree contrib/ builds but it's not so good for PGXS, which should not be making assumptions about the build directory name. How about this: it's most likely that modules that install include files will also be using MODULE_big, so use that as the default name; if a makefile that uses only MODULES also wants to install include files, have it define MODULE_NAME (or some such variable) itself. -- Andrew (irc:RhodiumToad)
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: Tom> BTW, it's somewhat interesting to think about whether we ought to Tom> change the coding conventions so that extensions refer to their Tom> own headers with a subdirectory, e.g., #include "bloom/bloom.h". Tom> Having done that, all of contrib could build with a single Tom> centrally-provided -I switch pointing at BUILDDIR/contrib/, and Tom> there would be a path to allowing the code to build out of tree by Tom> pointing that common -I at $(includedir_server)/ or Tom> $(includedir_server)/MODULEDIR. This seems like it could be a lot Tom> less messy as we accrete more cross-module references. I'm slightly skeptical of this because it could cause unexpected issues when you rebuild (especially in the PGXS case) a module that has already been installed; without care, you'd end up getting the module's own headers from the installed version rather than the one being built, which would be very bad. -- Andrew (irc:RhodiumToad)
On 2018-07-02 16:11:07 +0100, Andrew Gierth wrote: > >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: > > >> A slight snag in trying to use a subdir for each module is that > >> there is not in fact anywhere in the existing makefiles that uses or > >> assigns such a name. Indeed some contrib subdirs install multiple > >> modules. > > Tom> So, given that we have to add something to the module makefiles > Tom> anyway, we could also add a macro specifying the subdirectory name > Tom> to use. (Although in practice this should always be equal to the > Tom> contrib/ subdirectory name, so maybe we could extract it on that > Tom> basis?) > > Using the subdir name may work for in-tree contrib/ builds but it's not > so good for PGXS, which should not be making assumptions about the build > directory name. > > How about this: it's most likely that modules that install include files > will also be using MODULE_big, so use that as the default name; if a > makefile that uses only MODULES also wants to install include files, > have it define MODULE_NAME (or some such variable) itself. For LLVM bitcode generation I made it so MODULE_big uses that as the directory name, whereas MODULES just iterates over the components. Now for the bitcode there was less need to make it selective, so it's a bit easier. We could just define that HEADERS_$component is referenced for each component in MODULES or something like that? Greetings, Andres Freund
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> Tom> So, given that we have to add something to the module makefiles
> Tom> anyway, we could also add a macro specifying the subdirectory name
> Tom> to use. (Although in practice this should always be equal to the
> Tom> contrib/ subdirectory name, so maybe we could extract it on that
> Tom> basis?)
> Using the subdir name may work for in-tree contrib/ builds but it's not
> so good for PGXS, which should not be making assumptions about the build
> directory name.
Fair point.
> How about this: it's most likely that modules that install include files
> will also be using MODULE_big, so use that as the default name; if a
> makefile that uses only MODULES also wants to install include files,
> have it define MODULE_NAME (or some such variable) itself.
AFAIR, you're supposed to define at most one of those macros anyway,
so I don't see why it couldn't be like "use MODULE_big if set, else
use MODULE if set, else fail if MODULE_NAME isn't set".
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> How about this: it's most likely that modules that install include >> files will also be using MODULE_big, so use that as the default >> name; if a makefile that uses only MODULES also wants to install >> include files, have it define MODULE_NAME (or some such variable) >> itself. Tom> AFAIR, you're supposed to define at most one of those macros Tom> anyway, so I don't see why it couldn't be like "use MODULE_big if Tom> set, else use MODULE if set, else fail if MODULE_NAME isn't set". OK, I'm working on an updated patch, that will allow this: if using MODULE_big: MODULE_big = mymodule HEADERS = whatever.h # gets installed as mymodule/whatever.h in whatever dir we decide on # also works if you define HEADERS_mymodule = whatever.h if not using MODULE_big: MODULES = foo bar baz HEADERS_foo = foo.h HEADERS_bar = bar.h # baz doesn't have any headers # foo.h installed as foo/foo.h # bar.h installed as bar/bar.h Two questions arise: 1) include/server has a lot of files and subdirs, so using include/server/$(MODULE)/ looks likely to be error-prone. So it should be something like include/server/contrib/$(MODULE)/ or include/server/extension/$(MODULE)/. Which one, or should it use $(MODULEDIR) to choose between the two the way that DATA and DOCS do? Or something else? 2) Specifying HEADERS_blah for some name "blah" that's not listed in MODULES or MODULE_big should do what: a) install into blah/ anyway b) be ignored with a warning c) be silently ignored d) be an error -- Andrew (irc:RhodiumToad)
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
Andrew> OK, I'm working on an updated patch
and here it is.
This installs to $(includedir_server)/$(MODULEDIR)/$MODULE/file.h
(e.g. include/server/extension/hstore/hstore.h for an actual example),
and errors if HEADERS_xxx is defined for anything that's not a module
listed in MODULES or MODULE_big.
--
Andrew (irc:RhodiumToad)
diff --git a/contrib/hstore/Makefile b/contrib/hstore/Makefile
index ab7fef3979..46d26f8052 100644
--- a/contrib/hstore/Makefile
+++ b/contrib/hstore/Makefile
@@ -11,6 +11,8 @@ DATA = hstore--1.4.sql hstore--1.4--1.5.sql \
hstore--unpackaged--1.0.sql
PGFILEDESC = "hstore - key/value pair data type"
+HEADERS = hstore.h
+
REGRESS = hstore
ifdef USE_PGXS
diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk
index bfae02f90c..3e7817931e 100644
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -38,6 +38,10 @@
# SCRIPTS -- script files (not binaries) to install into $PREFIX/bin
# SCRIPTS_built -- script files (not binaries) to install into $PREFIX/bin,
# which need to be built first
+# HEADERS -- files to install into $(includedir_server)/$MODULEDIR/$MODULE_big
+# HEADERS_$(MODULE) -- files to install into
+# $(includedir_server)/$MODULEDIR/$MODULE; the value of $MODULE must be
+# listed in MODULES or MODULE_big
# REGRESS -- list of regression test cases (without suffix)
# REGRESS_OPTS -- additional switches to pass to pg_regress
# NO_INSTALLCHECK -- don't define an installcheck target, useful e.g. if
@@ -94,13 +98,16 @@ endif
ifdef MODULEDIR
datamoduledir := $(MODULEDIR)
docmoduledir := $(MODULEDIR)
+incmoduledir := $(MODULEDIR)
else
ifdef EXTENSION
datamoduledir := extension
docmoduledir := extension
+incmoduledir := extension
else
datamoduledir := contrib
docmoduledir := contrib
+incmoduledir := contrib
endif
endif
@@ -108,6 +115,41 @@ ifdef PG_CPPFLAGS
override CPPFLAGS := $(PG_CPPFLAGS) $(CPPFLAGS)
endif
+HEADER_alldirs := $(patsubst HEADERS_%,%,$(filter HEADERS_%, $(.VARIABLES)))
+
+# HEADERS is an error in the absence of MODULE_big to provide a dir name
+ifdef MODULE_big
+HEADER_dirs := $(MODULE_big)
+ifdef HEADERS
+HEADERS_$(MODULE_big) := $(HEADERS)
+endif
+else
+ifdef HEADERS
+$(error HEADERS requires MODULE_big to be set)
+endif
+HEADER_dirs := $(filter $(MODULES),$(HEADER_alldirs))
+endif
+
+# HEADERS_foo requires that "foo" is in MODULES as a sanity check
+ifneq ($(filter-out $(HEADER_dirs),$(HEADER_alldirs)),)
+$(error $(patsubst %,HEADERS_%,$(filter-out $(HEADER_dirs),$(HEADER_alldirs))) defined with no module)
+endif
+
+# Functions for generating install/uninstall commands; the blank lines
+# before the "endef" are required, don't lose them
+# $(call install_headers,dir,headers)
+define install_headers
+$(MKDIR_P) '$(DESTDIR)$(includedir_server)/$(incmoduledir)/$(1)/'
+$(INSTALL_DATA) $(addprefix $(srcdir)/, $(2)) '$(DESTDIR)$(includedir_server)/$(incmoduledir)/$(1)/'
+
+endef
+# $(call uninstall_headers,dir,headers)
+define uninstall_headers
+rm -f $(addprefix '$(DESTDIR)$(includedir_server)/$(incmoduledir)/$(1)'/, $(notdir $(2)))
+
+endef
+
+
all: $(PROGRAM) $(DATA_built) $(SCRIPTS_built) $(addsuffix $(DLSUFFIX), $(MODULES)) $(addsuffix .control,
$(EXTENSION))
ifeq ($(with_llvm), yes)
@@ -154,6 +196,9 @@ endif # SCRIPTS
ifdef SCRIPTS_built
$(INSTALL_SCRIPT) $(SCRIPTS_built) '$(DESTDIR)$(bindir)/'
endif # SCRIPTS_built
+ifneq ($(strip $(HEADER_dirs)),)
+ $(foreach dir,$(HEADER_dirs),$(call install_headers,$(dir),$(HEADERS_$(dir))))
+endif # HEADERS
ifdef MODULE_big
ifeq ($(with_llvm), yes)
$(call install_llvm_module,$(MODULE_big),$(OBJS))
@@ -218,6 +263,9 @@ endif
ifdef SCRIPTS_built
rm -f $(addprefix '$(DESTDIR)$(bindir)'/, $(SCRIPTS_built))
endif
+ifneq ($(strip $(HEADER_dirs)),)
+ $(foreach dir,$(HEADER_dirs),$(call uninstall_headers,$(dir),$(HEADERS_$(dir))))
+endif # HEADERS
ifdef MODULE_big
ifeq ($(with_llvm), yes)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> Two questions arise:
> 1) include/server has a lot of files and subdirs, so using
> include/server/$(MODULE)/ looks likely to be error-prone. So it
> should be something like include/server/contrib/$(MODULE)/ or
> include/server/extension/$(MODULE)/. Which one, or should it use
> $(MODULEDIR) to choose between the two the way that DATA and DOCS do?
> Or something else?
Might as well follow the MODULEDIR precedent (though I'm not wedded
to that if somebody has an argument for something else).
> 2) Specifying HEADERS_blah for some name "blah" that's not listed in
> MODULES or MODULE_big should do what:
> a) install into blah/ anyway
> b) be ignored with a warning
> c) be silently ignored
> d) be an error
I'd definitely vote for "error". Likewise if any .h file listed in
the macro doesn't exist.
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: Tom> Might as well follow the MODULEDIR precedent (though I'm not wedded Tom> to that if somebody has an argument for something else). [...] Tom> I'd definitely vote for "error". Likewise if any .h file listed in Tom> the macro doesn't exist. OK, so that matches the patch I just posted. If any .h file listed doesn't exist, then $(INSTALL_DATA) should fail (and a quick test seems to confirm that it does). I only added one HEADERS= entry, for contrib/hstore; the other modules should also be reviewed to see if they should install any headers, but the basic stuff should be nailed down first. -- Andrew (irc:RhodiumToad)
On 2 July 2018 at 02:23, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote:
So I have this immediate problem: a PGXS build of a module, specifically
an hstore transform for a non-core PL, is much harder than it should be
because it has no way to get at hstore.h since that file is never
installed anywhere.
Should that be changed?
I think there's agreement in the thread that it should, and strong +1 from me.
I just wanted to pipe up with something Petr pointed out during pglogical development, which is that Pg offers a handy tool to help extensions link up with each other - find_rendezvous_variable(...) from dfmgr.c / fmgr.h .
It's a real shame it's not more visible in contrib/ examples and the docs. Any suggestions on where it should appear in the docs? Somewhere in extend.sgml, presumably.
2018-07-03 6:43 GMT+02:00 Craig Ringer <craig@2ndquadrant.com>:
On 2 July 2018 at 02:23, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote:So I have this immediate problem: a PGXS build of a module, specifically
an hstore transform for a non-core PL, is much harder than it should be
because it has no way to get at hstore.h since that file is never
installed anywhere.
Should that be changed?I think there's agreement in the thread that it should, and strong +1 from me.
+1
similar issue is with plpgsql. The header files are exported by not generic way.
Regards
Pavel
I just wanted to pipe up with something Petr pointed out during pglogical development, which is that Pg offers a handy tool to help extensions link up with each other - find_rendezvous_variable(...) from dfmgr.c / fmgr.h . It's a real shame it's not more visible in contrib/ examples and the docs. Any suggestions on where it should appear in the docs? Somewhere in extend.sgml, presumably.You still need a header from the other extension to *use* it, but it provides a massively easier way to find a struct of API function pointers. Prior to using it, I had a hack where I dlopen()ed the other shared library directly, and I'd also trialled using the fmgr to call a 'returns internal' function to get the API pointers struct that way.--
On 02.07.18 15:26, Tom Lane wrote: > FWIW, I agree with Andres' thought that each contrib module should have > its own subdirectory under $(includedir_server). Otherwise we're going > to be faced with questions about whether .h files need to be renamed > because they're not globally unique enough. Then they perhaps should be renamed. That seems like a much simpler solution. The use case being discussed here is installing a data type extension's header so you can write a transform for it. The extension's name as well as the data type's own name already have to be pretty much globally unique if you want it to be useful. So it doesn't seem very difficult to me to have the extension install a single header file with that same name. The other side of this is that the PLs have to install their header files. Which the in-core PLs already do. Would we we want to move their header files under a new per-extension directory scheme? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> This installs to $(includedir_server)/$(MODULEDIR)/$MODULE/file.h
> (e.g. include/server/extension/hstore/hstore.h for an actual example),
> and errors if HEADERS_xxx is defined for anything that's not a module
> listed in MODULES or MODULE_big.
I've not studied this patch in any great detail, but the basic plan seems
sane. However, don't we also need to teach the MSVC build system about
this?
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> This installs to $(includedir_server)/$(MODULEDIR)/$MODULE/file.h >> (e.g. include/server/extension/hstore/hstore.h for an actual >> example), and errors if HEADERS_xxx is defined for anything that's >> not a module listed in MODULES or MODULE_big. Tom> I've not studied this patch in any great detail, but the basic Tom> plan seems sane. However, don't we also need to teach the MSVC Tom> build system about this? Yes, also needs to update the pgxs docs. Looking at the windows build, is there anything needed beyond adding a check for the HEADERS* vars in Install.pm? -- Andrew (irc:RhodiumToad)
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes: Patch with docs (including an example) and Install.pm changes (blind). I'll stick this in the open CF to see what cfbot thinks of it. Anyone have any objection to putting this into 11beta if it works, as well as 12devel? -- Andrew (irc:RhodiumToad)
Attachment
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes: Andrew> Patch with docs (including an example) and Install.pm changes (blind). Andrew> I'll stick this in the open CF to see what cfbot thinks of it. Meh. Oversight in handling empty or missing vars. Also incorrect dir in msvc version. Fixed I hope. -- Andrew (irc:RhodiumToad)
Attachment
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes: Andrew> Patch with docs (including an example) and Install.pm changes Andrew> (blind). I'll stick this in the open CF to see what cfbot Andrew> thinks of it. Andrew> Meh. Oversight in handling empty or missing vars. Also Andrew> incorrect dir in msvc version. Fixed I hope. Take 3 for the windows version: don't assume the directories exist because contrib install is done before the main include files stuff. -- Andrew (irc:RhodiumToad)
Attachment
On Thu, Jul 05, 2018 at 03:25:24PM +0100, Andrew Gierth wrote: > Take 3 for the windows version: don't assume the directories exist > because contrib install is done before the main include files stuff. It seems to me that this gives a good reason to not play with REL_11_STABLE at this point. -- Michael
Attachment
On Wed, Jul 04, 2018 at 05:00:56PM -0400, Tom Lane wrote: > Andrew Gierth <andrew@tao11.riddles.org.uk> writes: >> This installs to $(includedir_server)/$(MODULEDIR)/$MODULE/file.h >> (e.g. include/server/extension/hstore/hstore.h for an actual example), >> and errors if HEADERS_xxx is defined for anything that's not a module >> listed in MODULES or MODULE_big. > > I've not studied this patch in any great detail, but the basic plan seems > sane. However, don't we also need to teach the MSVC build system about > this? +1. I have not looked at this patch in details myself, but the concept looks good. -- Michael
Attachment
On 05.07.18 14:51, Andrew Gierth wrote: > Anyone have any objection to putting this into 11beta if it works, as > well as 12devel? Yes, I have expressed concerns about this approach elsewhere in this thread. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > On 02.07.18 15:26, Tom Lane wrote: >> FWIW, I agree with Andres' thought that each contrib module should >> have its own subdirectory under $(includedir_server). Otherwise >> we're going to be faced with questions about whether .h files need >> to be renamed because they're not globally unique enough. Peter> Then they perhaps should be renamed. That seems like a much Peter> simpler solution. Personally I think that more -I options is less pain than having to rename things or deal with conflicts. Where exactly are you suggesting that they should be installed? Directly in $(installdir_server), or in $(installdir_server)/extension or equivalent? Peter> The use case being discussed here is installing a data type Peter> extension's header so you can write a transform for it. The Peter> extension's name as well as the data type's own name already Peter> have to be pretty much globally unique if you want it to be Peter> useful. So it doesn't seem very difficult to me to have the Peter> extension install a single header file with that same name. That's assuming a single header file, which might be a bit more restrictive than absolutely necessary. Peter> The other side of this is that the PLs have to install their Peter> header files. Which the in-core PLs already do. Would we we want Peter> to move their header files under a new per-extension directory Peter> scheme? The in-core PLs could reasonably be grandfathered in in their current locations, at least for now. -- Andrew (irc:RhodiumToad)
>>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> Anyone have any objection to putting this into 11beta if it works, >> as well as 12devel? Peter> Yes, I have expressed concerns about this approach elsewhere in Peter> this thread. No response to my followup to you in 2+ weeks. Last call? -- Andrew (irc:RhodiumToad)
On Mon, Jul 23, 2018 at 01:48:21AM +0100, Andrew Gierth wrote: > >>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > >>> Anyone have any objection to putting this into 11beta if it works, >>> as well as 12devel? > > Peter> Yes, I have expressed concerns about this approach elsewhere in > Peter> this thread. > > No response to my followup to you in 2+ weeks. Last call? FWIW, I don't have any objections with experimenting on HEAD, but I would vote for letting 11 out of this. -- Michael
Attachment
>>>>> "Michael" == Michael Paquier <michael@paquier.xyz> writes: >> No response to my followup to you in 2+ weeks. Last call? Michael> FWIW, I don't have any objections with experimenting on HEAD, Michael> but I would vote for letting 11 out of this. Why? -- Andrew.
Hi, On 2018-07-23 02:02:51 +0100, Andrew Gierth wrote: > >>>>> "Michael" == Michael Paquier <michael@paquier.xyz> writes: > > >> No response to my followup to you in 2+ weeks. Last call? > > Michael> FWIW, I don't have any objections with experimenting on HEAD, > Michael> but I would vote for letting 11 out of this. > > Why? Because that's the default assumption when considerint to committ a new feature to a feature frozen branch? I don't really have a strong feeling about this specific case either way, personally. But it's still the default assumption. Greetings, Andres Freund
Greetings, * Andrew Gierth (andrew@tao11.riddles.org.uk) wrote: > >>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > > > On 02.07.18 15:26, Tom Lane wrote: > >> FWIW, I agree with Andres' thought that each contrib module should > >> have its own subdirectory under $(includedir_server). Otherwise > >> we're going to be faced with questions about whether .h files need > >> to be renamed because they're not globally unique enough. > > Peter> Then they perhaps should be renamed. That seems like a much > Peter> simpler solution. > > Personally I think that more -I options is less pain than having to > rename things or deal with conflicts. Yeah, I don't care for the idea that we should expect all extensions, forever going forward, to provide one single .h file which has to be unique and non-conflicting with all other extensions, ever. When I think about the demands of extensions, I tend to consider PostGIS the prime example and I certainly would understand if they wanted to install multiple headers (they have some 72 .h files from what I'm seeing...). So, +1 from me for having a directory for each extension. > Where exactly are you suggesting that they should be installed? Directly > in $(installdir_server), or in $(installdir_server)/extension or > equivalent? I certainly wouldn't want extension headers being mixed in with server headers. I haven't got any great ideas about contrib-vs-extension, but I'm, at least, leaning towards 'extension' as being the best answer here. > Peter> The use case being discussed here is installing a data type > Peter> extension's header so you can write a transform for it. The > Peter> extension's name as well as the data type's own name already > Peter> have to be pretty much globally unique if you want it to be > Peter> useful. So it doesn't seem very difficult to me to have the > Peter> extension install a single header file with that same name. > > That's assuming a single header file, which might be a bit more > restrictive than absolutely necessary. Agreed that having a single header file is overly and unnecessairly restrictive. > Peter> The other side of this is that the PLs have to install their > Peter> header files. Which the in-core PLs already do. Would we we want > Peter> to move their header files under a new per-extension directory > Peter> scheme? > > The in-core PLs could reasonably be grandfathered in in their current > locations, at least for now. Grandfathering them seems fine to me, but I don't hold that position very strongly. Thanks! Stephen
Attachment
Greetings, * Andrew Gierth (andrew@tao11.riddles.org.uk) wrote: > >>>>> "Michael" == Michael Paquier <michael@paquier.xyz> writes: > > >> No response to my followup to you in 2+ weeks. Last call? > > Michael> FWIW, I don't have any objections with experimenting on HEAD, > Michael> but I would vote for letting 11 out of this. > > Why? While I agree that this patch is a good improvements for us to have, it's not something we'd back-patch to released versions and while we can play a little looser with v11 since it's in beta, I tend to agree with Michael that this change isn't really appropriate to include in v11 now that we're in beta. The current situation isn't great, but it's what people are familiar with and may already be expecting in v11. Ideally, we want extension authors and others testing v11 to find actual bugs and breaking things for them post feature-freeze and beta release isn't going to encourage them to play with beta versions in the future, meaning we'll end up with folks waiting until the release to do testing and end up with a poorer release for it. Thanks! Stephen
Attachment
On Sun, Jul 22, 2018 at 09:42:08PM -0400, Stephen Frost wrote: > When I think about the demands of extensions, I tend to consider PostGIS > the prime example and I certainly would understand if they wanted to > install multiple headers (they have some 72 .h files from what I'm > seeing...). > > So, +1 from me for having a directory for each extension. Definitely. If we were to choose the one-file per extension choice, most large extension maintainers would logically scream at us. If for example you look at Citus, in src/include/distributed there are a bunch of them. Then based on that folks could always tweak their CFLAGS pointing to the path of the extension if they need to. We cannot ensure either that multiple extensions do not use the same header file names, which discards any design using a single installation location with multiple files. So, like Stephen, that's a +1 from me. -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> On Sun, Jul 22, 2018 at 09:42:08PM -0400, Stephen Frost wrote:
>> So, +1 from me for having a directory for each extension.
> So, like Stephen, that's a +1 from me.
Same here. One-file-per-extension is too strongly biased to tiny
extensions (like most of our contrib examples).
I don't have a real strong opinion on whether it's too late to
push this into v11. I do not think it'd break anything other than
packagers' lists of files to be installed ... but it does seem
like a new feature, and we're past feature freeze.
regards, tom lane
On 2018-Jul-23, Tom Lane wrote: > Michael Paquier <michael@paquier.xyz> writes: > > On Sun, Jul 22, 2018 at 09:42:08PM -0400, Stephen Frost wrote: > >> So, +1 from me for having a directory for each extension. > > > So, like Stephen, that's a +1 from me. > > Same here. One-file-per-extension is too strongly biased to tiny > extensions (like most of our contrib examples). > > I don't have a real strong opinion on whether it's too late to > push this into v11. I do not think it'd break anything other than > packagers' lists of files to be installed ... but it does seem > like a new feature, and we're past feature freeze. Frankly, I'd rather make things as easy as possible for third-party extension writers. I'd go as far as backpatching further (considering transforms were introduced in 9.5) but I hesitate on that, because of the packagers argument. pg11 seems fair game to me, though. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 23.07.18 06:15, Tom Lane wrote: > Michael Paquier <michael@paquier.xyz> writes: >> On Sun, Jul 22, 2018 at 09:42:08PM -0400, Stephen Frost wrote: >>> So, +1 from me for having a directory for each extension. > >> So, like Stephen, that's a +1 from me. > > Same here. One-file-per-extension is too strongly biased to tiny > extensions (like most of our contrib examples). Nobody said anything about one-file-per-extension. You can of course have hstore_this.h and hstore_that.h or if you want to have many, use postgis/this.h and postgis/that.h. That's how every C package in the world works. We don't need to legislate further here other than, use sensible naming. Also, let's recall that the point of this exercise is that you want to install the header files so that you can build things (another extension) that somehow interacts with those extensions. Then, even if you put things in separate directories per extension, you still need to make sure that all the installed header files don't clash, since you'll be adding the -I options of several of them. In a way, doing it this way will make things less robust, since it will appear to give extension authors license to use generic header names. > I don't have a real strong opinion on whether it's too late to > push this into v11. I do not think it'd break anything other than > packagers' lists of files to be installed ... but it does seem > like a new feature, and we're past feature freeze. Certainly a new feature. I suggest submitting it to the next commit fest. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: Peter> Nobody said anything about one-file-per-extension. You can of Peter> course have hstore_this.h and hstore_that.h or if you want to Peter> have many, use postgis/this.h and postgis/that.h. So now you want the extension to be able to _optionally_ specify a subdirectory? (just having the extension do HEADERS=foo/bar.h will not work, because as with every other similar makefile variable, that means "install the file foo/bar.h as bar.h, not as foo/bar.h".) -- Andrew (irc:RhodiumToad)
On 2018-07-23 17:22:16 +0200, Peter Eisentraut wrote: > On 23.07.18 06:15, Tom Lane wrote: > > Michael Paquier <michael@paquier.xyz> writes: > >> On Sun, Jul 22, 2018 at 09:42:08PM -0400, Stephen Frost wrote: > >>> So, +1 from me for having a directory for each extension. > > > >> So, like Stephen, that's a +1 from me. > > > > Same here. One-file-per-extension is too strongly biased to tiny > > extensions (like most of our contrib examples). > > Nobody said anything about one-file-per-extension. You can of course > have hstore_this.h and hstore_that.h or if you want to have many, use > postgis/this.h and postgis/that.h. That's how every C package in the > world works. We don't need to legislate further here other than, use > sensible naming. PGXS provides a certain way of doing things. It's far from insane for us to procscribe that we go for $extension/. I fail to see any sort of advantage of using $extension_$header.h. It only serves to create additional work. > Also, let's recall that the point of this exercise is that you want to > install the header files so that you can build things (another > extension) that somehow interacts with those extensions. Then, even if > you put things in separate directories per extension, you still need to > make sure that all the installed header files don't clash, since you'll > be adding the -I options of several of them. I'd suggest just using the name with the $extension/ prefix going forward. > > I don't have a real strong opinion on whether it's too late to > > push this into v11. I do not think it'd break anything other than > > packagers' lists of files to be installed ... but it does seem > > like a new feature, and we're past feature freeze. > > Certainly a new feature. I suggest submitting it to the next commit fest. Given that there seems to be fairly widespread agreement, anyone that commented but you, and the patch has been reviewed, I see zero reason to wait for the next CF for a committer's patch. Greetings, Andres Freund
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> Also, let's recall that the point of this exercise is that you want to
> install the header files so that you can build things (another
> extension) that somehow interacts with those extensions. Then, even if
> you put things in separate directories per extension, you still need to
> make sure that all the installed header files don't clash, since you'll
> be adding the -I options of several of them. In a way, doing it this
> way will make things less robust, since it will appear to give extension
> authors license to use generic header names.
Personally, I'd recommend using *one* -I switch and having .c files
reference extension headers with #include "extensionname/headername.h".
As I said before, I think that we should change the existing contrib
modules to be coded likewise, all using a single -I switch that points
at SRCDIR/contrib. That'd help give people the right coding model
to follow.
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: Tom> As I said before, I think that we should change the existing Tom> contrib modules to be coded likewise, all using a single -I switch Tom> that points at SRCDIR/contrib. That'd help give people the right Tom> coding model to follow. I don't see that playing nicely with PGXS? -- Andrew.
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes: Final patch just to let cfbot test the windows code for me. A review of contrib/ suggested that cube, hstore, isn, ltree and seg were the only modules that had useful headers to install, so do those. -- Andrew (irc:RhodiumToad) From df163230b92b79468c241e9480e7fb85b7614440 Mon Sep 17 00:00:00 2001 From: Andrew Gierth <rhodiumtoad@postgresql.org> Date: Tue, 31 Jul 2018 19:58:39 +0100 Subject: [PATCH] Provide for contrib and pgxs modules to install include files. This allows out-of-tree PLs and similar code to get access to definitions needed to work with extension data types. The following existing modules now install headers: contrib/cube, contrib/hstore, contrib/isn, contrib/ltree, contrib/seg. Discussion: https://postgr.es/m/87y3euomjh.fsf%40news-spur.riddles.org.uk --- contrib/cube/Makefile | 2 ++ contrib/hstore/Makefile | 2 ++ contrib/isn/Makefile | 3 +++ contrib/ltree/Makefile | 2 ++ contrib/seg/Makefile | 2 ++ doc/src/sgml/extend.sgml | 28 ++++++++++++++++++++++++-- src/makefiles/pgxs.mk | 50 +++++++++++++++++++++++++++++++++++++++++++++++ src/tools/msvc/Install.pm | 43 ++++++++++++++++++++++++++++++++++++++++ 8 files changed, 130 insertions(+), 2 deletions(-) diff --git a/contrib/cube/Makefile b/contrib/cube/Makefile index accb7d28a3..a679ac626e 100644 --- a/contrib/cube/Makefile +++ b/contrib/cube/Makefile @@ -9,6 +9,8 @@ DATA = cube--1.2.sql cube--1.2--1.3.sql cube--1.3--1.4.sql \ cube--unpackaged--1.0.sql PGFILEDESC = "cube - multidimensional cube data type" +HEADERS = cubedata.h + REGRESS = cube EXTRA_CLEAN = y.tab.c y.tab.h diff --git a/contrib/hstore/Makefile b/contrib/hstore/Makefile index ab7fef3979..46d26f8052 100644 --- a/contrib/hstore/Makefile +++ b/contrib/hstore/Makefile @@ -11,6 +11,8 @@ DATA = hstore--1.4.sql hstore--1.4--1.5.sql \ hstore--unpackaged--1.0.sql PGFILEDESC = "hstore - key/value pair data type" +HEADERS = hstore.h + REGRESS = hstore ifdef USE_PGXS diff --git a/contrib/isn/Makefile b/contrib/isn/Makefile index ab6b175f9a..c3600dac30 100644 --- a/contrib/isn/Makefile +++ b/contrib/isn/Makefile @@ -7,6 +7,9 @@ DATA = isn--1.1.sql isn--1.1--1.2.sql \ isn--1.0--1.1.sql isn--unpackaged--1.0.sql PGFILEDESC = "isn - data types for international product numbering standards" +# the other .h files are data tables, we don't install those +HEADERS_isn = isn.h + REGRESS = isn ifdef USE_PGXS diff --git a/contrib/ltree/Makefile b/contrib/ltree/Makefile index c101603e6c..416c8da312 100644 --- a/contrib/ltree/Makefile +++ b/contrib/ltree/Makefile @@ -9,6 +9,8 @@ EXTENSION = ltree DATA = ltree--1.1.sql ltree--1.0--1.1.sql ltree--unpackaged--1.0.sql PGFILEDESC = "ltree - hierarchical label data type" +HEADERS = ltree.h + REGRESS = ltree ifdef USE_PGXS diff --git a/contrib/seg/Makefile b/contrib/seg/Makefile index 41270f84f6..62b658e724 100644 --- a/contrib/seg/Makefile +++ b/contrib/seg/Makefile @@ -8,6 +8,8 @@ DATA = seg--1.1.sql seg--1.1--1.2.sql seg--1.2--1.3.sql \ seg--1.0--1.1.sql seg--unpackaged--1.0.sql PGFILEDESC = "seg - line segment data type" +HEADERS = segdata.h + REGRESS = seg EXTRA_CLEAN = y.tab.c y.tab.h diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml index 348ae71423..a3cb064131 100644 --- a/doc/src/sgml/extend.sgml +++ b/doc/src/sgml/extend.sgml @@ -1100,13 +1100,15 @@ include $(PGXS) and include the global <acronym>PGXS</acronym> makefile. Here is an example that builds an extension module named <literal>isbn_issn</literal>, consisting of a shared library containing - some C code, an extension control file, a SQL script, and a documentation - text file: + some C code, an extension control file, a SQL script, an include file + (only needed if other modules might need to access the extension functions + without going via SQL), and a documentation text file: <programlisting> MODULES = isbn_issn EXTENSION = isbn_issn DATA = isbn_issn--1.0.sql DOCS = README.isbn_issn +HEADERS_isbn_issn = isbn_issn.h PG_CONFIG = pg_config PGXS := $(shell $(PG_CONFIG) --pgxs) @@ -1221,6 +1223,28 @@ include $(PGXS) </varlistentry> <varlistentry> + <term><varname>HEADERS</varname></term> + <listitem> + <para> + files to install under + <literal><replaceable>prefix</replaceable>/include/server/$MODULEDIR/$MODULE_big</literal> + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><varname>HEADERS_$MODULE</varname></term> + <listitem> + <para> + files to install under + <literal><replaceable>prefix</replaceable>/include/server/$MODULEDIR/$MODULE</literal>, + where <literal>$MODULE</literal> must be a module name used + in <literal>MODULES</literal> or <literal>MODULE_big</literal> + </para> + </listitem> + </varlistentry> + + <varlistentry> <term><varname>SCRIPTS</varname></term> <listitem> <para> diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk index bfae02f90c..158581b3f5 100644 --- a/src/makefiles/pgxs.mk +++ b/src/makefiles/pgxs.mk @@ -38,6 +38,10 @@ # SCRIPTS -- script files (not binaries) to install into $PREFIX/bin # SCRIPTS_built -- script files (not binaries) to install into $PREFIX/bin, # which need to be built first +# HEADERS -- files to install into $(includedir_server)/$MODULEDIR/$MODULE_big +# HEADERS_$(MODULE) -- files to install into +# $(includedir_server)/$MODULEDIR/$MODULE; the value of $MODULE must be +# listed in MODULES or MODULE_big # REGRESS -- list of regression test cases (without suffix) # REGRESS_OPTS -- additional switches to pass to pg_regress # NO_INSTALLCHECK -- don't define an installcheck target, useful e.g. if @@ -94,13 +98,16 @@ endif ifdef MODULEDIR datamoduledir := $(MODULEDIR) docmoduledir := $(MODULEDIR) +incmoduledir := $(MODULEDIR) else ifdef EXTENSION datamoduledir := extension docmoduledir := extension +incmoduledir := extension else datamoduledir := contrib docmoduledir := contrib +incmoduledir := contrib endif endif @@ -108,6 +115,43 @@ ifdef PG_CPPFLAGS override CPPFLAGS := $(PG_CPPFLAGS) $(CPPFLAGS) endif +HEADER_alldirs := $(patsubst HEADERS_%,%,$(filter HEADERS_%, $(.VARIABLES))) + +# HEADERS is an error in the absence of MODULE_big to provide a dir name +ifdef MODULE_big +ifdef HEADERS +HEADER_dirs := $(MODULE_big) +HEADERS_$(MODULE_big) = $(HEADERS) +else +HEADER_dirs := $(filter $(MODULE_big),$(HEADER_alldirs)) +endif +else +ifdef HEADERS +$(error HEADERS requires MODULE_big to be set) +endif +HEADER_dirs := $(filter $(MODULES),$(HEADER_alldirs)) +endif + +# HEADERS_foo requires that "foo" is in MODULES as a sanity check +ifneq ($(filter-out $(HEADER_dirs),$(HEADER_alldirs)),) +$(error $(patsubst %,HEADERS_%,$(filter-out $(HEADER_dirs),$(HEADER_alldirs))) defined with no module) +endif + +# Functions for generating install/uninstall commands; the blank lines +# before the "endef" are required, don't lose them +# $(call install_headers,dir,headers) +define install_headers +$(MKDIR_P) '$(DESTDIR)$(includedir_server)/$(incmoduledir)/$(1)/' +$(INSTALL_DATA) $(addprefix $(srcdir)/, $(2)) '$(DESTDIR)$(includedir_server)/$(incmoduledir)/$(1)/' + +endef +# $(call uninstall_headers,dir,headers) +define uninstall_headers +rm -f $(addprefix '$(DESTDIR)$(includedir_server)/$(incmoduledir)/$(1)'/, $(notdir $(2))) + +endef + + all: $(PROGRAM) $(DATA_built) $(SCRIPTS_built) $(addsuffix $(DLSUFFIX), $(MODULES)) $(addsuffix .control, $(EXTENSION)) ifeq ($(with_llvm), yes) @@ -154,6 +198,9 @@ endif # SCRIPTS ifdef SCRIPTS_built $(INSTALL_SCRIPT) $(SCRIPTS_built) '$(DESTDIR)$(bindir)/' endif # SCRIPTS_built +ifneq ($(strip $(HEADER_dirs)),) + $(foreach dir,$(HEADER_dirs),$(if $(HEADERS_$(dir)),$(call install_headers,$(dir),$(HEADERS_$(dir))))) +endif # HEADERS ifdef MODULE_big ifeq ($(with_llvm), yes) $(call install_llvm_module,$(MODULE_big),$(OBJS)) @@ -218,6 +265,9 @@ endif ifdef SCRIPTS_built rm -f $(addprefix '$(DESTDIR)$(bindir)'/, $(SCRIPTS_built)) endif +ifneq ($(strip $(HEADER_dirs)),) + $(foreach dir,$(HEADER_dirs),$(if $(HEADERS_$(dir)),$(call uninstall_headers,$(dir),$(HEADERS_$(dir))))) +endif # HEADERS ifdef MODULE_big ifeq ($(with_llvm), yes) diff --git a/src/tools/msvc/Install.pm b/src/tools/msvc/Install.pm index 429608fde6..60b5639df8 100644 --- a/src/tools/msvc/Install.pm +++ b/src/tools/msvc/Install.pm @@ -554,6 +554,49 @@ sub CopySubdirFiles } } + { + $flist = ''; + if ($mf =~ /^HEADERS\s*=\s*(.*)$/m) { $flist .= $1 } + my @modlist = (); + my %fmodlist = (); + while ($mf =~ /^HEADERS_([^\s=]+)\s*=\s*(.*)$/mg) { $fmodlist{$1} .= $2 } + + if ($mf =~ /^MODULE_big\s*=\s*(.*)$/m) + { + push @modlist, $1; + if ($flist ne '') + { + $fmodlist{$1} = $flist; + $flist = ''; + } + } + elsif ($mf =~ /^MODULES\s*=\s*(.*)$/m) + { + push @modlist, split /\s+/, $1; + } + + croak "HEADERS requires MODULE_big in $subdir $module" + if $flist ne ''; + + foreach my $mod (keys %fmodlist) + { + croak "HEADERS_$mod for unknown module in $subdir $module" + unless grep { $_ eq $mod } @modlist; + $flist = ParseAndCleanRule($fmodlist{$mod}, $mf); + EnsureDirectories($target, + "include", "include/server", + "include/server/$moduledir", + "include/server/$moduledir/$mod"); + foreach my $f (split /\s+/, $flist) + { + lcopy("$subdir/$module/$f", + "$target/include/server/$moduledir/$mod/" . basename($f)) + || croak("Could not copy file $f in $subdir $module"); + print '.'; + } + } + } + $flist = ''; if ($mf =~ /^DOCS\s*=\s*(.*)$/mg) { $flist .= $1 } if ($flist ne '') -- 2.11.1
On 2018-07-31 20:19:49 +0100, Andrew Gierth wrote: > Final patch just to let cfbot test the windows code for me. > > A review of contrib/ suggested that cube, hstore, isn, ltree and seg > were the only modules that had useful headers to install, so do those. I'm a bit surprised that you decided to push to the 11 branch - the consensus in this thread seem to have gone the other way by my read? Given that that's the rule, and pushing non-fixes is the exception, I'm not particularly ok with just ignoring that? - Andres
On Tue, Jul 31, 2018 at 1:17 PM, Andres Freund <andres@anarazel.de> wrote: > I'm a bit surprised that you decided to push to the 11 branch - the > consensus in this thread seem to have gone the other way by my read? > Given that that's the rule, and pushing non-fixes is the exception, I'm > not particularly ok with just ignoring that? +1 -- Peter Geoghegan
>>>>> "Andres" == Andres Freund <andres@anarazel.de> writes: Andres> I'm a bit surprised that you decided to push to the 11 branch - Andres> the consensus in this thread seem to have gone the other way by Andres> my read? There were also quite a few non-objections or "don't care"s to the idea of pushing it to 11beta, and no real justification for not doing so was ever given other than "it's a new feature", which I don't really agree with (it's certainly fixing a defect, even if you don't want to call it a bug). I actually think an argument could be made for backpatching it even further (though I don't intend to pursue that). The reason for making the change in the first place is to support out-of-tree development (particularly of PLs and their transforms) which isn't strongly tied to pg's development and release cycle; not pushing to 11-beta would mean delaying the issue for another year and forcing yet another cycle of workarounds. (Right now for example I can take advantage of the fact that hstore.h didn't change between 9.5, 9.6 and 10 except in non-significant whitespace; if I can rely on hstore.h being actually installed in pg11 onwards, then I can draw a line under the issue and move on. Otherwise, I have to deal with the fact that hstore.h _does_ change between 10 and 11 and then wait for 12 to see what that ends up doing.) (Obviously I'm biased by the fact that this is an issue that impacts me personally.) -- Andrew (irc:RhodiumToad)
Peter Geoghegan <pg@bowt.ie> writes:
> On Tue, Jul 31, 2018 at 1:17 PM, Andres Freund <andres@anarazel.de> wrote:
>> I'm a bit surprised that you decided to push to the 11 branch - the
>> consensus in this thread seem to have gone the other way by my read?
>> Given that that's the rule, and pushing non-fixes is the exception, I'm
>> not particularly ok with just ignoring that?
> +1
By my count of people expressing opinions, we had Michael -1, Stephen -1,
me -0.1 or so, Alvaro +1, Peter -1, presumably +1 from Andrew; and Andres
made a comment about not waiting, which perhaps Andrew read as a +1 for
backpatching. So it's not unreasonable for Andrew to have decided that
it was about tied. Nonetheless, it does seem like a feature and we're
past feature freeze, so the default assumption ought to be "no backpatch"
IMO.
regards, tom lane
On 2018-07-31 17:53:19 -0400, Tom Lane wrote: > Peter Geoghegan <pg@bowt.ie> writes: > > On Tue, Jul 31, 2018 at 1:17 PM, Andres Freund <andres@anarazel.de> wrote: > >> I'm a bit surprised that you decided to push to the 11 branch - the > >> consensus in this thread seem to have gone the other way by my read? > >> Given that that's the rule, and pushing non-fixes is the exception, I'm > >> not particularly ok with just ignoring that? > > > +1 > > By my count of people expressing opinions, we had Michael -1, Stephen -1, > me -0.1 or so, Alvaro +1, Peter -1, presumably +1 from Andrew; and Andres > made a comment about not waiting, which perhaps Andrew read as a +1 for > backpatching. So it's not unreasonable for Andrew to have decided that > it was about tied. Nonetheless, it does seem like a feature and we're > past feature freeze, so the default assumption ought to be "no backpatch" > IMO. Yea, I don't think it's an entirely unreasonable to decide to backpatch based on these votes, but I think if the stated opinions are like you count, it's pretty reasonable to at least announce that the more controversial choice is the plan and give a chance to more vigorously object. Greetings, Andres Freund
On 23/07/2018 18:32, Andrew Gierth wrote: >>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: > > Tom> As I said before, I think that we should change the existing > Tom> contrib modules to be coded likewise, all using a single -I switch > Tom> that points at SRCDIR/contrib. That'd help give people the right > Tom> coding model to follow. > > I don't see that playing nicely with PGXS? I'm also not on board that my random third-party extension now has to refer to its own header files as "subdirectory/headerfile.h". Which will mess up existing extensions that have header files in their tree. Or at least I'm not totally sure what the exact proposal and real-world implications are, with regard to existing extensions with one or more header files. By all means, let's make it easier for large or small extensions to manage their header files with PGXS. But let's separate what PGXS can and should do from what the extension's own file layout is. But I think there are some fundamentally incompatible goals here with regard to how the final -I options are supposed to look. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 31/07/2018 23:46, Andrew Gierth wrote: > not pushing > to 11-beta would mean delaying the issue for another year and forcing > yet another cycle of workarounds. But that applies to every single patch. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 31/07/2018 21:19, Andrew Gierth wrote: >>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes: > > Final patch just to let cfbot test the windows code for me. > > A review of contrib/ suggested that cube, hstore, isn, ltree and seg > were the only modules that had useful headers to install, so do those. I'm missing some guidance what an extension using those headers is supposed to do. How does it get the right -I options? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> A review of contrib/ suggested that cube, hstore, isn, ltree and seg >> were the only modules that had useful headers to install, so do >> those. Peter> I'm missing some guidance what an extension using those headers Peter> is supposed to do. How does it get the right -I options? All of the below assumes PGXS. If your extension is relying on pg11+, or you have checked the pg version when constructing the makefile, you can just do: PG_CPPFLAGS += -I$(includedir_server)/extension/hstore and #include "hstore.h" will work. If you need to workaround for old versions in one makefile, as I do, then you can do something along these lines (this goes before the include $(PGXS) line): # MAJORVERSION and includedir_server are not defined yet, but will be # defined before PG_CPPFLAGS is expanded. So we use conditional # expansions rather than 'ifeq' syntax. # for pg11+, hstore.h will be installed here HSTORE_INCDIR = $(includedir_server)/extension/hstore # for pg 9.5/9.6/10, we have a local copy of hstore.h since it happens # to be the same, barring non-semantic whitespace, between the three # versions HSTORE_INCDIR_OLD = old_inc PG_CPPFLAGS += -I$(HSTORE_INCDIR$(if $(filter 9.% 10,$(MAJORVERSION)),_OLD)) If you need to distinguish more versions for whatever reason you can do this: HSTORE_INCDIR = $(includedir_server)/extension/hstore HSTORE_INCDIR_10 = whatever HSTORE_INCDIR_9.6 = whatever HSTORE_INCDIR_9.5 = whatever HSTORE_INCDIR_9.4 = whatever HSTORE_INCDIR_9.3 = whatever PG_CPPFLAGS += -I$(HSTORE_INCDIR$(if $(filter 9.% 10,$(MAJORVERSION)),_$(MAJORVERSION))) -- Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> Peter> I'm missing some guidance what an extension using those headers
> Peter> is supposed to do. How does it get the right -I options?
> If your extension is relying on pg11+, or you have checked the pg
> version when constructing the makefile, you can just do:
> PG_CPPFLAGS += -I$(includedir_server)/extension/hstore
> and #include "hstore.h" will work.
I remain of the opinion that it'd be smarter to do
PG_CPPFLAGS += -I$(includedir_server)/extension
then
#include "hstore/hstore.h"
This way requires fewer -I options and is far more robust against header
name conflicts.
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> If your extension is relying on pg11+, or you have checked the pg >> version when constructing the makefile, you can just do: >> PG_CPPFLAGS += -I$(includedir_server)/extension/hstore >> and #include "hstore.h" will work. Tom> I remain of the opinion that it'd be smarter to do Tom> PG_CPPFLAGS += -I$(includedir_server)/extension Tom> then Tom> #include "hstore/hstore.h" Tom> This way requires fewer -I options and is far more robust against Tom> header name conflicts. Sure, it works for the simple cases like hstore, but how does it handle the case of a pgxs extension that installs more than one include file, one of which includes another? -- Andrew (irc:RhodiumToad)
On 2018-08-01 04:52:28 +0100, Andrew Gierth wrote: > >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: > > >> If your extension is relying on pg11+, or you have checked the pg > >> version when constructing the makefile, you can just do: > >> PG_CPPFLAGS += -I$(includedir_server)/extension/hstore > >> and #include "hstore.h" will work. > > Tom> I remain of the opinion that it'd be smarter to do > > Tom> PG_CPPFLAGS += -I$(includedir_server)/extension > > Tom> then > > Tom> #include "hstore/hstore.h" > > Tom> This way requires fewer -I options and is far more robust against > Tom> header name conflicts. > > Sure, it works for the simple cases like hstore, but how does it handle > the case of a pgxs extension that installs more than one include file, > one of which includes another? I might be momentarily daft (just was on a conference call for a while ;)), but why'd that be problematic? The header files can just specify the full path, and they'll find each other because of the aforementioned -I? Greetings, Andres Freund
>>>>> "Andres" == Andres Freund <andres@anarazel.de> writes: >> Sure, it works for the simple cases like hstore, but how does it >> handle the case of a pgxs extension that installs more than one >> include file, one of which includes another? Andres> I might be momentarily daft (just was on a conference call for Andres> a while ;)), but why'd that be problematic? The header files Andres> can just specify the full path, and they'll find each other Andres> because of the aforementioned -I? The #includes in the header files need to work both for the module's own build, and also for building modules depending on it. So existing practice would be that the module's own source dir (for module "foo") would look something like: ./Makefile ./foo.h (contains #include "foo_2.h") ./foo_2.h ./foo.c (contains #include "foo.h") and eventually foo.h and foo_int.h get installed as $(includedir_server)/extension/foo/foo.h and /foo_2.h To make this work with Tom's scheme means changing the directory layout of the original module. For contrib modules it's easy because the "." in the above paths is already "contrib/foo", but for PGXS the module should not be making assumptions about the name of its build dir, so you end up having to rejig the layout to something like ./Makefile ./foo ./foo/foo.h (contains #include "foo/foo_2.h") ./foo/foo_2.h ./foo.c (contains #include "foo/foo.h", compiled with -I$(srcdir)) or ./Makefile ./include/foo ./include/foo/foo.h ./include/foo/foo_2.h ./foo.c (compiled with -I$(srcdir)/include) Either way, it's a forced change to the PGXS module's file layout if it wants to install headers that work for other people using Tom's suggested approach. I'm not on board with this unless there's a better solution than I've seen so far. -- Andrew (irc:RhodiumToad)
On 01/08/2018 06:17, Andrew Gierth wrote: > Either way, it's a forced change to the PGXS module's file layout if it > wants to install headers that work for other people using Tom's > suggested approach. I'm not on board with this unless there's a better > solution than I've seen so far. The problem is that the way we've left it now is so that 50% of the users will do it wrong. This reminds me vividly of libxml headers. For example, on my system there is a file at /usr/include/libxml2/libxml/parser.h How do you use that? -I/usr/include and #include "libxml2/libxml/parser.h" or -I/usr/include/libxml2 and #include "libxml/parser.h" or -I/usr/include/libxml2/libxml and #include "parser.h" Obviously, one can find this out with some experimentation or research, but in our case there won't be much guidance available. In the libxml case, the best solution is xml2-config or pkg-config. I would like to see something similar here, where users don't have to fiddle with PG_CPPFLAGS directly. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Jul 31, 2018 at 5:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > By my count of people expressing opinions, we had Michael -1, Stephen -1, > me -0.1 or so, Alvaro +1, Peter -1, presumably +1 from Andrew; and Andres > made a comment about not waiting, which perhaps Andrew read as a +1 for > backpatching. So it's not unreasonable for Andrew to have decided that > it was about tied. Nonetheless, it does seem like a feature and we're > past feature freeze, so the default assumption ought to be "no backpatch" > IMO. Yeah, I would have voted -1 if I'd realized that it was close. Now we're in a situation where we have patch not everyone likes not only in master (which is OK, because we've got a year to decide whether to change anything) but also in v11 (where we have a lot less time). That's not so great. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> Yeah, I would have voted -1 if I'd realized that it was close. Now
> we're in a situation where we have patch not everyone likes not only
> in master (which is OK, because we've got a year to decide whether to
> change anything) but also in v11 (where we have a lot less time).
> That's not so great.
It seems like there were two separate areas of disagreement/questioning,
one being the file layout (whether to add per-extension subdirectories)
and then one about how one would actually use this, ie what would the
-I switch(es) look like and where would they get injected.
My impression is that there was consensus for per-extension
subdirectories, but the usage scenario isn't totally designed yet.
In principle, only the layout question has to be resolved to make
it OK to ship this in v11. On the other hand, if there's no
very practical way to use the per-extension subdirectory layout,
we might have to rethink that layout. So I'd be happier about this
if that question were answered promptly. Failing a satisfactory
answer in the near future, I think we need to revert in v11.
Also, "near future" means "before Monday". I don't want to ship beta3
with this in place if we end up reverting later, because it'd mean
thrashing packagers' file manifests, which they won't appreciate.
It might be best to revert in v11 for now, and then we can put it back
after beta3 if there's agreement that the questions are satisfactorily
resolved.
regards, tom lane
On 2018-08-02 10:54:00 -0400, Tom Lane wrote: > Also, "near future" means "before Monday". I don't want to ship beta3 > with this in place if we end up reverting later, because it'd mean > thrashing packagers' file manifests, which they won't appreciate. > It might be best to revert in v11 for now, and then we can put it back > after beta3 if there's agreement that the questions are satisfactorily > resolved. +1
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: Tom> It seems like there were two separate areas of Tom> disagreement/questioning, one being the file layout (whether to Tom> add per-extension subdirectories) and then one about how one would Tom> actually use this, ie what would the -I switch(es) look like and Tom> where would they get injected. Tom> My impression is that there was consensus for per-extension Tom> subdirectories, but the usage scenario isn't totally designed yet. Tom> In principle, only the layout question has to be resolved to make Tom> it OK to ship this in v11. Currently, everything is agnostic about the usage scenario - the existing extension include files will work with either -I$(includedir_server)/extension and #include "hstore/hstore.h", or with -I$(includedir_server)/extension/hstore and #include "hstore.h". Tom> On the other hand, if there's no very practical way to use the Tom> per-extension subdirectory layout, What constitutes "practical"? Right now it seems unlikely that there's much of a use case for referring to more than two different extensions at a time (a pl and a data type, for building a transform module outside either the pl's or the type's source tree). Referring to one is more likely (in my case, hstore_pllua is written to build inside pllua-ng's source tree, so all it needs is to get at hstore.h). -- Andrew (irc:RhodiumToad)
>>>>> "Andres" == Andres Freund <andres@anarazel.de> writes: >> Also, "near future" means "before Monday". I don't want to ship >> beta3 with this in place if we end up reverting later, because it'd >> mean thrashing packagers' file manifests, which they won't >> appreciate. It might be best to revert in v11 for now, and then we >> can put it back after beta3 if there's agreement that the questions >> are satisfactorily resolved. Andres> +1 On the other hand, _I'm_ getting pressure from at least one packager to nail down a final release of pllua-ng so they can build it along with beta3 (in place of the old broken pllua), which obviously I can't do if I keep having to fiddle with workarounds for hstore.h. -- Andrew (irc:RhodiumToad)
On 2018-08-02 17:53:17 +0100, Andrew Gierth wrote: > >>>>> "Andres" == Andres Freund <andres@anarazel.de> writes: > > >> Also, "near future" means "before Monday". I don't want to ship > >> beta3 with this in place if we end up reverting later, because it'd > >> mean thrashing packagers' file manifests, which they won't > >> appreciate. It might be best to revert in v11 for now, and then we > >> can put it back after beta3 if there's agreement that the questions > >> are satisfactorily resolved. > > Andres> +1 > > On the other hand, _I'm_ getting pressure from at least one packager to > nail down a final release of pllua-ng so they can build it along with > beta3 (in place of the old broken pllua), which obviously I can't do if > I keep having to fiddle with workarounds for hstore.h. I just don't have a lot of sympathy for that, given the months-after-freeze-window timing. It's not like *any* of this just started to be problematic in v11. Greetings, Andres Freund
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> Tom> My impression is that there was consensus for per-extension
> Tom> subdirectories, but the usage scenario isn't totally designed yet.
> Tom> In principle, only the layout question has to be resolved to make
> Tom> it OK to ship this in v11.
> Currently, everything is agnostic about the usage scenario - the
> existing extension include files will work with either
> -I$(includedir_server)/extension and #include "hstore/hstore.h", or with
> -I$(includedir_server)/extension/hstore and #include "hstore.h".
Well, the point is we don't want agnosticism --- we want a clear
usage model for people to follow.
> Tom> On the other hand, if there's no very practical way to use the
> Tom> per-extension subdirectory layout,
> What constitutes "practical"?
Something that copes with selecting the right headers if you're rebuilding
a module whose headers are already installed (as you mentioned upthread).
Something that copes with different modules installing headers with the
same base name. Allowing for that was the driving force for going with
subdirectory-per-extension, but if we really want that to work, there
seems to be no alternative but for extensions to write qualified header
names (#include "hstore/hstore.h" not #include "hstore.h"). Andres,
for one, seemed to think that wouldn't play nicely with PGXS, though
I'm not sure why not. Maybe he only meant that an extension's *own*
headers shouldn't be referenced that way.
Maybe this all just works without much thought, but given that smart
people like Peter E. seem to be unsure of that, I'd sure like to see
a concrete set of rules that extensions should follow for this.
There's also a question of whether we need to change anything in
contrib/ so that it plays by whatever rules we set. There's an
expectation that contrib modules should be buildable with PGXS,
so they need to follow the rules.
regards, tom lane
On Thu, Aug 2, 2018 at 12:56 PM, Andres Freund <andres@anarazel.de> wrote: >> On the other hand, _I'm_ getting pressure from at least one packager to >> nail down a final release of pllua-ng so they can build it along with >> beta3 (in place of the old broken pllua), which obviously I can't do if >> I keep having to fiddle with workarounds for hstore.h. > > I just don't have a lot of sympathy for that, given the > months-after-freeze-window timing. It's not like *any* of this just > started to be problematic in v11. Yeah, I agree. Andrew, it seems to me that you brought this problem on yourself. You rammed a patch through four months after feature freeze with a marginal consensus and are now complaining about having to spend more time on it. Well, that's a self-inflicted injury. If you don't commit features with a marginal consensus and/or don't do it four months after feature freeze, you won't get nearly as much pushback. Don't get me wrong -- I think you've done a lot of great work over the years and I'm glad to have you involved both as a contributor and as a committer of your own patches and those of others. It's just that your email here reads as if you think that your commit privileges are for your benefit rather than that of the project, and that's not how it works. Everybody here is free to pursue the things they think are interesting and the diversity of such things is part of the strength of the project, but nobody is free to ram through changes that make life better for them and worse for other people, even if they don't agree with the complaints those other people make. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: Tom> Maybe this all just works without much thought, but given that Tom> smart people like Peter E. seem to be unsure of that, I'd sure Tom> like to see a concrete set of rules that extensions should follow Tom> for this. I'll comment on the more substantive stuff later since I just noticed a few relevant points that I need to investigate. But while investigating, I found... Tom> There's also a question of whether we need to change anything in Tom> contrib/ so that it plays by whatever rules we set. There's an Tom> expectation that contrib modules should be buildable with PGXS, Tom> so they need to follow the rules. ... that at least all of the *_plperl transform modules in contrib/ fail to build with USE_PGXS already (i.e. for as long as they have ever existed), because they rely on plperl_helpers.h which is never installed anywhere, and trying to get it via $(top_srcdir) obviously can't work in PGXS. Haven't tried the python ones yet. -- Andrew.
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes: Tom> There's also a question of whether we need to change anything in Tom> contrib/ so that it plays by whatever rules we set. There's an Tom> expectation that contrib modules should be buildable with PGXS, Tom> so they need to follow the rules. Andrew> ... that at least all of the *_plperl transform modules in Andrew> contrib/ fail to build with USE_PGXS already (i.e. for as long Andrew> as they have ever existed), because they rely on Andrew> plperl_helpers.h which is never installed anywhere, and trying Andrew> to get it via $(top_srcdir) obviously can't work in PGXS. Andrew> Haven't tried the python ones yet. And none of the plpython transforms can even parse their makefiles with USE_PGXS, let alone build, because they have an "include" directive pointing into src/pl/plpython. -- Andrew (irc:RhodiumToad)
On 2018-08-02 19:13:05 +0100, Andrew Gierth wrote: > >>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes: > > Tom> There's also a question of whether we need to change anything in > Tom> contrib/ so that it plays by whatever rules we set. There's an > Tom> expectation that contrib modules should be buildable with PGXS, > Tom> so they need to follow the rules. > > Andrew> ... that at least all of the *_plperl transform modules in > Andrew> contrib/ fail to build with USE_PGXS already (i.e. for as long > Andrew> as they have ever existed), because they rely on > Andrew> plperl_helpers.h which is never installed anywhere, and trying > Andrew> to get it via $(top_srcdir) obviously can't work in PGXS. > > Andrew> Haven't tried the python ones yet. > > And none of the plpython transforms can even parse their makefiles with > USE_PGXS, let alone build, because they have an "include" directive > pointing into src/pl/plpython. FWIW, I'd be perfectly on board with just burying this policy. Designating one contrib module (or something in src/test/examples or such) as a PGXS example, and always building it with pgxs seems like it'd do a much better job than the current policy. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2018-08-02 19:13:05 +0100, Andrew Gierth wrote:
>> And none of the plpython transforms can even parse their makefiles with
>> USE_PGXS, let alone build, because they have an "include" directive
>> pointing into src/pl/plpython.
> FWIW, I'd be perfectly on board with just burying this policy. Designating
> one contrib module (or something in src/test/examples or such) as a PGXS
> example, and always building it with pgxs seems like it'd do a much
> better job than the current policy.
No, I think that'd be pretty wrongheaded. One of the big reasons for
contrib to exist is to serve as a testbed proving that our extension
features actually work. What you suggest would reduce the scope of
that testing, which seems like the wrong direction to be going in.
It's particularly bad for these cases, since what they demonstrate is
that it's impossible to build transform modules for plperl or plpython
out-of-tree at the moment. That doesn't seem to me to be something
we should just ignore; it goes against not only our larger commitment
to extensibility, but also the entire point of the transforms feature.
regards, tom lane
>>>>> "Andres" == Andres Freund <andres@anarazel.de> writes: Tom> There's also a question of whether we need to change anything in Tom> contrib/ so that it plays by whatever rules we set. There's an Tom> expectation that contrib modules should be buildable with PGXS, Tom> so they need to follow the rules. Andrew> ... that at least all of the *_plperl transform modules in Andrew> contrib/ fail to build with USE_PGXS already (i.e. for as long Andrew> as they have ever existed), because they rely on Andrew> plperl_helpers.h which is never installed anywhere, and trying Andrew> to get it via $(top_srcdir) obviously can't work in PGXS. Andrew> Haven't tried the python ones yet. >> And none of the plpython transforms can even parse their makefiles >> with USE_PGXS, let alone build, because they have an "include" >> directive pointing into src/pl/plpython. Andres> FWIW, I'd be perfectly on board with just burying this policy. Andres> Designating one contrib module (or something in Andres> src/test/examples or such) as a PGXS example, and always Andres> building it with pgxs seems like it'd do a much better job than Andres> the current policy. I suggest that modules that actually _can't_ build with PGXS should have the PGXS logic removed from their makefiles (perhaps replaced with an error). But for the rest, it's a convenience to be able to build single modules using USE_PGXS without having to configure the whole source tree. -- Andrew (irc:RhodiumToad)
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: Tom> It's particularly bad for these cases, since what they demonstrate Tom> is that it's impossible to build transform modules for plperl or Tom> plpython out-of-tree at the moment. Right. Both plperl and plpython install _some_ header files (which behavior was added in the commit for the transforms feature), but they don't install all of the header files that the existing transform modules actually use. Is this supposed to be a principled choice, with an out-of-tree transform expected to provide their own code instead of using those headers, or is it just an oversight? Tom> That doesn't seem to me to be something we should just ignore; it Tom> goes against not only our larger commitment to extensibility, but Tom> also the entire point of the transforms feature. Yeah, if an out-of-tree data type can't provide a plperl or plpython transform for itself, something's broken. And that does seem to be the case at present. -- Andrew (irc:RhodiumToad)
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: Tom> Something that copes with different modules installing headers Tom> with the same base name. Allowing for that was the driving force Tom> for going with subdirectory-per-extension, but if we really want Tom> that to work, there seems to be no alternative but for extensions Tom> to write qualified header names (#include "hstore/hstore.h" not Tom> #include "hstore.h"). Andres, for one, seemed to think that Tom> wouldn't play nicely with PGXS, I think that was me, not Andres? But I think I was partially wrong and that it's possible that this can be made to work at least in most cases, as long as we can rely on the same-directory rule for #include "foo.h". (i.e. the first place to look is always the same directory as the file containing the #include statement). I'm going to test this now, trying to do an out-of-both-trees build of a transform function for an out-of-tree PL that uses multiple .h files. -- Andrew (irc:RhodiumToad)
On Tue, Jul 31, 2018 at 01:22:27PM -0700, Peter Geoghegan wrote: > On Tue, Jul 31, 2018 at 1:17 PM, Andres Freund <andres@anarazel.de> wrote: >> I'm a bit surprised that you decided to push to the 11 branch - the >> consensus in this thread seem to have gone the other way by my read? >> Given that that's the rule, and pushing non-fixes is the exception, I'm >> not particularly ok with just ignoring that? > > +1 +1. I have been surprised to see that patch pushed to REL_11_STABLE when looking at the new log entries today. -- Michael
Attachment
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: Tom> On the other hand, if there's no very practical way to use the Tom> per-extension subdirectory layout, >> What constitutes "practical"? OK, after considerable experiment I think I can answer these points: the most "practical" way is to do this (or an equivalent), as you originally suggested: PG_CPPFLAGS = -I$(includedir_server)/extension We could add a mention of this to the PGXS docs and the header comment of pgxs.mk as being the recommended practice; would that be enough? Or should the logic go into the pgxs makefile as suggested below? Tom> Something that copes with selecting the right headers if you're Tom> rebuilding a module whose headers are already installed (as you Tom> mentioned upthread). The module would reference its own headers using #include "foo.h", which would not find extension/module/foo.h, so no problem here. No additional constraints apply to the module's own source layout as long as foo.h, if it includes say foo_2.h, does so as #include "foo_2.h" (if that's not in the same subdir as foo.h, the module's own build would need to supply -I options accordingly - no change here). Tom> Something that copes with different modules installing headers Tom> with the same base name. A client of two modules foo and bar can under this scheme do #include "foo/x.h" #include "bar/x.h" and it will reference the correct files; however, if both x.h files use conflicting symbols for their multiple-inclusion guards, then they can't both be included in the same source file without explicit #undef hacks (but this is really an unrelated problem) Internally to foo and bar, they would both do #include "x.h" If both foo/x.h and bar/x.h contain #include "y.h", then that will resolve to extension/foo/y.h in the first case and extension/bar/y.h in the second case under the "look in the directory of the including file first" rule. One case that doesn't "just work" would be what PostGIS currently does. Like other extensions it doesn't (afaik) currently try and install any PG-related header files, but if it was modified to do so, some changes in those header files would be needed because a lot of them have things like #include "../postgis_config.h" which would fail. Another case that doesn't "just work" would be if some extension has a file foo.h that does #include "x/y.h" to include another file that's part of the same extension, expecting to get extension/foo/x/y.h. Right now, the install rule I added to the pgxs makefile puts all header files for a module in the same dir; if we wanted to support making a whole subdirectory tree under extension/modulename, then that'd require more work in the makefiles. Making an out-of-tree build for hstore_plperl etc. work under this scheme would require changes. The in-tree build has this: PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl -I$(top_srcdir)/contrib/hstore #include "hstore.h" This would have to be: PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl -I$(top_srcdir)/contrib #include "hstore/hstore.h" for the in-tree build, and PG_CPPFLAGS = -I$(includedir_server)/extension #include "hstore/hstore.h" for the out-of-tree build. This logic could perhaps be best moved into the pgxs makefile itself, either unconditionally adding -I options to CPPFLAGS, or conditionally adding them based on a WANT_EXTENSION_HEADERS flag of some sort set by the module makefile. -- Andrew (irc:RhodiumToad)
[ back to this after a detour to ON CONFLICT land ]
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> OK, after considerable experiment I think I can answer these points: the
> most "practical" way is to do this (or an equivalent), as you originally
> suggested:
> PG_CPPFLAGS = -I$(includedir_server)/extension
Yeah, that's where I thought we'd end up.
> We could add a mention of this to the PGXS docs and the header comment
> of pgxs.mk as being the recommended practice; would that be enough?
> Or should the logic go into the pgxs makefile as suggested below?
My thought is just to document how to do this. It'll still be true that
most extensions have no dependencies on other extensions, so they won't
need any such headers; sticking extra -I switches into their builds by
default can only cause trouble.
> Tom> Something that copes with selecting the right headers if you're
> Tom> rebuilding a module whose headers are already installed (as you
> Tom> mentioned upthread).
> The module would reference its own headers using #include "foo.h",
> which would not find extension/module/foo.h, so no problem here.
Check, although we also need to document that you should do it that
way. Also, at least with gcc, the rule about "look in the calling
file's directory first" would prevent problems (except in VPATH builds
... does PGXS support that? Should we recommend "-I." before the
"-I$(includedir_server)/extension" switch?)
> One case that doesn't "just work" would be what PostGIS currently does.
> Like other extensions it doesn't (afaik) currently try and install any
> PG-related header files, but if it was modified to do so, some changes
> in those header files would be needed because a lot of them have things
> like #include "../postgis_config.h" which would fail.
Yeah, I don't doubt that extensions will have to make minor mods to
adapt to this scheme. As long as they're minor, I don't think it's
a problem.
> Another case that doesn't "just work" would be if some extension has a
> file foo.h that does #include "x/y.h" to include another file that's
> part of the same extension, expecting to get extension/foo/x/y.h. Right
> now, the install rule I added to the pgxs makefile puts all header files
> for a module in the same dir; if we wanted to support making a whole
> subdirectory tree under extension/modulename, then that'd require more
> work in the makefiles.
Hm. Do we know of any extensions big enough that they need subdirectories
of headers? I don't mind leaving that for later as long as it's not a
present need somewhere. On the other hand, couldn't it "just work" to
write "x/y.h" in the list of headers to install?
> Making an out-of-tree build for hstore_plperl etc. work under this
> scheme would require changes. The in-tree build has this:
> PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl -I$(top_srcdir)/contrib/hstore
> #include "hstore.h"
> This would have to be:
> PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl -I$(top_srcdir)/contrib
> #include "hstore/hstore.h"
> for the in-tree build, and
> PG_CPPFLAGS = -I$(includedir_server)/extension
> #include "hstore/hstore.h"
> for the out-of-tree build.
> This logic could perhaps be best moved into the pgxs makefile itself,
> either unconditionally adding -I options to CPPFLAGS, or conditionally
> adding them based on a WANT_EXTENSION_HEADERS flag of some sort set by
> the module makefile.
I think we'd want to press forward on making that happen, so that
hstore_plperl and friends can serve as copy-and-pasteable prototype
code for out-of-tree transform modules. Do you have an idea how to
fix the other problem you mentioned with the plpython makefiles?
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> The module would reference its own headers using #include "foo.h", >> which would not find extension/module/foo.h, so no problem here. Tom> Check, although we also need to document that you should do it Tom> that way. Also, at least with gcc, the rule about "look in the Tom> calling file's directory first" would prevent problems (except in Tom> VPATH builds ... does PGXS support that? Should we recommend "-I." Tom> before the "-I$(includedir_server)/extension" switch?) PGXS is supposed to support VPATH builds. I do not believe -I. is needed in the normal case, but we should probably document that if the module uses any -I flags of its own they should normally go first. Tom> Hm. Do we know of any extensions big enough that they need Tom> subdirectories of headers? I don't mind leaving that for later as Tom> long as it's not a present need somewhere. On the other hand, Tom> couldn't it "just work" to write "x/y.h" in the list of headers to Tom> install? It doesn't "just work" because (a) all the existing makefile variables that give files to install assume that any path given is the source path only, not to be preserved in the copy; (b) we don't want to constrain the source file layout in a way that would force .h files to be in a specific place. Compare the DATA variable in pgxs: DATA = foo/bar.sql means to install $(srcdir)/foo/bar.sql as $(DESTDIR)$(datadir)/$(datamoduledir)/bar.sql, _not_ as $(DESTDIR)$(datadir)/$(datamoduledir)/foo/bar.sql. Making HEADERS behave otherwise would be inconsistent and inflexible. For example, suppose my extension source dir looks like this: ./Makefile ./foo.control ./src ./src/foo.h ./src/foo1.c ./src/foo2.c ./scripts/foo--1.0.sql I would have these in the makefile: HEADERS = src/foo.h DATA = scripts/foo--1.0.sql and it seems clear to me that that should install foo.h as $(includedir_server)/extension/foo/foo.h and not as foo/src/foo.h. >> Making an out-of-tree build for hstore_plperl etc. work [...] Tom> I think we'd want to press forward on making that happen, so that Tom> hstore_plperl and friends can serve as copy-and-pasteable Tom> prototype code for out-of-tree transform modules. Do you have an Tom> idea how to fix the other problem you mentioned with the plpython Tom> makefiles? The plpython makefiles are including a makefile to munge regression tests for python3 vs python2. The most obvious fix would be to install this makefile as lib/pgxs/src/pl/plpython/regress-python3-mangle.mk (I don't think any other changes would be needed). I'll do some tests on this. -- Andrew (irc:RhodiumToad)
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>> This logic could perhaps be best moved into the pgxs makefile
>> itself, either unconditionally adding -I options to CPPFLAGS, or
>> conditionally adding them based on a WANT_EXTENSION_HEADERS flag of
>> some sort set by the module makefile.
Tom> I think we'd want to press forward on making that happen, so that
Tom> hstore_plperl and friends can serve as copy-and-pasteable
Tom> prototype code for out-of-tree transform modules. Do you have an
Tom> idea how to fix the other problem you mentioned with the plpython
Tom> makefiles?
Here's a patch that fixes (not necessarily in the best way) the PGXS
builds of all the contrib/*_pl{perl,python} modules.
Open questions:
- is there a better way of doing the conditional setting of
PG_CPPFLAGS?
- the choice of which .h files to install from plperl and plpython is
not principled - I just installed the ones needed for the contrib
modules to work. Particularly for plpython this list needs to be
reviewed - but I'm not a pythonist and that should be done by someone
who is.
--
Andrew (irc:RhodiumToad)
diff --git a/contrib/hstore_plperl/Makefile b/contrib/hstore_plperl/Makefile
index f63cba2745..32ecaa43cb 100644
--- a/contrib/hstore_plperl/Makefile
+++ b/contrib/hstore_plperl/Makefile
@@ -4,7 +4,6 @@ MODULE_big = hstore_plperl
OBJS = hstore_plperl.o $(WIN32RES)
PGFILEDESC = "hstore_plperl - hstore transform for plperl"
-PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl -I$(top_srcdir)/contrib/hstore
EXTENSION = hstore_plperl hstore_plperlu
DATA = hstore_plperl--1.0.sql hstore_plperlu--1.0.sql
@@ -13,10 +12,12 @@ REGRESS = hstore_plperl hstore_plperlu create_transform
EXTRA_INSTALL = contrib/hstore
ifdef USE_PGXS
+PG_CPPFLAGS = -I$(includedir_server)/extension
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)
else
+PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl -I$(top_srcdir)/contrib
subdir = contrib/hstore_plperl
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
diff --git a/contrib/hstore_plperl/hstore_plperl.c b/contrib/hstore_plperl/hstore_plperl.c
index c09bd38d09..61b5557421 100644
--- a/contrib/hstore_plperl/hstore_plperl.c
+++ b/contrib/hstore_plperl/hstore_plperl.c
@@ -5,7 +5,7 @@
#include "fmgr.h"
#include "plperl.h"
#include "plperl_helpers.h"
-#include "hstore.h"
+#include "hstore/hstore.h"
PG_MODULE_MAGIC;
diff --git a/contrib/hstore_plpython/Makefile b/contrib/hstore_plpython/Makefile
index b81735ab91..93f3507130 100644
--- a/contrib/hstore_plpython/Makefile
+++ b/contrib/hstore_plpython/Makefile
@@ -4,7 +4,6 @@ MODULE_big = hstore_plpython$(python_majorversion)
OBJS = hstore_plpython.o $(WIN32RES)
PGFILEDESC = "hstore_plpython - hstore transform for plpython"
-PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plpython $(python_includespec) -I$(top_srcdir)/contrib/hstore
-DPLPYTHON_LIBNAME='"plpython$(python_majorversion)"'
EXTENSION = hstore_plpythonu hstore_plpython2u hstore_plpython3u
DATA = hstore_plpythonu--1.0.sql hstore_plpython2u--1.0.sql hstore_plpython3u--1.0.sql
@@ -13,10 +12,12 @@ REGRESS = hstore_plpython
REGRESS_PLPYTHON3_MANGLE := $(REGRESS)
ifdef USE_PGXS
+PG_CPPFLAGS = $(python_includespec) -I$(includedir_server)/extension
-DPLPYTHON_LIBNAME='"plpython$(python_majorversion)"'
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)
else
+PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plpython $(python_includespec) -I$(top_srcdir)/contrib
-DPLPYTHON_LIBNAME='"plpython$(python_majorversion)"'
subdir = contrib/hstore_plpython
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
diff --git a/contrib/hstore_plpython/hstore_plpython.c b/contrib/hstore_plpython/hstore_plpython.c
index 218e6612b1..2f24090ff3 100644
--- a/contrib/hstore_plpython/hstore_plpython.c
+++ b/contrib/hstore_plpython/hstore_plpython.c
@@ -3,7 +3,7 @@
#include "fmgr.h"
#include "plpython.h"
#include "plpy_typeio.h"
-#include "hstore.h"
+#include "hstore/hstore.h"
PG_MODULE_MAGIC;
diff --git a/contrib/ltree_plpython/Makefile b/contrib/ltree_plpython/Makefile
index 7e988c7993..f9e9e8d734 100644
--- a/contrib/ltree_plpython/Makefile
+++ b/contrib/ltree_plpython/Makefile
@@ -4,8 +4,6 @@ MODULE_big = ltree_plpython$(python_majorversion)
OBJS = ltree_plpython.o $(WIN32RES)
PGFILEDESC = "ltree_plpython - ltree transform for plpython"
-PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plpython $(python_includespec) -I$(top_srcdir)/contrib/ltree
-DPLPYTHON_LIBNAME='"plpython$(python_majorversion)"'
-
EXTENSION = ltree_plpythonu ltree_plpython2u ltree_plpython3u
DATA = ltree_plpythonu--1.0.sql ltree_plpython2u--1.0.sql ltree_plpython3u--1.0.sql
@@ -13,10 +11,12 @@ REGRESS = ltree_plpython
REGRESS_PLPYTHON3_MANGLE := $(REGRESS)
ifdef USE_PGXS
+PG_CPPFLAGS = $(python_includespec) -I$(includedir_server)/extension
-DPLPYTHON_LIBNAME='"plpython$(python_majorversion)"'
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)
else
+PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plpython $(python_includespec) -I$(top_srcdir)/contrib
-DPLPYTHON_LIBNAME='"plpython$(python_majorversion)"'
subdir = contrib/ltree_plpython
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
diff --git a/contrib/ltree_plpython/ltree_plpython.c b/contrib/ltree_plpython/ltree_plpython.c
index e88636a0a9..b254aa558d 100644
--- a/contrib/ltree_plpython/ltree_plpython.c
+++ b/contrib/ltree_plpython/ltree_plpython.c
@@ -2,7 +2,7 @@
#include "fmgr.h"
#include "plpython.h"
-#include "ltree.h"
+#include "ltree/ltree.h"
PG_MODULE_MAGIC;
diff --git a/src/pl/plperl/GNUmakefile b/src/pl/plperl/GNUmakefile
index 933abb47c4..39dacf8b2e 100644
--- a/src/pl/plperl/GNUmakefile
+++ b/src/pl/plperl/GNUmakefile
@@ -100,7 +100,7 @@ uninstall: uninstall-lib uninstall-data
install-data: installdirs
$(INSTALL_DATA) $(addprefix $(srcdir)/, $(DATA)) '$(DESTDIR)$(datadir)/extension/'
- $(INSTALL_DATA) $(srcdir)/plperl.h $(srcdir)/ppport.h '$(DESTDIR)$(includedir_server)'
+ $(INSTALL_DATA) $(srcdir)/plperl.h $(srcdir)/ppport.h $(srcdir)/plperl_helpers.h '$(DESTDIR)$(includedir_server)'
uninstall-data:
rm -f $(addprefix '$(DESTDIR)$(datadir)/extension'/, $(notdir $(DATA)))
diff --git a/src/pl/plpython/Makefile b/src/pl/plpython/Makefile
index c17015bbdf..dcc0b07583 100644
--- a/src/pl/plpython/Makefile
+++ b/src/pl/plpython/Makefile
@@ -105,13 +105,14 @@ $(OBJS): | submake-generated-headers
install: all install-lib install-data
installdirs: installdirs-lib
- $(MKDIR_P) '$(DESTDIR)$(datadir)/extension' '$(DESTDIR)$(includedir_server)'
+ $(MKDIR_P) '$(DESTDIR)$(datadir)/extension' '$(DESTDIR)$(includedir_server)'
'$(DESTDIR)$(pgxsdir)/src/pl/plpython'
uninstall: uninstall-lib uninstall-data
install-data: installdirs
$(INSTALL_DATA) $(addprefix $(srcdir)/, $(DATA)) '$(DESTDIR)$(datadir)/extension/'
- $(INSTALL_DATA) $(srcdir)/plpython.h $(srcdir)/plpy_util.h '$(DESTDIR)$(includedir_server)'
+ $(INSTALL_DATA) $(srcdir)/plpython.h $(srcdir)/plpy_util.h $(srcdir)/plpy_typeio.h $(srcdir)/plpy_elog.h
'$(DESTDIR)$(includedir_server)'
+ $(INSTALL_DATA) $(srcdir)/regress-python3-mangle.mk '$(DESTDIR)$(pgxsdir)/src/pl/plpython'
uninstall-data:
rm -f $(addprefix '$(DESTDIR)$(datadir)/extension'/, $(notdir $(DATA)))
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
Andrew> Here's a patch that fixes (not necessarily in the best way) the
Andrew> PGXS builds of all the contrib/*_pl{perl,python} modules.
Oh, obviously this patch doesn't fix the windows Install.pm yet, but
that'd be easier to do after finalizing the list of include files to
install.
--
Andrew (irc:RhodiumToad)
On 01/08/2018 00:34, Peter Eisentraut wrote: > On 23/07/2018 18:32, Andrew Gierth wrote: >>>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> >> Tom> As I said before, I think that we should change the existing >> Tom> contrib modules to be coded likewise, all using a single -I switch >> Tom> that points at SRCDIR/contrib. That'd help give people the right >> Tom> coding model to follow. >> >> I don't see that playing nicely with PGXS? > > I'm also not on board that my random third-party extension now has to > refer to its own header files as "subdirectory/headerfile.h". Which > will mess up existing extensions that have header files in their tree. > > Or at least I'm not totally sure what the exact proposal and real-world > implications are, with regard to existing extensions with one or more > header files. > > By all means, let's make it easier for large or small extensions to > manage their header files with PGXS. But let's separate what PGXS can > and should do from what the extension's own file layout is. > > But I think there are some fundamentally incompatible goals here with > regard to how the final -I options are supposed to look. Was this ever resolved? Seems necessary to resolve for PG11. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> But I think there are some fundamentally incompatible goals here >> with regard to how the final -I options are supposed to look. Peter> Was this ever resolved? There are open questions about plperl and plpython, which currently don't install what's needed for out-of-tree transforms. See the subthread under this message: https://www.postgresql.org/message-id/87o9ej8bgl.fsf%40news-spur.riddles.org.uk Fixing contrib/hstore_plperl etc. to work as examples of how to do things would obviously first require fixing plperl and plpython, and while I'm happy to do that I do need feedback first regarding the plpython headers (I do not use python myself) and the other issues I mentioned in the message that contains a draft patch. -- Andrew (irc:RhodiumToad)
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
Andrew> Here's a patch that fixes (not necessarily in the best way) the
Andrew> PGXS builds of all the contrib/*_pl{perl,python} modules.
Andrew> Oh, obviously this patch doesn't fix the windows Install.pm
Andrew> yet, but that'd be easier to do after finalizing the list of
Andrew> include files to install.
So while looking into this, I found that to the best of my ability to
determine, the windows Install.pm has _never_ installed the plperl or
plpython header files. That's more than I'm in a position to fix, having
no windows box to test with.
So what I propose to do is to commit a cleaned-up version of the patch
posted above, with these changes:
- install all the plpy_*.h headers, not just a few; I know of no reason
to exclude any of them, and in the absence of feedback it seems
better to install them all;
- tidy up the makefile variables in the python transform modules to
remove some duplication
- fix Mkvcbuild.pm to account for the changes
This will allow hstore_plperl etc. to stand as examples of how to do
cross-module #includes both in and out of tree.
I'll post the patch for review shortly.
--
Andrew (irc:RhodiumToad)
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
Andrew> So what I propose to do is to commit a cleaned-up version of
Andrew> the patch posted above, with these changes:
Andrew> - install all the plpy_*.h headers, not just a few; I know of no reason
Andrew> to exclude any of them, and in the absence of feedback it seems
Andrew> better to install them all;
Andrew> - tidy up the makefile variables in the python transform modules to
Andrew> remove some duplication
Andrew> - fix Mkvcbuild.pm to account for the changes
Andrew> This will allow hstore_plperl etc. to stand as examples of how
Andrew> to do cross-module #includes both in and out of tree.
Andrew> I'll post the patch for review shortly.
And here it is.
--
Andrew (irc:RhodiumToad)
diff --git a/contrib/hstore_plperl/Makefile b/contrib/hstore_plperl/Makefile
index f63cba2745..32ecaa43cb 100644
--- a/contrib/hstore_plperl/Makefile
+++ b/contrib/hstore_plperl/Makefile
@@ -4,7 +4,6 @@ MODULE_big = hstore_plperl
OBJS = hstore_plperl.o $(WIN32RES)
PGFILEDESC = "hstore_plperl - hstore transform for plperl"
-PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl -I$(top_srcdir)/contrib/hstore
EXTENSION = hstore_plperl hstore_plperlu
DATA = hstore_plperl--1.0.sql hstore_plperlu--1.0.sql
@@ -13,10 +12,12 @@ REGRESS = hstore_plperl hstore_plperlu create_transform
EXTRA_INSTALL = contrib/hstore
ifdef USE_PGXS
+PG_CPPFLAGS = -I$(includedir_server)/extension
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)
else
+PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl -I$(top_srcdir)/contrib
subdir = contrib/hstore_plperl
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
diff --git a/contrib/hstore_plperl/hstore_plperl.c b/contrib/hstore_plperl/hstore_plperl.c
index c09bd38d09..61b5557421 100644
--- a/contrib/hstore_plperl/hstore_plperl.c
+++ b/contrib/hstore_plperl/hstore_plperl.c
@@ -5,7 +5,7 @@
#include "fmgr.h"
#include "plperl.h"
#include "plperl_helpers.h"
-#include "hstore.h"
+#include "hstore/hstore.h"
PG_MODULE_MAGIC;
diff --git a/contrib/hstore_plpython/Makefile b/contrib/hstore_plpython/Makefile
index b81735ab91..6877e7a072 100644
--- a/contrib/hstore_plpython/Makefile
+++ b/contrib/hstore_plpython/Makefile
@@ -4,19 +4,21 @@ MODULE_big = hstore_plpython$(python_majorversion)
OBJS = hstore_plpython.o $(WIN32RES)
PGFILEDESC = "hstore_plpython - hstore transform for plpython"
-PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plpython $(python_includespec) -I$(top_srcdir)/contrib/hstore
-DPLPYTHON_LIBNAME='"plpython$(python_majorversion)"'
-
EXTENSION = hstore_plpythonu hstore_plpython2u hstore_plpython3u
DATA = hstore_plpythonu--1.0.sql hstore_plpython2u--1.0.sql hstore_plpython3u--1.0.sql
REGRESS = hstore_plpython
REGRESS_PLPYTHON3_MANGLE := $(REGRESS)
+PG_CPPFLAGS = $(python_includespec) -DPLPYTHON_LIBNAME='"plpython$(python_majorversion)"'
+
ifdef USE_PGXS
+PG_CPPFLAGS += -I$(includedir_server)/extension
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)
else
+PG_CPPFLAGS += -I$(top_srcdir)/src/pl/plpython -I$(top_srcdir)/contrib
subdir = contrib/hstore_plpython
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
diff --git a/contrib/hstore_plpython/hstore_plpython.c b/contrib/hstore_plpython/hstore_plpython.c
index 218e6612b1..2f24090ff3 100644
--- a/contrib/hstore_plpython/hstore_plpython.c
+++ b/contrib/hstore_plpython/hstore_plpython.c
@@ -3,7 +3,7 @@
#include "fmgr.h"
#include "plpython.h"
#include "plpy_typeio.h"
-#include "hstore.h"
+#include "hstore/hstore.h"
PG_MODULE_MAGIC;
diff --git a/contrib/ltree_plpython/Makefile b/contrib/ltree_plpython/Makefile
index 7e988c7993..ce2c0cd2e2 100644
--- a/contrib/ltree_plpython/Makefile
+++ b/contrib/ltree_plpython/Makefile
@@ -4,19 +4,21 @@ MODULE_big = ltree_plpython$(python_majorversion)
OBJS = ltree_plpython.o $(WIN32RES)
PGFILEDESC = "ltree_plpython - ltree transform for plpython"
-PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plpython $(python_includespec) -I$(top_srcdir)/contrib/ltree
-DPLPYTHON_LIBNAME='"plpython$(python_majorversion)"'
-
EXTENSION = ltree_plpythonu ltree_plpython2u ltree_plpython3u
DATA = ltree_plpythonu--1.0.sql ltree_plpython2u--1.0.sql ltree_plpython3u--1.0.sql
REGRESS = ltree_plpython
REGRESS_PLPYTHON3_MANGLE := $(REGRESS)
+PG_CPPFLAGS = $(python_includespec) -DPLPYTHON_LIBNAME='"plpython$(python_majorversion)"'
+
ifdef USE_PGXS
+PG_CPPFLAGS += -I$(includedir_server)/extension
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)
else
+PG_CPPFLAGS += -I$(top_srcdir)/src/pl/plpython -I$(top_srcdir)/contrib
subdir = contrib/ltree_plpython
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
diff --git a/contrib/ltree_plpython/ltree_plpython.c b/contrib/ltree_plpython/ltree_plpython.c
index e88636a0a9..b254aa558d 100644
--- a/contrib/ltree_plpython/ltree_plpython.c
+++ b/contrib/ltree_plpython/ltree_plpython.c
@@ -2,7 +2,7 @@
#include "fmgr.h"
#include "plpython.h"
-#include "ltree.h"
+#include "ltree/ltree.h"
PG_MODULE_MAGIC;
diff --git a/src/pl/plperl/GNUmakefile b/src/pl/plperl/GNUmakefile
index 933abb47c4..39dacf8b2e 100644
--- a/src/pl/plperl/GNUmakefile
+++ b/src/pl/plperl/GNUmakefile
@@ -100,7 +100,7 @@ uninstall: uninstall-lib uninstall-data
install-data: installdirs
$(INSTALL_DATA) $(addprefix $(srcdir)/, $(DATA)) '$(DESTDIR)$(datadir)/extension/'
- $(INSTALL_DATA) $(srcdir)/plperl.h $(srcdir)/ppport.h '$(DESTDIR)$(includedir_server)'
+ $(INSTALL_DATA) $(srcdir)/plperl.h $(srcdir)/ppport.h $(srcdir)/plperl_helpers.h '$(DESTDIR)$(includedir_server)'
uninstall-data:
rm -f $(addprefix '$(DESTDIR)$(datadir)/extension'/, $(notdir $(DATA)))
diff --git a/src/pl/plpython/Makefile b/src/pl/plpython/Makefile
index c17015bbdf..667a74469e 100644
--- a/src/pl/plpython/Makefile
+++ b/src/pl/plpython/Makefile
@@ -39,6 +39,21 @@ ifeq ($(python_majorversion),2)
DATA += plpythonu.control plpythonu--1.0.sql plpythonu--unpackaged--1.0.sql
endif
+# header files to install - it's not clear which of these might be needed
+# so install them all.
+INCS = plpython.h \
+ plpy_cursorobject.h \
+ plpy_elog.h \
+ plpy_exec.h \
+ plpy_main.h \
+ plpy_planobject.h \
+ plpy_plpymodule.h \
+ plpy_procedure.h \
+ plpy_resultobject.h \
+ plpy_spi.h \
+ plpy_subxactobject.h \
+ plpy_typeio.h \
+ plpy_util.h
# Python on win32 ships with import libraries only for Microsoft Visual C++,
# which are not compatible with mingw gcc. Therefore we need to build a
@@ -105,13 +120,14 @@ $(OBJS): | submake-generated-headers
install: all install-lib install-data
installdirs: installdirs-lib
- $(MKDIR_P) '$(DESTDIR)$(datadir)/extension' '$(DESTDIR)$(includedir_server)'
+ $(MKDIR_P) '$(DESTDIR)$(datadir)/extension' '$(DESTDIR)$(includedir_server)'
'$(DESTDIR)$(pgxsdir)/src/pl/plpython'
uninstall: uninstall-lib uninstall-data
install-data: installdirs
$(INSTALL_DATA) $(addprefix $(srcdir)/, $(DATA)) '$(DESTDIR)$(datadir)/extension/'
- $(INSTALL_DATA) $(srcdir)/plpython.h $(srcdir)/plpy_util.h '$(DESTDIR)$(includedir_server)'
+ $(INSTALL_DATA) $(addprefix $(srcdir)/, $(INCS)) '$(DESTDIR)$(includedir_server)'
+ $(INSTALL_DATA) $(srcdir)/regress-python3-mangle.mk '$(DESTDIR)$(pgxsdir)/src/pl/plpython'
uninstall-data:
rm -f $(addprefix '$(DESTDIR)$(datadir)/extension'/, $(notdir $(DATA)))
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 9ce03ce684..6484a67223 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -506,7 +506,7 @@ sub mkvcbuild
my $hstore_plpython = AddTransformModule(
'hstore_plpython' . $pymajorver, 'contrib/hstore_plpython',
'plpython' . $pymajorver, 'src/pl/plpython',
- 'hstore', 'contrib/hstore');
+ 'hstore', 'contrib');
$hstore_plpython->AddDefine(
'PLPYTHON_LIBNAME="plpython' . $pymajorver . '"');
my $jsonb_plpython = AddTransformModule(
@@ -517,7 +517,7 @@ sub mkvcbuild
my $ltree_plpython = AddTransformModule(
'ltree_plpython' . $pymajorver, 'contrib/ltree_plpython',
'plpython' . $pymajorver, 'src/pl/plpython',
- 'ltree', 'contrib/ltree');
+ 'ltree', 'contrib');
$ltree_plpython->AddDefine(
'PLPYTHON_LIBNAME="plpython' . $pymajorver . '"');
}
@@ -753,7 +753,7 @@ sub mkvcbuild
my $hstore_plperl = AddTransformModule(
'hstore_plperl', 'contrib/hstore_plperl',
'plperl', 'src/pl/plperl',
- 'hstore', 'contrib/hstore');
+ 'hstore', 'contrib');
my $jsonb_plperl = AddTransformModule(
'jsonb_plperl', 'contrib/jsonb_plperl',
'plperl', 'src/pl/plperl');