Обсуждение: TAP tests and symlinks on Windows

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

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

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 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 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]?  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@2ndQuadrant.com
--
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

On 7/3/20 10:11 AM, Peter Eisentraut wrote:
> 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.



I'll see what I can come up with.


cheers


andrew


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

On Sun, Jul 05, 2020 at 08:18:46AM -0400, Andrew Dunstan wrote:
> On 7/3/20 10:11 AM, Peter Eisentraut wrote:
>> I think all of this is still a bit too fragile it needs further
>> consideration.

Indeed.  I would need a MSYS2 environment to dig into that.  This
looks trickier than what I am used to on Windows.

> I'll see what I can come up with.

Thanks, Andrew.
--
Michael

On 7/3/20 10:11 AM, Peter Eisentraut wrote:
> 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.



Yeah. I have a fix for that, which also checks to see if the grandparent
directory exists:


-               chdir $parent or die "could not chdir \"$parent\": $!";
+               if (! chdir $parent)
+               {
+                       $leaf = '/' . basename ($parent) . $leaf;
+                       $parent = dirname $parent;
+                       chdir $parent or die "could not chdir
\"$parent\": $!";
+               }


We could generalize it to walk all the way up the path, but I think this
is sufficient.


Incidentally, perl2host is arguably a bad name for this routine - there
is nothing perl-specific about the paths, they are provided by the msys
environment. Maybe virtual2host or some such would be a better name, or
even just host_path or native_path.


>
> 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.



The symlink built into msys2 perl is distinctly fragile. I was only able
to get it to work by doing this:


+       mkdir "$tempdir/tblspc1";
+       mkdir "$tempdir/tbl=spc2";
+       mkdir "$tempdir/$superlongname";
+       open (my $x, '>', "$tempdir/tblspc1/stuff") || die $!; print $x
"hi\n"; close($x);
+       open ($x, '>', "$tempdir/tbl=spc2/stuff") || die $!; print $x
"hi\n"; close($x);
+       open ($x, '>', "$tempdir/$superlongname/stuff") || die $!; print
$x "hi\n"; close($x);
        symlink "$tempdir", $shorter_tempdir;
+       unlink "$tempdir/tblspc1/stuff";
+       unlink "$tempdir/tbl=spc2/stuff";
+       unlink "$tempdir/$superlongname/stuff";


which is sufficiently ugly that I don't think we should contemplate it.


Luckily there is an alternative, which doesn't require the use of
Win32::Symlink. Windows can be made to create junctions that function
exactly as expected quite easily - it's a builtin of the cmd.exe
processor, and it's been supported in at least every release since
Windows Vista. Here's a perl function that calls it:


sub dsymlink
{
        my $oldname = shift;
        my $newname = shift;
        if ($windows_os)
        {
                $oldname = TestLib::perl2host($oldname);
                $newname = TestLib::perl2host($newname);
                $oldname =~ s,/,\\,g;
                $newname =~ s,/,\\,g;
                my $cmd = "cmd //c 'mklink /j $newname $oldname'";
                system($cmd);
        }
        else
        {
                symlink $oldname, $newname;
        }
        die "No $newname" unless -e $newname;
}


It might need a little more quoting to be more robust.


Give that, we can simply replace


    symlink "$tempdir", $shorter_tempdir;


with


    dsymlink "$tempdir", $shorter_tempdir;


Then, with a little more sprinkling of perl2host the pg_basebackup tests
can be made to work on msys2.


I'm going to prepare patches along these lines.


cheers


andrew

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

On 7/8/20 11:07 AM, Juan José Santamaría Flecha wrote:
>
> On Wed, Jul 8, 2020 at 3:54 PM Andrew Dunstan
> <andrew.dunstan@2ndquadrant.com
> <mailto:andrew.dunstan@2ndquadrant.com>> wrote:
>
>
>     Incidentally, perl2host is arguably a bad name for this routine -
>     there
>     is nothing perl-specific about the paths, they are provided by the
>     msys
>     environment. Maybe virtual2host or some such would be a better
>     name, or
>     even just host_path or native_path.
>
>
> There is a utility cygpath [1] meant for the conversion between Unix
> and Windows path formats, that might be a meaningful name also.
>
> [1] http://cygwin.net/cygwin-ug-net/cygpath.html
>
>


