[PATCH] guix: pack: Only wrap executable files.

DoneSubmitted by Eric Bavier.
Details
2 participants
  • Eric Bavier
  • Ludovic Courtès
Owner
unassigned
Severity
normal
E
E
Eric Bavier wrote on 27 Feb 2020 05:36
(address . guix-patches@gnu.org)(name . Eric Bavier)(address . bavier@member.fsf.org)
20200227043604.13102-1-bavier@posteo.net
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 shellscripts in libexec would try to source other shell files that had beenwrapped and were no longer a valid shell file.
I feel like a test should be added to tests/guix-pack-relocatable.sh, butI'm not sure how to do that while keeping the test lightweight. Suggestionswelcome.
Cheers,`~Eric

* guix/scripts/pack.scm (wrapped-package)<build>: Build wrappers forexecutable files and symlink others.--- guix/scripts/pack.scm | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)
Toggle diff (62 lines)diff --git a/guix/scripts/pack.scm b/guix/scripts/pack.scmindex 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)-- 2.25.1
L
L
Ludovic Courtès wrote on 6 Mar 2020 12:16
(name . Eric Bavier)(address . bavier@posteo.net)
87d09pohrj.fsf@gnu.org
Hi,
Eric Bavier <bavier@posteo.net> skribis:
Toggle quote (10 lines)> 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!
Toggle quote (4 lines)> 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 couldhave a manifest containing a ‘computed-file’ with a file that shouldn’tbe wrapped, and then you could ensure that’s indeed the case. Or youcould try with ‘git-minimal’ or some other package that exhibits theproblem?
Toggle quote (3 lines)> * guix/scripts/pack.scm (wrapped-package)<build>: Build wrappers for> executable files and symlink others.
[...]
Toggle quote (6 lines)> - (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’.
L
L
Ludovic Courtès wrote on 24 Mar 2020 18:51
(name . Eric Bavier)(address . bavier@posteo.net)
87h7ydljyv.fsf@gnu.org
Ping! :-)
Ludovic Courtès <ludo@gnu.org> skribis:
Toggle quote (42 lines)> 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’.
E
E
Eric Bavier wrote on 27 Mar 2020 03:29
(address . 39807@debbugs.gnu.org)
9e3d0e2f06b5d7994d35d10d687773ac@posteo.net
On 06.03.2020 05:16, Ludovic Courtès wrote:
Toggle quote (19 lines)> 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.
Toggle quote (16 lines)> >> * 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.
Toggle quote (2 lines)> Otherwise LGTM, thanks!
Thanks for review (and ping)!
-- `~Eric
E
E
Eric Bavier wrote on 27 Mar 2020 03:53
(name . 39807)(address . 39807@debbugs.gnu.org)
9929712b169123d7f35169919dfaaa9c@posteo.net
Latest patch attached.-- `~Eric
L
L
Ludovic Courtès wrote on 29 Mar 2020 16:39
(name . Eric Bavier)(address . bavier@posteo.net)(address . 39807@debbugs.gnu.org)
87a73z1azh.fsf@gnu.org
Hi Eric,
Eric Bavier <bavier@posteo.net> skribis:
Toggle quote (2 lines)> On 06.03.2020 05:16, Ludovic Courtès wrote:
[...]
Toggle quote (22 lines)>>> 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.
Toggle quote (16 lines)>>> - (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 nicerthan both of these.
Thanks,Ludo’.
E
E
Eric Bavier wrote on 27 Jul 2020 23:42
(address . 39807@debbugs.gnu.org)
3ace941f920d87da65de6e4fdc16add5b9725434.camel@posteo.net
Patch rebased on latest master attached. The new test implements asmall 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 suchfile or directory.
Could someone more familiar with user namespaces, etc help me workthis out?
`~Eric
From b31e63c3668e5163eba40f7815ccae19cb2af236 Mon Sep 17 00:00:00 2001From: Eric Bavier <bavier@member.fsf.org>Date: Mon, 24 Feb 2020 23:47:02 -0600Subject: [PATCH] guix: pack: Only wrap executable files.
* guix/scripts/pack.scm (wrapped-package)<build>: Build wrappers forexecutable files and symlink others.* tests/guix-pack-relocatable.sh: Test relocatable example of mixedexecutable and non-executable files.--- guix/scripts/pack.scm | 31 ++++++++++++++++++++++--------- tests/guix-pack-relocatable.sh | 31 +++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 9 deletions(-)
Toggle diff (121 lines)diff --git a/guix/scripts/pack.scm b/guix/scripts/pack.scmindex 5fb6aaae0c..1b9609753d 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. ;;;@@ -747,11 +748,12 @@ 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) (ice-9 binary-ports) (ice-9 ftw) (ice-9 match)+ (ice-9 receive) (srfi srfi-1) (rnrs bytevectors)) @@ -847,16 +849,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.shindex 52d7212594..2ed497027d 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. #@@ -99,6 +100,7 @@ case "`uname -m`" in 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"/* ;;@@ -111,8 +113,37 @@ esac tarball="`guix pack -R -S /share=share groff:doc`" (cd "$test_directory"; tar xvf "$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"+ #~(format #t "~d~%" (load "../libexec/msg"))+ #:guile %bootstrap-guile))+ ("libexec/msg"+ ,(plain-file "msg" "42"))))))))+EOF+tarball="`guix pack $relocatable_option -S /opt= -m $test_directory/manifest.scm`"+(cd "$test_directory"; tar xvf "$tarball")+do_test='"$test_directory/opt/bin/hello" >"$test_directory/output"'+if unshare -r true # Are user namespaces supported?+then+ unshare -mrf sh -c 'mount -t tmpfs none "$STORE_PARENT"; '"$do_test"+else+ ( $do_test )+fi+test "`cat $test_directory/output`" = "42"-- 2.27.0
E
E
Eric Bavier wrote on 21 Oct 2020 07:09
(address . 39807@debbugs.gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
83416910901e77b537b64de392ee02598ccd72ff.camel@posteo.net
On Mon, 2020-07-27 at 16:42 -0500, Eric Bavier wrote:
Toggle quote (8 lines)> 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 mostlywith 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 theabove error:
mount("/tmp/pack-dir/gnu/store/80", "/tmp/guix-exec-YMr7WJ//gnu/store", 0x4810a7, MS_RDONLY|MS_BIND|MS_REC, NULL) = -1ENOENT (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. Sofar 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 testpass 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 lengthsare the same, right? It seems like we maybe want to ignore @WRAPPED_PROGRAM@ and use only /proc/self/exe andoriginal_store to find the relocated store directory? A regex search might be too costly. We could use strstr tosearch for the first occurrence of original_store, if we don't mind assuming that most people will probably not unpackinto $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
L
L
Ludovic Courtès wrote on 21 Oct 2020 11:07
(name . Eric Bavier)(address . bavier@posteo.net)(address . 39807@debbugs.gnu.org)
87wnzk7xs0.fsf@gnu.org
Hi Eric,
Eric Bavier <bavier@posteo.net> skribis:
Toggle quote (25 lines)> 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 usethe file name of the wrapper (as opposed to WRAPPED_PROGRAM) in theindex calculation? Along these lines:
Toggle diff (25 lines)diff --git a/gnu/packages/aux-files/run-in-namespace.c b/gnu/packages/aux-files/run-in-namespace.cindex 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.scmindex 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 "\""))
Thanks, Ludo’.
E
E
Eric Bavier wrote on 21 Oct 2020 17:12
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 39807@debbugs.gnu.org)
69f5446e2accd24cd1de9e3964373ab525b0875d.camel@posteo.net
Hi Ludo,
On Wed, 2020-10-21 at 11:07 +0200, Ludovic Courtès wrote:
Toggle quote (6 lines)> 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 thewrapper 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:
Toggle diff (59 lines)diff --git a/gnu/packages/aux-files/run-in-namespace.c b/gnu/packages/aux-files/run-in-namespace.cindex 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.scmindex 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
L
L
Ludovic Courtès wrote on 21 Oct 2020 17:35
(name . Eric Bavier)(address . bavier@posteo.net)(address . 39807@debbugs.gnu.org)
87pn5b383l.fsf@gnu.org
Hi Eric,
Eric Bavier <bavier@posteo.net> skribis:
Toggle quote (13 lines)> 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.
Toggle quote (5 lines)> (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’tneed ‘find-input-files’ and related changes.
Thanks,Ludo’.
E
E
Eric Bavier wrote on 21 Oct 2020 18:21
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 39807@debbugs.gnu.org)
48f280214fca0f8bd17aeff46c42ee76a04c90f2.camel@posteo.net
On Wed, 2020-10-21 at 17:35 +0200, Ludovic Courtès wrote:
Toggle quote (13 lines)> 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 notalready exist. We could create a dummy file at result, then callcanonicalize-path? but that seems clumsier than forming a canonicalname in the first place?
`~Eric
L
L
Ludovic Courtès wrote on 21 Oct 2020 23:31
(name . Eric Bavier)(address . bavier@posteo.net)(address . 39807@debbugs.gnu.org)
87mu0fxo4e.fsf@gnu.org
Hi,
Eric Bavier <bavier@posteo.net> skribis:
Toggle quote (19 lines)> 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?
Oh you’re right, sorry.
The patch you sent has ‘find-files’ use ‘stat’ instead of the trailingslash. It introduces a difference: by using ‘stat’ all the way, itfollows all symlinks, not just the higher-level ones. I don’t know ifthis could have undesired implications, like wrapping the same filetwice 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’.
E
E
Eric Bavier wrote on 22 Oct 2020 01:51
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 39807@debbugs.gnu.org)
f4341fdb29c69687372a53bc1b29314f24fecfc3.camel@posteo.net
On Wed, 2020-10-21 at 23:31 +0200, Ludovic Courtès wrote:
Toggle quote (35 lines)> 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, Ithink.
Following are current two patches. I can add copyright to run-in-namespace.c if you think it's needed.
`~Eric
From 7caaea0b21e5b97836b1a40a44efd2f38dbab7ae Mon Sep 17 00:00:00 2001From: Eric Bavier <bavier@member.fsf.org>Date: Wed, 21 Oct 2020 18:33:52 -0500Subject: [PATCH 1/2] guix: pack: Fix offset calculation for store directory mount point.
Fixes wrapping of non-package things, where the target store directory maydiffer in length from the original.
* guix/scripts/pack.scm (wrapped-package)<build-wrapper>: DefineWRAPPER_PROGRAM macro with wrapper's file name.* gnu/packages/aux-files/run-in-namespace.c (main): Offset index by len ofthat file name.--- gnu/packages/aux-files/run-in-namespace.c | 2 +- guix/scripts/pack.scm | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-)
Toggle diff (46 lines)diff --git a/gnu/packages/aux-files/run-in-namespace.c b/gnu/packages/aux-files/run-in-namespace.cindex 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.scmindex 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
From 85bd962e929924b016a85d3a0b3dff434ebe8de3 Mon Sep 17 00:00:00 2001From: Eric Bavier <bavier@member.fsf.org>Date: Mon, 24 Feb 2020 23:47:02 -0600Subject: [PATCH 2/2] guix: pack: Only wrap executable files.
* guix/scripts/pack.scm (wrapped-package)<build>: Build wrappers forexecutable files and symlink others.* tests/guix-pack-relocatable.sh: Test relocatable example of mixedexecutable and non-executable files.--- guix/scripts/pack.scm | 30 +++++++++++++++++++++--------- tests/guix-pack-relocatable.sh | 25 +++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 9 deletions(-)
Toggle diff (108 lines)diff --git a/guix/scripts/pack.scm b/guix/scripts/pack.scmindex 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.shindex 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
L
L
Ludovic Courtès wrote on 23 Oct 2020 12:48
(name . Eric Bavier)(address . bavier@posteo.net)(address . 39807@debbugs.gnu.org)
87pn59qktr.fsf@gnu.org
Hi Eric,
Eric Bavier <bavier@posteo.net> skribis:
Toggle quote (14 lines)> 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.
LGTM! Perhaps add a link to this discussion in the commit log.
Toggle quote (10 lines)> 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.
LGTM too!
Thanks,Ludo’.
E
E
Eric Bavier wrote on 30 Oct 2020 16:13
(address . 39807-done@debbugs.gnu.org)
66edab14afe77962ab4cd033a08bdea570e61000.camel@posteo.net
On Fri, 2020-10-23 at 12:48 +0200, Ludovic Courtès wrote:
Toggle quote (35 lines)> Hi Eric,> > Eric Bavier <bavier@posteo.net> skribis:> > > 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.> > 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@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 and4184998c70f9c4af101feb28cc19c5550abffcec after some small changes tothe test to more faithfully mimic the failure case that was causingwrapped git commands to fail.
`~Eric
Toggle diff (40 lines)diff --git a/tests/guix-pack-relocatable.sh b/tests/guix-pack-relocatable.shindex 5ac8cdc718..0255075fb1 100644--- a/tests/guix-pack-relocatable.sh+++ b/tests/guix-pack-relocatable.sh@@ -145,7 +145,7 @@ 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)+(use-modules (guix) (guix profiles) (guix search-paths) (gnu packages bootstrap)) (manifest (list (manifest-entry@@ -154,12 +154,22 @@ cat >"$test_directory/manifest.scm" <<'EOF' `(("bin/hello" ,(program-file "hello"- #~(begin (display (load "../libexec/msg"))(newline))+ #~(begin+ (add-to-load-path (getenv "HELLO_EXEC_PATH"))+ (display (load-from-path "msg"))(newline)) #:guile %bootstrap-guile))- ("libexec/msg"- ,(plain-file "msg" "42"))))))))+ ("libexec/hello/msg"+ ,(plain-file "msg" "42")))))+ (search-paths+ (list (search-path-specification+ (variable "HELLO_EXEC_PATH")+ (files '("libexec/hello"))+ (separator #f))))))) 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"+( export GUIX_PROFILE=$test_directory/opt+ . $GUIX_PROFILE/etc/profile+ run_without_store "$test_directory/opt/bin/hello" > "$test_directory/output" )+cat "$test_directory/output" test "`cat $test_directory/output`" = "42"
Closed
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send email to 39807@debbugs.gnu.org