bug#60566: guix container with FHS emulation and env vars

bug#60566: guix container with FHS emulation and env vars

[jman via Bug reports for GNU Guix]

Hello,

When emulating a FHS I observe that env vars seems to be not preserved. Example,
the following command will not preserve $PATH:

    guix shell --container --emulate-fhs --preserve='^PATH$'

When creating a container *without* emulating a FHS, env vars are available,
example:

    guix shell --container --preserve='^PATH$'

Pastebin log of a sample of this behaviour:
https://paste.sr.ht/~jman/65e7f96c445504e11f55595b237280e0c1e3ad34

ref: https://lists.gnu.org/archive/html/help-guix/2023-01/msg00002.html

Thanks for an opinion on this

bug#60566: [PATCH] environment: Fix '--emulate-fhs' option overriding $PATH.

[John Kehayias via Bug reports for GNU Guix]

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

On Wed, Jan 04, 2023 at 11:33 PM, jman wrote:

> Hello,
>
> When emulating a FHS I observe that env vars seems to be not preserved. Example,
> the following command will not preserve $PATH:
>
>     guix shell --container --emulate-fhs --preserve='^PATH$'
>
> When creating a container *without* emulating a FHS, env vars are available,
> example:
>
>     guix shell --container --preserve='^PATH$'
>
> Pastebin log of a sample of this behaviour:
> https://paste.sr.ht/~jman/65e7f96c445504e11f55595b237280e0c1e3ad34
>
> ref: https://lists.gnu.org/archive/html/help-guix/2023-01/msg00002.html
>
> Thanks for an opinion on this

Thanks for reporting, I can confirm this behavior.

Here is a patch for this where the FHS directories are added to the
current value of $PATH. I believe this should in general be fine since
this is the last step before actually calling the command given to 'guix
shell' and thus $PATH has been set or preserved as needed already.

CC'ing Ludo as most familiar with this code. Anything we should be aware
of here? This change to $PATH in the first place wasn't strictly needed
('guix shell' already has the profile bin directory) but I thought made
sense to make it look most like FHS.

Thanks!
John


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-environment-Fix-emulate-fhs-option-overriding-PATH.patch --]
[-- Type: text/x-patch; name=0001-environment-Fix-emulate-fhs-option-overriding-PATH.patch, Size: 1587 bytes --]

From 57cdc3a8f9c6451aaf17f1fafae0bcf29faeea03 Mon Sep 17 00:00:00 2001
From: John Kehayias <john.kehayias@protonmail.com>
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.
---
 guix/scripts/environment.scm | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm
index c7fd8fd340..20fa5850c4 100644
--- a/guix/scripts/environment.scm
+++ b/guix/scripts/environment.scm
@@ -475,10 +475,11 @@ (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:"
+                                         (getenv "PATH")))
            (invoke "ldconfig" "-X"))
          (apply execlp program program args))
        (lambda _
-- 
2.38.1

bug#60566: [PATCH] environment: Fix '--emulate-fhs' option overriding $PATH.

[Ludovic Courtès]

Hi,

John Kehayias <john.kehayias@protonmail.com> skribis:

> From 57cdc3a8f9c6451aaf17f1fafae0bcf29faeea03 Mon Sep 17 00:00:00 2001
> From: John Kehayias <john.kehayias@protonmail.com>
> 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.
> ---
>  guix/scripts/environment.scm | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm
> index c7fd8fd340..20fa5850c4 100644
> --- a/guix/scripts/environment.scm
> +++ b/guix/scripts/environment.scm
> @@ -475,10 +475,11 @@ (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:"
> +                                         (getenv "PATH")))

To be safe, you need to account for (getenv "PATH") returning #f, and
not add a trailing colon in that case.

Other than that, I agree this is a valid change because that would be
consistent with:

--8<---------------cut here---------------start------------->8---
$ PATH=/foo $(type -P guix) shell -E ^PATH$ -C coreutils -- env |grep ^PATH
PATH=/gnu/store/pfl0lyqbs557khv7rw90bzp24qp2lqsn-profile/bin:/foo
--8<---------------cut here---------------end--------------->8---

Perhaps you can add a line to test it in
‘tests/guix-environment-container.sh’?

Thanks,
Ludo’.

bug#60566: [PATCH] environment: Fix '--emulate-fhs' option overriding $PATH.

