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

[Chris Marusich <cmmarusich@gmail.com>]

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

Hi Ludo,

Ludovic Courtès <ludo@gnu.org> writes:

> So you confirm that a single “echo” is not enough, right?

I didn't test one specifically. It might work with just one, but it did
work with three.  If we want to proceed with the "echo" approach, let me
know and I'll test just one echo to see if that is reliable enough.

> Perhaps we should unroll the ‘for’ loop for portability, to be on the
> safe side.  Initially I tested with Bash, Zsh, and Fish:
>
>   https://issues.guix.gnu.org/51285#0-lineno49
>
> I think Fish has a very non-POSIX syntax, hence the suggestion to avoid
> ‘for’.

I see.  Yes, I'll do that if we decide to go with the echo-based
approach.

> I realized that setting PS1 could interfere with the logic below that
> checks for PS1.  And since it doesn’t seem to help, perhaps we can
> remove “PS1=;”?

I recall that I tried removing PS1, and I still had trouble.  I believe
it was because even if we unset PS1 as the very first command we do, the
original prompt is still printed.  Foreign distros usually set PS1 to
something, and whatever that is will be printed before we have a chance
to input any commands.  It's hard to avoid that in general.

> Thoughts?

One alternative method I tried successfully in a variety of shells was
to use shell redirection (see attached).  I like this approach.
However, this will only work in shells that support redirection.  I
recall testing with bash, ash (busybox's shell), dash, zsh, fish, ksh,
and csh.  I recall that only csh failed, since it doesn't support
redirection.

I personally like the attached patch better than what I proposed
earlier.  The earlier patch just echoes a few times.  Presumably, it
only works because the PS1 prompt is likely (but not guaranteed) to be
emitted before the last of the echo commands finishes printing.  I'd
rather just control the desired output and ignore PS1 entirely, and that
is what the attached patch accomplishes using FDs.  However, if support
for shells without redirection is a requirement, then maybe the original
hack (echo a few times) is OK, or perhaps we need something else.

How would you like to proceed?  Is it OK to rely on shell redirection?

-- 
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: [PATCH] environment: Prevent PS1 from clobbering output in 'check'. --]
[-- Type: text/x-patch, Size: 4221 bytes --]

From 9a1cef589abf01b61e22656f44c76441f4c50ffd 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) [shell-pipe]
[shell-pipe-in, shell-pipe-out]: New local variables.
[script]: Redirect the stdout of each command to the file descriptor of the
shell-pipe-out port.
[lines]: In the child, close shell-pipe-in before starting the shell.  In the
parent, close shell-pipe-out before sending the script to the shell.  Read
lines from shell-pipe-in, not port, so that the shell's PS1 prompt cannot
clobber the lines.  Close shell-pipe-in just before waiting on the child.
---
 guix/scripts/environment.scm | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm
index 3216235937..f0cb341aab 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,11 +419,23 @@ (define (child-shell-environment shell profile manifest)
   (define-values (controller inferior)
     (openpty))
 
+  (define shell-pipe (pipe))
+  (define shell-pipe-in (car shell-pipe))
+  (define shell-pipe-out (cdr shell-pipe))
+
   (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 dedicated file descriptor, there is a
+    ;; risk that the shell's PS1 prompt might clobber the output.  See:
+    ;; https://issues.guix.gnu.org/53355
+    (let ((out-fd (port->fdes shell-pipe-out)))
+      (format
+       #f "env 1>&~d || /usr/bin/env 1>&~d || set 1>&~d; \
+echo GUIX-CHECK-DONE 1>&~d; read x; exit\n" out-fd out-fd out-fd out-fd)))
 
   (define lines
     (match (primitive-fork)
@@ -432,17 +445,22 @@ (define lines
            (load-profile profile manifest #:pure? #t)
            (setenv "GUIX_ENVIRONMENT" profile)
            (close-fdes controller)
+           (close-port shell-pipe-in)
            (login-tty inferior)
            (execl shell shell))
          (lambda _
            (primitive-exit 127))))
       (pid
        (close-fdes inferior)
+       (close-port shell-pipe-out)
        (let* ((port   (fdopen controller "r+l"))
               (result (begin
                         (display script port)
                         (let loop ((lines '()))
-                          (match (read-line port)
+                          ;; Read from our special port, not from the
+                          ;; controller port, to prevent the shell's PS1
+                          ;; prompt from getting mixed into what we read.
+                          (match (read-line shell-pipe-in)
                             ((? eof-object?) (reverse lines))
                             ("GUIX-CHECK-DONE\r"
                              (display "done\n" port)
@@ -452,6 +470,7 @@ (define lines
                              (loop (cons (string-drop-right line 1)
                                          lines))))))))
          (close-port port)
+         (close-port shell-pipe-in)
          (waitpid pid)
          result))))
 

base-commit: adf5ae5a412ed13302186dd4ce8e2df783d4515d
-- 
2.34.0


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