Hi,
On 2019-05-21 14:48:27 -0400, Andrew Dunstan wrote:
> On 5/20/19 9:58 PM, Andres Freund wrote:
> > Hi Andrew,
> >
> > On 2019-03-30 16:42:16 -0400, Andrew Dunstan wrote:
> >> On some machines (*cough* Mingw *cough*) installs are very slow. We've
> >> ameliorated this by allowing temp installs to be reused, but the
> >> pg_upgrade Makefile never got the message. Here's a patch that does
> >> that. I'd like to backpatch it, at least to 9.5 where we switched the
> >> pg_upgrade location. The risk seems appropriately low and it only
> >> affects our test regime.
> > I'm confused as to why this was done as a purely optional path, rather
> > than just ripping out the pg_upgrade specific install?
> >
> > See also discussion around
> > https://www.postgresql.org/message-id/21766.1558397960%40sss.pgh.pa.us
> >
>
> By specifying NO_TEMP_INSTALL you are in effect certifying that there is
> already a suitable temp install available. But that might well not be
> the case.
But all that takes is adding a dependency to temp-install in
src/bin/pg_upgrade/Makefile's check target? Like many other regression
test? And the temp-install rule already honors NO_TEMP_INSTALL:
temp-install: | submake-generated-headers
ifndef NO_TEMP_INSTALL
ifneq ($(abs_top_builddir),)
ifeq ($(MAKELEVEL),0)
rm -rf '$(abs_top_builddir)'/tmp_install
$(MKDIR_P) '$(abs_top_builddir)'/tmp_install/log
$(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install
install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
$(MAKE) -j1 $(if $(CHECKPREP_TOP),-C $(CHECKPREP_TOP),) checkprep
>>'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
endif
endif
endif
I'm not saying that you shouldn't have added NO_TEMP_INSTALL support or
something, I'm confused as to why the support for custom installations
inside test.sh was retained.
Roughly like in the attached?
Greetings,
Andres Freund
diff --git a/src/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile
index 5a189484251..305257f3bcd 100644
--- a/src/bin/pg_upgrade/Makefile
+++ b/src/bin/pg_upgrade/Makefile
@@ -14,16 +14,6 @@ OBJS = check.o controldata.o dump.o exec.o file.o function.o info.o \
override CPPFLAGS := -DDLSUFFIX=\"$(DLSUFFIX)\" -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
-ifdef NO_TEMP_INSTALL
- tbindir=$(abs_top_builddir)/tmp_install/$(bindir)
- tlibdir=$(abs_top_builddir)/tmp_install/$(libdir)
- DOINST =
-else
- tbindir=$(bindir)
- tlibdir=$(libdir)
- DOINST = --install
-endif
-
all: pg_upgrade
pg_upgrade: $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
@@ -45,8 +35,8 @@ clean distclean maintainer-clean:
pg_upgrade_dump_globals.sql \
pg_upgrade_dump_*.custom pg_upgrade_*.log
-check: test.sh all
- MAKE=$(MAKE) bindir="$(tbindir)" libdir="$(tlibdir)" EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $< $(DOINST)
+check: test.sh all temp-install
+ MAKE=$(MAKE) $(with_temp_install) bindir=$(abs_top_builddir)/tmp_install/$(bindir) MAKE=$(MAKE) EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $< $(DOINST)
# installcheck is not supported because there's no meaningful way to test
# pg_upgrade against a single already-running server
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
index 598f4a1e11b..be0055ee6bc 100644
--- a/src/bin/pg_upgrade/test.sh
+++ b/src/bin/pg_upgrade/test.sh
@@ -70,39 +70,15 @@ export PGHOST
# don't rely on $PWD here, as old shells don't set it
temp_root=`pwd`/tmp_check
-if [ "$1" = '--install' ]; then
- temp_install=$temp_root/install
- bindir=$temp_install/$bindir
- libdir=$temp_install/$libdir
-
- "$MAKE" -s -C ../.. install DESTDIR="$temp_install"
-
- # platform-specific magic to find the shared libraries; see pg_regress.c
- LD_LIBRARY_PATH=$libdir:$LD_LIBRARY_PATH
- export LD_LIBRARY_PATH
- DYLD_LIBRARY_PATH=$libdir:$DYLD_LIBRARY_PATH
- export DYLD_LIBRARY_PATH
- LIBPATH=$libdir:$LIBPATH
- export LIBPATH
- SHLIB_PATH=$libdir:$SHLIB_PATH
- export SHLIB_PATH
- PATH=$libdir:$PATH
-
- # We need to make it use psql from our temporary installation,
- # because otherwise the installcheck run below would try to
- # use psql from the proper installation directory, which might
- # be outdated or missing. But don't override anything else that's
- # already in EXTRA_REGRESS_OPTS.
- EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --bindir='$bindir'"
- export EXTRA_REGRESS_OPTS
-fi
-
: ${oldbindir=$bindir}
: ${oldsrc=../../..}
oldsrc=`cd "$oldsrc" && pwd`
newsrc=`cd ../../.. && pwd`
+# While in normal cases this will already be set up, adding bindir to
+# path allows test.sh to be invoked with different versions as
+# described in ./TESTING
PATH=$bindir:$PATH
export PATH
