pgsql: Reference test binary using TESTDIR in 001_libpq_pipeline.pl.

On 10/1/21 6:34 PM, Andres Freund wrote:

Reference test binary using TESTDIR in 001_libpq_pipeline.pl.

The previous approach didn't really work on windows, due to the PATH separator
being ';' not ':'. Instead of making the PATH change more complicated,
reference the binary using the TESTDIR environment.

Reported-By: Andres Freund <andres@anarazel.de>
Suggested-By: Andrew Dunstan <andrew@dunslane.net>
Discussion: /messages/by-id/20210930214040.odkdd42vknvzifm6@alap3.anarazel.de
Backpatch: 14-, where the test was introduced.

I don't think any of us were thinking very clearly about this. Of
course, MSVC doesn't build executables in $TESTDIR. If we want to pick
up the executable from where it's built we'll need a little help from
vcregress.pl. I haven't tested it but What I have in mind is something
like the attached.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 10/8/21 11:41 AM, Andrew Dunstan wrote:

On 10/1/21 6:34 PM, Andres Freund wrote:

Reference test binary using TESTDIR in 001_libpq_pipeline.pl.

The previous approach didn't really work on windows, due to the PATH separator
being ';' not ':'. Instead of making the PATH change more complicated,
reference the binary using the TESTDIR environment.

Reported-By: Andres Freund <andres@anarazel.de>
Suggested-By: Andrew Dunstan <andrew@dunslane.net>
Discussion: /messages/by-id/20210930214040.odkdd42vknvzifm6@alap3.anarazel.de
Backpatch: 14-, where the test was introduced.

I don't think any of us were thinking very clearly about this. Of
course, MSVC doesn't build executables in $TESTDIR. If we want to pick
up the executable from where it's built we'll need a little help from
vcregress.pl. I haven't tested it but What I have in mind is something
like the attached.

Confirmed that this makes the test pass.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi,

On 2021-10-08 11:41:50 -0400, Andrew Dunstan wrote:

On 10/1/21 6:34 PM, Andres Freund wrote:

Reference test binary using TESTDIR in 001_libpq_pipeline.pl.

The previous approach didn't really work on windows, due to the PATH separator
being ';' not ':'. Instead of making the PATH change more complicated,
reference the binary using the TESTDIR environment.

Reported-By: Andres Freund <andres@anarazel.de>
Suggested-By: Andrew Dunstan <andrew@dunslane.net>
Discussion: /messages/by-id/20210930214040.odkdd42vknvzifm6@alap3.anarazel.de
Backpatch: 14-, where the test was introduced.

I don't think any of us were thinking very clearly about this. Of
course, MSVC doesn't build executables in $TESTDIR. If we want to pick
up the executable from where it's built we'll need a little help from
vcregress.pl. I haven't tested it but What I have in mind is something
like the attached.

Hm. Clearly this needs more work. But I don't really like having checks for
MSBUILDDIR in individual tests - we should do this somewhere more central.
Afaictl the windows build just installs libpq_pipeline.exe. If we make sure
that on !msvc builds CURDIR is on PATH and on msvc temp_install/bin/ is on
PATH (which it should already be), then we should be good?

I guess the reason this didn't work for me before was that I invoked
install.pl from the top directory, which then didn't install
libpq_pipeline.exe, because of the config.pl issue I mentioned somewhere?

Greetings,

Andres Freund

On 10/8/21 5:14 PM, Andres Freund wrote:

Hi,

On 2021-10-08 11:41:50 -0400, Andrew Dunstan wrote:

On 10/1/21 6:34 PM, Andres Freund wrote:

Reference test binary using TESTDIR in 001_libpq_pipeline.pl.

The previous approach didn't really work on windows, due to the PATH separator
being ';' not ':'. Instead of making the PATH change more complicated,
reference the binary using the TESTDIR environment.

Reported-By: Andres Freund <andres@anarazel.de>
Suggested-By: Andrew Dunstan <andrew@dunslane.net>
Discussion: /messages/by-id/20210930214040.odkdd42vknvzifm6@alap3.anarazel.de
Backpatch: 14-, where the test was introduced.

I don't think any of us were thinking very clearly about this. Of
course, MSVC doesn't build executables in $TESTDIR. If we want to pick
up the executable from where it's built we'll need a little help from
vcregress.pl. I haven't tested it but What I have in mind is something
like the attached.

Hm. Clearly this needs more work. But I don't really like having checks for
MSBUILDDIR in individual tests - we should do this somewhere more central.
Afaictl the windows build just installs libpq_pipeline.exe. If we make sure
that on !msvc builds CURDIR is on PATH and on msvc temp_install/bin/ is on
PATH (which it should already be), then we should be good?

