bug#53355: guix shell --check: confusing error message

[Chris Marusich <cmmarusich@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1486 bytes --]

Hi Ludo & Everyone,

Chris Marusich <cmmarusich@gmail.com> writes:

> Is it OK to rely on shell redirection?

It turns out that it is probably not OK to rely on shell redirection in
this case, after all.  For example, "dash does not support multi-digit
file descriptors":

https://bugs.launchpad.net/ubuntu/+source/dash/+bug/249620

Indeed, the patch I proposed earlier to rely on shell redirection caused
a command like

./pre-inst-env env SHELL=/gnu/store/nm0hccsphymxi8c24xmg6ixm9vcf25xb-dash-0.5.11.5/bin/dash guix shell --check --container -D guix

to hang.  It hangs because the FD Guile chooses to create and embed in
the script is 19 (on my machine, at least).  A redirection like
"env >&19" causes dash to error out, so no environment information gets
sent back to the parent process.  The same issue seemed to occur for the
ksh from our oksh package.

To resolve this, I changed the code so that it just writes to a
temporary file.  This is simple and more robust.  With the attached
patch, I was able to use a command like the one above to verify that
"guix environment --check" works correctly for Guix-built bash, dash,
ksh, fish, zsh, and ash.  I also verified that it works for Fedora's
/bin/sh and /bin/bash.

What do you think of this file-based approach?  Supporting many
different shells is pretty tricky, but I think this patch does a good
enough job.

-- 
Chris

PGP: https://savannah.gnu.org/people/viewgpg.php?user_id=106836

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-environment-Prevent-PS1-from-clobbering-output-in-ch.patch --]
[-- Type: text/x-patch, Size: 5014 bytes --]

From ef8d12a1d44903eafca7153c9344263b1d5d7d56 Mon Sep 17 00:00:00 2001
From: Chris Marusich <cmmarusich@gmail.com>
Date: Fri, 11 Mar 2022 00:20:12 -0800
Subject: [PATCH] environment: Prevent PS1 from clobbering output in 'check'.

Fixes: <https://issues.guix.gnu.org/51466>.

* guix/scripts/environment.scm (child-shell-environment) [temporary-file-port]
[temporary-file]: New local variables.
[script]: Redirect stdout to the temporary file.
[lines]: In the parent, send the script to the shell, wait for the shell to
exit, and then read lines from the temporary file.  Use a dynamic-wind
expression to ensure we always close port, close temporary-file-port, and
delete temporary-file.
---
 guix/scripts/environment.scm | 63 ++++++++++++++++++++++++------------
 1 file changed, 43 insertions(+), 20 deletions(-)

diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm
index 3216235937..b02b0771e3 100644
--- a/guix/scripts/environment.scm
+++ b/guix/scripts/environment.scm
@@ -48,6 +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)
+  #:use-module (ice-9 format)
   #:autoload   (ice-9 rdelim) (read-line)
   #:use-module (ice-9 vlist)
   #:use-module (srfi srfi-1)
@@ -418,14 +419,27 @@ (define (child-shell-environment shell profile manifest)
   (define-values (controller inferior)
     (openpty))
 
+  (define temporary-file-port (mkstemp "/tmp/guix-env.XXXXXX"))
+
+  (define temporary-file (port-filename temporary-file-port))
+
   (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")
+    ;; 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).
+    ;;
+    ;; Unless we redirect output to a file, there is a risk that the shell's
+    ;; PS1 prompt might clobber the output.  See:
+    ;; https://issues.guix.gnu.org/53355
+    (format
+     #f "env >~a || /usr/bin/env >~a || set >~a; \
+echo GUIX-CHECK-DONE >>~a; exit\n"
+     temporary-file temporary-file temporary-file temporary-file))
 
   (define lines
     (match (primitive-fork)
+      ;; Child
       (0
        (catch #t
          (lambda ()
@@ -436,24 +450,33 @@ (define lines
            (execl shell shell))
          (lambda _
            (primitive-exit 127))))
+      ;; Parent
       (pid
        (close-fdes inferior)
-       (let* ((port   (fdopen controller "r+l"))
-              (result (begin
-                        (display script port)
-                        (let loop ((lines '()))
-                          (match (read-line 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))))
+       (let* ((port   (fdopen controller "r+l")))
+         (dynamic-wind
+           (const #t)
+           (lambda ()
+             (display script port)
+             ;; Wait until the shell is done writing to the temporary file.
+             (waitpid pid)
+             (let loop ((lines '()))
+               ;; Read from the temporary file, not from the controller port, to
+               ;; prevent the shell's PS1 prompt from getting mixed into what we
+               ;; read.  See: https://issues.guix.gnu.org/51466
+               (match (read-line temporary-file-port)
+                 ((? eof-object?) (reverse lines))
+                 ("GUIX-CHECK-DONE"
+                  (reverse lines))
+                 (line
+                  (loop (cons line lines))))))
+           (lambda ()
+             (close-port temporary-file-port)
+             ;; The temporary file might not exist if something weird happens
+             ;; in the child shell that prevents it from even writing to the
+             ;; file (e.g. the shell fails to start for some reason).
+             (false-if-exception (delete-file temporary-file))
+             (close-port port)))))))
 
   (fold (lambda (line table)
           ;; Note: 'set' in fish outputs "NAME VALUE" instead of "NAME=VALUE"

base-commit: 319b8331b2357e12ec9edb9665513c32bef56622
-- 
2.34.0


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