pgsql: Remove command checks in tests of pg_basebackup and pg_receivewa

On 2022-02-14 Mo 23:41, Michael Paquier wrote:

Remove command checks in tests of pg_basebackup and pg_receivewal

The TAP tests of those commands have been checking if commands of "gzip"
and "lz4" existed by launching them with an extra --version. Based on
the buildfarm, this is not required for "gzip" as the command always
exists. Since 1d084fb, "lz4" has a ./configure check doing the same
thing.

Reported-by: Andres Freund
Discussion: /messages/by-id/20220212220643.ozuvq2k4cjkcnr2v@alap3.anarazel.de
Discussion: /messages/by-id/Ygm2ADakjlqGc2Ro@paquier.xyz

This appears to have been misconceived, at least in the case of MSVC
builds, as I have just discovered. It is entirely possible to have the
lz4 libraries installed and build with them but not have the .exe, and
unlike the configure case the MSVC build system doesn't conduct any test
for it, resulting in a nasty looking TAP test failure. I assume similar
failures are possible with zstd and maybe gzip.

cheers

andrew

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

On Sat, Apr 30, 2022 at 04:52:54PM -0400, Andrew Dunstan wrote:

This appears to have been misconceived, at least in the case of MSVC
builds, as I have just discovered. It is entirely possible to have the
lz4 libraries installed and build with them but not have the .exe, and
unlike the configure case the MSVC build system doesn't conduct any test
for it, resulting in a nasty looking TAP test failure. I assume similar
failures are possible with zstd and maybe gzip.

Okay, we could change the code of vcregress.pl so as the different ENV
commands (tar, zstd, lz4 or gzip) are assigned as follows for the sake
of the tests:
- If a ENV value is available, trust the environment/user and rely on
it.
- If a ENV value is not available, try to look for it in the
environment by launching a simple $command --version (-version should
be fine across all the commands currently in need of coverage?).
-- On failure, set ENV{command} to an empty string.
-- On success, set ENV{command} = "$command"

Does something like this look better to you?
--
Michael

On 2022-05-01 Su 09:15, Michael Paquier wrote:

On Sat, Apr 30, 2022 at 04:52:54PM -0400, Andrew Dunstan wrote:

This appears to have been misconceived, at least in the case of MSVC
builds, as I have just discovered. It is entirely possible to have the
lz4 libraries installed and build with them but not have the .exe, and
unlike the configure case the MSVC build system doesn't conduct any test
for it, resulting in a nasty looking TAP test failure. I assume similar
failures are possible with zstd and maybe gzip.

Okay, we could change the code of vcregress.pl so as the different ENV
commands (tar, zstd, lz4 or gzip) are assigned as follows for the sake
of the tests:
- If a ENV value is available, trust the environment/user and rely on
it.
- If a ENV value is not available, try to look for it in the
environment by launching a simple $command --version (-version should
be fine across all the commands currently in need of coverage?).
-- On failure, set ENV{command} to an empty string.
-- On success, set ENV{command} = "$command"

Does something like this look better to you?

IIRC we know that tar will be available on Windows.

I don't think we should do that check for every time we call
vc_regress.pl, that seems wasteful. Maybe do it if the command is one
that might require these commands, which I think would be bincheck or
taptest. And/Or stash some status somewhere?

cheers

andrew

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

On Sun, May 01, 2022 at 10:18:37AM -0400, Andrew Dunstan wrote:

IIRC we know that tar will be available on Windows.

What about gzip? Are the binaries and the libraries split into
different packages for this package manager on Windows? We've never
assumed that this is possible on ./configure, but it would make sense
to make MSVC able to manage this case.

I don't think we should do that check for every time we call
vc_regress.pl, that seems wasteful. Maybe do it if the command is one
that might require these commands, which I think would be bincheck or
taptest. And/Or stash some status somewhere?

Mostly. We also care about gzip in contrib/basebackup_to_shell/,
though contribcheck does not run the TAP tests of contrib/ modules.
As you say, setting up those variables in bincheck and taptest would
be enough.
--
Michael

