MSYS / cygwin symlinks #246

hakonhagland · 2021-01-22T14:21:18Z

This fixes issue #213 on my machine.

hakonhagland · 2021-01-22T20:26:59Z

Also note that MSYS2 can emulate symbolic links in at least three ways (see this blog for more information). For example:

$ touch a.txt
# Create a symbolic link:
$ MSYS=winsymlinks:nativestrict ln -s a.txt b.txt
# Create a .lnk shortcut file:
$ MSYS=winsymlinks ln -s a.txt c.txt
# Emulate symbolic links by copying the file:
$ MSYS= ln -s a.txt d.txt
$ ls -l
total 4.0K
-rw-r--r-- 1 hakon hakon 0 Jan 22 20:54 a.txt
lrwxrwxrwx 1 hakon hakon 5 Jan 22 20:54 b.txt -> a.txt
lrwxrwxrwx 1 hakon hakon 5 Jan 22 20:55 c.txt -> a.txt
-rw-r--r-- 1 hakon hakon 0 Jan 22 20:54 d.txt

Maybe we also need to check the content of the MSYS environment variable to distinguish between the cases?

hakonhagland · 2021-01-23T21:18:25Z

MSYS=winsymlinks:nativestrict ln -s a.txt b.txt

I added a new commit that I hope is more accurate, in that it tests for this case specifically on Cygwin and MSYS2

compilet-Y7gS9.c

plicease · 2021-04-03T10:45:34Z

lib/Alien/Build/Util.pm

+            if ($nativesymlink) {
+              # NOTE: On linux, it is OK to create broken symlinks, but it is not allowed on 
+              #   windows MSYS2/Cygwin when nativestrict is used.
+              die "cannot create native symlink to nonexistent file $target on $^O";


I'm not sure why this is necessary? Trying to create the broken symlink already fails.

Trying to create the broken symlink already fails.

I think there were two things I was trying to do here. First, if the target does exist relative to the source we still need to temporarily create an empty file relative to the destination, see line 91 below, since the file might not be copied by File::Find::find() yet. Second I was trying to provide some more context for the error message, and maybe document in the code that we actually have thought about the issue, hopefully making the code more maintainable.

To solve the issue with the destination, I create a temporarily empty file relative to the destination, but in doing this the symlink call on line 97 will no longer fail. I think that is one reason why I included the check here..

plicease · 2021-04-03T10:46:16Z

lib/Alien/Build/Util.pm

+            $dst->parent->child($target)->touchpath;
+          }
+        }
+        my $curdir = Path::Tiny->cwd;


For AB I usually use File::chdir to avoid clutter of tracking temporary directory changes.

Good idea, I will update the code to use File::chdir instead.

Suggested change

my $curdir = Path::Tiny->cwd;

line can be removed now.

lib/Alien/Build/Util.pm

plicease · 2021-04-03T11:10:41Z

lib/Alien/Build/Util.pm

+        }
+        my $curdir = Path::Tiny->cwd;
+        # CD into the directory, such that symlink will work on MSYS2
+        chdir $dst->parent or die "could not chdir to $src->parent : $!";


I would like more illumination on why we are changing into the destination directory?

For a relative target, the symlink call will fail if the target does not exist. The existence check is done relative to the current directory (tested this now).

plicease · 2021-04-03T11:16:37Z

This patch seems invasive for a $^O eq 'msys' which is an unofficial fork of Perl which as far as I can tell is only really useful for running autoconf.

plicease · 2021-04-03T11:44:43Z

lib/Alien/Build/Util.pm

