[PATCH] Support built control file in PGXS VPATH builds

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Support built control file in PGXS VPATH builds

Craig Ringer-3
pgxs.mk assumes that if $(EXTENSION) is set, a file
$(EXTENSION).control must exist in the $(srcdir).

Extensions that need to support multiple Pg versions, multiple
variants of the extension, etc may need to template their extension
control file. PGXS's assumption prevents those extensions from
supporting read-only source trees for true vpath builds.

A workaround is to ignore the EXTENSION field in PGXS and leave it
unset, then set MODULEDIR to the value you would've set EXTENSION to
and install your control file with DATA_built . But that's more than a
tad ugly.

The attached patch fixes this by having PGXS resolve
$(EXTENSION).control along the VPATH.

Before:

/usr/bin/install: cannot stat
'/the/srcdir/path/the_extension.control': No such file or directory
make: *** [/the/postgres/path/lib/postgresql/pgxs/src/makefiles/pgxs.mk:229:
install] Error 1

After: no error, extension control file is found in builddir.

There's no reference to $(EXTENSION) outside pgxs.mk so this shouldn't
have any wider consequences.

The extension is responsible for the build rule for the control file,
like in DATA_built etc.

Please backpatch this build fix.

I could supply an alternative patch that follows PGXS's existing
convention of using a _built suffix, allowing the extension to specify
either EXTENSION or EXTENSION_built. For backward compat we'd have to
allow both to be set so long as they have the same value. Personally I
dislike this pattern and prefer to just resolve it in normal Make
fashion without caring if it's a built file or not, especially for the
EXTENSION var, so I'd prefer the first variant.

Frankly I'd rather we got rid of all the $(VAR) and $(VAR_built) stuff
entirely and just let make do proper vpath resolution. But I'm sure
it's that way for a reason...

I have a few other cleanup/fixup patches in the pipe for PGXS and
Makefile.global but I have to tidy them up a bit first. One to
eliminate undefined variables use, another to allow vpath directives
to be used instead of the big VPATH variable hammer. Keep an eye out.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 2ndQuadrant - PostgreSQL Solutions for the Enterprise

0001-PGXS-support-for-built-control-files.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Support built control file in PGXS VPATH builds

Peter Eisentraut-6
On 2020-02-07 04:14, Craig Ringer wrote:
> The attached patch fixes this by having PGXS resolve
> $(EXTENSION).control along the VPATH.

Simpler patch:

diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk
index 271e7eaba8..1cd750eecd 100644
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -229,7 +229,7 @@ endif # MODULE_big
 
 install: all installdirs
 ifneq (,$(EXTENSION))
- $(INSTALL_DATA) $(addprefix $(srcdir)/, $(addsuffix .control, $(EXTENSION))) '$(DESTDIR)$(datadir)/extension/'
+ $(INSTALL_DATA) $(call vpathsearch,$(addsuffix .control, $(EXTENSION))) '$(DESTDIR)$(datadir)/extension/'
 endif # EXTENSION
 ifneq (,$(DATA)$(DATA_built))
  $(INSTALL_DATA) $(addprefix $(srcdir)/, $(DATA)) $(DATA_built) '$(DESTDIR)$(datadir)/$(datamoduledir)/'

Does that work for you?

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Support built control file in PGXS VPATH builds

Craig Ringer-3


On Mon, 9 Mar 2020, 17:27 Peter Eisentraut, <[hidden email]> wrote:
On 2020-02-07 04:14, Craig Ringer wrote:
> The attached patch fixes this by having PGXS resolve
> $(EXTENSION).control along the VPATH.

Simpler patch:

diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk
index 271e7eaba8..1cd750eecd 100644
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -229,7 +229,7 @@ endif # MODULE_big

 install: all installdirs
 ifneq (,$(EXTENSION))
-       $(INSTALL_DATA) $(addprefix $(srcdir)/, $(addsuffix .control, $(EXTENSION))) '$(DESTDIR)$(datadir)/extension/'
+       $(INSTALL_DATA) $(call vpathsearch,$(addsuffix .control, $(EXTENSION))) '$(DESTDIR)$(datadir)/extension/'
 endif # EXTENSION
 ifneq (,$(DATA)$(DATA_built))
        $(INSTALL_DATA) $(addprefix $(srcdir)/, $(DATA)) $(DATA_built) '$(DESTDIR)$(datadir)/$(datamoduledir)/'