I guess the reason this didn't work for me before was that I invoked
install.pl from the top directory, which then didn't install
libpq_pipeline.exe, because of the config.pl issue I mentioned somewhere?

The whole point of 6abc8c2596 AIUI was to have this program not
installed (that's why for the test we need to get it from $TESTDIR where
it's built except when we're using MSVC). That commit should probably
have made sure the MSVC install process likewise didn't install it. So
ISTM that relying on it being installed is going in the wrong direction.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 10/8/21 8:33 PM, Andrew Dunstan wrote:

On 10/8/21 5:14 PM, Andres Freund wrote:

Hi,

On 2021-10-08 11:41:50 -0400, Andrew Dunstan wrote:

On 10/1/21 6:34 PM, Andres Freund wrote:

Reference test binary using TESTDIR in 001_libpq_pipeline.pl.

The previous approach didn't really work on windows, due to the PATH separator
being ';' not ':'. Instead of making the PATH change more complicated,
reference the binary using the TESTDIR environment.

Reported-By: Andres Freund <andres@anarazel.de>
Suggested-By: Andrew Dunstan <andrew@dunslane.net>
Discussion: /messages/by-id/20210930214040.odkdd42vknvzifm6@alap3.anarazel.de
Backpatch: 14-, where the test was introduced.

I don't think any of us were thinking very clearly about this. Of
course, MSVC doesn't build executables in $TESTDIR. If we want to pick
up the executable from where it's built we'll need a little help from
vcregress.pl. I haven't tested it but What I have in mind is something
like the attached.

Hm. Clearly this needs more work. But I don't really like having checks for
MSBUILDDIR in individual tests - we should do this somewhere more central.
Afaictl the windows build just installs libpq_pipeline.exe. If we make sure
that on !msvc builds CURDIR is on PATH and on msvc temp_install/bin/ is on
PATH (which it should already be), then we should be good?

I guess the reason this didn't work for me before was that I invoked
install.pl from the top directory, which then didn't install
libpq_pipeline.exe, because of the config.pl issue I mentioned somewhere?

The whole point of 6abc8c2596 AIUI was to have this program not
installed (that's why for the test we need to get it from $TESTDIR where
it's built except when we're using MSVC). That commit should probably
have made sure the MSVC install process likewise didn't install it. So
ISTM that relying on it being installed is going in the wrong direction.

It's been 10 days or so, so we really need to get this fixed.

Is the attached more to your taste?

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2021-Oct-17, Andrew Dunstan wrote:

