GNU bug report logs - #39807
[PATCH] guix: pack: Only wrap executable files.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 39807 in the body.
You can then email your comments to 39807 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eric Bavier <bavier <at> posteo.net>
To: guix-patches <at> gnu.org
Cc: Eric Bavier <bavier <at> member.fsf.org>
Subject: [PATCH] guix: pack: Only wrap executable files.
Date: Wed, 26 Feb 2020 22:36:04 -0600

From: Eric Bavier <bavier <at> member.fsf.org>

Hello Guix,

This patch fixes some uses of relocatable git (e.g.  octopus merge). 
Previously, guix pack would wrap all files in "bin", "sbin", and "libexec",
even non-executable files.  This would cause issues for git when its shell
scripts in libexec would try to source other shell files that had been
wrapped and were no longer a valid shell file.

I feel like a test should be added to tests/guix-pack-relocatable.sh, but
I'm not sure how to do that while keeping the test lightweight.  Suggestions
welcome.

Cheers,
`~Eric


* guix/scripts/pack.scm (wrapped-package)<build>: Build wrappers for
executable files and symlink others.
---
 guix/scripts/pack.scm | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/guix/scripts/pack.scm b/guix/scripts/pack.scm
index c8d8546e29..3634326102 100644
--- a/guix/scripts/pack.scm
+++ b/guix/scripts/pack.scm
@@ -4,6 +4,7 @@
 ;;; Copyright © 2018 Konrad Hinsen <konrad.hinsen <at> fastmail.net>
 ;;; Copyright © 2018 Chris Marusich <cmmarusich <at> gmail.com>
 ;;; Copyright © 2018 Efraim Flashner <efraim <at> flashner.co.il>
+;;; Copyright © 2020 Eric Bavier <bavier <at> posteo.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -673,9 +674,11 @@ last resort for relocation."
                               (guix build union)))
       #~(begin
           (use-modules (guix build utils)
-                       ((guix build union) #:select (relative-file-name))
+                       ((guix build union) #:select (symlink-relative))
+                       (srfi srfi-1)
                        (ice-9 ftw)
-                       (ice-9 match))
+                       (ice-9 match)
+                       (ice-9 receive))
 
           (define input
             ;; The OUTPUT* output of PACKAGE.
@@ -726,15 +729,26 @@ 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
-                    (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?
+                         (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)
-- 
2.25.1

Message #8 received at 39807 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Eric Bavier <bavier <at> posteo.net>
Cc: 39807 <at> debbugs.gnu.org, Eric Bavier <bavier <at> member.fsf.org>
Subject: Re: [bug#39807] [PATCH] guix: pack: Only wrap executable files.
Date: Fri, 06 Mar 2020 12:16:32 +0100

Hi,

Eric Bavier <bavier <at> posteo.net> skribis:

> From: Eric Bavier <bavier <at> member.fsf.org>
>
> Hello Guix,
>
> This patch fixes some uses of relocatable git (e.g.  octopus merge). 
> Previously, guix pack would wrap all files in "bin", "sbin", and "libexec",
> even non-executable files.  This would cause issues for git when its shell
> scripts in libexec would try to source other shell files that had been
> wrapped and were no longer a valid shell file.

Good catch!

> I feel like a test should be added to tests/guix-pack-relocatable.sh, but
> I'm not sure how to do that while keeping the test lightweight.  Suggestions
> welcome.

Not sure how to do that.  Since ‘guix pack’ accepts manifests, you could
have a manifest containing a ‘computed-file’ with a file that shouldn’t
be wrapped, and then you could ensure that’s indeed the case.  Or you
could try with ‘git-minimal’ or some other package that exhibits the
problem?

> * guix/scripts/pack.scm (wrapped-package)<build>: Build wrappers for
> executable files and symlink others.

[...]

> -          (for-each build-wrapper
> -                    (append (find-files (string-append input "/bin"))
> -                            (find-files (string-append input "/sbin"))
> -                            (find-files (string-append input "/libexec")))))))
> +          (receive (executables others)

I’d prefer srfi-11 ‘let-values’.  :-)

Otherwise LGTM, thanks!

Ludo’.

Message #11 received at 39807 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Eric Bavier <bavier <at> posteo.net>
Cc: 39807 <at> debbugs.gnu.org, Eric Bavier <bavier <at> member.fsf.org>
Subject: Re: [bug#39807] [PATCH] guix: pack: Only wrap executable files.
Date: Tue, 24 Mar 2020 18:51:36 +0100

Ping!  :-)

Ludovic Courtès <ludo <at> gnu.org> skribis:

> Hi,
>
> Eric Bavier <bavier <at> posteo.net> skribis:
>
>> From: Eric Bavier <bavier <at> member.fsf.org>
>>
>> Hello Guix,
>>
>> This patch fixes some uses of relocatable git (e.g.  octopus merge). 
>> Previously, guix pack would wrap all files in "bin", "sbin", and "libexec",
>> even non-executable files.  This would cause issues for git when its shell
>> scripts in libexec would try to source other shell files that had been
>> wrapped and were no longer a valid shell file.
>
> Good catch!
>
>> I feel like a test should be added to tests/guix-pack-relocatable.sh, but
>> I'm not sure how to do that while keeping the test lightweight.  Suggestions
>> welcome.
>
> Not sure how to do that.  Since ‘guix pack’ accepts manifests, you could
> have a manifest containing a ‘computed-file’ with a file that shouldn’t
> be wrapped, and then you could ensure that’s indeed the case.  Or you
> could try with ‘git-minimal’ or some other package that exhibits the
> problem?
>
>> * guix/scripts/pack.scm (wrapped-package)<build>: Build wrappers for
>> executable files and symlink others.
>
> [...]
>
>> -          (for-each build-wrapper
>> -                    (append (find-files (string-append input "/bin"))
>> -                            (find-files (string-append input "/sbin"))
>> -                            (find-files (string-append input "/libexec")))))))
>> +          (receive (executables others)
>
> I’d prefer srfi-11 ‘let-values’.  :-)
>
> Otherwise LGTM, thanks!
>
> Ludo’.

Message #14 received at 39807 <at> debbugs.gnu.org (full text, mbox):

From: Eric Bavier <bavier <at> posteo.net>
To: 39807 <at> debbugs.gnu.org
Subject: Re: [bug#39807] [PATCH] guix: pack: Only wrap executable files.
Date: Thu, 26 Mar 2020 21:29:56 -0500

On 06.03.2020 05:16, Ludovic Courtès wrote:
> Hi,
> 
> Eric Bavier <bavier <at> posteo.net> skribis:
> 
>> From: Eric Bavier <bavier <at> member.fsf.org>
>> 
>> I feel like a test should be added to tests/guix-pack-relocatable.sh, 
>> but
>> I'm not sure how to do that while keeping the test lightweight.  
>> Suggestions
>> welcome.
> 
> Not sure how to do that.  Since ‘guix pack’ accepts manifests, you 
> could
> have a manifest containing a ‘computed-file’ with a file that shouldn’t
> be wrapped, and then you could ensure that’s indeed the case.  Or you
> could try with ‘git-minimal’ or some other package that exhibits the
> problem?

I almost have a working test using 'git-minimal', but I'm not happy with 
the quantity of code needed to setup, and I'm worried now that that test 
would be relying on an implementation detail that could change in the 
future without us noticing (e.g. a git subcommand that's currently a 
shell script is subsumed into git so the test no longer checks what we 
want).

So I think I'll try going the manifest/computed-file route instead.

> 
>> * guix/scripts/pack.scm (wrapped-package)<build>: Build wrappers for
>> executable files and symlink others.
> 
> [...]
> 
>> -          (for-each build-wrapper
>> -                    (append (find-files (string-append input "/bin"))
>> -                            (find-files (string-append input 
>> "/sbin"))
>> -                            (find-files (string-append input 
>> "/libexec")))))))
>> +          (receive (executables others)
> 
> I’d prefer srfi-11 ‘let-values’.  :-)

I tried let-values to begin with, but I found 'receive' to be much 
easier on the eyes.  For the case of binding values from a single 
expression, does let-values offer benefits?  And there are no other uses 
of let-values in this module, so precedent/consistency doesn't seem to 
have weight.

> Otherwise LGTM, thanks!

Thanks for review (and ping)!

-- 
`~Eric

