tools/dev/install_files.pl did not care about symlinks

[gerd]

By executing the command:

make -n install DESTDIR=/home/<user>/<testdir>

you can see that the perl script "tools/dev/install_files" will be executed:

/usr/bin/perl tools/dev/install_files.pl \

--buildprefix= \ --prefix=/usr/local \ --exec-prefix=/usr/local \ --bindir=/usr/local/bin \ --libdir=/usr/local/lib \ --includedir=/usr/local/include \ --destdir=/home/gz016/test \ --docdir=/usr/local/share/doc \ --versiondir=/parrot/1.0.0-devel \ MANIFEST MANIFEST.generated

In the destination directory for example /home/gz016/test/local/lib you can see that libparrot.so is not a symlink to libparrot.so.1.0.0 but in the build directory blib/lib it was. To leave this as symlink is important for building rpms.

Gerd

[allison]

Thanks Gerd, but on systems that don't support symbolic linking, this patch will cause a fatal runtime error. The installation runs fine without the unversioned symbolic link, so you can skip it.

Allison

[gerd]

Generated with: diff -u tools/dev/install_files.pl tools/dev/install_files.pl.new > install_files.patch

[gerd]

I extend the patch to prove if the system does support symbolic linking. I only tested it on my Linux. Does it now run on every system and can be applied? Or may be it is required to add more checks?

[gerd]

patch for the file lib/Parrot/Install.pm

[gerd]

The file copying part as gone from tools/dev/install_files.pl to lib/Parrot/Install.pm. So I add the new patch Install.pm.diff to take care of links. Is is tested under Fedora and by building rpms.

[jkeenan]

Replying to gerd:

The file copying part as gone from tools/dev/install_files.pl to lib/Parrot/Install.pm. So I add the new patch Install.pm.diff to take care of links. Is is tested under Fedora and by building rpms.

gerd,

I did a diff between the patch you submitted 2 months ago and the one submitted 12 hours ago. I found no material difference between them:

$ diff -w gerd.install_files.patch gerd.Install.pm.diff 
1,3c1,3
< --- tools/dev/install_files.pl        2009-03-18 05:56:44.000000000 +0100
< +++ tools/dev/install_files.pl.new    2009-03-28 07:30:57.000000000 +0100
< @@ -292,6 +292,16 @@
---
> --- lib/Parrot/Install.pm     2009-06-01 09:29:57.000000000 +0200
> +++ lib/Parrot/Install.pm.new 2009-06-03 08:41:22.000000000 +0200
> @@ -220,6 +220,16 @@
17c17
<          copy( $src, $dest ) or die "copy $src to $dest: $!\n";
---
>              copy( $src, $dest ) or die "Error: couldn't copy $src to $dest: $!\n";

Only the content of the die message has changed.

Assuming that my analysis is correct, it would seem that Allison's objection to your original patch would apply to the new one as well.

I don't claim tremendous expertise in this area, but I wonder if there's some other rationale you can provide here.

Thank you very much.
kid51

[jkeenan]

If there are no further comments on this ticket, I will close it within 24 hours.

kid51

[gerd]

It would be nice if 'make install-dev' support linking in version 1.4.0.

[Infinoid]

Replying to allison:

Thanks Gerd, but on systems that don't support symbolic linking, this patch will cause a fatal runtime error. The installation runs fine without the unversioned symbolic link, so you can skip it.

On systems that don't support symbolic linking, $Config{d_symlink} is false. The patch checks that, so there shouldn't be a problem there. Or am I missing something?

I asked for some additional information on IRC and got:

[06:25] < Gerd> Infinoid: The benefit is only for building rpms
[06:26] < Gerd> Infinoid: On the system library there are only symlink from noversionnumber library to version librarys
[06:28] < Gerd> If the symlink is done with make from Parrot there is no need to take for it in the spec file of the rpm

[06:32] < Gerd> Infinoid: The symlink is create first and get lost with install_files script

Preserving that symlink would result in a somewhat smaller installation image. Or rather, it would prevent us from pushing the same thing into the distro scripts. It does seem a bit silly that we make the symlink (during build), clobber it (during install_files) and then recreate it (during packaging)...

Mark

[gerd]

On which operating systems the patch cause a fatal runtime error? I am very busy the next month. But although I would like to setup a system to reproduce the errror. So perhaps I can find a better solution that works in general.

[allison]

I'm happy to go ahead and apply it if we get testing on our big 3 platforms. But, we should wrap the symlink call in an eval providing a more meaningful error message to future-proof porting. There is nothing more frustrating when tackling a new platform than vague and unhelpful fatal errors in the build and installation process.

[jkeenan]

Replying to allison:

I'm happy to go ahead and apply it if we get testing on our big 3 platforms. But, we should wrap the symlink call in an eval providing a more meaningful error message to future-proof porting.

I have created the tt509_install_files branch in SVN to implement this. Here is what I am applying in that branch:

$ svn diff
Index: lib/Parrot/Install.pm
===================================================================
--- lib/Parrot/Install.pm       (revision 40089)
+++ lib/Parrot/Install.pm       (working copy)
@@ -226,6 +226,21 @@
         else {
             next unless -e $src;
             next if $^O eq 'cygwin' and -e "$src.exe"; # stat works, copy not
+            if (-l $src) { 
+                # check if the system supports symbolic linking 
+                use Config; 
+                if ($Config{d_symlink} && $Config{d_readlink}) { 
+                    # copy as symbolic link;
+                    # be extra cautious about existence of symlinks
+                    # on a given OS
+                    my $symlink_exists = eval {
+                        symlink(readlink($src), $dest); 1;
+                    };
+                    die "$@" unless $symlink_exists;
+                    print "$dest\n"; 
+                    next; 
+                } 
+            } 
             copy( $src, $dest ) or die "Error: couldn't copy $src to $dest: $!\n";
             print "$dest\n";
         }

