Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSYS / cygwin symlinks #246

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

MSYS / cygwin symlinks #246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
585acd0
Fixes issue #213
hakonhagland Jan 21, 2021
cad2b39
Be more specific.
hakonhagland Jan 23, 2021
aa4787c
Update to clarify code.
hakonhagland Jun 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 47 additions & 2 deletions lib/Alien/Build/Util.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use 5.008004;
use base qw( Exporter );
use Path::Tiny qw( path );
use Config;
use File::chdir;

# ABSTRACT: Private utility functions for Alien::Build
# VERSION
Expand All @@ -28,6 +29,21 @@ L<Alien::Build>

our @EXPORT_OK = qw( _mirror _dump _destdir_prefix _perl_config _ssl_reqs _has_ssl );

# This helper sub is intended to be called with string argument "MSYS" or "CYGWIN"
# According to https://cygwin.com/cygwin-ug-net/using-cygwinenv.html :
# The CYGWIN environment variable is used to configure many global settings for the Cygwin
# runtime system. It contain options separated by blank characters.
# TODO: We assume the same format for the MSYS environment variable. Where is it documented?
sub _check_native_symlink {
my ($var) = @_;
if (defined $ENV{$var}) {
if ($ENV{$var} =~ /(?:^|\s+)\Qwinsymlinks:nativestrict\E(?:$|\s+)/) {
return 1;
}
}
return 0;
}

# usage: _mirror $source_directory, $dest_direction, \%options
#
# options:
Expand All @@ -44,7 +60,6 @@ sub _mirror
require Alien::Build;
require File::Find;
require File::Copy;

File::Find::find({
wanted => sub {
next unless -e $File::Find::name;
Expand All @@ -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";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (path($target)->is_relative) {
my $nativesymlink = (($^O eq "msys" && _check_native_symlink("MSYS"))
|| ($^O eq "cygwin" && _check_native_symlink("CYGWIN")));
# NOTE: there are two cases to consider here, 1. the target might not
# exist relative to the source dir, and 2. the target might not exist relative
# to the destination directory.
#
# 1. If the file does not exist relative to the source, it is a broken symlink,
# 2. If the file does not exist relative to the destination, it means that
# it has not been copied by this File::Find::find() call yet. So it will only
# be temporarily broken.
if (!$src->parent->child($target)->exists) {
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";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
if ($nativesymlink) {
# If the target does not exist relative to the parent yet (it should be existing at the end of
# this File::Find::find() call), make a temporary empty file such that the symlink
# call does not fail.
$dst->parent->child($target)->touchpath;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
my $curdir = Path::Tiny->cwd;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

line can be removed now.

{
local $CWD = $dst->parent;
# CD into the directory, such that symlink will work on MSYS2
symlink($target, $dst) || die "unable to symlink $target => $dst";
}
}
elsif(-d "$src")
{
Expand Down
11 changes: 8 additions & 3 deletions t/alien_build_util.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,16 @@ subtest 'mirror' => sub {

if($Config{d_symlink})
{
foreach my $new (map { $tmp1->child("lib/libfoo$_") } qw( .so.1.2 .so.1 .so ))
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: $!";
Comment on lines +50 to +53
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

foreach my $new (map { "libfoo$_" } qw( .so.1.2 .so.1 .so ))
{
my $old = 'libfoo.so.1.2.3';
symlink($old, $new->stringify) || die "unable to symlink $new => $old $!";
my $old = $lib->basename;
symlink($old, $new) || die "unable to symlink $new => $old $!";
}
chdir $savedir or die "unable to chdir to $savedir: $!";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

my $tmp2 = Path::Tiny->tempdir("mirror_dst_XXXX");
Expand Down