[bug#51285] [PATCH 0/3] Add 'guix shell --check'

[bug#51285] [PATCH 0/3] Add 'guix shell --check'

[Ludovic Courtès]

Hello Guix!

You may be familiar with the crucial footnote about shell startup files:

  https://guix.gnu.org/manual/en/html_node/Invoking-guix-environment.html#FOOT11

In short, if your shell startup file mess up with environment variables,
‘--pure’ won’t deliver its promise.  That’s a common source of annoyance.

This patch set adds a ‘--check’ option to ‘guix shell’ (and ‘guix environment’
as well, mostly out of simplicity).  Here’s an example:

--8<---------------cut here---------------start------------->8---
$ tail -2 ~/.bashrc
echo "hi from bashrc!"
export PATH=muahahaaa:$PATH
$ ./pre-inst-env guix shell --check coreutils 
guix shell: checking the environment variables visible from shell '/gnu/store/87kif0bpf0anwbsaw0jvg8fyciw4sz67-bash-5.0.16/bin/bash'...
guix shell: warning: variable 'PATH' is clobbered: 'muahahaaa:/gnu/store/cq8sfmm6in0r4nfx0zxjvx1fg5ky6a7r-profile/bin'
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.
--8<---------------cut here---------------end--------------->8---

Once I’ve cleaned up ~/.bashrc, I can check again and proceed:

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix shell --check coreutils 
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.
[env]$ uname -o
GNU/Linux
--8<---------------cut here---------------end--------------->8---

This works by launching an interactive shell in a pseudo-terminal, and
running “env” or “set” in that shell to retrieve the list of environment
variables once it has sourced its startup files.

It works well with Bash, Zsh, and presumably any POSIX shell.  It kinda
works with fish, but that’s not entirely clear.

To allow users to learn about this option, the last patch has ‘guix shell’
print a suggest to run ‘--check’ if (1) the hint has never been printed,
(2) the user is doing an interactive session, and (3) the user did not
pass ‘--container’ or ‘--search-paths’.

Thoughts?

Ludo’.

Ludovic Courtès (3):
  syscalls: Add 'openpty' and 'login-tty'.
  environment: Add '--check'.
  shell: Suggest running '--check' once for interactive use.

 doc/guix.texi                |  39 ++++++---
 guix/build/syscalls.scm      |  39 +++++++++
 guix/scripts/environment.scm | 162 ++++++++++++++++++++++++++++++++++-
 guix/scripts/shell.scm       |  41 ++++++++-
 tests/syscalls.scm           |  35 ++++++++
 5 files changed, 303 insertions(+), 13 deletions(-)


base-commit: 467d599bd972f16a958a394827bc925bc497c19a
-- 
2.33.0

[bug#51285] [PATCH 1/3] syscalls: Add 'openpty' and 'login-tty'.

[Ludovic Courtès]

* guix/build/syscalls.scm (openpty, login-pty): New procedures.
* tests/syscalls.scm ("openpty", "openpty + login-tty"): New tests.
---
 guix/build/syscalls.scm | 39 +++++++++++++++++++++++++++++++++++++++
 tests/syscalls.scm      | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/guix/build/syscalls.scm b/guix/build/syscalls.scm
index 99a3b45004..7ea6b56e54 100644
--- a/guix/build/syscalls.scm
+++ b/guix/build/syscalls.scm
@@ -180,6 +180,8 @@ (define-module (guix build syscalls)
             terminal-window-size
             terminal-columns
             terminal-rows
+            openpty
+            login-tty
 
             utmpx?
             utmpx-login-type
@@ -2286,6 +2288,43 @@ (define* (terminal-rows #:optional (port (current-output-port)))
 always a positive integer."
   (terminal-dimension window-size-rows port (const 25)))
 
+(define openpty
+  (let* ((ptr  (dynamic-func "openpty" (dynamic-link "libutil")))
+         (proc (pointer->procedure int ptr '(* * * * *)
+                                   #:return-errno? #t)))
+    (lambda ()
+      "Return two file descriptors: one for the pseudo-terminal control side,
+and one for the controlled side."
+      (let ((head     (make-bytevector (sizeof int)))
+            (inferior (make-bytevector (sizeof int))))
+        (let-values (((ret err)
+                      (proc (bytevector->pointer head)
+                            (bytevector->pointer inferior)
+                            %null-pointer %null-pointer %null-pointer)))
+          (unless (zero? ret)
+            (throw 'system-error "openpty" "~A"
+                   (list (strerror err))
+                   (list err))))
+
+        (let ((* (lambda (bv)
+                   (bytevector-sint-ref bv 0 (native-endianness)
+                                        (sizeof int)))))
+          (values (* head) (* inferior)))))))
+
+(define login-tty
+  (let* ((ptr  (dynamic-func "login_tty" (dynamic-link "libutil")))
+         (proc (pointer->procedure int ptr (list int)
+                                   #:return-errno? #t)))
+    (lambda (fd)
+      "Make FD the controlling terminal of the current process (with the
+TIOCSCTTY ioctl), redirect standard input, standard output and standard error
+output to this terminal, and close FD."
+      (let-values (((ret err) (proc fd)))
+        (unless (zero? ret)
+          (throw 'system-error "login-pty" "~A"
+                 (list (strerror err))
+                 (list err)))))))
+
 \f
 ;;;
 ;;; utmpx.
diff --git a/tests/syscalls.scm b/tests/syscalls.scm
index 706dd4177f..c9e011f453 100644
--- a/tests/syscalls.scm
+++ b/tests/syscalls.scm
@@ -26,6 +26,7 @@ (define-module (test-syscalls)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-64)
+  #:use-module (srfi srfi-71)
   #:use-module (system foreign)
   #:use-module ((ice-9 ftw) #:select (scandir))
   #:use-module (ice-9 match))
@@ -582,6 +583,40 @@ (define perform-container-tests?
 (test-assert "terminal-rows"
   (> (terminal-rows) 0))
 
+(test-assert "openpty"
+  (let ((head inferior (openpty)))
+    (and (integer? head) (integer? inferior)
+         (let ((port (fdopen inferior "r+0")))
+           (and (isatty? port)
+                (begin
+                  (close-port port)
+                  (close-fdes head)
+                  #t))))))
+
+(test-equal "openpty + login-tty"
+  '(hello world)
+  (let ((head inferior (openpty)))
+    (match (primitive-fork)
+      (0
+       (dynamic-wind
+         (const #t)
+         (lambda ()
+           (setvbuf (current-input-port) 'none)
+           (close-fdes head)
+           (login-tty inferior)
+           (write (read))
+           (read))                          ;this gets EIO when HEAD is closed
+         (lambda ()
+           (primitive-_exit 42))))
+      (pid
+       (close-fdes inferior)
+       (let ((head (fdopen head "r+0")))
+         (write '(hello world) head)
+         (let ((result (read head)))
+           (close-port head)
+           (waitpid pid)
+           result))))))
+
 (test-assert "utmpx-entries"
   (match (utmpx-entries)
     (((? utmpx? entries) ...)
-- 
2.33.0

[bug#51285] [PATCH 2/3] environment: Add '--check'.

[Ludovic Courtès]

From: Ludovic Courtès <ludovic.courtes@inria.fr>

* guix/scripts/environment.scm (show-environment-options-help)
(%options): Add '--check'.
* guix/scripts/environment.scm (child-shell-environment)
(validate-child-shell-environment): New procedures.
(guix-environment*): Call 'validate-child-shell-environment' when
'check?' key is in OPTS.
* doc/guix.texi (Invoking guix shell): Shorten footnote about Bash
startup files.  Document '--check' and mention startup files.
(Invoking guix environment): Document '--check'.
---
 doc/guix.texi                |  39 ++++++---
 guix/scripts/environment.scm | 162 ++++++++++++++++++++++++++++++++++-
 2 files changed, 189 insertions(+), 12 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index dabd7fea1e..e860ccc9b2 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -5640,17 +5640,11 @@ environment, where the new packages are added to search path environment
 variables such as @code{PATH}.  You can, instead, choose to create an
 @emph{isolated} environment containing nothing but the packages you
 asked for.  Passing the @option{--pure} option clears environment
-variable definitions found in the parent environment@footnote{Users
-sometimes wrongfully augment environment variables such as @env{PATH} in
-their @file{~/.bashrc} file.  As a consequence, when @command{guix
-environment} launches it, Bash may read @file{~/.bashrc}, thereby
-introducing ``impurities'' in these environment variables.  It is an
-error to define such environment variables in @file{.bashrc}; instead,
-they should be defined in @file{.bash_profile}, which is sourced only by
-log-in shells.  @xref{Bash Startup Files,,, bash, The GNU Bash Reference
-Manual}, for details on Bash start-up files.}; passing
-@option{--container} goes one step further by spawning a @dfn{container}
-isolated from the rest of the system:
+variable definitions found in the parent environment@footnote{Be sure to
+use the @option{--check} option the first time you use @command{guix
+shell} interactively to make sure the shell does not undo the effect of
+@option{--pure}.}; passing @option{--container} goes one step further by
+spawning a @dfn{container} isolated from the rest of the system:
 
 @example
 guix shell --container emacs gcc-toolchain
@@ -5699,6 +5693,24 @@ $ ls "$GUIX_ENVIRONMENT/bin"
 The available options are summarized below.
 
 @table @code
+@item --check
+Set up the environment and check whether the shell would clobber
+environment variables.  It's a good idea to use this option the first
+time you run @command{guix shell} for an interactive session to make
+sure your setup is correct.
+
+For example, if the shell modifies the @env{PATH} environment variable,
+report it since you would get a different environment than what you
+asked for.
+
+Such problems usually indicate that the 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 @file{~/.bash_profile} and @emph{not} in @file{~/.bashrc}---the
+former is sourced only by log-in shells.  @xref{Bash Startup Files,,,
+bash, The GNU Bash Reference Manual}, for details on Bash start-up
+files.
+
 @item --development
 @itemx -D
 Cause @command{guix shell} to include in the environment the
@@ -6065,6 +6077,11 @@ guix environment --preserve='^DISPLAY$' --container --network \
 The available options are summarized below.
 
 @table @code
+@item --check
+Set up the environment and check whether the shell would clobber
+environment variables.  @xref{Invoking guix shell, @option{--check}},
+for more info.
+
 @item --root=@var{file}
 @itemx -r @var{file}
 @cindex persistent environment
diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm
index 05a43659da..7b97a8e39a 100644
--- a/guix/scripts/environment.scm
+++ b/guix/scripts/environment.scm
@@ -41,12 +41,14 @@ (define-module (guix scripts environment)
   #:autoload   (gnu build accounts) (password-entry group-entry
                                      password-entry-name password-entry-directory
                                      write-passwd write-group)
-  #:autoload   (guix build syscalls) (set-network-interface-up)
+  #:autoload   (guix build syscalls) (set-network-interface-up openpty login-tty)
   #:use-module (gnu system file-systems)
   #:autoload   (gnu packages) (specification->package+output)
   #:autoload   (gnu packages bash) (bash)
   #:autoload   (gnu packages bootstrap) (bootstrap-executable %bootstrap-guile)
   #:use-module (ice-9 match)
+  #:autoload   (ice-9 rdelim) (read-line)
+  #:use-module (ice-9 vlist)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-11)
   #:use-module (srfi srfi-26)
@@ -83,6 +85,8 @@ (define (show-environment-options-help)
   -m, --manifest=FILE    create environment with the manifest from FILE"))
   (display (G_ "
   -p, --profile=PATH     create environment from profile at PATH"))
+  (display (G_ "
+      --check            check if the shell clobbers environment variables"))
   (display (G_ "
       --pure             unset existing environment variables"))
   (display (G_ "
@@ -178,6 +182,9 @@ (define %options
          (option '(#\V "version") #f #f
                  (lambda args
                    (show-version-and-exit "guix environment")))
+         (option '("check") #f #f
+                 (lambda (opt name arg result)
+                   (alist-cons 'check? #t result)))
          (option '("pure") #f #f
                  (lambda (opt name arg result)
                    (alist-cons 'pure #t result)))
@@ -396,6 +403,155 @@ (define* (launch-environment command profile manifest
     ((program . args)
      (apply execlp program program args))))
 
+(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
+    (match (primitive-fork)
+      (0
+       (catch #t
+         (lambda ()
+           (load-profile profile manifest #:pure? #t)
+           (setenv "GUIX_ENVIRONMENT" profile)
+           (close-fdes controller)
+           (login-tty inferior)
+           (execl shell shell))
+         (lambda _
+           (primitive-exit 127))))
+      (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))))
+
+  (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)
+                            (string-drop line (+ 1 index))
+                            table)
+                table)))
+        vlist-null
+        lines))
+
+(define* (validate-child-shell-environment profile manifest
+                                           #:optional (shell %default-shell))
+  "Run SHELL in interactive mode in an environment for PROFILE and MANIFEST
+and report clobbered environment variables."
+  (define warned? #f)
+  (define-syntax-rule (warn exp ...)
+    (begin
+      (set! warned? #t)
+      (warning exp ...)))
+
+  (info (G_ "checking the environment variables visible from shell '~a'...~%")
+        shell)
+  (let ((actual (child-shell-environment shell profile manifest)))
+    (when (vlist-null? actual)
+      (leave (G_ "failed to determine environment of shell '~a'~%")
+             shell))
+    (for-each (match-lambda
+                ((spec . expected)
+                 (let ((name (search-path-specification-variable spec)))
+                   (match (vhash-assoc name actual)
+                     (#f
+                      (warn (G_ "variable '~a' is missing from shell \
+environment~%")
+                            name))
+                     ((_ . actual)
+                      (cond ((string=? expected actual)
+                             #t)
+                            ((string-prefix? expected actual)
+                             (warn (G_ "variable '~a' has unexpected \
+suffix '~a'~%")
+                                   name
+                                   (string-drop actual
+                                                (string-length expected))))
+                            (else
+                             (warn (G_ "variable '~a' is clobbered: '~a'~%")
+                                   name actual))))))))
+              (profile-search-paths profile manifest))
+
+    ;; Special case.
+    (match (vhash-assoc "GUIX_ENVIRONMENT" actual)
+      (#f
+       (warn (G_ "'GUIX_ENVIRONMENT' is missing from the shell \
+environment~%")))
+      ((_ . value)
+       (unless (string=? value profile)
+         (warn (G_ "'GUIX_ENVIRONMENT' is set to '~a' instead of '~a'~%")
+               value profile))))
+
+    ;; Check the prompt unless we have more important warnings.
+    (unless warned?
+      (match (vhash-assoc "PS1" actual)
+        (#f #f)
+        (str
+         (when (and (getenv "PS1") (string=? str (getenv "PS1")))
+           (warning (G_ "'PS1' is the same in sub-shell~%"))
+           (display-hint (G_ "Consider setting a different prompt for
+environment shells to make them distinguishable.
+
+If you are using Bash, you can do that by adding these lines to
+@file{~/.bashrc}:
+
+@example
+if [ -n \"$GUIX_ENVIRONMENT\" ]
+then
+  export PS1=\"\\u@@\\h \\w [env]\\$ \"
+fi
+@end example
+"))))))
+
+    (if warned?
+        (begin
+          (display-hint (G_ "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
+@file{~/.bash_profile} and @emph{not} in @file{~/.bashrc}.  For more
+information on Bash startup files, run:
+
+@example
+info \"(bash) Bash Startup Files\"
+@end example
+
+Alternatively, you can avoid the problem by passing the @option{--container}
+or @option{-C} option.  That will give you a fully isolated environment
+running in a \"container\", immune to the issue described above."))
+          (exit 1))
+        (info (G_ "All is good!  The shell gets correct environment \
+variables.~%")))))
+
 (define* (launch-environment/fork command profile manifest
                                   #:key pure? (white-list '()))
   "Run COMMAND in a new process with an environment containing PROFILE, with
@@ -775,6 +931,10 @@ (define manifest
                   (mwhen gc-root
                     (register-gc-root profile gc-root))
 
+                  (mwhen (assoc-ref opts 'check?)
+                    (return
+                     (validate-child-shell-environment profile manifest)))
+
                   (cond
                    ((assoc-ref opts 'search-paths)
                     (show-search-paths profile manifest #:pure? pure?)
-- 
2.33.0

[bug#51285] [PATCH 3/3] shell: Suggest running '--check' once for interactive use.

[Ludovic Courtès]

* guix/scripts/shell.scm (hint-directory, hint-file, record-hint)
(hint-given?): New procedures.
(guix-shell): Record and probe the 'shell-check' hint.
---
 guix/scripts/shell.scm | 41 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/guix/scripts/shell.scm b/guix/scripts/shell.scm
index 4062e8155d..43e8484333 100644
--- a/guix/scripts/shell.scm
+++ b/guix/scripts/shell.scm
@@ -331,6 +331,30 @@ (define (profile-cached-gc-root file)
     (#f  #f)
     (key (string-append (%profile-cache-directory) "/" key))))
 
+\f
+;;;
+;;; One-time hints.
+;;;
+
+(define (hint-directory)
+  "Return the directory name where previously given hints are recorded."
+  (let ((directory (string-append (cache-directory #:ensure? #f)
+                                  "/hints")))
+    (mkdir-p directory)
+    directory))
+
+(define (hint-file hint)
+  "Return the name of the file that marks HINT as already printed."
+  (string-append (hint-directory) "/" (symbol->string hint)))
+
+(define (record-hint hint)
+  "Mark HINT as already given."
+  (close-port (open-file (hint-file hint) "w")))
+
+(define (hint-given? hint)
+  "Return true if HINT was already given."
+  (file-exists? (hint-file hint)))
+
 \f
 (define-command (guix-shell . args)
   (category development)
@@ -348,7 +372,22 @@ (define* (entry-expiration file)
       (#f 0)                       ;FILE may have been deleted in the meantime
       (st (+ (stat:atime st) (* 60 60 24 7)))))
 
-  (let ((result (guix-environment* (parse-args args))))
+  (define opts
+    (parse-args args))
+
+  (define interactive?
+    (not (assoc-ref opts 'exec)))
+
+  (if (assoc-ref opts 'check?)
+      (record-hint 'shell-check)
+      (when (and interactive?
+                 (not (hint-given? 'shell-check))
+                 (not (assoc-ref opts 'container?))
+                 (not (assoc-ref opts 'search-paths)))
+        (display-hint (G_ "Consider passing the @option{--check} option once
+to make sure your shell does not clobber environment variables."))) )
+
+  (let ((result (guix-environment* opts)))
     (maybe-remove-expired-cache-entries (%profile-cache-directory)
                                         cache-entries
                                         #:entry-expiration entry-expiration)
-- 
2.33.0

bug#51285: [PATCH 0/3] Add 'guix shell --check'

[Ludovic Courtès]

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

> You may be familiar with the crucial footnote about shell startup files:
>
>   https://guix.gnu.org/manual/en/html_node/Invoking-guix-environment.html#FOOT11
>
> In short, if your shell startup file mess up with environment variables,
> ‘--pure’ won’t deliver its promise.  That’s a common source of annoyance.
>
> This patch set adds a ‘--check’ option to ‘guix shell’ (and ‘guix environment’
> as well, mostly out of simplicity).  Here’s an example:

Pushed as 409f538d651a7ba26a41f714915e6b7d59e0a82f!

Ludo’.