Does that work for you?

It wouldn't be my preference because it relies on the VPATH variable. AFAICS the extension  cannot use finer grained vpath directives for this. And if anything relies on VPATH it must be set so you can't really benefit from vpath directives for anything else.

Currently it's possible to build extensions by unsetting VPATH after including pgxs.mk and defining vpath directives only the things you want to search for. This is immensely useful. You can prevent make from looking for build products in the srcdir so you don't get issues with stale files if someone does a vpath build from a dirty worktree that has files left in it from a previous in tree build. Lots of other things too.

So while your patch would work it would definitely not be my preference. Frankly I'd rather be moving on the other direction - doing away with all this DATA vs DATA_BUILT mess entirely and switch everything to using make vpath directives + automatic variable path resolution.

Our vpathsearch function is IMO a bit of a hack we shouldn't need to use at all. The only time it's necessary is when we absolutely need to get the vpath resolved path into a Make variable. Since I don't think make offers its own vpath directive aware search function there's no convenient way to get a make var with the resolved path in it before the target is invoked.

So really I think we should be letting make resolve the targets for us using automatic variables like $< $^ and $@ with the target search logic.

BTW it's definitely rather frustrating that make doesn't appear to have a $(vpathsearch foo) or $(vpathlookup foo) or whatever of its own. Seems very silly to not have that when there are vpath directives. 
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Support built control file in PGXS VPATH builds

Craig Ringer-3
On Mon, 30 Mar 2020 at 11:50, Craig Ringer <[hidden email]> wrote:


On Mon, 9 Mar 2020, 17:27 Peter Eisentraut, <[hidden email]> wrote:
On 2020-02-07 04:14, Craig Ringer wrote:
> The attached patch fixes this by having PGXS resolve
> $(EXTENSION).control along the VPATH.

Simpler patch:

diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk
index 271e7eaba8..1cd750eecd 100644
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -229,7 +229,7 @@ endif # MODULE_big

 install: all installdirs
 ifneq (,$(EXTENSION))
-       $(INSTALL_DATA) $(addprefix $(srcdir)/, $(addsuffix .control, $(EXTENSION))) '$(DESTDIR)$(datadir)/extension/'
+       $(INSTALL_DATA) $(call vpathsearch,$(addsuffix .control, $(EXTENSION))) '$(DESTDIR)$(datadir)/extension/'
 endif # EXTENSION
 ifneq (,$(DATA)$(DATA_built))
        $(INSTALL_DATA) $(addprefix $(srcdir)/, $(DATA)) $(DATA_built) '$(DESTDIR)$(datadir)/$(datamoduledir)/'

Does that work for you?

It wouldn't be my preference because it relies on the VPATH variable. AFAICS the extension  cannot use finer grained vpath directives for this. And if anything relies on VPATH it must be set so you can't really benefit from vpath directives for anything else.


Any thoughts here? I'd like to get it merged if possible and I hope my explanation of why I did it that way clears things up.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 2ndQuadrant - PostgreSQL Solutions for the Enterprise
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Support built control file in PGXS VPATH builds

Daniel Gustafsson
> On 9 Apr 2020, at 05:54, Craig Ringer <[hidden email]> wrote:

> Any thoughts here? I'd like to get it merged if possible and I hope my explanation of why I did it that way clears things up.

According to the CFBot patch tester, this fails the test_extensions and
test_extdepend test suites.  I've marked the patch Waiting on Author.

cheers ./daniel

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Support built control file in PGXS VPATH builds

Daniel Gustafsson
> On 1 Jul 2020, at 13:34, Daniel Gustafsson <[hidden email]> wrote:
>
>> On 9 Apr 2020, at 05:54, Craig Ringer <[hidden email]> wrote:
>
>> Any thoughts here? I'd like to get it merged if possible and I hope my explanation of why I did it that way clears things up.
>
> According to the CFBot patch tester, this fails the test_extensions and
> test_extdepend test suites.  I've marked the patch Waiting on Author.

With the thread stalled and the tests still failing, I've marked this patch
Returned with Feedback.  Feel free to create a new entry when there is a new
version of the patch.

cheers ./daniel