+            }
+          }
+          if ($nativesymlink) {
+            $dst->parent->child($target)->touchpath;


I guess this is assuming that the target will later be updated as part of the copy? There is a corner case here though when the link really should be broken at the destination because of relative links, will leave an empty file instead.

will leave an empty file instead.

That should not happen since we do the check of existence relative to the source at line 83. So if the target does not exist relative to the source, the code dies at line 87.

Be more specific/accurate when testing the cases that cannot create broken symlinks.

plicease · 2021-06-30T22:17:31Z

lib/Alien/Build/Util.pm

@@ -65,7 +80,37 @@ sub _mirror
        { unlink "$dst" }
        my $target = readlink "$src";
        Alien::Build->log("ln -s $target $dst") if $opt->{verbose};
-        symlink($target, $dst) || die "unable to symlink $target => $dst";


I would like to keep this simplified behavior when $^O is not msys or cygwin.

plicease · 2021-06-30T22:21:19Z

t/alien_build_util.t

+    my $newdir = $tmp1->child("lib");
+    my $savedir = Path::Tiny->cwd;
+    # CD into the the $newdir such that symlink will work on MSYS2
+    chdir $newdir->stringify or die "unable to chdir to $newdir: $!";


Suggested change

my $newdir = $tmp1->child("lib");

my $savedir = Path::Tiny->cwd;

# CD into the the $newdir such that symlink will work on MSYS2

chdir $newdir->stringify or die "unable to chdir to $newdir: $!";

local $CWD = $tmp1->child("lib")->stringify;

plicease · 2021-06-30T22:21:40Z

t/alien_build_util.t

    }
+    chdir $savedir or die "unable to chdir to $savedir: $!";


Suggested change

chdir $savedir or die "unable to chdir to $savedir: $!";

plicease · 2021-06-30T22:23:47Z

I think this is close. Aside from a couple of very minor nits mentioned above, I would like to keep the original simple one-statement call to symlink on UNIX and only delve into the msys/cygwin logic if we are on that platform.

hakonhagland · 2021-06-30T22:32:32Z

Thanks for the comments! In the meantime I have found some more accurate information about the behavior of symlinks on Windows when working on this issue. Unfortunately it is quite complicated. The plan is to come back to this PR later and update it..

plicease self-requested a review January 22, 2021 18:07

plicease changed the title ~~Possible fix for issue #213~~ MSYS / cygwin symlinks Apr 3, 2021

plicease added 💣MSWin32 Windows is a weird best 🚉Platform Specific Often Windows lol, but could be something else labels Apr 3, 2021

plicease reviewed Apr 3, 2021

View reviewed changes

compilet-Y7gS9.c Outdated Show resolved Hide resolved

plicease reviewed Apr 3, 2021

View reviewed changes

lib/Alien/Build/Util.pm Outdated Show resolved Hide resolved

plicease reviewed Apr 3, 2021

View reviewed changes

hakonhagland added 3 commits June 18, 2021 09:26

Fixes issue PerlAlien#213

585acd0

Be more specific.

cad2b39

Be more specific/accurate when testing the cases that cannot create broken symlinks.

Update to clarify code.

aa4787c

hakonhagland force-pushed the msys2_symlink branch from 016196c to aa4787c Compare June 18, 2021 20:44

hakonhagland mentioned this pull request Jun 18, 2021

Test t/alien_build_plugin_pkgconfig_commandline.t fails on Windows 10, MSYS2 #210

Closed

plicease reviewed Jun 30, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MSYS / cygwin symlinks #246

MSYS / cygwin symlinks #246

hakonhagland commented Jan 22, 2021

hakonhagland commented Jan 22, 2021

hakonhagland commented Jan 23, 2021

plicease Apr 3, 2021

hakonhagland Jun 18, 2021

plicease Apr 3, 2021

hakonhagland Jun 18, 2021

plicease Jun 30, 2021

plicease Apr 3, 2021

hakonhagland Jun 18, 2021

plicease commented Apr 3, 2021

plicease Apr 3, 2021

hakonhagland Jun 18, 2021

plicease Jun 30, 2021

plicease Jun 30, 2021

plicease Jun 30, 2021

plicease commented Jun 30, 2021

hakonhagland commented Jun 30, 2021 •

edited

Loading

MSYS / cygwin symlinks #246

Are you sure you want to change the base?

MSYS / cygwin symlinks #246

Conversation

hakonhagland commented Jan 22, 2021

hakonhagland commented Jan 22, 2021

hakonhagland commented Jan 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plicease commented Apr 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plicease commented Jun 30, 2021

hakonhagland commented Jun 30, 2021 • edited Loading

hakonhagland commented Jun 30, 2021 •

edited

Loading