Thread: [PATCH] Support built control file in PGXS VPATH builds
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
Attachment
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
On Mon, 9 Mar 2020, 17:27 Peter Eisentraut, <peter.eisentraut@2ndquadrant.com> 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.
On Mon, 30 Mar 2020 at 11:50, Craig Ringer <craig@2ndquadrant.com> wrote:
On Mon, 9 Mar 2020, 17:27 Peter Eisentraut, <peter.eisentraut@2ndquadrant.com> 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.
> On 9 Apr 2020, at 05:54, Craig Ringer <craig@2ndquadrant.com> 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 thingsup. 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
> On 1 Jul 2020, at 13:34, Daniel Gustafsson <daniel@yesql.se> wrote: > >> On 9 Apr 2020, at 05:54, Craig Ringer <craig@2ndquadrant.com> 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 thingsup. > > 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