From: =?iso-8859-15?q?C=E9dric_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: Thu, 27 Jun 2013 11:07:25 +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>
 <201306251729.47025.cedric@2ndquadrant.com> <51CB0011.8020506@dunslane.net>
In-Reply-To: <51CB0011.8020506@dunslane.net>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart2293737.miWBrrN8Pu";
 protocol="application/pgp-signature"; micalg=pgp-sha1
Content-Transfer-Encoding: 7bit
Message-Id: <201306271107.33183.cedric@2ndquadrant.com>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

--nextPart2293737.miWBrrN8Pu
Content-Type: Text/Plain;
  charset="iso-8859-15"
Content-Transfer-Encoding: quoted-printable

Le mercredi 26 juin 2013 16:52:01, Andrew Dunstan a =E9crit :
> On 06/25/2013 11:29 AM, C=E9dric Villemain wrote:
> > Le mardi 25 juin 2013 17:18:51, Andrew Dunstan a =E9crit :
> >> On 06/24/2013 07:24 PM, C=E9dric Villemain wrote:
> >>> Le mardi 25 juin 2013 00:18:26, Andrew Dunstan a =E9crit :
> >>>> On 06/24/2013 04:02 PM, C=E9dric Villemain wrote:
> >>>>> WIth extension, we do have to set VPATH explicitely if we want to u=
se
> >>>>> VPATH (note that contribs/extensions must not care that postgresql
> >>>>> has been built with or without VPATH set). My patches try to fix
> >>>>> that.
> >>>>=20
> >>>> 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
> >>>>=20
> >>>> 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.
> >>>=20
> >>> 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=3D/mypath ..."
> >>> the VPATH provided won't be erased by the pgxs.mk logic.
> >>=20
> >> 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.
> >=20
> > 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.
>=20
> 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=20
invoke Makefile like we invoke configure for PostgreSQL, except that we don=
't =20
need a configure stage with the contribs.

Obviously it is not exactly the same because 'configure' is a script to=20
execute, but 'make' is a command with a configuration file (the Makefile)

=46or PostgreSQL it is:
$ mkdir build_dir
$ cd build_dir
$ path/to/source/tree/configure [options go here]
$ gmake

=46or 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.
>=20
> First the attached patch on top of yours.
>=20
> Second, I did:
>=20
>      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=
=20
looks a bit wrong and more work than required: not all the source directori=
es=20
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=20
suitable to use it with PGXS (PGXS is found with pg_config, when the postgr=
esql=20
source tree is not accessible anymore). We may add config/prep_buildtree to=
=20
INSTALL_PROGRAM.=20

The 'ln -s' can probably be copied by prep_buildtree.

> Then I created vpath.mk with these contents:
>=20
>      ext_srcdir =3D ../json_build
>      USE_VPATH =3D $(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:
>=20
>      make
>      make install
>      make installcheck
>      make clean
>=20
> 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=20
  make: *** Pas de r=E8gle pour fabriquer la cible =AB auth_delay.so =BB, n=
=E9cessaire=20
pour =AB all =BB. Arr=EAt.

This works:=20
  cd /tmp/auth_delay
  rm -rf *
  make -f /path/contrib/auth_delay/Makefile

PS: I exported USE_PGXS=3D1 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=
=20
them at some point. How ?

  mkdir /tmp/json_build && cd /tmp/json_build=20
  make
  make install
  make installcheck
  make clean

=2D> this will never work, make won't find anything to do.=20
You'll need the equivalent of configure (as we do for PostgreSQL, I agree) =
to=20
trigger prep_buildtree:

  mkdir /tmp/json_build && cd /tmp/json_build=20
  $ path/to/source/tree/configure [options go here]
  make
  make install
  make installcheck
  make 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=
=20
with VPATH and modification of current contribs' Makefile, but it uses the =
same=20
set of commands users are expected when building something (configure, make=
,=20
make install).

I believe you are right that users are not supposed to call make with -f=20
parameters to find the Makefile. They are supposed to execute the configure=
=20
script.
It should be possible to find a way to do that without breaking anything. I=
 am=20
just not sure (yet) it is worth the effort.

Andrew, I'll understand you have something more important to cook for this =
CF,=20
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,=20
=2D-=20
C=E9dric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - D=E9veloppement, Expertise et Formation

--nextPart2293737.miWBrrN8Pu
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)

iQEcBAABAgAGBQJRzADVAAoJENsgH0yzVBebYYwIAKchKRZuOI25qiTQS9Ynl1GL
vPIok2aw31a3lA7Xr6JhYHZW8LsjNa5zmn9gfp1qagAWqV1V73o+OXEYbG/0qwaX
2B0zu7DxetN232u7sdIqufksjlFNfaEtmTAU8jimFf4nR6onUKVc12DKQTcPO3jh
4p3ZSH3VTRfK7mc+lS3/1nHOXOr+n3n9HmblFUf+063kDRY0PSOs+feKB0Nrgupr
cHBMiEtBQOARutf5F/BETzxhoXkGYzBsiobV04kjCu+a0u/9KkqP1nWtJyx05sIO
uIoNloO4fMztoKhwBV3GBlN7GnwKiQyh9ZP6QAfZRkwEoDeWX0R1t5vGmYEatto=
=/gsO
-----END PGP SIGNATURE-----

--nextPart2293737.miWBrrN8Pu--