Message #17 received at 39807 <at> debbugs.gnu.org (full text, mbox):

From: Eric Bavier <bavier <at> posteo.net>
To: 39807 <39807 <at> debbugs.gnu.org>
Subject: Re: [bug#39807] [PATCH] guix: pack: Only wrap executable files.
Date: Thu, 26 Mar 2020 21:53:18 -0500

[Message part 1 (text/plain, inline)]

Latest patch attached.
-- 
`~Eric

[0001-wip-guix-pack-Only-wrap-executable-files.patch (text/x-diff, attachment)]

Message #20 received at 39807 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Eric Bavier <bavier <at> posteo.net>
Cc: 39807 <at> debbugs.gnu.org
Subject: Re: [bug#39807] [PATCH] guix: pack: Only wrap executable files.
Date: Sun, 29 Mar 2020 16:39:30 +0200

Hi Eric,

Eric Bavier <bavier <at> posteo.net> skribis:

> On 06.03.2020 05:16, Ludovic Courtès wrote:

[...]

>>> I feel like a test should be added to
>>> tests/guix-pack-relocatable.sh, but
>>> I'm not sure how to do that while keeping the test lightweight.
>>> Suggestions
>>> welcome.
>>
>> Not sure how to do that.  Since ‘guix pack’ accepts manifests, you
>> could
>> have a manifest containing a ‘computed-file’ with a file that shouldn’t
>> be wrapped, and then you could ensure that’s indeed the case.  Or you
>> could try with ‘git-minimal’ or some other package that exhibits the
>> problem?
>
> I almost have a working test using 'git-minimal', but I'm not happy
> with the quantity of code needed to setup, and I'm worried now that
> that test would be relying on an implementation detail that could
> change in the future without us noticing (e.g. a git subcommand that's
> currently a shell script is subsumed into git so the test no longer
> checks what we want).
>
> So I think I'll try going the manifest/computed-file route instead.

OK.

>>> -          (for-each build-wrapper
>>> -                    (append (find-files (string-append input "/bin"))
>>> -                            (find-files (string-append input
>>> "/sbin"))
>>> -                            (find-files (string-append input
>>> "/libexec")))))))
>>> +          (receive (executables others)
>>
>> I’d prefer srfi-11 ‘let-values’.  :-)
>
> I tried let-values to begin with, but I found 'receive' to be much
> easier on the eyes.  For the case of binding values from a single
> expression, does let-values offer benefits?  And there are no other
> uses of let-values in this module, so precedent/consistency doesn't
> seem to have weight.

OK, no big deal.

There are probably more uses of ‘let-values’ than ‘receive’ overall.
That said, I think we can start switching to srfi-71, which is nicer
than both of these.

Thanks,
Ludo’.

Message #23 received at 39807 <at> debbugs.gnu.org (full text, mbox):

From: Eric Bavier <bavier <at> posteo.net>
To: 39807 <at> debbugs.gnu.org
Subject: Re: [bug#39807] [PATCH] guix: pack: Only wrap executable files.
Date: Mon, 27 Jul 2020 16:42:00 -0500

[Message part 1 (text/plain, inline)]

Patch rebased on latest master attached.  The new test implements a
small proxy for the behavior exhibited by git and its libexec scripts.

Call for help: the test does not pass!  I get this error:

  hello: run.c:284: exec_in_user_namespace: Unexpected error: No such
file or directory.

Could someone more familiar with user namespaces, etc help me work
this out?

`~Eric

[0001-guix-pack-Only-wrap-executable-files.patch (text/x-patch, attachment)]

Message #26 received at 39807 <at> debbugs.gnu.org (full text, mbox):

From: Eric Bavier <bavier <at> posteo.net>
To: 39807 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: [bug#39807] [PATCH] guix: pack: Only wrap executable files.
Date: Wed, 21 Oct 2020 00:09:58 -0500

On Mon, 2020-07-27 at 16:42 -0500, Eric Bavier wrote:
> Call for help: the test does not pass!  I get this error:
> 
>   hello: run.c:284: exec_in_user_namespace: Unexpected error: No such
> file or directory.
> 
> Could someone more familiar with user namespaces, etc help me work
> this out?

After following a helpful suggestion to try using `strace`, turns out the issue is not with user namespaces but mostly
with string manipulation :)  Apologies in advance for wall-of-text.

  $ unshare -mrf strace -s 80 -o trace -ff \
  >  sh -c 'mount -t tmpfs -o ro none "/gnu/store"; 
            /tmp/pack-dir/opt/bin/hello'

and in one the log file corresponding to the exec of the wrapper `hello` I see

  readlink("/proc/self/exe",
           "/tmp/pack-dir/gnu/store/80kbbxnzn3kgs1jkc6m6ydw2m44lnfaq-wrapperR/bin/hello", 4095) = 75
  lstat("/gnu/store/zc92ghli8ws31qshf4bhzw1npzqhs4my-test/bin//hello", 
        0x7ffe308a4980) = -1 ENOENT (No such file or directory)

and in the log corresponding to the child after forking in exec_in_user_namespace we see the call that leads to the
above error:

  mount("/tmp/pack-dir/gnu/store/80", "/tmp/guix-exec-YMr7WJ//gnu/store", 0x4810a7, MS_RDONLY|MS_BIND|MS_REC, NULL) = -1
ENOENT (No such file or directory)
  write(2, "hello: run.c:284: exec_in_user_namespace: Unexpected error: No such file or dire"..., 87) = 87

So exec_in_user_namespace is trying to mount "/tmp/pack-dir/gnu/store/80", which is not a directory.

In gnu/packages/aux-files/run-in-namespace.c:620-626 we try to calculate the name of the relocated store directory.  So
far this calculation seems to "accidentaly" work:

/tmp/pack-dir/gnu/store/78xrsg1z...-emacs-no-x-27.1R/bin/emacs
             /gnu/store/w9csar3m...-emacs-no-x-27.1/bin//emacs

The "R" suffix appended to the wrapper store directory name and the double-slash we get from find-files (c.f.
guix/scripts/pack.scm:881) "cancel out".   But we might not be so fortunate and can get something like this:

                          |
/tmp/pack-dir/gnu/store/80|kbbxnz...-wrapperR/bin/hello (self)
                /gnu/store|/zc92ghli...-test/bin//hello (@PROG@)
                /gnu/store|                     (original_store)
                          |

Because the manifest entry used in the tests added in this patch enters the "else" case of `wrapped-package` (c.f.
guix/scripts/pack.scm:904) the index calculation strays and we get a non-directory mount point.  I can make the test
pass by using a slightly longer name of "testing" for the file-union :)

