bug#51466: guix shell --check reports missing PKG_CONFIG_PATH on Debian bookworm

bug#51466: guix shell --check reports missing PKG_CONFIG_PATH on Debian bookworm

[Vagrant Cascadian]

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

This is a recently installed Debian bookworm system, initially using the
package from debian experimental (guix 1.3.0-3, built with guile 3.0),
and "guix pull" up to a recent guix master:

vagrant@vagranttdgxbookworm:~$ guix describe
Generation 7    Oct 28 2021 11:04:25    (current)
  guix 0e6470b
    repository URL: /home/vagrant/src/guix
    branch: master
    commit: 0e6470b47f00470c213fbf20bddc5bcf1e2f8e2a


Most things seem to work fine, but noticed an oddity with guix shell:

vagrant@vagranttdgxbookworm:~$ guix shell --pure --check --development guix guix git less

guix shell: checking the environment variables visible from shell
'/bin/bash'...
guix shell: warning: variable 'PKG_CONFIG_PATH' is missing from shell
environment
hint: One or more environment variables have a different value in the
shell than the one we set.  This means that you may find yourself
running code in an environment different from the one you asked Guix to
prepare.

This usually indicates that your shell startup files are unexpectedly
modifying those environment variables.  For example, if you
are using Bash, make sure that environment variables are set or modified
in `~/.bash_profile' and _not_ in `~/.bashrc'.  For more
information on Bash startup files, run:

     info "(bash) Bash Startup Files"

Alternatively, you can avoid the problem by passing the `--container' or
`-C' option.  That will give you a fully isolated
environment running in a "container", immune to the issue described
above.

vagrant@vagranttdgxbookworm:~$ guix shell --pure --development guix guix git less

vagrant@vagranttdgxbookworm:~$ echo $PKG_CONFIG_PATH
/gnu/store/9vk59alg27y0cp1za91nfdjiy718cn1f-profile/lib/pkgconfig


So, --check seems to think the environment variable is missing but
running without --check the variable is defined...

I don't see anything obviously relevent in /etc/profile/ or
/etc/profile.d/guix.sh or /etc/profile.d/bash-completions.sh or
~/.profile or ~/.bashrc ... just used the defaults that shipped in
Debian.


live well,
  vagrant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

bug#51466: guix shell --check reports missing PKG_CONFIG_PATH on Debian bookworm

[Ludovic Courtès]

Hi!

Vagrant Cascadian <vagrant@debian.org> skribis:

> Most things seem to work fine, but noticed an oddity with guix shell:
>
> vagrant@vagranttdgxbookworm:~$ guix shell --pure --check --development guix guix git less
>
> guix shell: checking the environment variables visible from shell
> '/bin/bash'...
> guix shell: warning: variable 'PKG_CONFIG_PATH' is missing from shell

[...]

> vagrant@vagranttdgxbookworm:~$ guix shell --pure --development guix guix git less
>
> vagrant@vagranttdgxbookworm:~$ echo $PKG_CONFIG_PATH
> /gnu/store/9vk59alg27y0cp1za91nfdjiy718cn1f-profile/lib/pkgconfig

Notice that it doesn’t complain about any of the other environment
variables (there are 10 of them according to ‘guix shell -D guix
--search-paths|wc -l’).

If you look at ‘child-shell-environment’ in (guix scripts environment),
it runs this in the child shell:

  env || /usr/bin/env || set; echo GUIX-CHECK-DONE; read x; exit

If the shell prints non-newline-terminated stuff before the output of
‘env’, the first line of ‘env’ would be swallowed by the parser below.

Could you run:

  strace -o log -s 500 guix shell --check -D guix

to see exactly what ‘guix shell’ reads?

If there’s nothing obvious, you know the story: we can always add ‘pk’
calls in ‘child-shell-environment’.  :-)

Thanks,
Ludo’.

bug#51466: guix shell --check reports missing PKG_CONFIG_PATH on Debian bookworm

[Vagrant Cascadian]

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

