Thread: PGXS "check" target forcing an install ?
I've noted that upgrading from PostgreSQL 9.3 to 9.5 I'm suddenly unable to specify a "check" rule in the Makefile that includes the PGXS one. The error is: $ make checkrm -rf ''/tmp_installmake -C '/home/postgresql-9.5/lib/pgxs/src/makefiles/../..' DESTDIR=''/tmp_install installmake[1]:Entering directory `/home/postgresql-9.5/lib/pgxs'make[1]: *** No rule to make target `install'. Stop.make[1]:Leaving directory `/home/postgresql-9.5/lib/pgxs'make: *** [temp-install] Error 2 I tracked the dangerous -rf to come from Makefile.global and it's empty string being due to abs_top_builddir not being define in my own Makefile. But beside that, which I can probably fix, it doesn't sound correct that a "check" rule insists in finding an "install" rule. I'm also surprised that there's no warning coming out from the "make" invocation given I'm defining a "check" rule myself (after inclusion). Minimal Makefile reproducing the error: PGXS := /home/postgresql-9.3/lib/pgxs/src/makefiles/pgxs.mk # succeeds PGXS := /home/postgresql-9.5/lib/pgxs/src/makefiles/pgxs.mk# fails include $(PGXS) check: echo "Checking" To verify, just run "make check" --strk; () Free GIS & Flash consultant/developer /\ http://strk.keybit.net/services.html
On Tue, Jun 23, 2015 at 12:11 AM, Sandro Santilli <strk@keybit.net> wrote: > I've noted that upgrading from PostgreSQL 9.3 to 9.5 I'm suddenly > unable to specify a "check" rule in the Makefile that includes the > PGXS one. The error is: > > $ make check > rm -rf ''/tmp_install > make -C '/home/postgresql-9.5/lib/pgxs/src/makefiles/../..' DESTDIR=''/tmp_install install > make[1]: Entering directory `/home/postgresql-9.5/lib/pgxs' > make[1]: *** No rule to make target `install'. Stop. > make[1]: Leaving directory `/home/postgresql-9.5/lib/pgxs' > make: *** [temp-install] Error 2 > > I tracked the dangerous -rf to come from Makefile.global and it's empty > string being due to abs_top_builddir not being define in my own Makefile. > But beside that, which I can probably fix, it doesn't sound correct > that a "check" rule insists in finding an "install" rule. Oops, this is a regression, and a dangerous one indeed. This is caused by dcae5fac. One fix is to use NO_TEMP_INSTALL=yes in Makefile.global in the context of PGXS, like in the patch attached, this variable needing to be set before Makefile.global is loaded. We could as well use directly PGXS in the section "Testing", but that does not sound appealing for Makefile.global's readability. > I'm also > surprised that there's no warning coming out from the "make" invocation > given I'm defining a "check" rule myself (after inclusion). Why? It looks perfectly normal to me to be able to define your own check rule. That's more flexible this way. Thoughts? -- Michael
Attachment
On Tue, Jun 23, 2015 at 02:31:03PM +0900, Michael Paquier wrote: > On Tue, Jun 23, 2015 at 12:11 AM, Sandro Santilli <strk@keybit.net> wrote: > > I've noted that upgrading from PostgreSQL 9.3 to 9.5 I'm suddenly > > unable to specify a "check" rule in the Makefile that includes the > > PGXS one. The error is: > > > > $ make check > > rm -rf ''/tmp_install > > make -C '/home/postgresql-9.5/lib/pgxs/src/makefiles/../..' DESTDIR=''/tmp_install install > > make[1]: Entering directory `/home/postgresql-9.5/lib/pgxs' > > make[1]: *** No rule to make target `install'. Stop. > > make[1]: Leaving directory `/home/postgresql-9.5/lib/pgxs' > > make: *** [temp-install] Error 2 > > > > I tracked the dangerous -rf to come from Makefile.global and it's empty > > string being due to abs_top_builddir not being define in my own Makefile. > > But beside that, which I can probably fix, it doesn't sound correct > > that a "check" rule insists in finding an "install" rule. > > Oops, this is a regression, and a dangerous one indeed. This is caused > by dcae5fac. > > One fix is to use NO_TEMP_INSTALL=yes in Makefile.global in the > context of PGXS, like in the patch attached, this variable needing to > be set before Makefile.global is loaded. We could as well use directly > PGXS in the section "Testing", but that does not sound appealing for > Makefile.global's readability. Thanks, setting NO_TEMP_INSTALL=yes in the including Makefile fixes this issue. > > I'm also > > surprised that there's no warning coming out from the "make" invocation > > given I'm defining a "check" rule myself (after inclusion). > > Why? It looks perfectly normal to me to be able to define your own > check rule. That's more flexible this way. I'm surprised because I used to get warnings on overrides, and I actually still get them for other rules. For example: Makefile:192: warning: overriding commands for target `install' /home/postgresql-9.3.4/lib/pgxs/src/makefiles/pgxs.mk:120: warning: ignoring old commands for target `install' The same warning isn't raised for the "check" rule, while it is clearly defined in some upper Makefile (as shown by the forced-install-bug). > Thoughts? Mixed... One one hand I'm happy to implement my own rules, but in this specific case the lack of a warning left me with no hint about where the offending "check" rule was defined. --strk;
On Tue, Jun 23, 2015 at 1:31 AM, Michael Paquier <michael.paquier@gmail.com> wrote: >> I tracked the dangerous -rf to come from Makefile.global and it's empty >> string being due to abs_top_builddir not being define in my own Makefile. >> But beside that, which I can probably fix, it doesn't sound correct >> that a "check" rule insists in finding an "install" rule. > > Oops, this is a regression, and a dangerous one indeed. This is caused > by dcae5fac. > > One fix is to use NO_TEMP_INSTALL=yes in Makefile.global in the > context of PGXS, like in the patch attached, this variable needing to > be set before Makefile.global is loaded. We could as well use directly > PGXS in the section "Testing", but that does not sound appealing for > Makefile.global's readability. Gulp. I certainly agree that emitting rm -rf /tmp_install is a scary thing for a PostgreSQL Makefile to be doing. Fortunately, people aren't likely to have a directory under / by that name, and maybe not permissions on it even if they did, but all the same it's not good. I propose trying to guard against that a bit more explicitly, as in the attached. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Fri, Jun 26, 2015 at 09:09:15AM -0400, Robert Haas wrote: > On Tue, Jun 23, 2015 at 1:31 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > >> I tracked the dangerous -rf to come from Makefile.global and it's empty > >> string being due to abs_top_builddir not being define in my own Makefile. > >> But beside that, which I can probably fix, it doesn't sound correct > >> that a "check" rule insists in finding an "install" rule. > > > > Oops, this is a regression, and a dangerous one indeed. This is caused > > by dcae5fac. > > > > One fix is to use NO_TEMP_INSTALL=yes in Makefile.global in the > > context of PGXS, like in the patch attached, this variable needing to > > be set before Makefile.global is loaded. This seems reasonable in concept, though the patch's addition is off-topic in a section marked "# Support for VPATH builds". However, ... > Gulp. I certainly agree that emitting rm -rf /tmp_install is a scary > thing for a PostgreSQL Makefile to be doing. Fortunately, people > aren't likely to have a directory under / by that name, and maybe not > permissions on it even if they did, but all the same it's not good. I > propose trying to guard against that a bit more explicitly, as in the > attached. ... agreed. > --- a/src/Makefile.global.in > +++ b/src/Makefile.global.in > @@ -304,12 +304,14 @@ check: temp-install > .PHONY: temp-install > temp-install: > ifndef NO_TEMP_INSTALL > +ifneq ($(abs_top_builddir),) > ifeq ($(MAKELEVEL),0) > rm -rf '$(abs_top_builddir)'/tmp_install > $(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install install > endif > $(if $(EXTRA_INSTALL),for extra in $(EXTRA_INSTALL); do $(MAKE) -C '$(top_builddir)'/$$extra DESTDIR='$(abs_top_builddir)'/tmp_installinstall || exit; done) > endif > +endif With this in place, there's no need for the NO_TEMP_INSTALL change.
On Thu, Sep 24, 2015 at 4:21 PM, Noah Misch <noah@leadboat.com> wrote: > On Fri, Jun 26, 2015 at 09:09:15AM -0400, Robert Haas wrote: >> On Tue, Jun 23, 2015 at 1:31 AM, Michael Paquier <michael.paquier@gmail.com> wrote: >> >> I tracked the dangerous -rf to come from Makefile.global and it's empty >> >> string being due to abs_top_builddir not being define in my own Makefile. >> >> But beside that, which I can probably fix, it doesn't sound correct >> >> that a "check" rule insists in finding an "install" rule. >> > >> > Oops, this is a regression, and a dangerous one indeed. This is caused >> > by dcae5fac. >> > >> > One fix is to use NO_TEMP_INSTALL=yes in Makefile.global in the >> > context of PGXS, like in the patch attached, this variable needing to >> > be set before Makefile.global is loaded. > > This seems reasonable in concept, though the patch's addition is off-topic in > a section marked "# Support for VPATH builds". However, ... > >> Gulp. I certainly agree that emitting rm -rf /tmp_install is a scary >> thing for a PostgreSQL Makefile to be doing. Fortunately, people >> aren't likely to have a directory under / by that name, and maybe not >> permissions on it even if they did, but all the same it's not good. I >> propose trying to guard against that a bit more explicitly, as in the >> attached. > > ... agreed. Thanks for reminding me about this patch. I've rebased it and committed it to master and 9.5. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company