Re: [HACKERS] PGXS "check" target forcing an install ?

2015-09-28 Thread Robert Haas 
On Thu, Sep 24, 2015 at 4:21 PM, Noah Misch  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  
>> 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

Re: [HACKERS] PGXS "check" target forcing an install ?

2015-09-24 Thread Noah Misch 
On Fri, Jun 26, 2015 at 09:09:15AM -0400, Robert Haas wrote:
> On Tue, Jun 23, 2015 at 1:31 AM, Michael Paquier  
> 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

Re: [HACKERS] PGXS check target forcing an install ?

2015-06-26 Thread Robert Haas 
On Tue, Jun 23, 2015 at 1:31 AM, Michael Paquier
michael.paqu...@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
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/

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

Re: [HACKERS] PGXS check target forcing an install ?

2015-06-23 Thread Sandro Santilli 
On Tue, Jun 23, 2015 at 02:31:03PM +0900, Michael Paquier wrote:
 On Tue, Jun 23, 2015 at 12:11 AM, Sandro Santilli s...@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

[HACKERS] PGXS check target forcing an install ?

2015-06-22 Thread Sandro Santilli 
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

Re: [HACKERS] PGXS check target forcing an install ?

2015-06-22 Thread Michael Paquier 
On Tue, Jun 23, 2015 at 12:11 AM, Sandro Santilli s...@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
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`
 

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