From: =?utf-8?q?C=C3=A9dric_Villemain?= <cedric@2ndquadrant.com>
Reply-To: cedric@2ndquadrant.com
Organization: 2ndQuadrant
To: Andrew Dunstan <andrew@dunslane.net>
Subject: Re: Bugfix and new feature for PGXS
Date: Mon, 24 Jun 2013 22:02:00 +0200
User-Agent: KMail/1.13.7 (Linux/3.8-2-amd64; KDE/4.8.4; x86_64; ; )
Cc: pgsql-hackers@postgresql.org,
 Craig Ringer <craig@2ndquadrant.com>
References: <201306181552.36673.cedric@2ndquadrant.com>
 <51C88483.7090900@dunslane.net>
In-Reply-To: <51C88483.7090900@dunslane.net>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart1410936.73Hoef9ldO";
 protocol="application/pgp-signature"; micalg=pgp-sha1
Content-Transfer-Encoding: 7bit
Message-Id: <201306242202.04162.cedric@2ndquadrant.com>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

--nextPart1410936.73Hoef9ldO
Content-Type: Text/Plain;
  charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Le lundi 24 juin 2013 19:40:19, Andrew Dunstan a =C3=A9crit :
> On 06/18/2013 09:52 AM, C=C3=A9dric Villemain wrote:
> > I've rebased the current set of pending patches I had, to fix pgxs and
> > added 2 new patches.
> >=20
> > Bugfixes have already been presented and sent in another thread, except
> > the last one which only fix comment in pgxs.mk.
> >=20
> > 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 t=
he
> > header files to install.
> >=20
> > 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).
> >=20
> > This allows for example to install hstore header and be able to include
> > them
> >=20
> > in another extension like that:
> >    # include "contrib/hstore/hstore.h"
> >=20
> > 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-functi=
on
> > dance previously required.
> >=20
> > I've not updated contribs' Makfefile: I'm not sure what we want to
> > expose.
> >=20
> > 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.
> >=20
> >=20
> > 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
> >=20
> > New feature:
> > 0005-Allows-extensions-to-install-header-file.patch
> >=20
> > I'll do a documentation patch based on what is accepted in HEAD and/or
> > previous branches.
>=20
> 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.
>=20
> 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.
>=20
> FYI I deliberately chose a non-core extension with an uncommon layout
> for testing: json_build <https://github.com/pgexperts/json_build>
>=20
> 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 wit=
h=20
VPATH.

We have 2 main options with core postgresql: in-tree build or out-of-tree. =
The=20
former is the classical build process, the later is what's frequently use f=
or=20
packaging because it allows not to pollute the source tree with intermediat=
e=20
or fully built files.

You're right that we don't need to set VPATH explicitely, it is set by=20
'configure' when you exec configure out-of-source-tree. It is transparent t=
o the=20
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=
=20
(note that contribs/extensions must not care that postgresql has been built=
=20
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=3D1 -f /path/to/json_build/Makefile

There is another way to do the same thing which is:
$ mkdir /tmp/json_build
$ make USE_PGXS=3D1 -C /tmp/json_build -f /path/to/json_build/Makefile

Thus packagers can now use :=20
$ make USE_PGXS=3D1 -C $vtarget -f /path/to/Makefile=20
DESTDIR=3D/my/packaging/area/json_build

instead of (short version):
$ make USE_PGXS=3D1 -C $vtarget -f /path/to/Makefile=20
DESTDIR=3D"$srcdir/debian/$package"  VPATH=3D"$srcdir" srcdir=3D"$srcdir"

(please note the $srcdir to workaround the pgxs way of working)

=46or patch 0003, the point is to make the usage of VPATH transparent for t=
he=20
users (as we do for core postgresql)

=46or patch 0004, the point is to take the required files from the good loc=
ation.=20
$^ and $< contains the filepath which is where 'make' found the files, but=
=20
$(DATA) and others PGXS variables contains the list of required files, not=
=20
their filepath. As long as you build in-tree you won't hit this problem, bu=
t if=20
you try to build out-of-source tree, the .control file for example, then ma=
ke=20
won't find the .control you just *built*, it'll try to copy it from origina=
l=20
source-tree to follow this recipe:
$(INSTALL_DATA) $(addprefix $(srcdir)/, $(addsuffix .control, $(EXTENSION))=
)=20
'$(DESTDIR)$(datadir)/extension/'

But this is 'make' job to find the file, we should not write that strongly =
as we=20
do. This should be:
$(INSTALL_DATA) $< '$(DESTDIR)$(datadir)/extension/'

This is what this patch fixes.

Are those explanations ok ?=20

PS: out-of-source tree builds are a nice way to prevent collision between=20
32bit and 64bits built share object, or even more worse situation with=20
kfreeBSD vs linux, etc. this also makes 'make clean' as fast as a a 'drop=20
table'.
=2D-=20
C=C3=A9dric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - D=C3=A9veloppement, Expertise et Formation

--nextPart1410936.73Hoef9ldO
Content-Type: application/pgp-signature; name=signature.asc 
Content-Description: This is a digitally signed message part.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.20 (GNU/Linux)

iQEcBAABAgAGBQJRyKW8AAoJENsgH0yzVBeb4fYH/0sZ+vidm22EkDIReGcx8Kz/
9v5GR+ui3hHwGoDBGoC4M+zEWHxL55tqmwFgj0Z44oNcS50utkQonsoOUGau7/L5
WHtxHzpXMvSOyMQSITs4TsjmGsJUJSYY2/qmN2Ch3seGxY+IOYcNund2oEPqosly
Mrmi5fM0SvqGnR/W9OPDhmyfW+cMfuYwSxHCT0k52ikMeoW4XuEH2RXaPh+LdXPA
23/IubZzKEUlW/9BpIQ0zQyzwNpGQBIbLi1U5S3byO6ZGiYa4DNRsoigwKDCqCP5
vj0W/u7GuCBFfYylJM9munnXefHBJvPaT59ss42NTVzlFYzhqIrYvbONgmKsnSg=
=ffC4
-----END PGP SIGNATURE-----

--nextPart1410936.73Hoef9ldO--