Hi,
On 2019-05-20 17:47:17 -0700, Andres Freund wrote:
> On 2019-05-20 20:19:20 -0400, Tom Lane wrote:
> > Andres Freund <and...@anarazel.de> writes:
> > > On 2019-05-20 14:09:40 -0400, Tom Lane wrote:
> > > 3) The fact that src/bin/pg_upgrade/Makefile invokes test.sh with
> > > MAKE=$(MAKE) triggers make, for reasons I do not yet understand, to
> > > *disable* output synchronization. Which annoys the heck out of me,
> > > because it makes parallel check output neigh unparsable.
> >
> > Probably the same thing as your 2)?
>
> It's not quite. I've not yet fully understood it yet. It's *not* tied to
> MAKEFLAGS. If I understand correctly, when make sees $(MAKE)/${MAKE} in
> a recipe, or the recipe starts with +, it assumes that the sub-command
> is *also* make. And the reason that output synchronization doesn't work
> is that make *intentionally* doesn't enable that for submakes - it only
> wants it for commands run by those submakes (otherwise it could not
> individually output individual targets processed in submakes).
>
> I think the right approach might be to just *never* invoke make inside
> test.sh:
> 1) We can fairly easily fix the make install to be unnecessary
> 2) I think the installcheck-parallel could be replaced by instead
> passing in the the necessary pg_regress command. That'd also have the
> benefit of not having another make invocation stat'ing a lot of files
> etc.
>
> I'll play with these.
1) was easy enough (posted at [1], and also attached here). I think 2)
is a bit harder, because according to src/bin/pg_upgrade/TESTING we
currently support invoking test.sh from commandline.
Therefore I think we ought to just work around that make heuristic, by
referring to $(MAKE) via an indirection. Like in the attached. After
that I don't get any interspersed output anymore if I run with -Otarget.
Greetings,
Andres Freund
[1] https://postgr.es/m/20190521191918.z7kwnrlj45mk2k67%40alap3.anarazel.de
>From 2a903c94e61e2e0ca7596accbf96d884bb4c40fb Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Tue, 21 May 2019 12:39:45 -0700
Subject: [PATCH v2 1/2] pg_upgrade: Don't use separate installation for test.
Author: Andres Freund
Discussion: https://postgr.es/m/20190521191918.z7kwnrlj45mk2...@alap3.anarazel.de
---
src/bin/pg_upgrade/Makefile | 14 ++------------
src/bin/pg_upgrade/test.sh | 30 +++---------------------------
2 files changed, 5 insertions(+), 39 deletions(-)
diff --git a/src/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile
index 5a189484251..062afe9938d 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) 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
--
2.21.0.dirty
>From 869025e0db38a1d041ecd8de669d92f24dbcd131 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Tue, 21 May 2019 12:45:03 -0700
Subject: [PATCH v2 2/2] pg_upgrade: Avoid check target accidentally breaking
make's --output-sync.
When $(MAKE) is present in a rule, make assumes that target is a
submake, and it doesn't need to buffer its output. But in this case
it's a shell script that needs buffered output. Avoid that heuristic,
by referring to $(MAKE) via an indirection.
Discussion: https://postgr.es/m/20190521004717.qsktdsugj3sha...@alap3.anarazel.de
---
src/bin/pg_upgrade/Makefile | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile
index 062afe9938d..70edaade478 100644
--- a/src/bin/pg_upgrade/Makefile
+++ b/src/bin/pg_upgrade/Makefile
@@ -35,8 +35,17 @@ clean distclean maintainer-clean:
pg_upgrade_dump_globals.sql \
pg_upgrade_dump_*.custom pg_upgrade_*.log
+# When $(MAKE) is present, make automatically infers that this is a
+# recursive make. which is not actually what we want here, as that
+# e.g. prevents output synchronization from working (as make things
+# that the subsidiary make knows how to deal with that itself, but
+# we're invoking a shell script that doesn't know). Referencing
+# $(MAKE) indirectly avoids that behaviour.
+# See https://www.gnu.org/software/make/manual/html_node/MAKE-Variable.html#MAKE-Variable
+NOTSUBMAKEMAKE=$(MAKE)
+
check: test.sh all temp-install
- MAKE=$(MAKE) $(with_temp_install) bindir=$(abs_top_builddir)/tmp_install/$(bindir) EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $< $(DOINST)
+ MAKE=$(NOTSUBMAKEMAKE) $(with_temp_install) bindir=$(abs_top_builddir)/tmp_install/$(bindir) 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
--
2.21.0.dirty