+sub find_built_program
+{
+	my $program = shift;
+	my $path;
+
+	if (defined $ENV{MSBUILDDIR})
+	{
+		# vcregress.pl sets MSBUILDDIR which is the root of all the build dirs
+		my $wanted = sub { $_ eq "$program.exe" && $path = $File::Find::name;};
+		File::Find::find($wanted,$ENV{MSBUILDDIR});
+	}

Hmm, it seems weird to have to use File::Find when we already know where
the program is, right? I mean, per your previous patch, we know that
the program is in $MSBUILDDIR/libpq_pipeline/libpq_pipeline.exe, so why
don't we make this one be

if (defined $ENV{MSBUILDDIR})
{
return $ENV{MSBUILDDIR} . "$program/$program.exe";
}

?

I noticed that drongo is still red, and reporting that no tests are
being run. While this makes sense (because the list of tests is
obtained by running libpq_pipeline), I am surprised that this isn't
reported as such. I would have expected it to die with an error message
starting with "oops: " ... did run_command() fail to return stderr?

... oh, run_command() seems a bit too optimistic: it doesn't check
whether IPC::Run::run() failed. I think that's worth fixing
independently.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Subversion to GIT: the shortest path to happiness I've ever heard of
(Alexey Klyukin)

On 10/18/21 10:23 AM, Alvaro Herrera wrote:

On 2021-Oct-17, Andrew Dunstan wrote:

+sub find_built_program
+{
+	my $program = shift;
+	my $path;
+
+	if (defined $ENV{MSBUILDDIR})
+	{
+		# vcregress.pl sets MSBUILDDIR which is the root of all the build dirs
+		my $wanted = sub { $_ eq "$program.exe" && $path = $File::Find::name;};
+		File::Find::find($wanted,$ENV{MSBUILDDIR});
+	}

Hmm, it seems weird to have to use File::Find when we already know where
the program is, right? I mean, per your previous patch, we know that
the program is in $MSBUILDDIR/libpq_pipeline/libpq_pipeline.exe, so why
don't we make this one be

if (defined $ENV{MSBUILDDIR})
{
return $ENV{MSBUILDDIR} . "$program/$program.exe";
}

Could do, but say the module had two programs? Mostly we don't do that
but Release/pg_isolation_regress has two executables, so it's more than
just theoretically possible.

File::Find on the build directory is likely to be fairly quick, but a
simpler way might be to use glob expansion:

maybe something like:

my @progs = glob("$ENV{MSBUILDDIR}/*/$program.exe");

plus some logic to disambiguate any duplicates.

?

I noticed that drongo is still red, and reporting that no tests are
being run. While this makes sense (because the list of tests is
obtained by running libpq_pipeline), I am surprised that this isn't
reported as such. I would have expected it to die with an error message
starting with "oops: " ... did run_command() fail to return stderr?

... oh, run_command() seems a bit too optimistic: it doesn't check
whether IPC::Run::run() failed. I think that's worth fixing
independently.

Yeah.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi,

On 2021-10-18 11:39:08 -0400, Andrew Dunstan wrote:

On 10/18/21 10:23 AM, Alvaro Herrera wrote:

On 2021-Oct-17, Andrew Dunstan wrote:

+sub find_built_program
+{
+	my $program = shift;
+	my $path;
+
+	if (defined $ENV{MSBUILDDIR})
+	{
+		# vcregress.pl sets MSBUILDDIR which is the root of all the build dirs
+		my $wanted = sub { $_ eq "$program.exe" && $path = $File::Find::name;};
+		File::Find::find($wanted,$ENV{MSBUILDDIR});
+	}

Hmm, it seems weird to have to use File::Find when we already know where
the program is, right? I mean, per your previous patch, we know that
the program is in $MSBUILDDIR/libpq_pipeline/libpq_pipeline.exe, so why
don't we make this one be

if (defined $ENV{MSBUILDDIR})
{
return $ENV{MSBUILDDIR} . "$program/$program.exe";
}

Could do, but say the module had two programs? Mostly we don't do that
but Release/pg_isolation_regress has two executables, so it's more than
just theoretically possible.

ISTM that we should just make the "test invocation framework" responsible to
put the relevant directory onto PATH? I.e. Makefile.global should add the
build directory of the test to PATH, and vcregress.pl should put the relevant
$configuration/$project/ directory there?

Greetings,

Andres Freund

On 10/20/21 12:31 PM, Andres Freund wrote:

ISTM that we should just make the "test invocation framework" responsible to
put the relevant directory onto PATH? I.e. Makefile.global should add the
build directory of the test to PATH, and vcregress.pl should put the relevant
$configuration/$project/ directory there?

Like this? That seems reasonable.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi,

On 2021-10-20 14:30:49 -0400, Andrew Dunstan wrote:

On 10/20/21 12:31 PM, Andres Freund wrote:

ISTM that we should just make the "test invocation framework" responsible to
put the relevant directory onto PATH? I.e. Makefile.global should add the
build directory of the test to PATH, and vcregress.pl should put the relevant
$configuration/$project/ directory there?

Like this? That seems reasonable.

Yea, that's along the lines of what I was thinking...

Greetings,

Andres Freund

On 10/20/21 3:09 PM, Andres Freund wrote:

Hi,

On 2021-10-20 14:30:49 -0400, Andrew Dunstan wrote:

On 10/20/21 12:31 PM, Andres Freund wrote:

ISTM that we should just make the "test invocation framework" responsible to
put the relevant directory onto PATH? I.e. Makefile.global should add the
build directory of the test to PATH, and vcregress.pl should put the relevant
$configuration/$project/ directory there?

Like this? That seems reasonable.

Yea, that's along the lines of what I was thinking...

OK, it turns out that it's better to put the addition at the end of the
PATH, otherise initdb complains that it can't find postgres in the same
directory.

This patch has been tested on Linux but not yet on Windows/MSVC,
although I have little doubt it will work there too.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

OK, it turns out that it's better to put the addition at the end of the
PATH, otherise initdb complains that it can't find postgres in the same
directory.

That seems horribly unsafe, or at least prone to pulling in the
program from some other versuion of Postgres. How about ordering
the path like

PATH="$(abs_top_builddir)/tmp_install$(bindir):$(CURDIR):$$PATH"

regards, tom lane

On 10/20/21 5:48 PM, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

OK, it turns out that it's better to put the addition at the end of the
PATH, otherise initdb complains that it can't find postgres in the same
directory.

That seems horribly unsafe, or at least prone to pulling in the
program from some other versuion of Postgres. How about ordering
the path like

PATH="$(abs_top_builddir)/tmp_install$(bindir):$(CURDIR):$$PATH"

fair point. See attached. Tested on MSVC and Linux

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com