Can we get some people on Win32 systems to check out this branch, configure with perl Configure.pl --prefix=C:\tmp\some\appropriate\directory, then make manifest_tests && make && make install and post results in this ticket?

(I specify make manifest_tests because that's where the tests of the install tools are located, make-wise.)

make manifest_tests and make install work properly on Linux -- but then you'd expect them to, wouldn't you?

Thank you very much.
kid51

[particle]

Replying to jkeenan:

Replying to allison:

I'm happy to go ahead and apply it if we get testing on our big 3 platforms. But, we should wrap the symlink call in an eval providing a more meaningful error message to future-proof porting.

I have created the tt509_install_files branch in SVN to implement this. Here is what I am applying in that branch: {{{ $ svn diff Index: lib/Parrot/Install.pm =================================================================== --- lib/Parrot/Install.pm (revision 40089) +++ lib/Parrot/Install.pm (working copy) @@ -226,6 +226,21 @@ else { next unless -e $src; next if $^{O eq 'cygwin' and -e "$src.exe"; # stat works, copy not + if (-l $src) { + # check if the system supports symbolic linking + use Config; + if ($Config{d_symlink} && $Config{d_readlink}) { + # copy as symbolic link; + # be extra cautious about existence of symlinks + # on a given OS + my $symlink_exists = eval { + symlink(readlink($src), $dest); 1; + }; + die "$@" unless $symlink_exists; + print "$dest\n"; + next; + } + } copy( $src, $dest ) or die "Error: couldn't copy $src to $dest: $!\n"; print "$dest\n"; } }}} Can we get some people on Win32 systems to check out this branch, configure with perl Configure.pl --prefix=C:\tmp\some\appropriate\directory, then make manifest_tests && make && make install and post results in this ticket? (I specify make manifest_tests because that's where the tests of the install tools are located, make-wise.) make manifest_tests and make install work properly on Linux -- but then you'd expect them to, wouldn't you? Thank you very much.
kid51}

i'm afraid this won't be good enough. for example, many linux distros allow a user to mount fat32 filesystems, which do not support symlinks. therefore, it is not sufficient to test the 'system' for symlink support. the filesystem (or filesystems) where the build is performed must be tested for symlink support during the build, and the filesystem (or filesystems) where parrot is to be installed must be tested for symlink support during install.

anything less will not be portable.

this brings up an interesting point, we have no linux systems with fat32 mounts to my knowledge in our smoke farm. these configurations would make interesting additions, to help us challenge our assumptions about filesystems throughout our codebase.

~particle

[jkeenan]

Replying to particle:

i'm afraid this won't be good enough. for example, many linux distros allow a user to mount fat32 filesystems, which do not support symlinks. therefore, it is not sufficient to test the 'system' for symlink support. the filesystem (or filesystems) where the build is performed must be tested for symlink support during the build, and the filesystem (or filesystems) where parrot is to be installed must be tested for symlink support during install.

anything less will not be portable.

this brings up an interesting point, we have no linux systems with fat32 mounts to my knowledge in our smoke farm. these configurations would make interesting additions, to help us challenge our assumptions about filesystems throughout our codebase.

Okay. I think your point is very similar to the point made by Austin in TT #799.

kid51

[gerd]

So the perfect solution would be to have a config variable that signal if the build directory supports symlinks and another variable in the config hash that signals if the install directory supports syslinks. So a C funktion has to been called to determine if a directory or filesystem is able to have syslinks. Did anyone know if there is a C funktion to determine if a directory supports symlinks?

[wayland]

Updated patch

[wayland]

The patch I just added should do the same thing as the previous patches, but should also fail gracefully on a FAT32 filesystem on a Linux box. I haven't run it or tested it at all at this point, but hopefully it will point someone in the right direction. "man Errno" for further info.

[gerd]

I updated the patch file "Install.pm.patch". The symlink is now created in an eval block to protect the parrot installation from being aborted if a destination directory do not support symlinks. I only tested the patch on a system that supports symlinks. There it works. By a destination directory where symlinks are not possible I expect that the warning will given out and the copy() function will be executed as next.

[jkeenan]

Replying to wayland:

The patch I just added should do the same thing as the previous patches, but should also fail gracefully on a FAT32 filesystem on a Linux box. I haven't run it or tested it at all at this point, but hopefully it will point someone in the right direction. "man Errno" for further info.

Prodded by whiteknight++ as to the status of this ticket/branch, I applied wayland's patch to lib/Parrot/Install.pm in the tt509_install_files branch. This will need testing on systems that don't support symlinks at all, as well as those non-symlinkable filesystem situations discussed earlier in this TT.

Please review that branch. Thank you very much.

kid51

[jkeenan]

Replying to jkeenan:

Prodded by whiteknight++ as to the status of this ticket/branch, I applied wayland's patch to lib/Parrot/Install.pm in the tt509_install_files branch. This will need testing on systems that don't support symlinks at all, as well as those non-symlinkable filesystem situations discussed earlier in this TT. Please review that branch. Thank you very much.

We still need the testing described in comment:16. Can anyone help out?

Thank you very much.
kid51

[gerd]

To get this ticket closed I created a FAT32 partition on my Linux-PC and formated the partition as a vfat-filesystem. I tried all the patch of this ticket, but none of the patches works. I put my new solution in r48115.

[jkeenan]

Replying to gerd:

To get this ticket closed I created a FAT32 partition on my Linux-PC and formated the partition as a vfat-filesystem. I tried all the patch of this ticket, but none of the patches works. I put my new solution in r48115.

gerd,

I'll trust you on this. Closing ticket. Thanks for looking into this long-delayed matter.

kid51