Oh, good find. But unfortunately it's not present in my msys1 installations.


So we'll still need to use the 'pwd -W` trick for those.


cheers


andrew


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

On 2020-Jul-08, Andrew Dunstan wrote:

> On 7/8/20 11:07 AM, Juan José Santamaría Flecha wrote:

> > There is a utility cygpath [1] meant for the conversion between Unix
> > and Windows path formats, that might be a meaningful name also.
> >
> > [1] http://cygwin.net/cygwin-ug-net/cygpath.html
> 
> Oh, good find. But unfortunately it's not present in my msys1 installations.
> 
> So we'll still need to use the 'pwd -W` trick for those.

I think his point is not to use that utility, just to use its name as
the name of the perl routine.

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

On 7/8/20 12:22 PM, Alvaro Herrera wrote:
> On 2020-Jul-08, Andrew Dunstan wrote:
>
>> On 7/8/20 11:07 AM, Juan José Santamaría Flecha wrote:
>>> There is a utility cygpath [1] meant for the conversion between Unix
>>> and Windows path formats, that might be a meaningful name also.
>>>
>>> [1] http://cygwin.net/cygwin-ug-net/cygpath.html
>> Oh, good find. But unfortunately it's not present in my msys1 installations.
>>
>> So we'll still need to use the 'pwd -W` trick for those.
> I think his point is not to use that utility, just to use its name as
> the name of the perl routine.
>


That would be wholly misleading, since it's not needed at all when we're
running running under cygwin.


cheers


andrew

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

On 7/8/20 9:54 AM, Andrew Dunstan wrote:
>
>
>
> Then, with a little more sprinkling of perl2host the pg_basebackup tests
> can be made to work on msys2.
>
>
> I'm going to prepare patches along these lines.
>
>



After much frustration and gnashing of teeth here's a patch that allows
almost all the TAP tests involving symlinks to work as expected on all
Windows build environments, without requiring an additional Perl module.
I have tested this on a system that is very similar to that running
drongo and fairywren, with both msys2 and  MSVC builds.


I didn't change the name of perl2host - Sufficient unto the day is the
evil thereof. But I did modify it  a) to allow use of cygpath if
available and b) to allow it to succeed if the grandparent directory
exists when cygpath isn't available.


cheers


andrew



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

On Fri, Jul 10, 2020 at 07:58:02AM -0400, Andrew Dunstan wrote:
> After much frustration and gnashing of teeth here's a patch that allows
> almost all the TAP tests involving symlinks to work as expected on all
> Windows build environments, without requiring an additional Perl module.
> I have tested this on a system that is very similar to that running
> drongo and fairywren, with both msys2 and MSVC builds.

Thanks Andrew for looking at the part with MSYS.  The tests pass for
me with MSVC.  The trick with mklink is cool.  I have not considered
that, and the test code gets simpler.

+       my $cmd = qq{mklink /j "$newname" "$oldname"};
+       if ($Config{osname} eq 'msys')
+       {
+           # need some indirection on msys
+           $cmd = qq{echo '$cmd' | \$COMSPEC /Q};
+       }
+       note("dir_symlink cmd: $cmd");
+       system($cmd);
From the quoting perspective, wouldn't it be simpler to build an array
with all those arguments and call system() with @cmd?

+# Create files that look like temporary relations to ensure they are ignored
+# in a tablespace.
+my @tempRelationFiles = qw(t888_888 t888888_888888_vm.1);
This variable conflicts with a previous declaration, creating a
warning.

+   skip "symlink check not implemented on Windows", 1
+     if ($windows_os);
    opendir(my $dh, "$pgdata/pg_tblspc") or die;
I think that this would be cleaner with a SKIP block.

+Portably create a symlink for a director. On Windows this creates a junction.
+Elsewhere it just calls perl's builtin symlink.
s/director/directory/
s/junction/junction point/

   <para>
     The TAP tests require the Perl module <literal>IPC::Run</literal>.
     This module is available from CPAN or an operating system package.
+    On Windows, <literal>Win32API::File</literal> is also required .
   </para>
This part should be backpatched IMO.

Some of the indentation is weird, this needs a cleanup with perltidy.
--
Michael

On 2020-07-10 13:58, Andrew Dunstan wrote:
> After much frustration and gnashing of teeth here's a patch that allows
> almost all the TAP tests involving symlinks to work as expected on all
> Windows build environments, without requiring an additional Perl module.
> I have tested this on a system that is very similar to that running
> drongo and fairywren, with both msys2 and  MSVC builds.

Thanks.  This patch works for me in my environment.  The code changes 
look very clean, so it seems like a good improvement.

Attached is a small fixup patch for some typos and a stray debug message.

A perltidy run might be worthwhile, as Michael already mentioned.

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

On 7/14/20 1:31 AM, Michael Paquier wrote:
> On Fri, Jul 10, 2020 at 07:58:02AM -0400, Andrew Dunstan wrote:
>> After much frustration and gnashing of teeth here's a patch that allows
>> almost all the TAP tests involving symlinks to work as expected on all
>> Windows build environments, without requiring an additional Perl module.
>> I have tested this on a system that is very similar to that running
>> drongo and fairywren, with both msys2 and MSVC builds.
> Thanks Andrew for looking at the part with MSYS.  The tests pass for
> me with MSVC.  The trick with mklink is cool.  I have not considered
> that, and the test code gets simpler.
>
> +       my $cmd = qq{mklink /j "$newname" "$oldname"};
> +       if ($Config{osname} eq 'msys')
> +       {
> +           # need some indirection on msys
> +           $cmd = qq{echo '$cmd' | \$COMSPEC /Q};
> +       }
> +       note("dir_symlink cmd: $cmd");
> +       system($cmd);
> From the quoting perspective, wouldn't it be simpler to build an array
> with all those arguments and call system() with @cmd?



This is the simplest invocation I found to be reliable on msys2 (and it
took me a long time to find). If you have a tested alternative please
let me know.


> +# Create files that look like temporary relations to ensure they are ignored
> +# in a tablespace.
> +my @tempRelationFiles = qw(t888_888 t888888_888888_vm.1);
> This variable conflicts with a previous declaration, creating a
> warning.
>
> +   skip "symlink check not implemented on Windows", 1
> +     if ($windows_os);
>     opendir(my $dh, "$pgdata/pg_tblspc") or die;
> I think that this would be cleaner with a SKIP block.



I don't understand this comment. The skip statement here is in a SKIP
block. In fact skip only works inside SKIP blocks. (perldoc Test::More
for details). Maybe you got confused by the diff format.


>
> +Portably create a symlink for a director. On Windows this creates a junction.
> +Elsewhere it just calls perl's builtin symlink.
> s/director/directory/
> s/junction/junction point/



fixed.


>
>    <para>
>      The TAP tests require the Perl module <literal>IPC::Run</literal>.
>      This module is available from CPAN or an operating system package.
> +    On Windows, <literal>Win32API::File</literal> is also required .
>    </para>
> This part should be backpatched IMO.



I will do this in  a separate backpatched commit.



>
> Some of the indentation is weird, this needs a cleanup with perltidy.


Done.


Revised patch attached.


cheers


andrew


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

On Wed, Jul 15, 2020 at 11:04:28AM -0400, Andrew Dunstan wrote:
> This is the simplest invocation I found to be reliable on msys2 (and it
> took me a long time to find). If you have a tested alternative please
> let me know.

Having a working MSYS environment is still on my TODO list :)

> I don't understand this comment. The skip statement here is in a SKIP
> block. In fact skip only works inside SKIP blocks. (perldoc Test::More
> for details). Maybe you got confused by the diff format.

Indeed, I got trapped by the diff here.  Thanks.

The patch looks good to me.
--
Michael

On Thu, Jul 16, 2020 at 07:21:27PM +0900, Michael Paquier wrote:
> The patch looks good to me.

For the sake of the archives, this has been applied as d66b23b0 and
the buildfarm is green.  I have also changed the related CF entry to
reflect what has been done, with Andrew as author, etc:
https://commitfest.postgresql.org/28/2612/
--
Michael