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"