[bug#39807] [PATCH] guix: pack: Only wrap executable files.

[Eric Bavier <bavier@posteo.net>]

[-- Attachment #1: Type: text/plain, Size: 1615 bytes --]

On Wed, 2020-10-21 at 23:31 +0200, Ludovic Courtès wrote:
> Hi,
> 
> Eric Bavier <bavier@posteo.net> skribis:
> 
> > On Wed, 2020-10-21 at 17:35 +0200, Ludovic Courtès wrote:
> > > Hi Eric,
> > > 
> > > Eric Bavier <bavier@posteo.net> skribis:
> > > 
> > > >                (mkdir-p (dirname result))
> > > >                (apply invoke #$compiler "-std=gnu99" "-static" "-Os" "-g0" "-Wall"
> > > >                       "run.c" "-o" result
> > > > +                     (string-append "-DWRAPPER_PROGRAM=\"" result "\"")
> > > 
> > > Can we just write (canonical-path result) here?  That way we wouldn’t
> > > need ‘find-input-files’ and related changes.
> > > 
> > 
> > Guile's canonicalize-path will raise and error if the path does not
> > already exist.  We could create a dummy file at result, then call
> > canonicalize-path? but that seems clumsier than forming a canonical
> > name in the first place?
> 
> The patch you sent has ‘find-files’ use ‘stat’ instead of the trailing
> slash.  It introduces a difference: by using ‘stat’ all the way, it
> follows all symlinks, not just the higher-level ones.  I don’t know if
> this could have undesired implications, like wrapping the same file
> twice because there’s a symlink pointing to it.  (Or am I too paranoid?)
> 
> To be on the safe side, we could write:
> 
>   (string-append (canonical-path (dirname) result) "/" result)
> 
> WDYT?

I don't like the "stat" change either.  Paranoia is not misplaced, I
think.

Following are current two patches.  I can add copyright to run-in-
namespace.c if you think it's needed.

`~Eric

[-- Attachment #2: 0001-guix-pack-Fix-offset-calculation-for-store-directory.patch --]
[-- Type: text/x-patch, Size: 2864 bytes --]

From 7caaea0b21e5b97836b1a40a44efd2f38dbab7ae Mon Sep 17 00:00:00 2001
From: Eric Bavier <bavier@member.fsf.org>
Date: Wed, 21 Oct 2020 18:33:52 -0500
Subject: [PATCH 1/2] guix: pack: Fix offset calculation for store directory
 mount point.

Fixes wrapping of non-package things, where the target store directory may
differ in length from the original.

* guix/scripts/pack.scm (wrapped-package)<build-wrapper>: Define
WRAPPER_PROGRAM macro with wrapper's file name.
* gnu/packages/aux-files/run-in-namespace.c (main): Offset index by len of
that file name.
---
 gnu/packages/aux-files/run-in-namespace.c | 2 +-
 guix/scripts/pack.scm                     | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/gnu/packages/aux-files/run-in-namespace.c b/gnu/packages/aux-files/run-in-namespace.c
index 52a16a5362..947ff02dda 100644
--- a/gnu/packages/aux-files/run-in-namespace.c
+++ b/gnu/packages/aux-files/run-in-namespace.c
@@ -620,7 +620,7 @@ main (int argc, char *argv[])
   /* SELF is something like "/home/ludo/.local/gnu/store/…-foo/bin/ls" and we
      want to extract "/home/ludo/.local/gnu/store".  */
   size_t index = strlen (self)
-    - strlen ("@WRAPPED_PROGRAM@") + strlen (original_store);
+    - strlen (WRAPPER_PROGRAM) + strlen (original_store);
   char *store = strdup (self);
   store[index] = '\0';
 
diff --git a/guix/scripts/pack.scm b/guix/scripts/pack.scm
index a5a70d5162..f02f3662a5 100644
--- a/guix/scripts/pack.scm
+++ b/guix/scripts/pack.scm
@@ -5,6 +5,7 @@
 ;;; Copyright © 2018 Chris Marusich <cmmarusich@gmail.com>
 ;;; Copyright © 2018 Efraim Flashner <efraim@flashner.co.il>
 ;;; Copyright © 2020 Tobias Geerinckx-Rice <me@tobias.gr>
+;;; Copyright © 2020 Eric Bavier <bavier@posteo.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -847,7 +848,7 @@ last resort for relocation."
               (("@STORE_DIRECTORY@") (%store-directory)))
 
             (let* ((base   (strip-store-prefix program))
-                   (result (string-append target "/" base))
+                   (result (string-append target base))
                    (proot  #$(and proot?
                                   #~(string-drop
                                      #$(file-append (proot) "/bin/proot")
@@ -856,6 +857,9 @@ last resort for relocation."
               (mkdir-p (dirname result))
               (apply invoke #$compiler "-std=gnu99" "-static" "-Os" "-g0" "-Wall"
                      "run.c" "-o" result
+                     (string-append "-DWRAPPER_PROGRAM=\""
+                                    (canonicalize-path (dirname result)) "/"
+                                    (basename result) "\"")
                      (append (if proot
                                  (list (string-append "-DPROOT_PROGRAM=\""
                                                       proot "\""))
-- 
2.28.0


[-- Attachment #3: 0002-guix-pack-Only-wrap-executable-files.patch --]
[-- Type: text/x-patch, Size: 5519 bytes --]

From 85bd962e929924b016a85d3a0b3dff434ebe8de3 Mon Sep 17 00:00:00 2001
From: Eric Bavier <bavier@member.fsf.org>
Date: Mon, 24 Feb 2020 23:47:02 -0600
Subject: [PATCH 2/2] guix: pack: Only wrap executable files.

* guix/scripts/pack.scm (wrapped-package)<build>: Build wrappers for
executable files and symlink others.
* tests/guix-pack-relocatable.sh: Test relocatable example of mixed
executable and non-executable files.
---
 guix/scripts/pack.scm          | 30 +++++++++++++++++++++---------
 tests/guix-pack-relocatable.sh | 25 +++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/guix/scripts/pack.scm b/guix/scripts/pack.scm
index f02f3662a5..8e694edbbe 100644
--- a/guix/scripts/pack.scm
+++ b/guix/scripts/pack.scm
@@ -749,12 +749,13 @@ last resort for relocation."
                               (guix elf)))
       #~(begin
           (use-modules (guix build utils)
-                       ((guix build union) #:select (relative-file-name))
+                       ((guix build union) #:select (symlink-relative))
                        (guix elf)
                        (guix build gremlin)
                        (ice-9 binary-ports)
                        (ice-9 ftw)
                        (ice-9 match)
+                       (ice-9 receive)
                        (srfi srfi-1)
                        (rnrs bytevectors))
 
@@ -874,16 +875,27 @@ last resort for relocation."
           (mkdir target)
           (for-each (lambda (file)
                       (unless (member file '("." ".." "bin" "sbin" "libexec"))
-                        (let ((file* (string-append input "/" file)))
-                          (symlink (relative-file-name target file*)
-                                   (string-append target "/" file)))))
+                        (symlink-relative (string-append input  "/" file)
+                                          (string-append target "/" file))))
                     (scandir input))
 
-          (for-each build-wrapper
-                    ;; Note: Trailing slash in case these are symlinks.
-                    (append (find-files (string-append input "/bin/"))
-                            (find-files (string-append input "/sbin/"))
-                            (find-files (string-append input "/libexec/")))))))
+          (receive (executables others)
+              (partition executable-file?
+			 ;; Note: Trailing slash in case these are symlinks.
+                         (append (find-files (string-append input "/bin/"))
+                                 (find-files (string-append input "/sbin/"))
+                                 (find-files (string-append input "/libexec/"))))
+            ;; Wrap only executables, since the wrapper will eventually need
+            ;; to execve them.  E.g. git's "libexec" directory contains many
+            ;; shell scripts that are source'd from elsewhere, which fails if
+            ;; they are wrapped.
+            (for-each build-wrapper executables)
+            ;; Link any other non-executable files
+            (for-each (lambda (old)
+                        (let ((new (string-append target (strip-store-prefix old))))
+                          (mkdir-p (dirname new))
+                          (symlink-relative old new)))
+                      others)))))
 
   (computed-file (string-append
                   (cond ((package? package)
diff --git a/tests/guix-pack-relocatable.sh b/tests/guix-pack-relocatable.sh
index a960ecd209..5ac8cdc718 100644
--- a/tests/guix-pack-relocatable.sh
+++ b/tests/guix-pack-relocatable.sh
@@ -1,5 +1,6 @@
 # GNU Guix --- Functional package management for GNU
 # Copyright © 2018, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
+# Copyright © 2020 Eric Bavier <bavier@posteo.net>
 #
 # This file is part of GNU Guix.
 #
@@ -98,6 +99,7 @@ case "`uname -m`" in
 	run_without_store GUIX_EXECUTION_ENGINE="fakechroot" \
 	"$test_directory/Bin/sed" --version > "$test_directory/output"
 	grep 'GNU sed' "$test_directory/output"
+	unset GUIX_EXECUTION_ENGINE
 
 	chmod -Rf +w "$test_directory"; rm -rf "$test_directory"/*
 
@@ -133,8 +135,31 @@ esac
 tarball="`guix pack -R -S /share=share groff:doc`"
 (cd "$test_directory"; tar xf "$tarball")
 test -d "$test_directory/share/doc/groff/html"
+chmod -Rf +w "$test_directory"; rm -rf "$test_directory"/*
 
 # Ensure '-R' applies to propagated inputs.  Failing to do that, it would fail
 # with a profile collision error in this case because 'python-scipy'
 # propagates 'python-numpy'.  See <https://bugs.gnu.org/42510>.
 guix pack -RR python-numpy python-scipy --no-grafts -n
+
+# Check that packages that mix executable and support files (e.g. git) in the
+# "binary" directories still work after wrapped.
+cat >"$test_directory/manifest.scm" <<'EOF'
+(use-modules (guix) (guix profiles)
+             (gnu packages bootstrap))
+(manifest
+ (list (manifest-entry
+	(name "test") (version "0")
+	(item (file-union "test"
+			  `(("bin/hello"
+			     ,(program-file
+                               "hello"
+                               #~(begin (display (load "../libexec/msg"))(newline))
+                               #:guile %bootstrap-guile))
+			    ("libexec/msg"
+			     ,(plain-file "msg" "42"))))))))
+EOF
+tarball="`guix pack -RR -S /opt= -m $test_directory/manifest.scm`"
+(cd "$test_directory"; tar xvf "$tarball")
+run_without_store "$test_directory/opt/bin/hello" > "$test_directory/output"
+test "`cat $test_directory/output`" = "42"
-- 
2.28.0