Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
On 5/24/19 11:22 AM, Andres Freund wrote: > Hi, > > On 2019-05-22 13:08:41 -0700, Andres Freund wrote: >> On 2019-05-22 16:04:34 -0400, Andrew Dunstan wrote: >>> If I disable install, the buildfarm fails the upgrade check even when >>> not using NO_TEMP_INSTALL. >>> >>> >>> excerpts from the log: >>> sh: /home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql: No such file or >>> directory >> That's the issue I was talking to Tom about above. Need to >> unconditionally have >> > Andrew, after the latest set of changes, the reversed order should now > work reliably? > With the latest changes I don't get the above failure: andrew@emma:pg_head (master)$ ~/bf/client-code/run_build.pl --skip-steps=install master:HEAD [19:21:45] creating vpath build dir /home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build ... master:HEAD [19:21:45] running configure ... master:HEAD [19:21:52] running make ... master:HEAD [19:23:42] running make check ... master:HEAD [19:24:37] running make contrib ... master:HEAD [19:24:45] running make testmodules ... master:HEAD [19:24:45] checking pg_upgrade master:HEAD [19:26:41] checking test-decoding master:HEAD [19:26:59] running make ecpg check ... master:HEAD [19:27:25] OK cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
Andres Freund writes: > Andrew, after the latest set of changes, the reversed order should now > work reliably? Also, Thomas should be able to revert his cfbot hack ... regards, tom lane
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
Hi, On 2019-05-22 13:08:41 -0700, Andres Freund wrote: > On 2019-05-22 16:04:34 -0400, Andrew Dunstan wrote: > > If I disable install, the buildfarm fails the upgrade check even when > > not using NO_TEMP_INSTALL. > > > > > > excerpts from the log: > > sh: /home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql: No such file or > > directory > > That's the issue I was talking to Tom about above. Need to > unconditionally have > Andrew, after the latest set of changes, the reversed order should now work reliably? Greetings, Andres Freund
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
On 2019-05-22 16:04:34 -0400, Andrew Dunstan wrote: > > On 5/22/19 2:42 PM, Andres Freund wrote: > > Hi, > > > > On 2019-05-22 14:27:51 -0400, Tom Lane wrote: > >> Andres Freund writes: > >>> On 2019-05-22 14:06:47 -0400, Tom Lane wrote: > Not sure about that last bit. pg_upgrade has the issue of possibly > wanting to deal with 2 installations, unlike the rest of the tree, > so I'm not sure that fixing its problem means there's something we > need to change everywhere else. > >>> I'm not quite following? We need to move it into global scope to fix the > >>> issue at hand (namely that we currently need to make install first, just > >>> to get psql). And at which scope could it be in master, other than > >>> global? > >> Maybe I misunderstood you --- I thought you were talking about something > >> like defining EXTRA_REGRESS_OPTS in Makefile.global. If you mean > >> running this unconditionally within test.sh, I've got no objection > >> to that. > > Oh, yes, that's what I meant. > > > > > >>> I do think we will have to move it to the global scope in the back > >>> branches too, because NO_TEMP_INSTALL does indeed fail without a global > >>> install first (rather than using the temp install, as intended): > >> Agreed, we should fix it in all branches, because it seems like it's > >> probably testing the wrong thing, ie using the later branch's psql > >> to run the earlier branch's regression tests. > > > > If I disable install, the buildfarm fails the upgrade check even when > not using NO_TEMP_INSTALL. > > > excerpts from the log: > > > > rm -rf '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install > /bin/mkdir -p > '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log > make -C '../../..' > DESTDIR='/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install > install > >'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log/install.log > 2>&1 > make -j1 checkprep > >>'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log/install.log > 2>&1 > MAKE=make > PATH="/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/tmp_install/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin:$PATH" > LD_LIBRARY_PATH="/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/tmp_install/home/pgl/npgl/pg_head/bfroot/HEAD/inst/lib" > > bindir=/home/pgl/npgl/pg_h > ead/bfroot/HEAD/pgsql.build/tmp_install//home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin > EXTRA_REGRESS_OPTS="--port=5678" /bin/sh > /home/pgl/npgl/pg_head/src/bin/pg_upgrade/test.sh > > > rm -rf ./testtablespace > mkdir ./testtablespace > ../../../src/test/regress/pg_regress > --inputdir=/home/pgl/npgl/pg_head/src/test/regress > --bindir='/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin' --port=5678 > --port=54464 --dlpath=. --max-concurrent-tests=20 --port=5678 > --port=54464 > --schedule=/home/pgl/npgl/pg_head/src/test/regress/parallel_schedule > (using postmaster on /tmp/pg_upgrade_check-GCUkGu, port 54464) > == dropping database "regression" == > sh: /home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql: No such file or > directory > command failed: "/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql" -X -c > "DROP DATABASE IF EXISTS \"regression\"" "postgres" > make[1]: *** [GNUmakefile:141: installcheck-parallel] Error 2 > make[1]: Leaving directory > '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/src/test/regress' > make: *** [GNUmakefile:68: installcheck-parallel] Error 2 > make: Leaving directory '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build' > waiting for server to shut down done > server stopped > make: *** [Makefile:48: check] Error 1 > That's the issue I was talking to Tom about above. Need to unconditionally have +# We need to make pg_regress use psql from the desired installation +# (likely a temporary one), 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='$oldbindir'" +export EXTRA_REGRESS_OPTS in all branches (i.e. ressurect in master, do it not just in the --install case in the back branches, and reference $oldbindir rather than $bindir in all branches). > It looks to me like the bindir needs to be passed to the make called by > test.sh (maybe LD_LIBRARY_PATH too?) Think we don't need LD_LIBRARY_PATH, due to the $(with_temp_install) logic in the makefile. In the back branches the --install branch contains adjustments to LD_LIBRARY_PATH (but still references $bindir rather than $oldbindr). Greetings, Andres Freund
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
On 5/22/19 2:42 PM, Andres Freund wrote: > Hi, > > On 2019-05-22 14:27:51 -0400, Tom Lane wrote: >> Andres Freund writes: >>> On 2019-05-22 14:06:47 -0400, Tom Lane wrote: Not sure about that last bit. pg_upgrade has the issue of possibly wanting to deal with 2 installations, unlike the rest of the tree, so I'm not sure that fixing its problem means there's something we need to change everywhere else. >>> I'm not quite following? We need to move it into global scope to fix the >>> issue at hand (namely that we currently need to make install first, just >>> to get psql). And at which scope could it be in master, other than >>> global? >> Maybe I misunderstood you --- I thought you were talking about something >> like defining EXTRA_REGRESS_OPTS in Makefile.global. If you mean >> running this unconditionally within test.sh, I've got no objection >> to that. > Oh, yes, that's what I meant. > > >>> I do think we will have to move it to the global scope in the back >>> branches too, because NO_TEMP_INSTALL does indeed fail without a global >>> install first (rather than using the temp install, as intended): >> Agreed, we should fix it in all branches, because it seems like it's >> probably testing the wrong thing, ie using the later branch's psql >> to run the earlier branch's regression tests. If I disable install, the buildfarm fails the upgrade check even when not using NO_TEMP_INSTALL. excerpts from the log: rm -rf '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install /bin/mkdir -p '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log make -C '../../..' DESTDIR='/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install install >'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log/install.log 2>&1 make -j1 checkprep >>'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log/install.log 2>&1 MAKE=make PATH="/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/tmp_install/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin:$PATH" LD_LIBRARY_PATH="/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/tmp_install/home/pgl/npgl/pg_head/bfroot/HEAD/inst/lib" bindir=/home/pgl/npgl/pg_h ead/bfroot/HEAD/pgsql.build/tmp_install//home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin EXTRA_REGRESS_OPTS="--port=5678" /bin/sh /home/pgl/npgl/pg_head/src/bin/pg_upgrade/test.sh rm -rf ./testtablespace mkdir ./testtablespace ../../../src/test/regress/pg_regress --inputdir=/home/pgl/npgl/pg_head/src/test/regress --bindir='/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin' --port=5678 --port=54464 --dlpath=. --max-concurrent-tests=20 --port=5678 --port=54464 --schedule=/home/pgl/npgl/pg_head/src/test/regress/parallel_schedule (using postmaster on /tmp/pg_upgrade_check-GCUkGu, port 54464) == dropping database "regression" == sh: /home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql: No such file or directory command failed: "/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql" -X -c "DROP DATABASE IF EXISTS \"regression\"" "postgres" make[1]: *** [GNUmakefile:141: installcheck-parallel] Error 2 make[1]: Leaving directory '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/src/test/regress' make: *** [GNUmakefile:68: installcheck-parallel] Error 2 make: Leaving directory '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build' waiting for server to shut down done server stopped make: *** [Makefile:48: check] Error 1 It looks to me like the bindir needs to be passed to the make called by test.sh (maybe LD_LIBRARY_PATH too?) cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
Hi, On 2019-05-22 14:27:51 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2019-05-22 14:06:47 -0400, Tom Lane wrote: > >> Not sure about that last bit. pg_upgrade has the issue of possibly > >> wanting to deal with 2 installations, unlike the rest of the tree, > >> so I'm not sure that fixing its problem means there's something we > >> need to change everywhere else. > > > I'm not quite following? We need to move it into global scope to fix the > > issue at hand (namely that we currently need to make install first, just > > to get psql). And at which scope could it be in master, other than > > global? > > Maybe I misunderstood you --- I thought you were talking about something > like defining EXTRA_REGRESS_OPTS in Makefile.global. If you mean > running this unconditionally within test.sh, I've got no objection > to that. Oh, yes, that's what I meant. > > I do think we will have to move it to the global scope in the back > > branches too, because NO_TEMP_INSTALL does indeed fail without a global > > install first (rather than using the temp install, as intended): > > Agreed, we should fix it in all branches, because it seems like it's > probably testing the wrong thing, ie using the later branch's psql > to run the earlier branch's regression tests. Ok, will do. Greetings, Andres Freund
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
Andres Freund writes: > On 2019-05-22 14:06:47 -0400, Tom Lane wrote: >> Not sure about that last bit. pg_upgrade has the issue of possibly >> wanting to deal with 2 installations, unlike the rest of the tree, >> so I'm not sure that fixing its problem means there's something we >> need to change everywhere else. > I'm not quite following? We need to move it into global scope to fix the > issue at hand (namely that we currently need to make install first, just > to get psql). And at which scope could it be in master, other than > global? Maybe I misunderstood you --- I thought you were talking about something like defining EXTRA_REGRESS_OPTS in Makefile.global. If you mean running this unconditionally within test.sh, I've got no objection to that. > I do think we will have to move it to the global scope in the back > branches too, because NO_TEMP_INSTALL does indeed fail without a global > install first (rather than using the temp install, as intended): Agreed, we should fix it in all branches, because it seems like it's probably testing the wrong thing, ie using the later branch's psql to run the earlier branch's regression tests. regards, tom lane
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
Hi, On 2019-05-22 14:06:47 -0400, Tom Lane wrote: > Andres Freund writes: > > Seems what we need to fix the immediate issue is to ressurect: > > > # 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 > > > and put that into global scope. > > Not sure about that last bit. pg_upgrade has the issue of possibly > wanting to deal with 2 installations, unlike the rest of the tree, > so I'm not sure that fixing its problem means there's something we > need to change everywhere else. I'm not quite following? We need to move it into global scope to fix the issue at hand (namely that we currently need to make install first, just to get psql). And at which scope could it be in master, other than global? I do think we will have to move it to the global scope in the back branches too, because NO_TEMP_INSTALL does indeed fail without a global install first (rather than using the temp install, as intended): On 11: $ make -j16 -s uninstall $ make -j16 -s temp-install $ make -j16 -s -C src/bin/pg_upgrade/ check NO_TEMP_INSTALL=1 ... ../../../src/test/regress/pg_regress --inputdir=/home/andres/src/postgresql-11/src/test/regress --bindir='/home/andres/build/postgres/11-assert//install/bin'--port=60851 --dlpath=. --max-concurrent-tests=20 --port=60851 --schedule=/home/andres/src/postgresql-11/src/test/regress/serial_schedule (using postmaster on /tmp/pg_upgrade_check-uEwhDs, port 60851) == dropping database "regression" == sh: 1: /home/andres/build/postgres/11-assert//install/bin/psql: not found $ make -j16 -s install $ make -j16 -s -C src/bin/pg_upgrade/ check NO_TEMP_INSTALL=1 && echo success ... success As you can see it uses pg_regress etc from the temp installation, but psql from the full installation. > (IOW, keep an eye on the cross-version-upgrade tests while > you mess with this...) I will. If you refer to the buildfarm ones: As far as I can tell they don't use test.sh at all. Which makes sense, as we need cleanup steps inbetween the regression run and pg_upgrade, and test.sh doesn't allow for that. https://github.com/PGBuildFarm/client-code/blob/master/PGBuild/Modules/TestUpgradeXversion.pm Greetings, Andres Freund
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
Andres Freund writes: > Seems what we need to fix the immediate issue is to ressurect: > # 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 > and put that into global scope. Not sure about that last bit. pg_upgrade has the issue of possibly wanting to deal with 2 installations, unlike the rest of the tree, so I'm not sure that fixing its problem means there's something we need to change everywhere else. (IOW, keep an eye on the cross-version-upgrade tests while you mess with this...) regards, tom lane
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
Hi, On 2019-05-22 10:58:54 -0400, Tom Lane wrote: > Thomas Munro writes: > > After these commits (and Tom's commit "Un-break pg_upgrade regression > > test."), cfbot broke: I should just have finished working two hours earlier yesterday :(. > > sh: 1: /usr/local/pgsql/bin/psql: not found > > I can confirm that here: check-world passes as long as I've done > "make install" beforehand ... but of course that should not be > necessary. If I blow away the install tree, pg_upgrade's > check fails at > ../../../src/test/regress/pg_regress --inputdir=. > --bindir='/home/postgres/testversion/bin'--port=54464 --dlpath=. > --max-concurrent-tests=20 --port=54464 --schedule=./parallel_schedule > (using postmaster on /tmp/pg_upgrade_check-Nitf3h, port 54464) > == dropping database "regression" == > sh: /home/postgres/testversion/bin/psql: No such file or directory > command failed: "/home/postgres/testversion/bin/psql" -X -c "DROP DATABASE IF > EXISTS \"regression\"" "postgres" > > pg_regress is being told the wrong --bindir, ie > the final install location not the temp install. Yea, that's indeed the problem. I suspect that problem already exists in the NO_TEMP_INSTALL solution committed in https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=47b3c26642e6850e8dfa7afe01db78320b11549e It's just that nobody noticed that due to: > (More generally, should we rearrange the buildfarm test > sequence so it doesn't run "make install" till after the > tests that aren't supposed to require an installed tree?) Seems what we need to fix the immediate issue is to ressurect: # 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 and put that into global scope. While that'd be unnecessary when invoking ./test.sh from commandline with explicitly already installed binaries, it should be harmless there. I wonder however, shouldn't the above stanza refer to $oldbindir? I think we need to backpatch the move of the above outside the --install path, because otherwise the buildfarm will break once we reorder the buildfarm's scripts to do the make install later. Unless I miss something? > (More generally, should we rearrange the buildfarm test > sequence so it doesn't run "make install" till after the > tests that aren't supposed to require an installed tree?) Seems like a good idea. On buildfarm's master the order is: make_check() unless $delay_check; # contrib is built under the standard build step for msvc make_contrib() unless ($using_msvc); make_testmodules() if (!$using_msvc && ($branch eq 'HEAD' || $branch ge 'REL9_5')); make_doc() if (check_optional_step('build_docs')); make_install(); # contrib is installed under standard install for msvc make_contrib_install() unless ($using_msvc); make_testmodules_install() if (!$using_msvc && ($branch eq 'HEAD' || $branch ge 'REL9_5')); make_check() if $delay_check; process_module_hooks('configure'); process_module_hooks('build'); process_module_hooks("check") unless $delay_check; process_module_hooks('install'); process_module_hooks("check") if $delay_check; run_bin_tests(); run_misc_tests(); ... locale tests, ecpg, typedefs Seems like we ought to at least move run_bin_tests, run_misc_tests up? I'm not quite sure what the idea of $delay_check is. I found: > * a new --delay-check switch delays the check step until after >install. This helps work around a bug or lack of capacity w.r.t. >LD_LIBRARY_PATH on Alpine Linux in an release announcement. But no further details. Greetings, Andres Freund
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
Thomas Munro writes: > After these commits (and Tom's commit "Un-break pg_upgrade regression > test."), cfbot broke: > sh: 1: /usr/local/pgsql/bin/psql: not found I can confirm that here: check-world passes as long as I've done "make install" beforehand ... but of course that should not be necessary. If I blow away the install tree, pg_upgrade's check fails at ../../../src/test/regress/pg_regress --inputdir=. --bindir='/home/postgres/testversion/bin'--port=54464 --dlpath=. --max-concurrent-tests=20 --port=54464 --schedule=./parallel_schedule (using postmaster on /tmp/pg_upgrade_check-Nitf3h, port 54464) == dropping database "regression" == sh: /home/postgres/testversion/bin/psql: No such file or directory command failed: "/home/postgres/testversion/bin/psql" -X -c "DROP DATABASE IF EXISTS \"regression\"" "postgres" pg_regress is being told the wrong --bindir, ie the final install location not the temp install. (More generally, should we rearrange the buildfarm test sequence so it doesn't run "make install" till after the tests that aren't supposed to require an installed tree?) regards, tom lane
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
On Wed, May 22, 2019 at 7:41 AM Andres Freund wrote:> > On 2019-05-21 12:19:18 -0700, Andres Freund wrote: > > Roughly like in the attached? > > > -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) > > minus the duplicated MAKE=$(MAKE) of course. After these commits (and Tom's commit "Un-break pg_upgrade regression test."), cfbot broke: (using postmaster on /tmp/pg_upgrade_check-YGuskp, port 54464) == dropping database "regression" == sh: 1: /usr/local/pgsql/bin/psql: not found command failed: "/usr/local/pgsql/bin/psql" -X -c "DROP DATABASE IF EXISTS \"regression\"" "postgres" make[1]: *** [installcheck-parallel] Error 2 make[1]: Leaving directory `/home/travis/build/postgresql-cfbot/postgresql/src/test/regress' Before that it had been running happily like this: ./configure --enable-debug --enable-cassert --enable-tap-tests --with-tcl --with-python --with-perl --with-ldap --with-openssl --with-gssapi --with-icu && echo "COPT=-Wall -Werror" > src/Makefile.custom && make -j4 all contrib docs && make check-world I added --prefix=$HOME/something and added "make install" before "make check-world", and now it's happy again. Was that expected? -- Thomas Munro https://enterprisedb.com
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
Hi, On 2019-05-21 12:19:18 -0700, Andres Freund wrote: > Roughly like in the attached? > -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) minus the duplicated MAKE=$(MAKE) of course. Greetings, Andres Freund
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
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
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
Andrew Dunstan writes: > On 5/20/19 9:58 PM, Andres Freund wrote: >> I'm confused as to why this was done as a purely optional path, rather >> than just ripping out the pg_upgrade specific install? > 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. In fact, there have been several iterations of code to get the > buildfarm client to check reasonable reliably that there is such an > install before it chooses to use the flag. Right. Issuing "make check" in src/bin/pg_upgrade certainly shouldn't skip making a new install. But if we're recursing down from a top-level check-world, we ought to be able to use the install it made. regards, tom lane
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
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. In fact, there have been several iterations of code to get the buildfarm client to check reasonable reliably that there is such an install before it chooses to use the flag. Note that the buildfarm doesn't run "make check-world" for reasons I have explained in the past. NO_TEMP_INSTALL is particularly valuable in saving time when running the TAP tests, especially on Mingw. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
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 Greetings, Andres Freund
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
Andrew Dunstan writes: > 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 haven't tested this, but it looks reasonable. I suspect you need double-quotes around the path values, as in the adjacent usage of EXTRA_REGRESS_OPTS. regards, tom lane
Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL
On Saturday, March 30, 2019 9:42 PM, 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. While I haven't tried the patch (yet), reading it it makes sense, so +1 on the fix. Nice catch! cheers ./daniel