Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL

Hi,

On 2019-05-22 10:58:54 -0400, Tom Lane wrote:
> Thomas Munro <thomas.munro@gmail.com> 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