On 2022-05-02 Mo 03:06, Michael Paquier wrote:

On Sun, May 01, 2022 at 10:18:37AM -0400, Andrew Dunstan wrote:

IIRC we know that tar will be available on Windows.

What about gzip? Are the binaries and the libraries split into
different packages for this package manager on Windows? We've never
assumed that this is possible on ./configure, but it would make sense
to make MSVC able to manage this case.

vcpkg doesn't install any.exe files AFAICS, just DLLs, headers, .lib
files etc.

I don't think we should do that check for every time we call
vc_regress.pl, that seems wasteful. Maybe do it if the command is one
that might require these commands, which I think would be bincheck or
taptest. And/Or stash some status somewhere?

Mostly. We also care about gzip in contrib/basebackup_to_shell/,
though contribcheck does not run the TAP tests of contrib/ modules.
As you say, setting up those variables in bincheck and taptest would
be enough.

That would be a start. Maybe meson will us get rid of some of this mess ...

cheers

andrew

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

On Fri, May 06, 2022 at 10:12:36AM -0400, Andrew Dunstan wrote:

On 2022-05-02 Mo 03:06, Michael Paquier wrote:
vcpkg doesn't install any.exe files AFAICS, just DLLs, headers, .lib
files etc.

Okay, thanks for confirming.

I don't think we should do that check for every time we call
vc_regress.pl, that seems wasteful. Maybe do it if the command is one
that might require these commands, which I think would be bincheck or
taptest. And/Or stash some status somewhere?

Mostly. We also care about gzip in contrib/basebackup_to_shell/,
though contribcheck does not run the TAP tests of contrib/ modules.
As you say, setting up those variables in bincheck and taptest would
be enough.

That would be a start. Maybe meson will us get rid of some of this mess ...

Let's do that for now then as we need a middle ground for HEAD. I'll
come up with something at the beginning of next week.
--
Michael

Hi Andrew,

On Sat, May 07, 2022 at 09:13:48AM +0900, Michael Paquier wrote:

Let's do that for now then as we need a middle ground for HEAD. I'll
come up with something at the beginning of next week.

And here you go as of the attached to show the idea. The CI is able
to execute and detect the default commands for lz4, gzip and zstd,
while one of my boxes without any of those commands would ignore them.
What do you think?
--
Michael

On 2022-05-09 Mo 01:28, Michael Paquier wrote:

Hi Andrew,

On Sat, May 07, 2022 at 09:13:48AM +0900, Michael Paquier wrote:

Let's do that for now then as we need a middle ground for HEAD. I'll
come up with something at the beginning of next week.

And here you go as of the attached to show the idea. The CI is able
to execute and detect the default commands for lz4, gzip and zstd,
while one of my boxes without any of those commands would ignore them.
What do you think?

The system() call should include redirection, since we only case about
the return code.

I would just write this

+    if ($rc != 0)
+    {
+        # Execution of the default command failed, so reset its
+        # environment value.
+        $ENV{$envname} = '';
+        return;
+    }
+
+    # Set the environment to the default as it exists.
+    $ENV{$envname} = $envdefault;
+    return;

as

    $ENV{$envname} = $envdefault if $rc == 0;

There should be no need to set it to the empty string in case if
failure, we already know it's undefined.

Otherwise looks good.

cheers

andrew

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

On Mon, May 09, 2022 at 10:10:27AM -0400, Andrew Dunstan wrote:

The system() call should include redirection, since we only case about
the return code.

Okay, done using File::Spec::devnull.

I would just write this

as

    $ENV{$envname} = $envdefault if $rc == 0;

There should be no need to set it to the empty string in case if
failure, we already know it's undefined.

This needs to be adjusted on all the stable branches, so I'll do that
once this week's version is tagged. Attached is an updated patch for
now.
--
Michael

On Tue, May 10, 2022 at 09:57:29AM +0900, Michael Paquier wrote:

This needs to be adjusted on all the stable branches, so I'll do that
once this week's version is tagged. Attached is an updated patch for
now.

The tags have been pushed, so done now.
--
Michael