[John Kehayias via Bug reports for GNU Guix]

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

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-environment-Fix-emulate-fhs-option-overriding-PATH.patch --]
[-- Type: text/x-patch; name=0001-environment-Fix-emulate-fhs-option-overriding-PATH.patch, Size: 3838 bytes --]

From beb6f9255fc62fe52e237f82c7e953a21b7f82f4 Mon Sep 17 00:00:00 2001
From: John Kehayias <john.kehayias@protonmail.com>
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 <davet@gnu.org>
 ;;; Copyright © 2015-2022 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2018 Mike Gerwitz <mtg@gnu.org>
-;;; Copyright © 2022 John Kehayias <john.kehayias@protonmail.com>
+;;; Copyright © 2022, 2023 John Kehayias <john.kehayias@protonmail.com>
 ;;;
 ;;; 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 <davet@gnu.org>
-# Copyright © 2022 John Kehayias <john.kehayias@protonmail.com>
+# Copyright © 2022, 2023 John Kehayias <john.kehayias@protonmail.com>
 #
 # 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

bug#60566: [PATCH] environment: Fix '--emulate-fhs' option overriding $PATH.

[Ludovic Courtès]

Hi John,

John Kehayias <john.kehayias@protonmail.com> skribis:

> From beb6f9255fc62fe52e237f82c7e953a21b7f82f4 Mon Sep 17 00:00:00 2001
> From: John Kehayias <john.kehayias@protonmail.com>
> 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.

[...]

> -           (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")))))

Remember that ‘when’ returns *unspecified* when the condition is false,
so you’d get a type error here when PATH is undefined.

Instead write: (if (getenv "PATH") … "").

> +# 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))'

Even (exit (string=? (getenv "PATH") "/bin:/usr/bin:/sbin:/usr/sbin")).

> +# 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))'

It might be slightly more concise with ‘env’:

  PATH=/foo $(type -P guix) shell -E ^PATH$ -C coreutils -- env |grep ^PATH=.*:/foo

(I think ‘--bootstrap’ doesn’t buy us much here because we have to
download/build ‘glibc-for-fhs’ anyway.  ‘--bootstrap’ and
‘guile-bootstrap’ are particularly useful for testse that can run
without network access and without building tons of stuff, as in
‘tests/guix-environment.sh’ for instance.)

Otherwise LGTM, thanks!

Ludo’.

bug#60566: [PATCH] environment: Fix '--emulate-fhs' option overriding $PATH.

[John Kehayias via Bug reports for GNU Guix]

Hi Ludo’,


On Sat, Jan 14, 2023 at 03:41 PM, Ludovic Courtès wrote:

> Hi John,
>
> John Kehayias <john.kehayias@protonmail.com> skribis:
>
> [...]
>
>> -           (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")))))
>
> Remember that ‘when’ returns *unspecified* when the condition is false,
> so you’d get a type error here when PATH is undefined.
>
> Instead write: (if (getenv "PATH") … "").
>

Ah yes, my Common Lisp showing through and relying on nil instead. Fixed and thanks!

>> +# 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))'
>
> Even (exit (string=? (getenv "PATH") "/bin:/usr/bin:/sbin:/usr/sbin")).
>

With this patch PATH now gets the FHS directories in addition to what it normally has (like the profile's bin directory). While slightly redundant, this seems to be better than clobbering it. Anyway, so we can't check that the PATH is completely equal here.

>> +# 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))'
>
> It might be slightly more concise with ‘env’:
>
>   PATH=/foo $(type -P guix) shell -E ^PATH$ -C coreutils -- env |grep ^PATH=.*:/foo
>
> (I think ‘--bootstrap’ doesn’t buy us much here because we have to
> download/build ‘glibc-for-fhs’ anyway.  ‘--bootstrap’ and
> ‘guile-bootstrap’ are particularly useful for testse that can run
> without network access and without building tons of stuff, as in
> ‘tests/guix-environment.sh’ for instance.)
>

Ah, thanks, that is nicer if we can just use coreutils. I rewrote the previous test to use that as well. Probably some other tests here could use that simplification, but outside of the scope here.

(Side note that 'type' in zsh works differently, one could use 'whence' there or even the built-in 'which'. For the tests we are running with bash or bash compliant here, so it is not a problem.)

> Otherwise LGTM, thanks!
>
> Ludo’.

Thanks again for your careful review! Pushed as 3bfbfa2946aebb7f68c8027ae80f272f6915c94f and closing this issue.

Thanks also for jman for reporting.

John