I don't think we can enforce a stricter match between the wrapper and target store item names to ensure their lengths
are the same, right?  It seems like we maybe want to ignore @WRAPPED_PROGRAM@ and use only /proc/self/exe and
original_store to find the relocated store directory?  A regex search might be too costly.  We could use strstr to
search for the first occurrence of original_store, if we don't mind assuming that most people will probably not unpack
into $HOME/.guix/gnu/store/mine/packs/foo e.g.

--- a/gnu/packages/aux-files/run-in-namespace.c
+++ b/gnu/packages/aux-files/run-in-namespace.c
@@ -619,10 +619,8 @@ 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);
   char *store = strdup (self);
-  store[index] = '\0';
+  strstr (store, original_store)[sizeof original_store - 1] = '\0';
 
   struct stat statbuf;
 

WDYT?


`~Eric

Message #29 received at 39807 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Eric Bavier <bavier <at> posteo.net>
Cc: 39807 <at> debbugs.gnu.org
Subject: Re: [bug#39807] [PATCH] guix: pack: Only wrap executable files.
Date: Wed, 21 Oct 2020 11:07:11 +0200

[Message part 1 (text/plain, inline)]

Hi Eric,

Eric Bavier <bavier <at> posteo.net> skribis:

> In gnu/packages/aux-files/run-in-namespace.c:620-626 we try to calculate the name of the relocated store directory.  So
> far this calculation seems to "accidentaly" work:
>
> /tmp/pack-dir/gnu/store/78xrsg1z...-emacs-no-x-27.1R/bin/emacs
>              /gnu/store/w9csar3m...-emacs-no-x-27.1/bin//emacs
>
> The "R" suffix appended to the wrapper store directory name and the double-slash we get from find-files (c.f.
> guix/scripts/pack.scm:881) "cancel out".   But we might not be so fortunate and can get something like this:
>
>                           |
> /tmp/pack-dir/gnu/store/80|kbbxnz...-wrapperR/bin/hello (self)
>                 /gnu/store|/zc92ghli...-test/bin//hello (@PROG@)
>                 /gnu/store|                     (original_store)
>                           |
>
> Because the manifest entry used in the tests added in this patch enters the "else" case of `wrapped-package` (c.f.
> guix/scripts/pack.scm:904) the index calculation strays and we get a non-directory mount point.  I can make the test
> pass by using a slightly longer name of "testing" for the file-union :)
>
> I don't think we can enforce a stricter match between the wrapper and target store item names to ensure their lengths
> are the same, right?  It seems like we maybe want to ignore @WRAPPED_PROGRAM@ and use only /proc/self/exe and
> original_store to find the relocated store directory?  A regex search might be too costly.  We could use strstr to
> search for the first occurrence of original_store, if we don't mind assuming that most people will probably not unpack
> into $HOME/.guix/gnu/store/mine/packs/foo e.g.

Good catch!  This is embarrassing.

Instead of searching for an occurrence of ORIGINAL_STORE, can’t we use
the file name of the wrapper (as opposed to WRAPPED_PROGRAM) in the
index calculation?  Along these lines:

[Message part 2 (text/x-patch, inline)]

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..c353f50ced 100644
--- a/guix/scripts/pack.scm
+++ b/guix/scripts/pack.scm
@@ -856,6 +856,7 @@ 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=\"" result "\"")
                      (append (if proot
                                  (list (string-append "-DPROOT_PROGRAM=\""
                                                       proot "\""))

[Message part 3 (text/plain, inline)]

Thanks,
Ludo’.

Message #32 received at 39807 <at> debbugs.gnu.org (full text, mbox):

From: Eric Bavier <bavier <at> posteo.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 39807 <at> debbugs.gnu.org
Subject: Re: [bug#39807] [PATCH] guix: pack: Only wrap executable files.
Date: Wed, 21 Oct 2020 10:12:14 -0500

Hi Ludo,

On Wed, 2020-10-21 at 11:07 +0200, Ludovic Courtès wrote:
> Hi Eric,
> 
> Instead of searching for an occurrence of ORIGINAL_STORE, can’t we use
> the file name of the wrapper (as opposed to WRAPPED_PROGRAM) in the
> index calculation?  Along these lines:

Good idea, I hadn't considered that we know the destination of the
wrapper in advance.  

This works as long as we make sure "result" is in canonical form, e.g.
no repeated separators, because /proc/self/exe is in canonical form:

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 ac578aa965..8106031d6d 100644
--- a/guix/scripts/pack.scm
+++ b/guix/scripts/pack.scm
@@ -776,6 +776,10 @@ last resort for relocation."
                 (#f    base)
                 (index (string-drop base index)))))
 
+          (define (find-input-files dir)
+            ;; Note: Use 'stat' so that symlinks are followed.
+            (find-files (string-append input "/" dir) #:stat stat))
+
           (define (elf-interpreter elf)
             ;; Return the interpreter of ELF as a string, or #f if ELF has no
             ;; interpreter segment.
@@ -849,7 +853,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")
@@ -858,6 +862,7 @@ 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=\"" result "\"")
                      (append (if proot
                                  (list (string-append "-DPROOT_PROGRAM=\""
                                                       proot "\""))
@@ -878,10 +883,9 @@ last resort for relocation."
 
           (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/"))))
+                         (append (find-input-files "bin")
+                                 (find-input-files "sbin")
+                                 (find-input-files "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

Message #35 received at 39807 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Eric Bavier <bavier <at> posteo.net>
Cc: 39807 <at> debbugs.gnu.org
Subject: Re: [bug#39807] [PATCH] guix: pack: Only wrap executable files.
Date: Wed, 21 Oct 2020 17:35:26 +0200

Hi Eric,

Eric Bavier <bavier <at> posteo.net> skribis:

> On Wed, 2020-10-21 at 11:07 +0200, Ludovic Courtès wrote:
>> Hi Eric,
>> 
>> Instead of searching for an occurrence of ORIGINAL_STORE, can’t we use
>> the file name of the wrapper (as opposed to WRAPPED_PROGRAM) in the
>> index calculation?  Along these lines:
>
> Good idea, I hadn't considered that we know the destination of the
> wrapper in advance.  
>
> This works as long as we make sure "result" is in canonical form, e.g.
> no repeated separators, because /proc/self/exe is in canonical form:

Good point.

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

Thanks,
Ludo’.

Message #38 received at 39807 <at> debbugs.gnu.org (full text, mbox):

From: Eric Bavier <bavier <at> posteo.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 39807 <at> debbugs.gnu.org
Subject: Re: [bug#39807] [PATCH] guix: pack: Only wrap executable files.
Date: Wed, 21 Oct 2020 11:21:57 -0500

On Wed, 2020-10-21 at 17:35 +0200, Ludovic Courtès wrote:
> Hi Eric,
> 
> Eric Bavier <bavier <at> 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?

`~Eric

