PGXS "check" target forcing an install ?

Started by Sandro Santilliover 10 years ago6 messages

Sandro Santilli

strk@keybit.net

over 10 years ago

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. 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier

michael.paquier@gmail.com

over 10 years ago

In reply to: Sandro Santilli (#1)

1 attachment(s)

Re: PGXS "check" target forcing an install ?

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

Attachments:

20150623_pgxs_check_fix.patchapplication/x-patch; name=20150623_pgxs_check_fix.patchDownload

diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index c583b44..82f0c1d 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -54,7 +54,9 @@ top_srcdir = $(abs_top_srcdir)
 srcdir = $(top_srcdir)/$(subdir)
 VPATH = $(srcdir)
 endif
-endif # not PGXS
+else # not PGXS
+NO_TEMP_INSTALL=yes
+endif # PGXS
 
 vpathsearch = `for f in $(addsuffix /$(1),$(subst :, ,. $(VPATH))); do test -r $$f && echo $$f && break; done`

Sandro Santilli

strk@keybit.net

over 10 years ago

In reply to: Michael Paquier (#2)

Re: PGXS "check" target forcing an install ?

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;

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

robertmhaas@gmail.com

over 10 years ago

In reply to: Michael Paquier (#2)

1 attachment(s)

Re: PGXS "check" target forcing an install ?

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

Attachments:

careful-with-rm-rf.patchtext/x-patch; charset=US-ASCII; name=careful-with-rm-rf.patchDownload

diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index c583b44..081ed5d 100644
--- 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_install install || exit; done)
 endif
+endif
 
 PROVE = @PROVE@
 PG_PROVE_FLAGS = -I $(top_srcdir)/src/test/perl/

noah@leadboat.com

over 10 years ago

In reply to: Robert Haas (#4)

Re: PGXS "check" target forcing an install ?

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_install install || exit; done)
endif
+endif

With this in place, there's no need for the NO_TEMP_INSTALL change.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

robertmhaas@gmail.com

over 10 years ago

In reply to: Noah Misch (#5)

Re: PGXS "check" target forcing an install ?

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers