[bug#39807] guix: pack: Only wrap executable files.
diff mbox series

Message ID 20200227043604.13102-1-bavier@posteo.net
State New
Headers show
Series
  • [bug#39807] guix: pack: Only wrap executable files.
Related show

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job

Commit Message

Eric Bavier Feb. 27, 2020, 4:36 a.m. UTC
From: Eric Bavier <bavier@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(-)

Comments

Ludovic Courtès March 6, 2020, 11:16 a.m. UTC | #1
Hi,

Eric Bavier <bavier@posteo.net> skribis:

> From: Eric Bavier <bavier@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’.
Ludovic Courtès March 24, 2020, 5:51 p.m. UTC | #2
Ping!  :-)

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

> Hi,
>
> Eric Bavier <bavier@posteo.net> skribis:
>
>> From: Eric Bavier <bavier@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’.
Eric Bavier March 27, 2020, 2:29 a.m. UTC | #3
On 06.03.2020 05:16, Ludovic Courtès wrote:
> Hi,
> 
> Eric Bavier <bavier@posteo.net> skribis:
> 
>> From: Eric Bavier <bavier@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)!
Ludovic Courtès March 29, 2020, 2:39 p.m. UTC | #4
Hi Eric,

Eric Bavier <bavier@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’.

Patch
diff mbox series

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@fastmail.net>
 ;;; Copyright © 2018 Chris Marusich <cmmarusich@gmail.com>
 ;;; Copyright © 2018 Efraim Flashner <efraim@flashner.co.il>
+;;; Copyright © 2020 Eric Bavier <bavier@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)