TAP tests and symlinks on Windows

On Mon, Jun 08, 2020 at 02:44:31PM +0200, Peter Eisentraut wrote:

both contain a TAP skip notice "symlinks not supported on Windows".

This is untrue. Symlinks certainly work on Windows, and we have other TAP
tests using them, for example for tablespaces.

pg_rewind/t/004_pg_xlog_symlink.pl passes for me on Windows if I just remove
the skip stuff. My attached patch does that.

What's the version of your perl installation on Windows? With 5.22, I
am still seeing that symlink() is not implemented, causing the tests
of pg_rewind to blow in flight with your patch (MSVC 2015 here).
--
Michael

On 2020-06-09 09:19, Michael Paquier wrote:

On Mon, Jun 08, 2020 at 02:44:31PM +0200, Peter Eisentraut wrote:

both contain a TAP skip notice "symlinks not supported on Windows".

This is untrue. Symlinks certainly work on Windows, and we have other TAP
tests using them, for example for tablespaces.

pg_rewind/t/004_pg_xlog_symlink.pl passes for me on Windows if I just remove
the skip stuff. My attached patch does that.

What's the version of your perl installation on Windows? With 5.22, I
am still seeing that symlink() is not implemented, causing the tests
of pg_rewind to blow in flight with your patch (MSVC 2015 here).

I was using MSYS2 and the Perl version appears to have been 5.30.2.
Note sure which one of these two factors makes the difference.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jun 9, 2020 at 9:28 AM Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

On 2020-06-09 09:19, Michael Paquier wrote:

On Mon, Jun 08, 2020 at 02:44:31PM +0200, Peter Eisentraut wrote:

both contain a TAP skip notice "symlinks not supported on Windows".

This is untrue. Symlinks certainly work on Windows, and we have other

TAP

tests using them, for example for tablespaces.

pg_rewind/t/004_pg_xlog_symlink.pl passes for me on Windows if I just

remove

the skip stuff. My attached patch does that.

What's the version of your perl installation on Windows? With 5.22, I
am still seeing that symlink() is not implemented, causing the tests
of pg_rewind to blow in flight with your patch (MSVC 2015 here).

I was using MSYS2 and the Perl version appears to have been 5.30.2.
Note sure which one of these two factors makes the difference.

The difference seems to be MSYS2, it also fails for me if I do not
include 'Win32::Symlink' with Perl 5.30.2.

Regards,

Juan José Santamaría Flecha

Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> writes:

On Tue, Jun 9, 2020 at 9:28 AM Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

On 2020-06-09 09:19, Michael Paquier wrote:

On Mon, Jun 08, 2020 at 02:44:31PM +0200, Peter Eisentraut wrote:

both contain a TAP skip notice "symlinks not supported on Windows".

This is untrue. Symlinks certainly work on Windows, and we have other

TAP

tests using them, for example for tablespaces.

pg_rewind/t/004_pg_xlog_symlink.pl passes for me on Windows if I just

remove

the skip stuff. My attached patch does that.

What's the version of your perl installation on Windows? With 5.22, I
am still seeing that symlink() is not implemented, causing the tests
of pg_rewind to blow in flight with your patch (MSVC 2015 here).

I was using MSYS2 and the Perl version appears to have been 5.30.2.
Note sure which one of these two factors makes the difference.

The difference seems to be MSYS2, it also fails for me if I do not
include 'Win32::Symlink' with Perl 5.30.2.

Amusingly, Win32::Symlink uses a copy of our pgsymlink(), which emulates
symlinks via junction points:

https://metacpan.org/source/AUDREYT/Win32-Symlink-0.06/pgsymlink.c

A portable way of using symlinks if possible would be:

# In a BEGIN block because it overrides CORE::GLOBAL::symlink, which
# only takes effect on code that's compiled after the override is
# installed. We don't care if it fails, since it works without on
# some Windows perls.
BEGIN {
eval { require Win32::Symlink; Win32::Symlink->import; }
}

# symlink() throws an exception if t
if (not eval { symlink("",""); 1; })
{
plan skip_all => 'symlinks not supported';
}
else
{
plan tests => 5;
}

Plus a note in the Win32 docs that Win32::Symlink may be required to run
some tests on some Perl/Windows versions..

- ilmari
--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl

On Tue, Jun 09, 2020 at 11:26:19AM +0100, Dagfinn Ilmari Mannsåker wrote:

Amusingly, Win32::Symlink uses a copy of our pgsymlink(), which emulates
symlinks via junction points:

https://metacpan.org/source/AUDREYT/Win32-Symlink-0.06/pgsymlink.c

Oh, interesting point. Thanks for the reference!

A portable way of using symlinks if possible would be:

# In a BEGIN block because it overrides CORE::GLOBAL::symlink, which
# only takes effect on code that's compiled after the override is
# installed. We don't care if it fails, since it works without on
# some Windows perls.
[...]

Plus a note in the Win32 docs that Win32::Symlink may be required to run
some tests on some Perl/Windows versions..

Planting such a check in individual scripts is not a good idea because
it would get forgotten. The best way to handle that is to add a new
check in the BEGIN block of TestLib.pm. Note that we already do that
with createFile, OsFHandleOpen and CloseHandle. Now the question is:
do we really want to make this a hard requirement? I would like to
answer yes so as we make sure that this gets always tested, and this
needs proper documentation as you say. Now it would be also possible
to check if the API is present in the BEGIN block of TestLib.pm, and
then use an independent variable similar to what we do with
$use_unix_sockets to decide if tests should be skipped or not, but you
cannot know if this gets actually, or ever, tested.
--
Michael

On Fri, Jun 12, 2020 at 9:00 AM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Jun 09, 2020 at 11:26:19AM +0100, Dagfinn Ilmari Mannsåker wrote:

Plus a note in the Win32 docs that Win32::Symlink may be required to run
some tests on some Perl/Windows versions..

Planting such a check in individual scripts is not a good idea because
it would get forgotten. The best way to handle that is to add a new
check in the BEGIN block of TestLib.pm. Note that we already do that
with createFile, OsFHandleOpen and CloseHandle. Now the question is:
do we really want to make this a hard requirement? I would like to
answer yes so as we make sure that this gets always tested, and this
needs proper documentation as you say. Now it would be also possible
to check if the API is present in the BEGIN block of TestLib.pm, and
then use an independent variable similar to what we do with
$use_unix_sockets to decide if tests should be skipped or not, but you
cannot know if this gets actually, or ever, tested.

The first thing that comes to mind is adding an option to vcregress to
choose whether symlinks will be tested or skipped, would that be an
acceptable solution?

Regards,

Juan José Santamaría Flecha

On Fri, Jun 12, 2020 at 02:02:52PM +0200, Juan José Santamaría Flecha wrote:

The first thing that comes to mind is adding an option to vcregress to
choose whether symlinks will be tested or skipped, would that be an
acceptable solution?

My take would be to actually enforce that as a requirement for 14~ if
that works reliably, and of course not backpatch that change as that's
clearly an improvement and not a bug fix. It would be good to check
the status of each buildfarm member first though. And I would need to
also check my own stuff to begin with..
--
Michael

On Sat, Jun 13, 2020 at 03:00:54PM +0900, Michael Paquier wrote:

My take would be to actually enforce that as a requirement for 14~ if
that works reliably, and of course not backpatch that change as that's
clearly an improvement and not a bug fix. It would be good to check
the status of each buildfarm member first though. And I would need to
also check my own stuff to begin with..

So, I have been looking at that. And indeed as Peter said we are
visibly missing one call to perl2host in 010_pg_basebackup.pl.

Another thing I spotted is that Win32::Symlink does not allow to
detect properly if a path is a symlink using -l, causing one of the
tests of pg_basebackup to fail when checking if a tablespace path has
been updted. It would be good to get more people to test this patch
with different environments than mine. I am also adding Andrew
Dunstan in CC as the owner of the buildfarm animals running currently
TAP tests for confirmation about the presence of Win32::Symlink
there as I am afraid it would cause failures: drongo, fairywen,
jacana and bowerbird.
--
Michael

On Mon, Jun 15, 2020 at 8:23 AM Michael Paquier <michael@paquier.xyz> wrote:

Another thing I spotted is that Win32::Symlink does not allow to
detect properly if a path is a symlink using -l, causing one of the
tests of pg_basebackup to fail when checking if a tablespace path has
been updted. It would be good to get more people to test this patch
with different environments than mine. I am also adding Andrew
Dunstan in CC as the owner of the buildfarm animals running currently
TAP tests for confirmation about the presence of Win32::Symlink
there as I am afraid it would cause failures: drongo, fairywen,
jacana and bowerbird.

This patch works on my Windows 10 / Visual Studio 2019 / Perl 5.30.2
machine.

Regards,

Juan José Santamaría Flecha

On 6/15/20 2:23 AM, Michael Paquier wrote:

On Sat, Jun 13, 2020 at 03:00:54PM +0900, Michael Paquier wrote:

My take would be to actually enforce that as a requirement for 14~ if
that works reliably, and of course not backpatch that change as that's
clearly an improvement and not a bug fix. It would be good to check
the status of each buildfarm member first though. And I would need to
also check my own stuff to begin with..

So, I have been looking at that. And indeed as Peter said we are
visibly missing one call to perl2host in 010_pg_basebackup.pl.

Another thing I spotted is that Win32::Symlink does not allow to
detect properly if a path is a symlink using -l, causing one of the
tests of pg_basebackup to fail when checking if a tablespace path has
been updted. It would be good to get more people to test this patch
with different environments than mine. I am also adding Andrew
Dunstan in CC as the owner of the buildfarm animals running currently
TAP tests for confirmation about the presence of Win32::Symlink
there as I am afraid it would cause failures: drongo, fairywen,
jacana and bowerbird.

Not one of them has it.

I think we'll need a dynamic test for its presence rather than just
assuming it's there. (Use require in an eval for this).

However, since all of them would currently fail we wouldn't actually
have any test coverage. I could see about installing it on one or two
animals (jacana would be a problem, it's using a very old and limited
perl to run TAP tests.)

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-06-09 09:33, Juan José Santamaría Flecha wrote:

The difference seems to be MSYS2, it also fails for me if I do not
include 'Win32::Symlink' with Perl 5.30.2.

MSYS2, which is basically Cygwin, emulates symlinks with junction
points, so this happens to work for our purpose. We could therefore
enable these tests in that environment, if we could come up with a
reliable way to detect it.

Also, if we are going to add Win32::Symlink to the mix, we should make
sure things continue to work correctly under MSYS2.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 6/16/20 8:24 AM, Peter Eisentraut wrote:

On 2020-06-09 09:33, Juan José Santamaría Flecha wrote:

The difference seems to be MSYS2, it also fails for me if I do not
include 'Win32::Symlink' with Perl 5.30.2.

MSYS2, which is basically Cygwin, emulates symlinks with junction
points, so this happens to work for our purpose.  We could therefore
enable these tests in that environment, if we could come up with a
reliable way to detect it.

From src/bin/pg_dump/t/010_dump_connstr.pl:

if ($^O eq 'msys' && `uname -or` =~ /^[2-9].*Msys/)

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jun 16, 2020 at 08:32:03AM -0400, Andrew Dunstan wrote:

On 6/16/20 8:24 AM, Peter Eisentraut wrote:

MSYS2, which is basically Cygwin, emulates symlinks with junction
points, so this happens to work for our purpose.  We could therefore
enable these tests in that environment, if we could come up with a
reliable way to detect it.

Hmm. In this case does perl's -l think that a junction point is
corrently a soft link or not? We have a check based on that in
pg_basebackup's test and -l fails when it sees to a junction point,
forcing us to skip this test.

From src/bin/pg_dump/t/010_dump_connstr.pl:

if ($^O eq 'msys' && `uname -or` =~ /^[2-9].*Msys/)

Smart. This could become a central variable in TestLib.pm.
--
Michael

On Tue, Jun 16, 2020 at 07:53:26AM -0400, Andrew Dunstan wrote:

Not one of them has it.

Argh.

I think we'll need a dynamic test for its presence rather than just
assuming it's there. (Use require in an eval for this).

Sure. No problem with implementing an automatic detection.

However, since all of them would currently fail we wouldn't actually
have any test coverage. I could see about installing it on one or two
animals (jacana would be a problem, it's using a very old and limited
perl to run TAP tests.)

Okay. This could be a problem as jacana is proving to have good
coverage AFAIK. So it looks like we are really heading in the
direction is still skipping the test if there is no support for
symlink in the environment. At least that makes less diffs in the
patch.
--
Michael

On Wed, Jun 17, 2020 at 04:44:34PM +0900, Michael Paquier wrote:

Okay. This could be a problem as jacana is proving to have good
coverage AFAIK. So it looks like we are really heading in the
direction is still skipping the test if there is no support for
symlink in the environment. At least that makes less diffs in the
patch.

I have implemented a patch based on the feedback received that does
the following, tested with all three patterns (MSVC only on Windows):
- Assume that all non-Windows platform have a proper symlink
implementation for perl.
- If on Windows, check for the presence of Win32::Symlink:
-- If the module is not detected, skip the tests not supported.
-- If the module is detected, run them.

I have added this patch to the next commit fest:
https://commitfest.postgresql.org/28/2612/

Thanks,
--
Michael

On 2020-06-23 12:55, Michael Paquier wrote:

I have implemented a patch based on the feedback received that does
the following, tested with all three patterns (MSVC only on Windows):
- Assume that all non-Windows platform have a proper symlink
implementation for perl.
- If on Windows, check for the presence of Win32::Symlink:
-- If the module is not detected, skip the tests not supported.
-- If the module is detected, run them.

We should be more accurate about things like this:

+# The following tests test symlinks. Windows may not have symlinks, so
+# skip there.

The issue isn't whether Windows has symlinks, since all versions of
Windows supported by PostgreSQL do (AFAIK). The issue is only whether
the Perl installation that runs the tests has symlink support. And that
is only necessary if the test itself wants to create or inspect
symlinks. For example, there are existing tests involving tablespaces
that work just fine on Windows.

Relatedly, your patch ends up skipping the tests on MSYS2, even though
Perl supports symlinks there out of the box.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Jun 26, 2020 at 02:00:37PM +0200, Peter Eisentraut wrote:

We should be more accurate about things like this:

+# The following tests test symlinks. Windows may not have symlinks, so
+# skip there.

The issue isn't whether Windows has symlinks, since all versions of Windows
supported by PostgreSQL do (AFAIK). The issue is only whether the Perl
installation that runs the tests has symlink support. And that is only
necessary if the test itself wants to create or inspect symlinks. For
example, there are existing tests involving tablespaces that work just fine
on Windows.

Check. Indeed that sounds confusing.

Relatedly, your patch ends up skipping the tests on MSYS2, even though Perl
supports symlinks there out of the box.

Do you think that it would be enough to use what Andrew has mentioned
in [1]/messages/by-id/6c5ffed0-20ee-8878-270f-ab56b7023802@2ndQuadrant.com -- Michael? I don't have a MSYS2 installation, so I am unfortunately not
able to confirm that, but I would just move the check to TestLib.pm
and save it in an extra variable.

[1]: /messages/by-id/6c5ffed0-20ee-8878-270f-ab56b7023802@2ndQuadrant.com -- Michael
--
Michael

On Mon, Jun 29, 2020 at 04:56:16PM +0900, Michael Paquier wrote:

On Fri, Jun 26, 2020 at 02:00:37PM +0200, Peter Eisentraut wrote:

We should be more accurate about things like this:

+# The following tests test symlinks. Windows may not have symlinks, so
+# skip there.

The issue isn't whether Windows has symlinks, since all versions of Windows
supported by PostgreSQL do (AFAIK). The issue is only whether the Perl
installation that runs the tests has symlink support. And that is only
necessary if the test itself wants to create or inspect symlinks. For
example, there are existing tests involving tablespaces that work just fine
on Windows.

Check. Indeed that sounds confusing.

Attached is an updated patch, where I have tried to use a better
wording in all the code paths involved.

Relatedly, your patch ends up skipping the tests on MSYS2, even though Perl
supports symlinks there out of the box.

Do you think that it would be enough to use what Andrew has mentioned
in [1]? I don't have a MSYS2 installation, so I am unfortunately not
able to confirm that, but I would just move the check to TestLib.pm
and save it in an extra variable.

Added an extra $is_msys2 to track that in TestLib.pm.  One thing I am
not sure of though: Win32::Symlink fails to work properly with -l, but
is that the case with MSYS2?  If that's able to work, it would be
possible to not skip the following test but I have taken the most
careful approach for now:
+   # This symlink check is not supported on Windows.  Win32::Symlink works
+   # around this situation by using junction points (actually PostgreSQL
+   # approach on the problem), and -l is not able to detect that situation.
+  SKIP:
+   {
+       skip "symlink check not implemented on Windows", 1
+         if ($windows_os)

Thanks,
--
Michael

On 2020-06-30 14:13, Michael Paquier wrote:

Attached is an updated patch, where I have tried to use a better
wording in all the code paths involved.

This new patch doesn't work for me on MSYS2 yet.

It fails right now in 010_pg_basebackup.pl at

my $realTsDir = TestLib::perl2host("$shorter_tempdir/tblspc1");

with chdir: No such file or directory. Because perl2host requires the
parent directory of the argument to exist, but here it doesn't.

If I add

mkdir $shorter_tempdir;

above it, then it proceeds past that point, but then the CREATE
TABLESPACE command fails with No such file or directory. I think the call

symlink "$tempdir", $shorter_tempdir;

creates a directory inside $shorter_tempdir, since it now exists, per my
above change, rather than in place of $shorter_tempdir.

I think all of this is still a bit too fragile it needs further
consideration.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services