Hi Ludo’, On Sat, Jan 07, 2023 at 12:03 AM, Ludovic Courtès wrote:
> To be safe, you need to account for (getenv "PATH") returning #f, and > not add a trailing colon in that case. > Ah, right. I think this would only happen if somehow unsetting PATH and preserving it? As 'guix shell' already sets PATH. Anyway, better to be safe here. I tweaked this, though not sure if there is a more elegant way to construct the string than what I did (suggestions always welcome!). > Other than that, I agree this is a valid change because that would be > consistent with: > > $ PATH=/foo $(type -P guix) shell -E ^PATH$ -C coreutils -- env |grep ^PATH > PATH=/gnu/store/pfl0lyqbs557khv7rw90bzp24qp2lqsn-profile/bin:/foo > > Perhaps you can add a line to test it in > ‘tests/guix-environment-container.sh’? > I added two tests while I was at it: one to check that PATH has the FHS modification in the container and a second for this particular bug. For the second one I just used a test string added to PATH as the entire thing will differ already from inside/outside the container, FHS or not. I checked the tests pass here and removing '--emulate-fhs' causes the first to fail while removing the '--preserve' argument causes the second test to fail. I could separate the first out as a separate commit if that makes more sense, but I do think the current behavior is just wrong in overwriting all of PATH when '--emuate-fhs' is given. New version attached, thanks for the suggestions! John
From beb6f9255fc62fe52e237f82c7e953a21b7f82f4 Mon Sep 17 00:00:00 2001 From: John Kehayias <[email protected]> Date: Thu, 5 Jan 2023 16:06:19 -0500 Subject: [PATCH] * environment: Fix '--emulate-fhs' option overriding $PATH. Fixes <https://issues.guix.gnu.org/60566> where even if "--preserve='^PATH$'" was passed to 'guix shell' it would be replaced by just the FHS directories when '--emulate-fhs' was also set. * gnu/scripts/environment.scm (launch-environment): Add the FHS directories to $PATH rather than overriding $PATH completely. * tests/guix-environment-container.sh: Test that FHS directories are in $PATH in the container and that $PATH can be preserved. --- guix/scripts/environment.scm | 8 +++++--- tests/guix-environment-container.sh | 18 +++++++++++++++++- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm index c7fd8fd340..19ba2f7bee 100644 --- a/guix/scripts/environment.scm +++ b/guix/scripts/environment.scm @@ -2,7 +2,7 @@ ;;; Copyright © 2014, 2015, 2018 David Thompson <[email protected]> ;;; Copyright © 2015-2022 Ludovic Courtès <[email protected]> ;;; Copyright © 2018 Mike Gerwitz <[email protected]> -;;; Copyright © 2022 John Kehayias <[email protected]> +;;; Copyright © 2022, 2023 John Kehayias <[email protected]> ;;; ;;; This file is part of GNU Guix. ;;; @@ -475,10 +475,12 @@ (define* (launch-environment command profile manifest (catch 'system-error (lambda () (when emulate-fhs? - ;; When running in a container with EMULATE-FHS?, override $PATH + ;; When running in a container with EMULATE-FHS?, augment $PATH ;; (optional, but to better match FHS expectations), and generate ;; /etc/ld.so.cache. - (setenv "PATH" "/bin:/usr/bin:/sbin:/usr/sbin") + (setenv "PATH" (string-append "/bin:/usr/bin:/sbin:/usr/sbin" + (when (getenv "PATH") + (string-append ":" (getenv "PATH"))))) (invoke "ldconfig" "-X")) (apply execlp program program args)) (lambda _ diff --git a/tests/guix-environment-container.sh b/tests/guix-environment-container.sh index 0306fc1744..198352c1e2 100644 --- a/tests/guix-environment-container.sh +++ b/tests/guix-environment-container.sh @@ -1,6 +1,6 @@ # GNU Guix --- Functional package management for GNU # Copyright © 2015 David Thompson <[email protected]> -# Copyright © 2022 John Kehayias <[email protected]> +# Copyright © 2022, 2023 John Kehayias <[email protected]> # # This file is part of GNU Guix. # @@ -242,6 +242,22 @@ guix shell -CF --bootstrap guile-bootstrap glibc \ 0 1))' +# Test that $PATH inside the container has FHS directories. +guix shell -CF --bootstrap guile-bootstrap \ + -- guile -c '(exit (if (string-contains (getenv "PATH") + "/bin:/usr/bin:/sbin:/usr/sbin") + 0 + 1))' + +# Make sure '--preserve' is honored for $PATH, which the '--emulate-fhs' +# option will modify. We can't (easily) check the whole $PATH as it will +# differ inside and outside the container, so just check for an added string. +PATH=this-is-a-test:$PATH guix shell -CF --bootstrap guile-bootstrap -E PATH \ + -- guile -c '(exit (if (string-contains (getenv "PATH") + "this-is-a-test") + 0 + 1))' + # '--symlink' works. echo "TESTING SYMLINK IN CONTAINER" guix shell --bootstrap guile-bootstrap --container \ -- 2.38.1
