TAP tests and symlinks on Windows

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
28 messages Options
12
Reply | Threaded
Open this post in threaded view
|

TAP tests and symlinks on Windows

Peter Eisentraut-6
The tests

src/bin/pg_basebackup/t/010_pg_basebackup.pl
src/bin/pg_rewind/t/004_pg_xlog_symlink.pl

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.

If I remove the skip stuff in pg_basebackup/t/010_pg_basebackup.pl, it
fails in various interesting ways.  Apparently, some more work is needed
to get the various paths handled correct on Windows.  (At least a
TestLib::perl2host call appears to be required.)  I don't have the
enthusiasm to fix this right now, but my patch at least updates the
comment from "this isn't supported" to "this doesn't work correctly yet".

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

0001-Enable-some-symlink-tests-on-Windows.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Peter Eisentraut-6
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


Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Juan José Santamaría Flecha

On Tue, Jun 9, 2020 at 9:28 AM Peter Eisentraut <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Dagfinn Ilmari Mannsåker
Juan José Santamaría Flecha <[hidden email]> writes:

> On Tue, Jun 9, 2020 at 9:28 AM Peter Eisentraut <
> [hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Juan José Santamaría Flecha

On Fri, Jun 12, 2020 at 9:00 AM Michael Paquier <[hidden email]> 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
 
Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Michael Paquier-2
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

win32-readlink-tap.patch (12K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Juan José Santamaría Flecha

On Mon, Jun 15, 2020 at 8:23 AM Michael Paquier <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Andrew Dunstan-8
In reply to this post by Michael Paquier-2

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



Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Peter Eisentraut-6
In reply to this post by Juan José Santamaría Flecha
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


Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Andrew Dunstan-8

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



Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Michael Paquier-2
In reply to this post by Andrew Dunstan-8
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Michael Paquier-2
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

win32-readlink-tap-v2.patch (5K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Peter Eisentraut-6
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


Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Michael Paquier-2
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]?  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]: https://www.postgresql.org/message-id/6c5ffed0-20ee-8878-270f-ab56b7023802@...
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Michael Paquier-2
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

win32-readlink-tap-v3.patch (6K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TAP tests and symlinks on Windows

Peter Eisentraut-6
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


12