Message #41 received at 39807 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Eric Bavier <bavier <at> posteo.net>
Cc: 39807 <at> debbugs.gnu.org
Subject: Re: [bug#39807] [PATCH] guix: pack: Only wrap executable files.
Date: Wed, 21 Oct 2020 23:31:13 +0200

Hi,

Eric Bavier <bavier <at> posteo.net> skribis:

> On Wed, 2020-10-21 at 17:35 +0200, Ludovic Courtès wrote:
>> Hi Eric,
>> 
>> Eric Bavier <bavier <at> 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?

Oh you’re right, sorry.

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?

Thanks,
Ludo’.

Message #44 received at 39807 <at> debbugs.gnu.org (full text, mbox):

From: Eric Bavier <bavier <at> posteo.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 39807 <at> debbugs.gnu.org
Subject: Re: [bug#39807] [PATCH] guix: pack: Only wrap executable files.
Date: Wed, 21 Oct 2020 18:51:31 -0500

[Message part 1 (text/plain, inline)]

On Wed, 2020-10-21 at 23:31 +0200, Ludovic Courtès wrote:
> Hi,
> 
> Eric Bavier <bavier <at> posteo.net> skribis:
> 
> > On Wed, 2020-10-21 at 17:35 +0200, Ludovic Courtès wrote:
> > > Hi Eric,
> > > 
> > > Eric Bavier <bavier <at> 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

[0001-guix-pack-Fix-offset-calculation-for-store-directory.patch (text/x-patch, attachment)]

[0002-guix-pack-Only-wrap-executable-files.patch (text/x-patch, attachment)]

Message #47 received at 39807 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Eric Bavier <bavier <at> posteo.net>
Cc: 39807 <at> debbugs.gnu.org
Subject: Re: [bug#39807] [PATCH] guix: pack: Only wrap executable files.
Date: Fri, 23 Oct 2020 12:48:48 +0200

Hi Eric,

Eric Bavier <bavier <at> posteo.net> skribis:

> From 7caaea0b21e5b97836b1a40a44efd2f38dbab7ae Mon Sep 17 00:00:00 2001
> From: Eric Bavier <bavier <at> 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.

LGTM!  Perhaps add a link to this discussion in the commit log.

> From 85bd962e929924b016a85d3a0b3dff434ebe8de3 Mon Sep 17 00:00:00 2001
> From: Eric Bavier <bavier <at> 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.

LGTM too!

Thanks,
Ludo’.

Message #52 received at 39807-done <at> debbugs.gnu.org (full text, mbox):

From: Eric Bavier <bavier <at> posteo.net>
To: 39807-done <at> debbugs.gnu.org
Subject: Re: [bug#39807] [PATCH] guix: pack: Only wrap executable files.
Date: Fri, 30 Oct 2020 10:13:41 -0500

[Message part 1 (text/plain, inline)]

On Fri, 2020-10-23 at 12:48 +0200, Ludovic Courtès wrote:
> Hi Eric,
> 
> Eric Bavier <bavier <at> posteo.net> skribis:
> 
> > From 7caaea0b21e5b97836b1a40a44efd2f38dbab7ae Mon Sep 17 00:00:00 2001
> > From: Eric Bavier <bavier <at> 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.
> 
> LGTM!  Perhaps add a link to this discussion in the commit log.
> 
> > From 85bd962e929924b016a85d3a0b3dff434ebe8de3 Mon Sep 17 00:00:00 2001
> > From: Eric Bavier <bavier <at> 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.
> 
> LGTM too!
> 
> Thanks,
> Ludo’.

Pushed in a73896425e92e5162766afdf042748b18f2462af and
4184998c70f9c4af101feb28cc19c5550abffcec after some small changes to
the test to more faithfully mimic the failure case that was causing
wrapped git commands to fail.

`~Eric

[pack-test-better.patch (text/x-patch, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.