Thread: Bugfix and new feature for PGXS
I've rebased the current set of pending patches I had, to fix pgxs and added 2 new patches. Bugfixes have already been presented and sent in another thread, except the last one which only fix comment in pgxs.mk. The new feature consists in a new variable to allow the installation of contrib header files. A new variable MODULEHEADER can be used in extension Makefile to list the header files to install. The installation path default to $includedir/$includedir_contrib/$extension. 2 new variables are set to define directories: $includedir_contrib and $includedir_extension, the former default to include/contrib and the later to $includedir_contrib/$extension ($extension is the name of the extension). This allows for example to install hstore header and be able to include them in another extension like that: # include "contrib/hstore/hstore.h" For packagers of PostgreSQL, this allows to have a postgresql-X.Y-contrib-dev package. For developers of extension it's killing the copy-and-paste-this-function dance previously required. I've not updated contribs' Makfefile: I'm not sure what we want to expose. I've 2 other patches to write to automatize a bit more the detection of things to do when building with USE_PGXS, based on the layout. Better get a good consensus on this before writing them. Bugfix: 0001-fix-SHLIB_PREREQS-when-building-with-USE_PGXS.patch 0002-Create-data-directory-if-extension-when-required.patch 0003-set-VPATH-for-out-of-tree-extension-builds.patch 0004-adds-support-for-VPATH-with-USE_PGXS.patch 0006-Fix-suggested-layout-for-extension.patch New feature: 0005-Allows-extensions-to-install-header-file.patch I'll do a documentation patch based on what is accepted in HEAD and/or previous branches. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
Attachment
- 0001-fix-SHLIB_PREREQS-when-building-with-USE_PGXS.patch
- 0002-Create-data-directory-if-extension-when-required.patch
- 0003-set-VPATH-for-out-of-tree-extension-builds.patch
- 0004-adds-support-for-VPATH-with-USE_PGXS.patch
- 0006-Fix-suggested-layout-for-extension.patch
- 0005-Allows-extensions-to-install-header-file.patch
- signature.asc
On Tue, 2013-06-18 at 15:52 +0200, Cédric Villemain wrote: > This allows for example to install hstore header and be able to > include them > in another extension like that: > > # include "contrib/hstore/hstore.h" That's not going to work. hstore's header file is included as #include "hstore.h" (cf. hstore source code). Having it included under different paths in different contexts will be a mess.
Le mercredi 19 juin 2013 04:48:11, Peter Eisentraut a écrit : > On Tue, 2013-06-18 at 15:52 +0200, Cédric Villemain wrote: > > This allows for example to install hstore header and be able to > > include them > > > > in another extension like that: > > # include "contrib/hstore/hstore.h" > > That's not going to work. hstore's header file is included as #include > "hstore.h" (cf. hstore source code). Having it included under different > paths in different contexts will be a mess. OK. At the beginning I though of putting headers in include/contrib but feared that some header may have the same name. If you think that it is suitable, I can do that ? (and include the clean: recipe that I missed on the first shot) Also, do we want to keep the word 'contrib' ? include/extension looks fine too... -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
On 6/19/13 5:55 AM, Cédric Villemain wrote: > Le mercredi 19 juin 2013 04:48:11, Peter Eisentraut a écrit : >> On Tue, 2013-06-18 at 15:52 +0200, Cédric Villemain wrote: >>> This allows for example to install hstore header and be able to >>> include them >>> >>> in another extension like that: >>> # include "contrib/hstore/hstore.h" >> >> That's not going to work. hstore's header file is included as #include >> "hstore.h" (cf. hstore source code). Having it included under different >> paths in different contexts will be a mess. > > OK. > At the beginning I though of putting headers in include/contrib but feared > that some header may have the same name. > If you think that it is suitable, I can do that ? (and include the clean: > recipe that I missed on the first shot) I don't think there is any value in moving the contrib header files around.
On 06/19/2013 10:06 AM, Peter Eisentraut wrote: > On 6/19/13 5:55 AM, Cédric Villemain wrote: >> Le mercredi 19 juin 2013 04:48:11, Peter Eisentraut a écrit : >>> On Tue, 2013-06-18 at 15:52 +0200, Cédric Villemain wrote: >>>> This allows for example to install hstore header and be able to >>>> include them >>>> >>>> in another extension like that: >>>> # include "contrib/hstore/hstore.h" >>> That's not going to work. hstore's header file is included as #include >>> "hstore.h" (cf. hstore source code). Having it included under different >>> paths in different contexts will be a mess. >> OK. >> At the beginning I though of putting headers in include/contrib but feared >> that some header may have the same name. >> If you think that it is suitable, I can do that ? (and include the clean: >> recipe that I missed on the first shot) > I don't think there is any value in moving the contrib header files around. > > > What are they going to be used for anyway? I rubbed up against this not too long ago. Things will blow up if you use stuff from the module and the module isn't already loaded. cheers andrew
On 6/19/13 10:19 AM, Andrew Dunstan wrote: > What are they going to be used for anyway? I rubbed up against this not > too long ago. Things will blow up if you use stuff from the module and > the module isn't already loaded. See transforms.
On 06/19/2013 11:26 AM, Peter Eisentraut wrote: > On 6/19/13 10:19 AM, Andrew Dunstan wrote: >> What are they going to be used for anyway? I rubbed up against this not >> too long ago. Things will blow up if you use stuff from the module and >> the module isn't already loaded. > See transforms. > So you're saying to install extension headers, but into the main directory where we put server headers? cheers andrew
Le mercredi 19 juin 2013 18:20:11, Andrew Dunstan a écrit : > On 06/19/2013 11:26 AM, Peter Eisentraut wrote: > > On 6/19/13 10:19 AM, Andrew Dunstan wrote: > >> What are they going to be used for anyway? I rubbed up against this not > >> too long ago. Things will blow up if you use stuff from the module and > >> the module isn't already loaded. > > > > See transforms. > > So you're saying to install extension headers, but into the main > directory where we put server headers? At the same level than server headers, but I'm open for suggestion. $ tree -d include include ├── libpq └── postgresql ├── contrib │ └── hstore ├── informix │ └── esql ├── internal │ └── libpq └── server And all subidrs of server/ -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
On 06/19/2013 12:32 PM, Cédric Villemain wrote: > Le mercredi 19 juin 2013 18:20:11, Andrew Dunstan a écrit : >> On 06/19/2013 11:26 AM, Peter Eisentraut wrote: >>> On 6/19/13 10:19 AM, Andrew Dunstan wrote: >>>> What are they going to be used for anyway? I rubbed up against this not >>>> too long ago. Things will blow up if you use stuff from the module and >>>> the module isn't already loaded. >>> See transforms. >> So you're saying to install extension headers, but into the main >> directory where we put server headers? > At the same level than server headers, but I'm open for suggestion. > > $ tree -d include > include > ├── libpq > └── postgresql > ├── contrib > │ └── hstore > ├── informix > │ └── esql > ├── internal > │ └── libpq > └── server > > And all subidrs of server/ This is what Peter was arguing against, isn't it? cheers andrew
On 6/19/13 12:20 PM, Andrew Dunstan wrote: > So you're saying to install extension headers, but into the main > directory where we put server headers? Yes, if we choose to install some extension headers, that is where we should put them.
Le mercredi 19 juin 2013 18:48:21, Andrew Dunstan a écrit : > On 06/19/2013 12:32 PM, Cédric Villemain wrote: > > Le mercredi 19 juin 2013 18:20:11, Andrew Dunstan a écrit : > >> On 06/19/2013 11:26 AM, Peter Eisentraut wrote: > >>> On 6/19/13 10:19 AM, Andrew Dunstan wrote: > >>>> What are they going to be used for anyway? I rubbed up against this > >>>> not too long ago. Things will blow up if you use stuff from the > >>>> module and the module isn't already loaded. > >>> > >>> See transforms. > >> > >> So you're saying to install extension headers, but into the main > >> directory where we put server headers? > > > > At the same level than server headers, but I'm open for suggestion. > > > > $ tree -d include > > include > > ├── libpq > > └── postgresql > > > > ├── contrib > > │ └── hstore > > ├── informix > > │ └── esql > > ├── internal > > │ └── libpq > > └── server > > > > And all subidrs of server/ > > This is what Peter was arguing against, isn't it? Now I have a doubt :) I believe he answered the proposal to put all headers on the same flat directory, instead of a tree. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
Peter Eisentraut wrote: > On 6/19/13 12:20 PM, Andrew Dunstan wrote: > > So you're saying to install extension headers, but into the main > > directory where we put server headers? > > Yes, if we choose to install some extension headers, that is where we > should put them. The question of the name of the directory still stands. "contrib" would be the easiest answer, but it's slightly wrong because externally-supplied modules could also want to install headers. "extension" might be it, but there are things that aren't extensions (right? if not, that would be my choice). -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Le mercredi 19 juin 2013 21:06:23, Alvaro Herrera a écrit : > Peter Eisentraut wrote: > > On 6/19/13 12:20 PM, Andrew Dunstan wrote: > > > So you're saying to install extension headers, but into the main > > > directory where we put server headers? > > > > Yes, if we choose to install some extension headers, that is where we > > should put them. > > The question of the name of the directory still stands. "contrib" would > be the easiest answer, but it's slightly wrong because > externally-supplied modules could also want to install headers. > "extension" might be it, but there are things that aren't extensions > (right? if not, that would be my choice). yes, I think the same. auth_delay for example is not an extension as in CREATE EXTENSION. So...it is probably better to postpone this decision and keep on the idea to just install headers where there should be will traditional name (contrib). -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
On Wed, 2013-06-19 at 20:58 +0200, Cédric Villemain wrote: > I believe he answered the proposal to put all headers on the same flat > directory, instead of a tree. The headers are used as #include "hstore.h" #include "ltree.h" etc. in the existing source code. If you want to install the for use by others, you can do one of three things: 1) Install them into $(pg_config --includedir-server), so other users will just include them in the same way as shown above. 2) Install them in a different directory, but keep the same #include lines. That would require PGXS changes, perhaps a new pg_config option, or something that produces the right -I option to find them. 3) Install them in a different directory and require a different #include line. But then you have to change the existing uses as well, which would probably require moving files around. Both 2 and 3 require a lot of work, possibly compatibility breaks, for no obvious reason.
On 06/20/2013 11:26 AM, Peter Eisentraut wrote:
> 3) Install them in a different directory and require a different
> #include line. But then you have to change the existing uses as well,
> which would probably require moving files around.
If I understand you correctly, your concern seems to be with referring
to the extensions headers within that extension. For example, a pgxs
build of hstore using "contrib/hstore/hstore.h" wouldn't work if
"hstore.h" was in the current location, the root of the hstore contrib
module. It'd be fine for a non-pgxs build, since we'd just add
$(top_srcdir) to the include path when building contrib modules in-tree.
So, for pgxs, we'd have to construct a temporary include dir, copying
hstore.h into a temporary 'contrib/hstore' subdir for the build. Which
is messy, but would work, and would confine the change mostly to pgxs.
Or we'd have to move it there permanently in the source tree, which
seems kind of excessive. There's a certain appeal in having extensions
follow the same model as the main server code:
hstore/ src/ contrib hstore crc32.c hstore_compat.c hstore_gin.c hstore_gist.c
hstore_io.c hstore_op.c include contrib hstore hstore.h
... but that's pretty significant disruption for something I was hoping
would be a rather small change.
As you note, the other option is to just refer to extension headers by
their unqualified name. I'm *really* not a fan of that idea due to the
obvious clash possibilities with Pg's own headers or system headers,
especially given that the whole idea of extensions is that they're user
supplied. Not having any kind of namespacing is worrying. What could
possibly work would be to install extension headers in
contrib/[modulename]/ and automatically have pgxs and the in-tree build
add appropriate -I directives to those subdirs based on dependencies
declared in the Makefile. Again, though, that makes the whole thing a
lot more complex than I'd like.
Drat.
A final option: When building the extension its self, use "hstore.h".
When referring to it from elsewhere, use "contrib/hstore/hstore.h". In
pgxs's case it'd be installed, in an in-tree build it'd be handled by
adding ${top_srcdir} to the include path when we're building contribs.
Opinions all?
* Give up on being able to use one extension's header from another
entirely, except in the special case of in-tree builds
* Install install-enabled contrib headers into an include/contrib/
that's always on the pgxs include path, so you can still just "#include
hstore.h" . For in-tree builds, explicit add other modules' contrib dirs
as Peter has done in the proposed plperl_hstore and plpython_hstore.
(Use unqualified names).
* Install install-enabled contrib headers to
include/contrib/[modulename]/ . Within the module, use the unqualified
header name. When referring to it from outside the module, use #include
"contrib/modulename/header.h". Add $(top_srcdir) to the include path
when building extensions in-tree.
Personally, I'm all for just using the local path when building the
module, and the qualified path elsewhere. It requires only a relatively
simple change, and I don't think that using "hstore.h" within hstore,
and "contrib/hstore/hstore.h" elsewhere is of great concern.
-- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Le jeudi 20 juin 2013 05:26:21, Peter Eisentraut a écrit :
> On Wed, 2013-06-19 at 20:58 +0200, Cédric Villemain wrote:
> > I believe he answered the proposal to put all headers on the same flat
> > directory, instead of a tree.
>
> The headers are used as
>
> #include "hstore.h"
> #include "ltree.h"
> etc.
>
> in the existing source code.
>
> If you want to install the for use by others, you can do one of three
> things:
>
> 1) Install them into $(pg_config --includedir-server), so other users
> will just include them in the same way as shown above.
I don't like this one because extension can overwrite sever headers.
> 2) Install them in a different directory, but keep the same #include
> lines. That would require PGXS changes, perhaps a new pg_config option,
> or something that produces the right -I option to find them.
Patch of PGXS is not a problem.
pg_config patch is a requirement only if users set their own
$(includedir_contrib) variable. I didn't though about it, but it is probably
better to add that. This looks trivial too.
To include hstore header we write «#include "hstore.h"» but we add :-I$(includedir_contrib)/hstore to the FLAGS in the
extensionmakefile which
does require hstore. It changes nothing to hstore Makefile itself.
The main difference is that before we had to -I$(top_srcdir)/../contrib/hstore
*iff* we are not building with PGXS (else we need to have the hstore source
available somewhere), now we can do -I$(includedir_contrib)/hstore with PGXS
(hstore need to be installed first, as we USE_PGXS)
Then PGXS offers to catch both case transparently so we can do :
-I${include_contrib_header) in Makefile
and pgxs.mk takes care of adjusting include_contrib_header (or whatever its
name) according to USE_PGXS value.
> 3) Install them in a different directory and require a different
> #include line. But then you have to change the existing uses as well,
> which would probably require moving files around.
Having to change existing source code do keep the same behavior is not
attractive at all.
> Both 2 and 3 require a lot of work, possibly compatibility breaks, for
> no obvious reason.
2 is a good solution and not a lot of work, IMO.
Removing the need of setting -I$(include...) in the contrib Makefile can be
done later by adding a PGXS variable to define the contrib requirements, this
is something different from the current feature (build contrib depending on
another(s) without full source tree)
--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation
> Opinions all? > > * Give up on being able to use one extension's header from another > entirely, except in the special case of in-tree builds There are already a good number of use cases with only hstore and intarray, I'm in favor of this feature. > * Install install-enabled contrib headers into an include/contrib/ > that's always on the pgxs include path, so you can still just "#include > hstore.h" . For in-tree builds, explicit add other modules' contrib dirs > as Peter has done in the proposed plperl_hstore and plpython_hstore. > (Use unqualified names). I don't like the idea to share a flat directory with different header files, filenames can overlap. > * Install install-enabled contrib headers to > include/contrib/[modulename]/ . Within the module, use the unqualified > header name. When referring to it from outside the module, use #include > "contrib/modulename/header.h". Add $(top_srcdir) to the include path > when building extensions in-tree. not either, see my other mail. we still #include hstore.h when we want, we just add that the header so it is available for PGXS builds. Contrib Makefile still need to -I$(includedir_contrib)/hstore. What's new is that the header is installed, not that we don't have to set FLAGS. > Personally, I'm all for just using the local path when building the > module, and the qualified path elsewhere. It requires only a relatively > simple change, and I don't think that using "hstore.h" within hstore, > and "contrib/hstore/hstore.h" elsewhere is of great concern. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
On 6/20/13 1:21 AM, Craig Ringer wrote: > As you note, the other option is to just refer to extension headers by > their unqualified name. I'm *really* not a fan of that idea due to the > obvious clash possibilities with Pg's own headers or system headers, > especially given that the whole idea of extensions is that they're user > supplied. Not having any kind of namespacing is worrying. We already install all PostgreSQL server header files into a separate directory, so the only clashes you have to worry about are with other extensions and with the backend. And you have to do that anyway, because you will have namespace issues for the shared libraries, the symbols in those libraries, and the extension names.
On 6/20/13 1:21 AM, Craig Ringer wrote: > Personally, I'm all for just using the local path when building the > module, and the qualified path elsewhere. It requires only a relatively > simple change, and I don't think that using "hstore.h" within hstore, > and "contrib/hstore/hstore.h" elsewhere is of great concern. It doesn't work if those header files include other header files (as in the case of plpython, for example).
On 06/18/2013 09:52 AM, Cédric Villemain wrote: > I've rebased the current set of pending patches I had, to fix pgxs and added 2 > new patches. > > Bugfixes have already been presented and sent in another thread, except the > last one which only fix comment in pgxs.mk. > > The new feature consists in a new variable to allow the installation of > contrib header files. > A new variable MODULEHEADER can be used in extension Makefile to list the > header files to install. > > The installation path default to $includedir/$includedir_contrib/$extension. > 2 new variables are set to define directories: $includedir_contrib and > $includedir_extension, the former default to include/contrib and the later to > $includedir_contrib/$extension ($extension is the name of the extension). > > This allows for example to install hstore header and be able to include them > in another extension like that: > > # include "contrib/hstore/hstore.h" > > For packagers of PostgreSQL, this allows to have a postgresql-X.Y-contrib-dev > package. > For developers of extension it's killing the copy-and-paste-this-function > dance previously required. > > I've not updated contribs' Makfefile: I'm not sure what we want to expose. > > I've 2 other patches to write to automatize a bit more the detection of things > to do when building with USE_PGXS, based on the layout. Better get a good > consensus on this before writing them. > > > Bugfix: > 0001-fix-SHLIB_PREREQS-when-building-with-USE_PGXS.patch > 0002-Create-data-directory-if-extension-when-required.patch > 0003-set-VPATH-for-out-of-tree-extension-builds.patch > 0004-adds-support-for-VPATH-with-USE_PGXS.patch > 0006-Fix-suggested-layout-for-extension.patch > > New feature: > 0005-Allows-extensions-to-install-header-file.patch > > I'll do a documentation patch based on what is accepted in HEAD and/or > previous branches. I have just spent an hour or two wrestling with the first four of these. Items 5 and six are really separate items, which I'm not considering at the moment. The trouble I have is that there are no instructions on how to modify your Makefile or prepare a vpath tree, so I have been flying blind. I started out doing what Postgres does, namely to prepare the tree using config/prep_buildtree. This doesn't work at all - the paths don't get set properly unless you invoke make with the path to the real makefile, and I'm not sure they all get set correctly then. But that's not what we expect of a vpath setup, or at least not what I expect. I expect that with an appropriately set up make file and a correctly set up build tree I can use an identical make invocation to the one I would use for an in-tree build. That is, after setting up I should be able to ignore the fact that it's a vpath build. FYI I deliberately chose a non-core extension with an uncommon layout for testing: json_build <https://github.com/pgexperts/json_build> So, patches 1, 2 and 6 look sane enough, and I'm inclined to commit them just to get them out of the way. That means two of the commitfest items I am signed up for will be committed and two marked "Returned with comments". I think we need some more discussion about how VPATH builds should work with extensions, and subject to what limitations if any. cheers andrew
Le lundi 24 juin 2013 19:40:19, Andrew Dunstan a écrit : > On 06/18/2013 09:52 AM, Cédric Villemain wrote: > > I've rebased the current set of pending patches I had, to fix pgxs and > > added 2 new patches. > > > > Bugfixes have already been presented and sent in another thread, except > > the last one which only fix comment in pgxs.mk. > > > > The new feature consists in a new variable to allow the installation of > > contrib header files. > > A new variable MODULEHEADER can be used in extension Makefile to list the > > header files to install. > > > > The installation path default to > > $includedir/$includedir_contrib/$extension. 2 new variables are set to > > define directories: $includedir_contrib and $includedir_extension, the > > former default to include/contrib and the later to > > $includedir_contrib/$extension ($extension is the name of the > > extension). > > > > This allows for example to install hstore header and be able to include > > them > > > > in another extension like that: > > # include "contrib/hstore/hstore.h" > > > > For packagers of PostgreSQL, this allows to have a > > postgresql-X.Y-contrib-dev package. > > For developers of extension it's killing the copy-and-paste-this-function > > dance previously required. > > > > I've not updated contribs' Makfefile: I'm not sure what we want to > > expose. > > > > I've 2 other patches to write to automatize a bit more the detection of > > things to do when building with USE_PGXS, based on the layout. Better > > get a good consensus on this before writing them. > > > > > > Bugfix: > > 0001-fix-SHLIB_PREREQS-when-building-with-USE_PGXS.patch > > 0002-Create-data-directory-if-extension-when-required.patch > > 0003-set-VPATH-for-out-of-tree-extension-builds.patch > > 0004-adds-support-for-VPATH-with-USE_PGXS.patch > > 0006-Fix-suggested-layout-for-extension.patch > > > > New feature: > > 0005-Allows-extensions-to-install-header-file.patch > > > > I'll do a documentation patch based on what is accepted in HEAD and/or > > previous branches. > > I have just spent an hour or two wrestling with the first four of these. > Items 5 and six are really separate items, which I'm not considering at > the moment. > > The trouble I have is that there are no instructions on how to modify > your Makefile or prepare a vpath tree, so I have been flying blind. I > started out doing what Postgres does, namely to prepare the tree using > config/prep_buildtree. This doesn't work at all - the paths don't get > set properly unless you invoke make with the path to the real makefile, > and I'm not sure they all get set correctly then. But that's not what we > expect of a vpath setup, or at least not what I expect. I expect that > with an appropriately set up make file and a correctly set up build tree > I can use an identical make invocation to the one I would use for an > in-tree build. That is, after setting up I should be able to ignore the > fact that it's a vpath build. > > FYI I deliberately chose a non-core extension with an uncommon layout > for testing: json_build <https://github.com/pgexperts/json_build> > > So, patches 1, 2 and 6 look sane enough, and I'm inclined to commit them > just to get them out of the way. That means two of the commitfest items > I am signed up for will be committed and two marked "Returned with > comments". I think we need some more discussion about how VPATH builds > should work with extensions, and subject to what limitations if any. Thank you for reviewing those patches. I'm sorry I haven't reproduced nor explained that much what is expected with VPATH. We have 2 main options with core postgresql: in-tree build or out-of-tree. The former is the classical build process, the later is what's frequently use for packaging because it allows not to pollute the source tree with intermediate or fully built files. You're right that we don't need to set VPATH explicitely, it is set by 'configure' when you exec configure out-of-source-tree. It is transparent to the user. Those 2 options are working as expected. Now, the extensions/contribs. We have 2^2 ways to build them. Either with PGXS or not, either with VPATH or not. NO_PGXS is how we build contribs shipped with postgresql. USE_PGXS is used by most of the others extensions (and contribs). WIth extension, we do have to set VPATH explicitely if we want to use VPATH (note that contribs/extensions must not care that postgresql has been built with or without VPATH set). My patches try to fix that. So the point is to be able to do exactly that : $ mkdir /tmp/json_build && cd /tmp/json_build $ make USE_PGXS=1 -f /path/to/json_build/Makefile There is another way to do the same thing which is: $ mkdir /tmp/json_build $ make USE_PGXS=1 -C /tmp/json_build -f /path/to/json_build/Makefile Thus packagers can now use : $ make USE_PGXS=1 -C $vtarget -f /path/to/Makefile DESTDIR=/my/packaging/area/json_build instead of (short version): $ make USE_PGXS=1 -C $vtarget -f /path/to/Makefile DESTDIR="$srcdir/debian/$package" VPATH="$srcdir" srcdir="$srcdir" (please note the $srcdir to workaround the pgxs way of working) For patch 0003, the point is to make the usage of VPATH transparent for the users (as we do for core postgresql) For patch 0004, the point is to take the required files from the good location. $^ and $< contains the filepath which is where 'make' found the files, but $(DATA) and others PGXS variables contains the list of required files, not their filepath. As long as you build in-tree you won't hit this problem, but if you try to build out-of-source tree, the .control file for example, then make won't find the .control you just *built*, it'll try to copy it from original source-tree to follow this recipe: $(INSTALL_DATA) $(addprefix $(srcdir)/, $(addsuffix .control, $(EXTENSION))) '$(DESTDIR)$(datadir)/extension/' But this is 'make' job to find the file, we should not write that strongly as we do. This should be: $(INSTALL_DATA) $< '$(DESTDIR)$(datadir)/extension/' This is what this patch fixes. Are those explanations ok ? PS: out-of-source tree builds are a nice way to prevent collision between 32bit and 64bits built share object, or even more worse situation with kfreeBSD vs linux, etc. this also makes 'make clean' as fast as a a 'drop table'. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
On 06/24/2013 04:02 PM, Cédric Villemain wrote: > > WIth extension, we do have to set VPATH explicitely if we want to use VPATH > (note that contribs/extensions must not care that postgresql has been built > with or without VPATH set). My patches try to fix that. No, this is exactly what I'm objecting to. I want to be able to do: invoke_vpath_magic standard_make_commands_as_for_non_vpath Your patches have been designed to overcome your particular issues, but they don't meet the general case, IMNSHO. This is why I want to have more discussion about how vpath builds could work for PGXS modules. > > So the point is to be able to do exactly that : > > $ mkdir /tmp/json_build && cd /tmp/json_build > $ make USE_PGXS=1 -f /path/to/json_build/Makefile It doesn't work: [andrew@emma vpath.json_build]$ PATH=../../inst.vpgxs.5706/bin:$PATH make -f `pwd`/../json_build/Makefile grep: json_build.control:No such file or directory grep: json_build.control: No such file or directory grep: json_build.control:No such file or directory grep: json_build.control: No such file or directory grep: json_build.control:No such file or directory grep: json_build.control: No such file or directory grep: json_build.control:No such file or directory grep: json_build.control: No such file or directory grep: json_build.control:No such file or directory grep: json_build.control: No such file or directory grep: json_build.control:No such file or directory gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement-Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv-fexcess-precision=standard -g -fpic -shared -o json_build.so -L/home/pgl/npgl/inst.vpgxs.5706/lib -Wl,--as-needed -Wl,-rpath,'/home/pgl/npgl/inst.vpgxs.5706/lib',--enable-new-dtags cp /home/andrew/pgl/extns/vpath.json_build/../json_build/sql/json_build.sql sql/json_build--.sql cp: cannot create regularfile `sql/json_build--.sql': No such file or directory make: *** [sql/json_build--.sql] Error 1 [andrew@emmavpath.json_build]$ cheers andrew
Le mardi 25 juin 2013 00:18:26, Andrew Dunstan a écrit : > On 06/24/2013 04:02 PM, Cédric Villemain wrote: > > WIth extension, we do have to set VPATH explicitely if we want to use > > VPATH (note that contribs/extensions must not care that postgresql has > > been built with or without VPATH set). My patches try to fix that. > > No, this is exactly what I'm objecting to. I want to be able to do: > > invoke_vpath_magic > standard_make_commands_as_for_non_vpath > > Your patches have been designed to overcome your particular issues, but > they don't meet the general case, IMNSHO. This is why I want to have > more discussion about how vpath builds could work for PGXS modules. The patch does not restrict anything, it is not supposed to lead to regression. The assignment of VPATH and srcdir are wrong in the PGXS case, the patch correct them. You're still free to do "make VPATH=/mypath ..." the VPATH provided won't be erased by the pgxs.mk logic. > > So the point is to be able to do exactly that : > > > > $ mkdir /tmp/json_build && cd /tmp/json_build > > $ make USE_PGXS=1 -f /path/to/json_build/Makefile > > It doesn't work: > > [andrew@emma vpath.json_build]$ PATH=../../inst.vpgxs.5706/bin:$PATH > make -f `pwd`/../json_build/Makefile > grep: json_build.control: No such file or directory > grep: json_build.control: No such file or directory > grep: json_build.control: No such file or directory > grep: json_build.control: No such file or directory > grep: json_build.control: No such file or directory > grep: json_build.control: No such file or directory > grep: json_build.control: No such file or directory > grep: json_build.control: No such file or directory > grep: json_build.control: No such file or directory > grep: json_build.control: No such file or directory > grep: json_build.control: No such file or directory > gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith > -Wdeclaration-after-statement -Wendif-labels > -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing > -fwrapv -fexcess-precision=standard -g -fpic -shared -o > json_build.so -L/home/pgl/npgl/inst.vpgxs.5706/lib -Wl,--as-needed > -Wl,-rpath,'/home/pgl/npgl/inst.vpgxs.5706/lib',--enable-new-dtags > cp > > /home/andrew/pgl/extns/vpath.json_build/../json_build/sql/json_build.sql > sql/json_build--.sql > cp: cannot create regular file `sql/json_build--.sql': No such file > or directory > make: *** [sql/json_build--.sql] Error 1 > [andrew@emma vpath.json_build]$ Ah, you make the point : your Makefile does not support VPATH or I failed to consider some cases. It seems to me that : EXTVERSION = $(shell grep default_version $(EXTENSION).control | sed -e "s/default_version[[:space:]]*=[[:space:]]*'\([^']*\)'/\1/") is not working. I'm going to check GNU Make guidelines about that case (should $(command) be executed on each path in the VPATH or not ?) [...] thinking a bit more... I suppose gmake expects the Makefile to list $(EXTENSION).control file as a prerequisite (thus its paths will be known during make invocation and your command will work) -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
On 06/24/2013 07:24 PM, Cédric Villemain wrote: > Le mardi 25 juin 2013 00:18:26, Andrew Dunstan a écrit : >> On 06/24/2013 04:02 PM, Cédric Villemain wrote: >>> WIth extension, we do have to set VPATH explicitely if we want to use >>> VPATH (note that contribs/extensions must not care that postgresql has >>> been built with or without VPATH set). My patches try to fix that. >> No, this is exactly what I'm objecting to. I want to be able to do: >> >> invoke_vpath_magic >> standard_make_commands_as_for_non_vpath >> >> Your patches have been designed to overcome your particular issues, but >> they don't meet the general case, IMNSHO. This is why I want to have >> more discussion about how vpath builds could work for PGXS modules. > The patch does not restrict anything, it is not supposed to lead to > regression. > The assignment of VPATH and srcdir are wrong in the PGXS case, the patch > correct them. You're still free to do "make VPATH=/mypath ..." the VPATH > provided won't be erased by the pgxs.mk logic. I still think this is premature. I have spent some more time trying to make it work the way I think it should, so far without success. I think we need more discussion about how we'd like VPATH to work for PGXS would. There's no urgency about this - we've lived with the current situation for quite a while. When I have more time I will work on it some more. cheers andrew
Le mardi 25 juin 2013 17:18:51, Andrew Dunstan a écrit : > On 06/24/2013 07:24 PM, Cédric Villemain wrote: > > Le mardi 25 juin 2013 00:18:26, Andrew Dunstan a écrit : > >> On 06/24/2013 04:02 PM, Cédric Villemain wrote: > >>> WIth extension, we do have to set VPATH explicitely if we want to use > >>> VPATH (note that contribs/extensions must not care that postgresql has > >>> been built with or without VPATH set). My patches try to fix that. > >> > >> No, this is exactly what I'm objecting to. I want to be able to do: > >> invoke_vpath_magic > >> standard_make_commands_as_for_non_vpath > >> > >> Your patches have been designed to overcome your particular issues, but > >> they don't meet the general case, IMNSHO. This is why I want to have > >> more discussion about how vpath builds could work for PGXS modules. > > > > The patch does not restrict anything, it is not supposed to lead to > > regression. > > The assignment of VPATH and srcdir are wrong in the PGXS case, the patch > > correct them. You're still free to do "make VPATH=/mypath ..." the VPATH > > provided won't be erased by the pgxs.mk logic. > > I still think this is premature. I have spent some more time trying to > make it work the way I think it should, so far without success. I think > we need more discussion about how we'd like VPATH to work for PGXS > would. There's no urgency about this - we've lived with the current > situation for quite a while. Sure... I did a quick and dirty patch (I just validate without lot of testing), attached to this email to fix json_build (at least for make, make install) As you're constructing targets based on commands to execute in the srcdir directory, and because srcdir is only set in pgxs.mk, it is possible to define srcdir early in the json_build/Makefile and use it. > When I have more time I will work on it some more. Thank you -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
Attachment
On 06/25/2013 11:29 AM, Cédric Villemain wrote:
> Le mardi 25 juin 2013 17:18:51, Andrew Dunstan a écrit :
>> On 06/24/2013 07:24 PM, Cédric Villemain wrote:
>>> Le mardi 25 juin 2013 00:18:26, Andrew Dunstan a écrit :
>>>> On 06/24/2013 04:02 PM, Cédric Villemain wrote:
>>>>> WIth extension, we do have to set VPATH explicitely if we want to use
>>>>> VPATH (note that contribs/extensions must not care that postgresql has
>>>>> been built with or without VPATH set). My patches try to fix that.
>>>> No, this is exactly what I'm objecting to. I want to be able to do:
>>>> invoke_vpath_magic
>>>> standard_make_commands_as_for_non_vpath
>>>>
>>>> Your patches have been designed to overcome your particular issues, but
>>>> they don't meet the general case, IMNSHO. This is why I want to have
>>>> more discussion about how vpath builds could work for PGXS modules.
>>> The patch does not restrict anything, it is not supposed to lead to
>>> regression.
>>> The assignment of VPATH and srcdir are wrong in the PGXS case, the patch
>>> correct them. You're still free to do "make VPATH=/mypath ..." the VPATH
>>> provided won't be erased by the pgxs.mk logic.
>> I still think this is premature. I have spent some more time trying to
>> make it work the way I think it should, so far without success. I think
>> we need more discussion about how we'd like VPATH to work for PGXS
>> would. There's no urgency about this - we've lived with the current
>> situation for quite a while.
> Sure...
> I did a quick and dirty patch (I just validate without lot of testing),
> attached to this email to fix json_build (at least for make, make install)
> As you're constructing targets based on commands to execute in the srcdir
> directory, and because srcdir is only set in pgxs.mk, it is possible to define
> srcdir early in the json_build/Makefile and use it.
This still doesn't do what I really want, which is to be able to invoke
make without anything special in the invocation that triggers VPATH
processing.
Here's what I did that works like I think it should.
First the attached patch on top of yours.
Second, I did:
mkdir vpath.json_build
cd vpath.json_build
sh/path/to/pgsource/config/prep_buildtree ../json_build .
ln -s ../json_build/json_build.control .
Then I created vpath.mk with these contents:
ext_srcdir = ../json_build
USE_VPATH = $(ext_srcdir)
Finally I vpath-ized the Makefile (see attached).
Given all of that, I was able to do, in the vpath directory:
make
make install
make installcheck
make clean
and it all just worked, with exactly the same make invocations as work
in an in-tree build.
So what's lacking to make this nice is a tool to automate the second and
third steps above.
Maybe there are other ways of doing this, but this does what I'd like to
see.
cheers
andrew
Attachment
Le mercredi 26 juin 2013 16:52:01, Andrew Dunstan a écrit : > On 06/25/2013 11:29 AM, Cédric Villemain wrote: > > Le mardi 25 juin 2013 17:18:51, Andrew Dunstan a écrit : > >> On 06/24/2013 07:24 PM, Cédric Villemain wrote: > >>> Le mardi 25 juin 2013 00:18:26, Andrew Dunstan a écrit : > >>>> On 06/24/2013 04:02 PM, Cédric Villemain wrote: > >>>>> WIth extension, we do have to set VPATH explicitely if we want to use > >>>>> VPATH (note that contribs/extensions must not care that postgresql > >>>>> has been built with or without VPATH set). My patches try to fix > >>>>> that. > >>>> > >>>> No, this is exactly what I'm objecting to. I want to be able to do: > >>>> invoke_vpath_magic > >>>> standard_make_commands_as_for_non_vpath > >>>> > >>>> Your patches have been designed to overcome your particular issues, > >>>> but they don't meet the general case, IMNSHO. This is why I want to > >>>> have more discussion about how vpath builds could work for PGXS > >>>> modules. > >>> > >>> The patch does not restrict anything, it is not supposed to lead to > >>> regression. > >>> The assignment of VPATH and srcdir are wrong in the PGXS case, the > >>> patch correct them. You're still free to do "make VPATH=/mypath ..." > >>> the VPATH provided won't be erased by the pgxs.mk logic. > >> > >> I still think this is premature. I have spent some more time trying to > >> make it work the way I think it should, so far without success. I think > >> we need more discussion about how we'd like VPATH to work for PGXS > >> would. There's no urgency about this - we've lived with the current > >> situation for quite a while. > > > > Sure... > > I did a quick and dirty patch (I just validate without lot of testing), > > attached to this email to fix json_build (at least for make, make > > install) As you're constructing targets based on commands to execute in > > the srcdir directory, and because srcdir is only set in pgxs.mk, it is > > possible to define srcdir early in the json_build/Makefile and use it. > > This still doesn't do what I really want, which is to be able to invoke > make without anything special in the invocation that triggers VPATH > processing. I believe it is the case currently (with my patches applyed), we just have to invoke Makefile like we invoke configure for PostgreSQL, except that we don't need a configure stage with the contribs. Obviously it is not exactly the same because 'configure' is a script to execute, but 'make' is a command with a configuration file (the Makefile) For PostgreSQL it is: $ mkdir build_dir $ cd build_dir $ path/to/source/tree/configure [options go here] $ gmake For contribs it is: $ mkdir build_dir $ cd build_dir $ gmake -f /path/to/source/tree/Makefile [options go here] > Here's what I did that works like I think it should. > > First the attached patch on top of yours. > > Second, I did: > > mkdir vpath.json_build > cd vpath.json_build > sh/path/to/pgsource/config/prep_buildtree ../json_build . > ln -s ../json_build/json_build.control . OK, this creates supposedly required directories in the build process. This looks a bit wrong and more work than required: not all the source directories are required by the build process. But I understand the point. Second, config/prep_buildtree is not installed (by make install), so it is not suitable to use it with PGXS (PGXS is found with pg_config, when the postgresql source tree is not accessible anymore). We may add config/prep_buildtree to INSTALL_PROGRAM. The 'ln -s' can probably be copied by prep_buildtree. > Then I created vpath.mk with these contents: > > ext_srcdir = ../json_build > USE_VPATH = $(ext_srcdir) OK. > Finally I vpath-ized the Makefile (see attached). I don't see how this is more pretty than other solution. > Given all of that, I was able to do, in the vpath directory: > > make > make install > make installcheck > make clean > > and it all just worked, with exactly the same make invocations as work > in an in-tree build. It breaks others: mkdir /tmp/auth_delay && cd /tmp/auth_delay sh /path/prep_buildtree /path/contrib/auth_delay/ . (no .control,it's not an extension) make make: *** Pas de règle pour fabriquer la cible « auth_delay.so », nécessaire pour « all ». Arrêt. This works: cd /tmp/auth_delay rm -rf * make -f /path/contrib/auth_delay/Makefile PS: I exported USE_PGXS=1 because of the switch case in the makefile. > So what's lacking to make this nice is a tool to automate the second and > third steps above. If the second and third steps are automatized, you'll still have to invoke them at some point. How ? mkdir /tmp/json_build && cd /tmp/json_build make make install make installcheck make clean -> this will never work, make won't find anything to do. You'll need the equivalent of configure (as we do for PostgreSQL, I agree) to trigger prep_buildtree: mkdir /tmp/json_build && cd /tmp/json_build $ path/to/source/tree/configure [options go here] make make install make installcheckmake clean > Maybe there are other ways of doing this, but this does what I'd like to > see. This is building contrib with PGXS and dependance to PostgreSQL source tree with VPATH and modification of current contribs' Makefile, but it uses the same set of commands users are expected when building something (configure, make, make install). I believe you are right that users are not supposed to call make with -f parameters to find the Makefile. They are supposed to execute the configure script. It should be possible to find a way to do that without breaking anything. I am just not sure (yet) it is worth the effort. Andrew, I'll understand you have something more important to cook for this CF, we can come back to this use case in the next CF if you prefer. Thanks again for the time you spent in the review, -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
On 06/26/2013 10:52 AM, Andrew Dunstan wrote: > > On 06/25/2013 11:29 AM, Cédric Villemain wrote: >> Le mardi 25 juin 2013 17:18:51, Andrew Dunstan a écrit : >>> On 06/24/2013 07:24 PM, Cédric Villemain wrote: >>>> Le mardi 25 juin 2013 00:18:26, Andrew Dunstan a écrit : >>>>> On 06/24/2013 04:02 PM, Cédric Villemain wrote: >>>>>> WIth extension, we do have to set VPATH explicitely if we want to >>>>>> use >>>>>> VPATH (note that contribs/extensions must not care that >>>>>> postgresql has >>>>>> been built with or without VPATH set). My patches try to fix that. >>>>> No, this is exactly what I'm objecting to. I want to be able to do: >>>>> invoke_vpath_magic >>>>> standard_make_commands_as_for_non_vpath >>>>> >>>>> Your patches have been designed to overcome your particular >>>>> issues, but >>>>> they don't meet the general case, IMNSHO. This is why I want to have >>>>> more discussion about how vpath builds could work for PGXS modules. >>>> The patch does not restrict anything, it is not supposed to lead to >>>> regression. >>>> The assignment of VPATH and srcdir are wrong in the PGXS case, the >>>> patch >>>> correct them. You're still free to do "make VPATH=/mypath ..." the >>>> VPATH >>>> provided won't be erased by the pgxs.mk logic. >>> I still think this is premature. I have spent some more time trying to >>> make it work the way I think it should, so far without success. I think >>> we need more discussion about how we'd like VPATH to work for PGXS >>> would. There's no urgency about this - we've lived with the current >>> situation for quite a while. >> Sure... >> I did a quick and dirty patch (I just validate without lot of testing), >> attached to this email to fix json_build (at least for make, make >> install) >> As you're constructing targets based on commands to execute in the >> srcdir >> directory, and because srcdir is only set in pgxs.mk, it is possible >> to define >> srcdir early in the json_build/Makefile and use it. > > > This still doesn't do what I really want, which is to be able to > invoke make without anything special in the invocation that triggers > VPATH processing. > > Here's what I did that works like I think it should. > > First the attached patch on top of yours. > > Second, I did: > > mkdir vpath.json_build > cd vpath.json_build > sh/path/to/pgsource/config/prep_buildtree ../json_build . > ln -s ../json_build/json_build.control . > > Then I created vpath.mk with these contents: > > ext_srcdir = ../json_build > USE_VPATH = $(ext_srcdir) > > Finally I vpath-ized the Makefile (see attached). > > Given all of that, I was able to do, in the vpath directory: > > make > make install > make installcheck > make clean > > and it all just worked, with exactly the same make invocations as work > in an in-tree build. > > So what's lacking to make this nice is a tool to automate the second > and third steps above. > > Maybe there are other ways of doing this, but this does what I'd like > to see. I haven't seen a response to this. One thing we are missing is documentation. Given that I'm inclined to commit all of this (i.e. cedric's patches 1,2,3, and 4 plus my addition). I'm also inclined to backpatch it, since without that it seems to me unlikely packagers will be able to make practical use of it for several years, and the risk is very low. cheers andrew
[it seems my first email didn't make it, sent again] Le mercredi 26 juin 2013 16:52:01, Andrew Dunstan a écrit : > On 06/25/2013 11:29 AM, Cédric Villemain wrote: > > Le mardi 25 juin 2013 17:18:51, Andrew Dunstan a écrit : > >> On 06/24/2013 07:24 PM, Cédric Villemain wrote: > >>> Le mardi 25 juin 2013 00:18:26, Andrew Dunstan a écrit : > >>>> On 06/24/2013 04:02 PM, Cédric Villemain wrote: > >>>>> WIth extension, we do have to set VPATH explicitely if we want to use > >>>>> VPATH (note that contribs/extensions must not care that postgresql > >>>>> has been built with or without VPATH set). My patches try to fix > >>>>> that. > >>>> > >>>> No, this is exactly what I'm objecting to. I want to be able to do: > >>>> invoke_vpath_magic > >>>> standard_make_commands_as_for_non_vpath > >>>> > >>>> Your patches have been designed to overcome your particular issues, > >>>> but they don't meet the general case, IMNSHO. This is why I want to > >>>> have more discussion about how vpath builds could work for PGXS > >>>> modules. > >>> > >>> The patch does not restrict anything, it is not supposed to lead to > >>> regression. > >>> The assignment of VPATH and srcdir are wrong in the PGXS case, the > >>> patch correct them. You're still free to do "make VPATH=/mypath ..." > >>> the VPATH provided won't be erased by the pgxs.mk logic. > >> > >> I still think this is premature. I have spent some more time trying to > >> make it work the way I think it should, so far without success. I think > >> we need more discussion about how we'd like VPATH to work for PGXS > >> would. There's no urgency about this - we've lived with the current > >> situation for quite a while. > > > > Sure... > > I did a quick and dirty patch (I just validate without lot of testing), > > attached to this email to fix json_build (at least for make, make > > install) As you're constructing targets based on commands to execute in > > the srcdir directory, and because srcdir is only set in pgxs.mk, it is > > possible to define srcdir early in the json_build/Makefile and use it. > > This still doesn't do what I really want, which is to be able to invoke > make without anything special in the invocation that triggers VPATH > processing. I believe it is the case currently (with my patches applyed), we just have to invoke Makefile like we invoke configure for PostgreSQL, except that we don't need a configure stage with the contribs. Obviously it is not exactly the same because 'configure' is a script to execute, but 'make' is a command with a configuration file (the Makefile) For PostgreSQL it is: $ mkdir build_dir $ cd build_dir $ path/to/source/tree/configure [options go here] $ gmake For contribs it is: $ mkdir build_dir $ cd build_dir $ gmake -f /path/to/source/tree/Makefile [options go here] > Here's what I did that works like I think it should. > > First the attached patch on top of yours. > > Second, I did: > > mkdir vpath.json_build > cd vpath.json_build > sh/path/to/pgsource/config/prep_buildtree ../json_build . > ln -s ../json_build/json_build.control . OK, this creates supposedly required directories in the build process. This looks a bit wrong and more work than required: not all the source directories are required by the build process. But I understand the point. Second, config/prep_buildtree is not installed (by make install), so it is not suitable to use it with PGXS (PGXS is found with pg_config, when the postgresql source tree is not accessible anymore). We may add config/prep_buildtree to INSTALL_PROGRAM. The 'ln -s' can probably be copied by prep_buildtree. > Then I created vpath.mk with these contents: > > ext_srcdir = ../json_build > USE_VPATH = $(ext_srcdir) OK. > Finally I vpath-ized the Makefile (see attached). I don't see how this is more pretty than other solution. > Given all of that, I was able to do, in the vpath directory: > > make > make install > make installcheck > make clean > > and it all just worked, with exactly the same make invocations as work > in an in-tree build. It breaks others: mkdir /tmp/auth_delay && cd /tmp/auth_delay sh /path/prep_buildtree /path/contrib/auth_delay/ . (no .control,it's not an extension) make make: *** Pas de règle pour fabriquer la cible « auth_delay.so », nécessaire pour « all ». Arrêt. This works: cd /tmp/auth_delay rm -rf * make -f /path/contrib/auth_delay/Makefile PS: I exported USE_PGXS=1 because of the switch case in the makefile. > So what's lacking to make this nice is a tool to automate the second and > third steps above. If the second and third steps are automatized, you'll still have to invoke them at some point. How ? mkdir /tmp/json_build && cd /tmp/json_build make make install make installcheck make clean -> this will never work, make won't find anything to do. You'll need the equivalent of configure (as we do for PostgreSQL, I agree) to trigger prep_buildtree: mkdir /tmp/json_build && cd /tmp/json_build $ path/to/source/tree/configure [options go here] make make install make installcheckmake clean > Maybe there are other ways of doing this, but this does what I'd like to > see. This is building contrib with PGXS and dependance to PostgreSQL source tree with VPATH and modification of current contribs' Makefile, but it uses the same set of commands users are expected when building something (configure, make, make install). I believe you are right that users are not supposed to call make with -f parameters to find the Makefile. They are supposed to execute the configure script. It should be possible to find a way to do that without breaking anything. I am just not sure (yet) it is worth the effort. Andrew, I'll understand you have something more important to cook for this CF, we can come back to this use case in the next CF if you prefer. Thanks again for the time you spent in the review, -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
Le samedi 29 juin 2013 19:54:53, Andrew Dunstan a écrit : > On 06/26/2013 10:52 AM, Andrew Dunstan wrote: > > On 06/25/2013 11:29 AM, Cédric Villemain wrote: > >> Le mardi 25 juin 2013 17:18:51, Andrew Dunstan a écrit : > >>> On 06/24/2013 07:24 PM, Cédric Villemain wrote: > >>>> Le mardi 25 juin 2013 00:18:26, Andrew Dunstan a écrit : > >>>>> On 06/24/2013 04:02 PM, Cédric Villemain wrote: > >>>>>> WIth extension, we do have to set VPATH explicitely if we want to > >>>>>> use > >>>>>> VPATH (note that contribs/extensions must not care that > >>>>>> postgresql has > >>>>>> been built with or without VPATH set). My patches try to fix that. > >>>>> > >>>>> No, this is exactly what I'm objecting to. I want to be able to do: > >>>>> invoke_vpath_magic > >>>>> standard_make_commands_as_for_non_vpath > >>>>> > >>>>> Your patches have been designed to overcome your particular > >>>>> issues, but > >>>>> they don't meet the general case, IMNSHO. This is why I want to have > >>>>> more discussion about how vpath builds could work for PGXS modules. > >>>> > >>>> The patch does not restrict anything, it is not supposed to lead to > >>>> regression. > >>>> The assignment of VPATH and srcdir are wrong in the PGXS case, the > >>>> patch > >>>> correct them. You're still free to do "make VPATH=/mypath ..." the > >>>> VPATH > >>>> provided won't be erased by the pgxs.mk logic. > >>> > >>> I still think this is premature. I have spent some more time trying to > >>> make it work the way I think it should, so far without success. I think > >>> we need more discussion about how we'd like VPATH to work for PGXS > >>> would. There's no urgency about this - we've lived with the current > >>> situation for quite a while. > >> > >> Sure... > >> I did a quick and dirty patch (I just validate without lot of testing), > >> attached to this email to fix json_build (at least for make, make > >> install) > >> As you're constructing targets based on commands to execute in the > >> srcdir > >> directory, and because srcdir is only set in pgxs.mk, it is possible > >> to define > >> srcdir early in the json_build/Makefile and use it. > > > > This still doesn't do what I really want, which is to be able to > > invoke make without anything special in the invocation that triggers > > VPATH processing. > > > > Here's what I did that works like I think it should. > > > > First the attached patch on top of yours. > > > > Second, I did: > > mkdir vpath.json_build > > cd vpath.json_build > > sh/path/to/pgsource/config/prep_buildtree ../json_build . > > ln -s ../json_build/json_build.control . > > > > Then I created vpath.mk with these contents: > > ext_srcdir = ../json_build > > USE_VPATH = $(ext_srcdir) > > > > Finally I vpath-ized the Makefile (see attached). > > > > Given all of that, I was able to do, in the vpath directory: > > make > > make install > > make installcheck > > make clean > > > > and it all just worked, with exactly the same make invocations as work > > in an in-tree build. > > > > So what's lacking to make this nice is a tool to automate the second > > and third steps above. > > > > Maybe there are other ways of doing this, but this does what I'd like > > to see. > > I haven't seen a response to this. One thing we are missing is > documentation. Given that I'm inclined to commit all of this (i.e. > cedric's patches 1,2,3, and 4 plus my addition). I did so I sent the mail again. I believe your addition need some changes to allow the PGXS+VPATH case as it currently depends on the source-tree tool. So it needs to be in postgresql-server-devel packages, thus installed by postgresql "make install". I'm going to update the doc. It's probably worth to have some examples. > I'm also inclined to backpatch it, since without that it seems to me > unlikely packagers will be able to make practical use of it for several > years, and the risk is very low. Yes, it is valuable to help packagers here, thanks. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
On 06/29/2013 02:27 PM, Cédric Villemain wrote: >> >> I haven't seen a response to this. One thing we are missing is >> documentation. Given that I'm inclined to commit all of this (i.e. >> cedric's patches 1,2,3, and 4 plus my addition). > I did so I sent the mail again. I believe your addition need some changes to > allow the PGXS+VPATH case as it currently depends on the source-tree tool. So > it needs to be in postgresql-server-devel packages, thus installed by > postgresql "make install". > I'm going to update the doc. It's probably worth to have some examples. I think we can survive for now without an additional tool. What I did was a proof of concept, it was not intended as a suggestion that we should install prep_buildtree. I am only suggesting that, in addition to your changes, if USE_VPATH is set then that path is used instead of a path inferred from the name of the Makefile. A tool to set up a full vpath build tree for extensions or external modules seems to be beyond the scope of this. > >> I'm also inclined to backpatch it, since without that it seems to me >> unlikely packagers will be able to make practical use of it for several >> years, and the risk is very low. > Yes, it is valuable to help packagers here, thanks. cheers andrew
On 06/29/2013 11:42 AM, Andrew Dunstan wrote: > > I think we can survive for now without an additional tool. What I did > was a proof of concept, it was not intended as a suggestion that we > should install prep_buildtree. I am only suggesting that, in addition to > your changes, if USE_VPATH is set then that path is used instead of a > path inferred from the name of the Makefile. SO is this patch "returned with feedback"? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
Le samedi 29 juin 2013 22:00:34, Josh Berkus a écrit : > On 06/29/2013 11:42 AM, Andrew Dunstan wrote: > > I think we can survive for now without an additional tool. What I did > > was a proof of concept, it was not intended as a suggestion that we > > should install prep_buildtree. I am only suggesting that, in addition to > > your changes, if USE_VPATH is set then that path is used instead of a > > path inferred from the name of the Makefile. > > SO is this patch "returned with feedback"? Only the 0005-Allows-extensions-to-install-header-file.patch , others are in the hands of Andrew. Additional patch required for documentation. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
On 6/29/13 1:54 PM, Andrew Dunstan wrote: > I haven't seen a response to this. One thing we are missing is > documentation. Given that I'm inclined to commit all of this (i.e. > cedric's patches 1,2,3, and 4 plus my addition). Could someone post an updated set of patches that is currently under consideration? > I'm also inclined to backpatch it, since without that it seems to me > unlikely packagers will be able to make practical use of it for several > years, and the risk is very low. Actually, the risk of makefile changes is pretty high, especially in cases involving advanced features such as vpath. GNU make hasn't been as stable is one might think, lately. We should carefully consider exactly which parts are worth backpatching.
On 07/01/2013 04:39 PM, Peter Eisentraut wrote: > On 6/29/13 1:54 PM, Andrew Dunstan wrote: >> I haven't seen a response to this. One thing we are missing is >> documentation. Given that I'm inclined to commit all of this (i.e. >> cedric's patches 1,2,3, and 4 plus my addition). > Could someone post an updated set of patches that is currently under > consideration? See what I actually committed today. > >> I'm also inclined to backpatch it, since without that it seems to me >> unlikely packagers will be able to make practical use of it for several >> years, and the risk is very low. > Actually, the risk of makefile changes is pretty high, especially in > cases involving advanced features such as vpath. GNU make hasn't been > as stable is one might think, lately. We should carefully consider > exactly which parts are worth backpatching. > > These changes are fairly small and mostly non-invasive, but if I've broken something we should find out about it fairly quickly, I hope. cheers andrew
On Mon, Jul 1, 2013 at 5:04 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > These changes are fairly small and mostly non-invasive, but if I've broken > something we should find out about it fairly quickly, I hope. Turns out you broke something. Specifically, contrib/spi now only installs autoinc.control instead of all the control files for extensions in that directory. Before: /usr/bin/install -c -m 644 ./autoinc.control ./insert_username.control ./moddatetime.control ./refint.control ./timetravel.control '/Users/rhaas/project/share/postgresql/extension/' /usr/bin/install -c -m 644 ./autoinc--1.0.sql ./autoinc--unpackaged--1.0.sql ./insert_username--1.0.sql ./insert_username--unpackaged--1.0.sql ./moddatetime--1.0.sql ./moddatetime--unpackaged--1.0.sql ./refint--1.0.sql ./refint--unpackaged--1.0.sql ./timetravel--1.0.sql ./timetravel--unpackaged--1.0.sql '/Users/rhaas/project/share/postgresql/extension/' /usr/bin/install -c -m 755 autoinc.so insert_username.so moddatetime.so refint.so timetravel.so '/Users/rhaas/project/lib/postgresql/' /usr/bin/install -c -m 644 ./autoinc.example ./insert_username.example ./moddatetime.example ./refint.example ./timetravel.example '/Users/rhaas/project/share/doc//postgresql/extension/' Now: /usr/bin/install -c -m 644 autoinc.control '/Users/rhaas/project/share/postgresql/extension/' /usr/bin/install -c -m 644 autoinc--1.0.sql autoinc--unpackaged--1.0.sql insert_username--1.0.sql insert_username--unpackaged--1.0.sql moddatetime--1.0.sql moddatetime--unpackaged--1.0.sql refint--1.0.sql refint--unpackaged--1.0.sql timetravel--1.0.sql timetravel--unpackaged--1.0.sql '/Users/rhaas/project/share/postgresql/extension/' /usr/bin/install -c -m 644 autoinc.example insert_username.example moddatetime.example refint.example timetravel.example '/Users/rhaas/project/share/doc//postgresql/extension/' /usr/bin/install -c -m 755 autoinc.so insert_username.so moddatetime.so refint.so timetravel.so '/Users/rhaas/project/lib/postgresql/' I'm not sure whether this is as simple as changing $< to $^ in the pgxs.mk's installcontrol rule, or if something more is required. Could you take a look? Thanks, -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 07/03/2013 05:52 PM, Robert Haas wrote: > On Mon, Jul 1, 2013 at 5:04 PM, Andrew Dunstan <andrew@dunslane.net> wrote: >> These changes are fairly small and mostly non-invasive, but if I've broken >> something we should find out about it fairly quickly, I hope. > Turns out you broke something. Specifically, contrib/spi now only > installs autoinc.control instead of all the control files for > extensions in that directory. > ... > I'm not sure whether this is as simple as changing $< to $^ in the > pgxs.mk's installcontrol rule, or if something more is required. > Could you take a look? > will do. cheers andrew
<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">>> I'm not sure whether this is as simple as changing $< to $^ in the<p style=" margin-top:0px;margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">>> pgxs.mk's installcontrol rule, or if something more is required.<p style=" margin-top:0px; margin-bottom:0px;margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > Couldyou take a look?<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0;text-indent:0px; -qt-user-state:0;">> ><p style=" margin-top:0px; margin-bottom:0px; margin-left:0px;margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> <p style=" margin-top:0px;margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">>will do.<p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px;-qt-block-indent:0; text-indent:0px; "> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px;-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">ah yes, good catch, I though .control file wereunique per contrib, but there aren't.<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px;-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">-- <p style=" margin-top:0px; margin-bottom:0px;margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">Cédric Villemain+33 (0)6 20 30 22 52<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0;text-indent:0px; -qt-user-state:0;">http://2ndQuadrant.fr/<p style=" margin-top:0px; margin-bottom:0px;margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">PostgreSQL:Support 24x7 - Développement, Expertise et Formation<p style="-qt-paragraph-type:empty; margin-top:0px;margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; ">
On 07/04/2013 09:14 AM, Cédric Villemain wrote: > > > > I'm not sure whether this is as simple as changing $< to $^ in the > > > > pgxs.mk's installcontrol rule, or if something more is required. > > > > Could you take a look? > > > > > > > > > > will do. > > ah yes, good catch, I though .control file were unique per contrib, > but there aren't. > > It's already been fixed. cheers andrew
On 07/04/2013 06:18 AM, Andrew Dunstan wrote: > > On 07/04/2013 09:14 AM, Cédric Villemain wrote: >> ah yes, good catch, I though .control file were unique per contrib, >> but there aren't. >> >> > > It's already been fixed. Does it look like this patch will get into committable shape in the next couple of days? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On 07/08/2013 03:40 PM, Josh Berkus wrote: > On 07/04/2013 06:18 AM, Andrew Dunstan wrote: >> On 07/04/2013 09:14 AM, Cédric Villemain wrote: >>> ah yes, good catch, I though .control file were unique per contrib, >>> but there aren't. >>> >>> >> It's already been fixed. > Does it look like this patch will get into committable shape in the next > couple of days? I think everything has been committed - as I think the CF app shows. The only thing left in this srea from Cédric is the insallation of headers, which Peter is down to review and is in "Waiting on Author" status. We are owed a docco patch though. cheers andrew
> I think everything has been committed - as I think the CF app shows. The > only thing left in this srea from Cédric is the insallation of headers, > which Peter is down to review and is in "Waiting on Author" status. Yeah, that's the one I'm asking about. is that going to get done in the next few days, or does it bounce? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On 07/08/2013 12:51 PM, Josh Berkus wrote: > >> I think everything has been committed - as I think the CF app shows. The >> only thing left in this srea from Cédric is the insallation of headers, >> which Peter is down to review and is in "Waiting on Author" status. > > Yeah, that's the one I'm asking about. is that going to get done in the > next few days, or does it bounce? > OK, I'm setting this to "returned with feedback" because there has been no further discussion of this patch here in the last several days. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
Le lundi 8 juillet 2013 21:46:39, Andrew Dunstan a écrit : > On 07/08/2013 03:40 PM, Josh Berkus wrote: > > On 07/04/2013 06:18 AM, Andrew Dunstan wrote: > >> On 07/04/2013 09:14 AM, Cédric Villemain wrote: > >>> ah yes, good catch, I though .control file were unique per contrib, > >>> but there aren't. > >> > >> It's already been fixed. > > > > Does it look like this patch will get into committable shape in the next > > couple of days? > > I think everything has been committed - as I think the CF app shows. The > only thing left in this srea from Cédric is the insallation of headers, > which Peter is down to review and is in "Waiting on Author" status. which is returned with feedback now, I can update if someone really wants it. > We are owed a docco patch though. Simple one, attached. I didn't document USE_VPATH, not sure how to explain that clearly. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
Attachment
> Simple one, attached. > I didn't document USE_VPATH, not sure how to explain that clearly. Just a remember that the doc is written and is waiting to be commited. There is also an issue spoted by Christoph with the installdirs prerequisite, the attached patch fix that. Also, the bugfixes were supposed to be backported. The behavior is not changed, nothing new, just fixing problem for some kind of extension builds. (However the USE_VPATH is new and is not needed in <9.3) -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
Attachment
On 09/03/2013 04:02 AM, Cédric Villemain wrote: >> Simple one, attached. >> I didn't document USE_VPATH, not sure how to explain that clearly. > Just a remember that the doc is written and is waiting to be commited. > > There is also an issue spoted by Christoph with the installdirs prerequisite, > the attached patch fix that. > > Also, the bugfixes were supposed to be backported. The behavior is not changed, > nothing new, just fixing problem for some kind of extension builds. (However > the USE_VPATH is new and is not needed in <9.3) > Darn, I knew I had forgotten something. Mea maxima culpa. Well, the 9.3 tarballs have been cut, unfortunately, but I'll get to this as soon as I can. cheers andrew
On 09/03/2013 04:04 AM, Cédric Villemain wrote: >> Simple one, attached. >> I didn't document USE_VPATH, not sure how to explain that clearly. > Just a remember that the doc is written and is waiting to be commited. > > There is also an issue spoted by Christoph with the installdirs prerequisite, > the attached patch fix that. > I applied this one line version of the patch, which seemed more elegant than the later one supplied. I backpatched that and the rest of the VPATH changes for extensiuons to 9.1 where we first provided for extensions, so people can have a reasonably uniform build system for their extensions. cheers andrew
On 09/29/2013 07:09 PM, Andrew Dunstan wrote: > > On 09/03/2013 04:04 AM, Cédric Villemain wrote: >>> Simple one, attached. >>> I didn't document USE_VPATH, not sure how to explain that clearly. >> Just a remember that the doc is written and is waiting to be commited. >> >> There is also an issue spoted by Christoph with the installdirs >> prerequisite, >> the attached patch fix that. >> > > I applied this one line version of the patch, which seemed more > elegant than the later one supplied. > > I backpatched that and the rest of the VPATH changes for extensiuons > to 9.1 where we first provided for extensions, so people can have a > reasonably uniform build system for their extensions. > > I have reverted this in the 9.2 and 9.1 branches until I can work out why it's breaking the buildfarm. 9.3 seems to be fine. cheers andrew
Le lundi 30 septembre 2013 00:10:09 Andrew Dunstan a écrit : > On 09/29/2013 07:09 PM, Andrew Dunstan wrote: > > On 09/03/2013 04:04 AM, Cédric Villemain wrote: > >>> Simple one, attached. > >>> I didn't document USE_VPATH, not sure how to explain that clearly. > >> > >> Just a remember that the doc is written and is waiting to be > >> commited. > >> > >> There is also an issue spoted by Christoph with the installdirs > >> prerequisite, > >> the attached patch fix that. > > > > I applied this one line version of the patch, which seemed more > > elegant than the later one supplied. > > > > I backpatched that and the rest of the VPATH changes for extensiuons > > to 9.1 where we first provided for extensions, so people can have a > > reasonably uniform build system for their extensions. > > I have reverted this in the 9.2 and 9.1 branches until I can work out > why it's breaking the buildfarm. 9.3 seems to be fine. Yes, please take the longer but better patch following the small one. There are eveidences that it fixes compilation to always build installdirs first. Previously installdirs may be build after copying files, so it fails. Chritstoph authored this good patch. I'm sorry not to have been clear on that. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
On Sun, 2013-09-29 at 19:09 -0400, Andrew Dunstan wrote: > On 09/03/2013 04:04 AM, Cédric Villemain wrote: > >> Simple one, attached. > >> I didn't document USE_VPATH, not sure how to explain that clearly. > > Just a remember that the doc is written and is waiting to be commited. > > > > There is also an issue spoted by Christoph with the installdirs prerequisite, > > the attached patch fix that. > > > > I applied this one line version of the patch, which seemed more elegant > than the later one supplied. > > I backpatched that and the rest of the VPATH changes for extensiuons to > 9.1 where we first provided for extensions, so people can have a > reasonably uniform build system for their extensions. I suspect this line submake-libpq: $(libdir)/libpq.so ; will cause problems on platforms with a different extension (e.g. OS X). Given that this supposedly small and noninvasive set of changes has caused a number of problems already, I suggest we revert this entire thing until we have had time to actually test it somewhere other than in the the stable branches. As it stands, it is a new feature being developed in the stable branches, which is clearly not acceptable.
On 10/07/2013 08:47 PM, Peter Eisentraut wrote: > On Sun, 2013-09-29 at 19:09 -0400, Andrew Dunstan wrote: >> On 09/03/2013 04:04 AM, Cédric Villemain wrote: >>>> Simple one, attached. >>>> I didn't document USE_VPATH, not sure how to explain that clearly. >>> Just a remember that the doc is written and is waiting to be commited. >>> >>> There is also an issue spoted by Christoph with the installdirs prerequisite, >>> the attached patch fix that. >>> >> I applied this one line version of the patch, which seemed more elegant >> than the later one supplied. >> >> I backpatched that and the rest of the VPATH changes for extensiuons to >> 9.1 where we first provided for extensions, so people can have a >> reasonably uniform build system for their extensions. > I suspect this line > > submake-libpq: $(libdir)/libpq.so ; > > will cause problems on platforms with a different extension (e.g. OS X). > > Given that this supposedly small and noninvasive set of changes has > caused a number of problems already, I suggest we revert this entire > thing until we have had time to actually test it somewhere other than in > the the stable branches. As it stands, it is a new feature being > developed in the stable branches, which is clearly not acceptable. If you want to revert it then go ahead, but the last statement is simply incorrect. The code has been sitting in HEAD for several months, and I committed on the back branches because it was wanted. cheers andrew
On 10/07/2013 08:47 PM, Peter Eisentraut wrote: > > I suspect this line > > submake-libpq: $(libdir)/libpq.so ; > > will cause problems on platforms with a different extension (e.g. OS X). suggested fix is below. cheers andrew diff --git a/src/Makefile.global.in b/src/Makefile.global.in index bb732bb..b562378 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -422,7 +422,11 @@ ifndef PGXS submake-libpq: $(MAKE) -C $(libpq_builddir) all else -submake-libpq: $(libdir)/libpq.so ; +ifneq ($(PORTNAME),cygwin) +submake-libpq: $(libdir)/libpq$(DLSUFFIX) ; +else +submake-libpq: $(libdir)/cygpq$(DLSUFFIX) ; +endif endif ifndef PGXS
On Tue, 2013-10-08 at 10:04 -0400, Andrew Dunstan wrote: > On 10/07/2013 08:47 PM, Peter Eisentraut wrote: > > > > I suspect this line > > > > submake-libpq: $(libdir)/libpq.so ; > > > > will cause problems on platforms with a different extension (e.g. OS X). > > > suggested fix is below. Hmm, this would duplicate information about shared library naming in a place outside of Makefile.shlib. That doesn't look right. > diff --git a/src/Makefile.global.in b/src/Makefile.global.in > index bb732bb..b562378 100644 > --- a/src/Makefile.global.in > +++ b/src/Makefile.global.in > @@ -422,7 +422,11 @@ ifndef PGXS > submake-libpq: > $(MAKE) -C $(libpq_builddir) all > else > -submake-libpq: $(libdir)/libpq.so ; > +ifneq ($(PORTNAME),cygwin) > +submake-libpq: $(libdir)/libpq$(DLSUFFIX) ; > +else > +submake-libpq: $(libdir)/cygpq$(DLSUFFIX) ; > +endif > endif > > ifndef PGXS > > >
On Mon, 2013-10-07 at 22:00 -0400, Andrew Dunstan wrote: > The code has been sitting in HEAD for several months, and I > committed on the back branches because it was wanted. New features normally go through a full development cycle including extensive beta testing, especially when they contain changes to external interfaces. The fact that the patch author wanted his feature released immediately (of course) doesn't warrant a free pass in this case, IMO.
On 10/10/2013 09:37 PM, Peter Eisentraut wrote: > On Mon, 2013-10-07 at 22:00 -0400, Andrew Dunstan wrote: >> The code has been sitting in HEAD for several months, and I >> committed on the back branches because it was wanted. > New features normally go through a full development cycle including > extensive beta testing, especially when they contain changes to external > interfaces. The fact that the patch author wanted his feature released > immediately (of course) doesn't warrant a free pass in this case, IMO. > > > Perhaps you should have stated your objection when this was being discussed, and saved me some time. I frankly think we can be a bit more tolerant about build system features than we would be about the actual software it builds. cheers andrew
On 10/10/2013 09:35 PM, Peter Eisentraut wrote: > On Tue, 2013-10-08 at 10:04 -0400, Andrew Dunstan wrote: >> On 10/07/2013 08:47 PM, Peter Eisentraut wrote: >>> I suspect this line >>> >>> submake-libpq: $(libdir)/libpq.so ; >>> >>> will cause problems on platforms with a different extension (e.g. OS X). >> >> suggested fix is below. > Hmm, this would duplicate information about shared library naming in a > place outside of Makefile.shlib. That doesn't look right. > > Please suggest an alternative. cheers andrew
Le jeudi 10 octobre 2013 21:37:24 Peter Eisentraut a écrit : > On Mon, 2013-10-07 at 22:00 -0400, Andrew Dunstan wrote: > > The code has been sitting in HEAD for several months, and I > > committed on the back branches because it was wanted. > > New features normally go through a full development cycle including > extensive beta testing, especially when they contain changes to > external interfaces. The fact that the patch author wanted his > feature released immediately (of course) doesn't warrant a free pass > in this case, IMO. What's new feature ? PGXS break when 19e231b was commited, the patch around this special submake is trying to bugfix that. See http://www.postgresql.org/message-id/201305281410.32535.cedric@2ndquadrant.com There are also reports like this one (and thread): http://www.postgresql.org/message-id/CABRT9RCJ6RvgMXbyebCgYMwBwiOBKO_W6zvwrzn75_f+USpQ5g@mail.gmail.com where you can read that I am not 'the' only guy who want to have this commited. And believeing this is worth a backpatch as it stands for a bugfix. Here also the problem is stated as something to fix. http://www.postgresql.org/message-id/20130516165318.GF15045@eldon.alvh.no-ip.org That's being said, you are correct about the problem you found and it's good to be able to release new version without a bug for OSX. I'm just sad you didn't get time to voice a bit before Andrew spent too much time on that. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation
On Thu, Oct 10, 2013 at 11:00:30PM -0400, Andrew Dunstan wrote: > > On 10/10/2013 09:35 PM, Peter Eisentraut wrote: > >On Tue, 2013-10-08 at 10:04 -0400, Andrew Dunstan wrote: > >>On 10/07/2013 08:47 PM, Peter Eisentraut wrote: > >>>I suspect this line > >>> > >>>submake-libpq: $(libdir)/libpq.so ; > >>> > >>>will cause problems on platforms with a different extension (e.g. OS X). > >> > >>suggested fix is below. > >Hmm, this would duplicate information about shared library naming in a > >place outside of Makefile.shlib. That doesn't look right. > > > > > > > Please suggest an alternative. Did we ever address this? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 01/31/2014 09:19 PM, Bruce Momjian wrote: > On Thu, Oct 10, 2013 at 11:00:30PM -0400, Andrew Dunstan wrote: >> On 10/10/2013 09:35 PM, Peter Eisentraut wrote: >>> On Tue, 2013-10-08 at 10:04 -0400, Andrew Dunstan wrote: >>>> On 10/07/2013 08:47 PM, Peter Eisentraut wrote: >>>>> I suspect this line >>>>> >>>>> submake-libpq: $(libdir)/libpq.so ; >>>>> >>>>> will cause problems on platforms with a different extension (e.g. OS X). >>>> suggested fix is below. >>> Hmm, this would duplicate information about shared library naming in a >>> place outside of Makefile.shlib. That doesn't look right. >>> >>> >> >> Please suggest an alternative. > Did we ever address this? > No. I never got a reply, AFAIK. cheers andrew
On Fri, Jan 31, 2014 at 09:28:06PM -0500, Andrew Dunstan wrote: > > On 01/31/2014 09:19 PM, Bruce Momjian wrote: > >On Thu, Oct 10, 2013 at 11:00:30PM -0400, Andrew Dunstan wrote: > >>On 10/10/2013 09:35 PM, Peter Eisentraut wrote: > >>>On Tue, 2013-10-08 at 10:04 -0400, Andrew Dunstan wrote: > >>>>On 10/07/2013 08:47 PM, Peter Eisentraut wrote: > >>>>>I suspect this line > >>>>> > >>>>>submake-libpq: $(libdir)/libpq.so ; > >>>>> > >>>>>will cause problems on platforms with a different extension (e.g. OS X). > >>>>suggested fix is below. > >>>Hmm, this would duplicate information about shared library naming in a > >>>place outside of Makefile.shlib. That doesn't look right. > >>> > >>> > >> > >>Please suggest an alternative. > >Did we ever address this? > > > > > No. I never got a reply, AFAIK. OK, seems there is no known fix then. Thanks. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 6/18/13 9:52 AM, Cédric Villemain wrote: > 0006-Fix-suggested-layout-for-extension.patch I have committed this patch.
On 1/31/14 9:28 PM, Bruce Momjian wrote: > On Fri, Jan 31, 2014 at 09:28:06PM -0500, Andrew Dunstan wrote: >> >> On 01/31/2014 09:19 PM, Bruce Momjian wrote: >>> On Thu, Oct 10, 2013 at 11:00:30PM -0400, Andrew Dunstan wrote: >>>> On 10/10/2013 09:35 PM, Peter Eisentraut wrote: >>>>> On Tue, 2013-10-08 at 10:04 -0400, Andrew Dunstan wrote: >>>>>> On 10/07/2013 08:47 PM, Peter Eisentraut wrote: >>>>>>> I suspect this line >>>>>>> >>>>>>> submake-libpq: $(libdir)/libpq.so ; >>>>>>> >>>>>>> will cause problems on platforms with a different extension (e.g. OS X). >>>>>> suggested fix is below. >>>>> Hmm, this would duplicate information about shared library naming in a >>>>> place outside of Makefile.shlib. That doesn't look right. >>>> >>>> Please suggest an alternative. >>> Did we ever address this? >>> >> >> No. I never got a reply, AFAIK. > > OK, seems there is no known fix then. Thanks. I noticed this item was still in the 9.4 code. Looking at the original submission (http://www.postgresql.org/message-id/201306181552.36673.cedric@2ndquadrant.com, patch 0001), I think the original reason for adding this was wrong to begin with. Attached patch 0001 fixes that. The applicable parts of that patch could be backpatched as a bug fix (but evidently no one cares about building contrib with pgxs (except when I submit a patch to remove it)). In that topic, I also propose attached patch 0002 for 9.4, which removes the use of the USE_VPATH variable and just uses VPATH. There is no need for this additional indirection. Also, USE_FOO variables are typically Boolean, so this is misnamed. I note that the documentation in this topic (http://www.postgresql.org/docs/devel/static/extend-pgxs.html) suggests the use of config/prep_buildtree, but that is not installed, so I don't know how much use that advice is. (I don't know at this point whether just installing it would be a solution.) Thirdly, I propose attached patch 0003, which reverts patch 0004 in the original submission. I believe that patch was unnecessary, and I think the new code is uglier. (Of course, I'm biased, because I wrote the old code.) Technically, this ought to be for 9.5 only. Finally, I have written a test suite for PGXS to support all these outrageous claims: https://github.com/petere/test_pgxs. This tests most of the variables supported in PGXS makefiles and tests that both documented ways of vpath builds work. I don't propose this for inclusion at the moment, because that would require more work, but it's a good reference.
Attachment
On Wed, Nov 19, 2014 at 11:11 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > The applicable parts of > that patch could be backpatched as a bug fix (but evidently no one cares > about building contrib with pgxs (except when I submit a patch to remove > it)). Touché. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 11/19/14 11:11 PM, Peter Eisentraut wrote: > I noticed this item was still in the 9.4 code. Looking at the original > submission > (http://www.postgresql.org/message-id/201306181552.36673.cedric@2ndquadrant.com, > patch 0001), I think the original reason for adding this was wrong to > begin with. I have applied all three patches to 9.4 and 9.5. So this issue is now resolved.
On 12/4/14 11:38 AM, Peter Eisentraut wrote: > On 11/19/14 11:11 PM, Peter Eisentraut wrote: >> I noticed this item was still in the 9.4 code. Looking at the original >> submission >> (http://www.postgresql.org/message-id/201306181552.36673.cedric@2ndquadrant.com, >> patch 0001), I think the original reason for adding this was wrong to >> begin with. > > I have applied all three patches to 9.4 and 9.5. So this issue is now > resolved. Apparently, some buildfarm module is unhappy with this: http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=sittella&dt=2014-12-04%2017%3A16%3A29&stg=RedisFDW-build Is there some custom code running there? I don't know how that error would happen otherwise?
On 12/04/2014 02:44 PM, Peter Eisentraut wrote: > On 12/4/14 11:38 AM, Peter Eisentraut wrote: >> On 11/19/14 11:11 PM, Peter Eisentraut wrote: >>> I noticed this item was still in the 9.4 code. Looking at the original >>> submission >>> (http://www.postgresql.org/message-id/201306181552.36673.cedric@2ndquadrant.com, >>> patch 0001), I think the original reason for adding this was wrong to >>> begin with. >> I have applied all three patches to 9.4 and 9.5. So this issue is now >> resolved. > Apparently, some buildfarm module is unhappy with this: > > http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=sittella&dt=2014-12-04%2017%3A16%3A29&stg=RedisFDW-build > > Is there some custom code running there? I don't know how that error > would happen otherwise? > > You have broken two buildfarm instances that build and test external modules - in one case the Redis FDW module and in the other the File Text Array FDW. I will see what can be retrieved. cheers andrew
On 12/04/2014 03:47 PM, Andrew Dunstan wrote: > > On 12/04/2014 02:44 PM, Peter Eisentraut wrote: >> On 12/4/14 11:38 AM, Peter Eisentraut wrote: >>> On 11/19/14 11:11 PM, Peter Eisentraut wrote: >>>> I noticed this item was still in the 9.4 code. Looking at the >>>> original >>>> submission >>>> (http://www.postgresql.org/message-id/201306181552.36673.cedric@2ndquadrant.com, >>>> >>>> patch 0001), I think the original reason for adding this was wrong to >>>> begin with. >>> I have applied all three patches to 9.4 and 9.5. So this issue is now >>> resolved. >> Apparently, some buildfarm module is unhappy with this: >> >> http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=sittella&dt=2014-12-04%2017%3A16%3A29&stg=RedisFDW-build >> >> >> Is there some custom code running there? I don't know how that error >> would happen otherwise? >> >> > > > You have broken two buildfarm instances that build and test external > modules - in one case the Redis FDW module and in the other the File > Text Array FDW. I will see what can be retrieved. > > I think this needs to be reverted. This has broken two modules that are not even using vpath builds. You can see the relevant Makefiles at: <https://github.com/adunstan/file_text_array_fdw/blob/master/Makefile> and <https://github.com/pg-redis-fdw/redis_fdw/blob/master/Makefile>. IIRC, the code you found convoluted and removed was required precisely to prevent this sort of error. cheers andrew
On 12/4/14 3:47 PM, Andrew Dunstan wrote: > You have broken two buildfarm instances that build and test external > modules - in one case the Redis FDW module and in the other the File > Text Array FDW. I will see what can be retrieved. This has been fixed. One thing I'll look into sometime is splitting up Makefile.global into parts that apply to PGXS and those that don't (and possibly common parts), because there are too many ifdef PGXS's popping up all over the place in an unorganized fashion.