On 2021-10-29, Ludovic Courtès wrote:
> Vagrant Cascadian <vagrant@debian.org> skribis:
>
>> Most things seem to work fine, but noticed an oddity with guix shell:
>>
>> vagrant@vagranttdgxbookworm:~$ guix shell --pure --check --development guix guix git less
>>
>> guix shell: checking the environment variables visible from shell
>> '/bin/bash'...
>> guix shell: warning: variable 'PKG_CONFIG_PATH' is missing from shell
>
> [...]
>
>> vagrant@vagranttdgxbookworm:~$ guix shell --pure --development guix guix git less
>>
>> vagrant@vagranttdgxbookworm:~$ echo $PKG_CONFIG_PATH
>> /gnu/store/9vk59alg27y0cp1za91nfdjiy718cn1f-profile/lib/pkgconfig
>
> Notice that it doesn’t complain about any of the other environment
> variables (there are 10 of them according to ‘guix shell -D guix
> --search-paths|wc -l’).
>
> If you look at ‘child-shell-environment’ in (guix scripts environment),
> it runs this in the child shell:
>
>   env || /usr/bin/env || set; echo GUIX-CHECK-DONE; read x; exit
>
> If the shell prints non-newline-terminated stuff before the output of
> ‘env’, the first line of ‘env’ would be swallowed by the parser below.
>
> Could you run:
>
>   strace -o log -s 500 guix shell --check -D guix
>
> to see exactly what ‘guix shell’ reads?

That showed nothing obvious to me; the log it spits out is about 3MB
(~320k compressed with zstd) I could attach if it is useful...

I did notice SHELL=/bin/bash, and tried an experiment:

  $ SHELL=/gnu/store/87kif0bpf0anwbsaw0jvg8fyciw4sz67-bash-5.0.16/bin/bash guix shell --check -D guix
  guix shell: checking the environment variables visible from shell
  '/gnu/store/87kif0bpf0anwbsaw0jvg8fyciw4sz67-bash-5.0.16/bin/bash'...
  guix shell: All is good!  The shell gets correct environment variables.

So, somehow the value of SHELL and/or the user's default shell is
triggering the issue?


live well,
  vagrant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

bug#51466: guix shell --check reports missing PKG_CONFIG_PATH on Debian bookworm

[Ludovic Courtès]

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

Howdy!

Vagrant Cascadian <vagrant@debian.org> skribis:

> On 2021-10-29, Ludovic Courtès wrote:

[...]

>> If you look at ‘child-shell-environment’ in (guix scripts environment),
>> it runs this in the child shell:
>>
>>   env || /usr/bin/env || set; echo GUIX-CHECK-DONE; read x; exit
>>
>> If the shell prints non-newline-terminated stuff before the output of
>> ‘env’, the first line of ‘env’ would be swallowed by the parser below.
>>
>> Could you run:
>>
>>   strace -o log -s 500 guix shell --check -D guix
>>
>> to see exactly what ‘guix shell’ reads?
>
> That showed nothing obvious to me; the log it spits out is about 3MB
> (~320k compressed with zstd) I could attach if it is useful...
>
> I did notice SHELL=/bin/bash, and tried an experiment:
>
>   $ SHELL=/gnu/store/87kif0bpf0anwbsaw0jvg8fyciw4sz67-bash-5.0.16/bin/bash guix shell --check -D guix
>   guix shell: checking the environment variables visible from shell
>   '/gnu/store/87kif0bpf0anwbsaw0jvg8fyciw4sz67-bash-5.0.16/bin/bash'...
>   guix shell: All is good!  The shell gets correct environment variables.
>
> So, somehow the value of SHELL and/or the user's default shell is
> triggering the issue?

Yes, ‘--check’ runs $SHELL.

To make sure, could you try with the attached patch to see exactly which
variables ‘guix shell’ “sees”, and whether it drops a line it shouldn’t
drop?

TIA,
Ludo’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 701 bytes --]

diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm
index cca0ad991b..7f3d3b9db8 100644
--- a/guix/scripts/environment.scm
+++ b/guix/scripts/environment.scm
@@ -452,10 +452,10 @@ (define lines
           ;; but it also truncates values anyway, so don't try to support it.
           (let ((index (string-index line #\=)))
             (if index
-                (vhash-cons (string-take line index)
+                (vhash-cons (pk 'variable line (string-take line index))
                             (string-drop line (+ 1 index))
                             table)
-                table)))
+                (pk 'dropped line table))))
         vlist-null
         lines))
 

bug#51466: [PATCH 0/1] environment: properly parse environment variables during --check

[Kevin Boulain]

The PS1=; for i in 1 2 3; do echo GUIX_FLUSH_$i; done; hack appears to work
for me but here is my alternate version. I don't think we have to hack our way
around the shell if we can dump the environment variables somewhere else.

I added a test but the potato machine I'm running that on is taking a very
long time to run the whole thing (I did run the test in isolation with and
without my patch), would you mind checking it works for you (and can we use
Bash there?).

Note I'm still not sure why we have env || /usr/bin/env (in case we mess with
the PATH I guess?) so I've kept it.

Comments welcome, I haven't coded much in Guile/Scheme.

Kevin Boulain (1):
  environment: properly parse environment variables during --check

 guix/scripts/environment.scm | 65 +++++++++++++++++++++++-------------
 tests/guix-environment.sh    | 16 +++++++++
 2 files changed, 57 insertions(+), 24 deletions(-)

bug#51466: [PATCH 1/1] environment: properly parse environment variables during --check

[Kevin Boulain]

Should solve https://issues.guix.gnu.org/51466.

This redirects the env command's output to a temporary file so that
there is no way to get it mixed with the shell's output (like PS1).
Additionnally:
 - Remove GUIX-CHECK-DONE and read: I don't think there is a need for
   them as discarding the output until the shell exits is enough to
   guarantee the environment has been dumped. Sadly, Linux doesn't
   report EOF but EIO when the shell exits and the pty gets closed hence
   the catch/throw.
 - Don't try to parse environment variables by splitting them on \n,
   instead use env's -0 option to separate environment variables with
   \0. This prevent incorrect parsing of some multi-line variables
   (e.g.: f() { echo "hello"; }; declare -fx f) but isn't standard.

* guix/scripts/environment.scm (child-shell-environment): make
environment variables parsing more robust.
* tests/guix-environment.sh: add a simple test that was reliably
failing on my machine.
---
 guix/scripts/environment.scm | 65 +++++++++++++++++++++++-------------
 tests/guix-environment.sh    | 16 +++++++++
 2 files changed, 57 insertions(+), 24 deletions(-)

diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm
index ec071402f4..4ff13c6bde 100644
--- a/guix/scripts/environment.scm
+++ b/guix/scripts/environment.scm
@@ -48,7 +48,7 @@ (define-module (guix scripts environment)
   #:autoload   (gnu packages bash) (bash)
   #:autoload   (gnu packages bootstrap) (bootstrap-executable %bootstrap-guile)
   #:use-module (ice-9 match)
-  #:autoload   (ice-9 rdelim) (read-line)
+  #:autoload   (ice-9 rdelim) (read-delimited read-line)
   #:use-module (ice-9 vlist)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-11)
@@ -413,16 +413,22 @@ (define (child-shell-environment shell profile manifest)
   "Create a child process, load PROFILE and MANIFEST, and then run SHELL in
 interactive mode in it.  Return a name/value vhash for all the variables shown
 by running 'set' in the shell."
-  (define-values (controller inferior)
-    (openpty))
-
-  (define script
-    ;; Script to obtain the list of environment variable values.  On a POSIX
-    ;; shell we can rely on 'set', but on fish we have to use 'env' (fish's
-    ;; 'set' truncates values and prints them in a different format.)
-    "env || /usr/bin/env || set; echo GUIX-CHECK-DONE; read x; exit\n")
-
   (define lines
+    (let* ((environment-port (mkstemp "/tmp/guix-enviroment-XXXXXX"))
+           (environment-file (port-filename environment-port))
+           ;; Script to obtain the list of environment variable values.  On a
+           ;; POSIX shell we can rely on 'set', but on fish we have to use 'env'
+           ;; (fish's 'set' truncates values and prints them in a different
+           ;; format.)
+           ;; We rely on env --null to mark the end of environment variables.
+           ;; This is expected to be safe because POSIX defines environment
+           ;; variables end with '\0' (but does't document the --null option).
+           ;; Some shells, like Bash, allow to export functions, which will span
+           ;; multiple lines and break any trivial parsing relying on '\n'.
+           (script (format #f "env --null > ~a || /usr/bin/env --null > ~a; exit\n"
+                           environment-file environment-file)))
+
+      (let-values (((controller inferior) (openpty)))
         (match (primitive-fork)
           (0
            (catch #t
@@ -436,26 +442,37 @@ (define lines
                (primitive-exit 127))))
           (pid
            (close-fdes inferior)
-       (let* ((port   (fdopen controller "r+l"))
-              (result (begin
+           (let ((port (fdopen controller "r+l")))
              (display script port)
+             (catch 'system-error
+               (lambda ()
+                 ;; We aren't interested in the output of the shell itself,
+                 ;; drop it.
+                 (while (not (eof-object? (read-line port)))))
+               (lambda args
+                 (let ((errno (system-error-errno args)))
+                   (cond
+                    ((= errno EIO)
+                     ;; On Linux, a read won't return EOF but will fail with EIO
+                     ;; when the device is closed:
+                     ;; https://bugs.python.org/issue5380#msg252544
+                     #t)
+                    (#t
+                     (apply throw args))))))
+             (close-port port)
+             (waitpid pid)))))
+
+      (let ((result (begin
                       (let loop ((lines '()))
-                          (match (read-line port)
+                        (match (read-delimited "\0" environment-port)
                           ((? eof-object?) (reverse lines))
-                            ("GUIX-CHECK-DONE\r"
-                             (display "done\n" port)
-                             (reverse lines))
                           (line
-                             ;; Drop the '\r' from LINE.
-                             (loop (cons (string-drop-right line 1)
-                                         lines))))))))
-         (close-port port)
-         (waitpid pid)
-         result))))
+                             (loop (cons line lines))))))))
+        (close-port environment-port)
+        (delete-file environment-file)
+        result)))
 
   (fold (lambda (line table)
-          ;; Note: 'set' in fish outputs "NAME VALUE" instead of "NAME=VALUE"
-          ;; but it also truncates values anyway, so don't try to support it.
           (let ((index (string-index line #\=)))
             (if index
                 (vhash-cons (string-take line index)
diff --git a/tests/guix-environment.sh b/tests/guix-environment.sh
index 95fe95b437..114bf9fbf5 100644
--- a/tests/guix-environment.sh
+++ b/tests/guix-environment.sh
@@ -258,3 +258,19 @@ then
 	guix gc --references "$profile" | grep "$dep"
     done
 fi
+
+# https://issues.guix.gnu.org/51466
+# guix environment --check was sometimes unable to find PKG_CONFIG_PATH
+# because the env command is sent before the prompt gets printed and there
+# is no proper way to deinterleave streams:
+#  ;;; (read-line "env || /usr/bin/env || set; echo GUIX-CHECK-DONE; read x; exit\r")
+#  ;;; (read-line "bash-5.1$ PKG_CONFIG_PATH=/gnu/store/0ysl5arpf0yf3pn15afr450k676lgdq3-profile/lib/pkgconfig\r")
+#  ;;; (read-line "PWD=/home/ether/source/guix\r")
+bash=$(mktemp)
+cat > "$bash" << 'BASH'
+#!/usr/bin/env bash
+exec bash --noprofile --rcfile /dev/null "$@"
+BASH
+chmod +x "$bash"
+env SHELL=$bash guix environment --check --pure guix -- env
+rm -f "$bash"