[bug#64359] [PATCH] [RFC] --search-paths: emit code compatible with set -u

[Zack Weinberg]

The shell script fragment emitted by the --search-paths option to
‘guix shell’, etc., uses this construct to prepend a directory to
a search-path environment variable:

    export VAR="/gnu/store/...${VAR:+:}$VAR"

If this is evaluated by a shell in set -u mode, and VAR was previously
unset, it will error out:

    $ bash -c '
        set -u
        unset PKG_CONFIG_PATH
        export 
PKG_CONFIG_PATH="/example/lib/pkgconfig${PKG_CONFIG_PATH:+:}$PKG_CONFIG_PATH"
        echo $PKG_CONFIG_PATH
    '
    bash: line 4: PKG_CONFIG_PATH: unbound variable

This happens in real life, for instance, if direnv[1] is being used in its
“strict_env” mode[2], which the documentation says will become the default
in a future release.

This patch makes the code emitted by --search-paths compatible with set -u,
by changing each use of bare `$VARIABLE` to `${VARIABLE:-}`, e.g.

    $ bash -c '
        set -u
        unset PKG_CONFIG_PATH
        export 
PKG_CONFIG_PATH="/example/lib/pkgconfig${PKG_CONFIG_PATH:+:}${PKG_CONFIG_PATH:-}"
        echo $PKG_CONFIG_PATH'
    /example/lib/pkgconfig

(Note: `${VARIABLE-}` would also be correct here.  When there are no
characters between the `-` and the `}`, `${VARIABLE:-}` and `${VARIABLE-}`
do exactly the same thing.  However, `${VARIABLE:+:}` and `${VARIABLE+:}`
are *not* equivalent, and the former is what we want.  Therefore I think
the code is easier to understand if we consistently use the : variant of
both substitution operators.)

For consistency I also made the same change to the code generated by
'wrap-program'.  (There's an argument for adding `set -euo pipefail`
to the top of those scripts, but that should probably be a separate patch.)

I tried to write tests to verify this new constraint, i.e. that the code
emitted by --search-paths works correctly in set -u mode.  The test suite
got stuck for upwards of six hours, apparently on tests that I didn’t
touch.  I _suspect_ this is a problem with my development machine, which is
a botched chimera of Guix and Debian at the moment, but I’d appreciate it
if someone could look carefully at the test code.  The code change itself
is straightforward.

Also, as far as I can tell, there is no existing package that causes
environment-variable-definition to be invoked with #:kind 'suffix, and
in fact I’m not sure it’s *possible* to write a package definition that
does that.  So that case is currently not being tested.

[1]: https://direnv.net/
[2]: https://direnv.net/man/direnv.toml.1.html#codestrictenvcode
---
 guix/build/utils.scm      |  8 ++++----
 guix/search-paths.scm     |  4 ++--
 tests/guix-environment.sh | 21 +++++++++++++++++++++
 tests/guix-shell.sh       | 20 ++++++++++++++++++++
 4 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index 2352a627e9..f2a18d698d 100644
--- a/guix/build/utils.scm
+++ b/guix/build/utils.scm
@@ -1384,19 +1384,19 @@ (define (export-variable lst)
        (format #f "export ~a=\"~a\""
                var (string-join rest sep)))
       ((var sep 'prefix rest)
-       (format #f "export ~a=\"~a${~a:+~a}$~a\""
+       (format #f "export ~a=\"~a${~a:+~a}${~a:-}\""
                var (string-join rest sep) var sep var))
       ((var sep 'suffix rest)
-       (format #f "export ~a=\"$~a${~a+~a}~a\""
+       (format #f "export ~a=\"${~a:-}${~a:+~a}~a\""
                var var var sep (string-join rest sep)))
       ((var '= rest)
        (format #f "export ~a=\"~a\""
                var (string-join rest ":")))
       ((var 'prefix rest)
-       (format #f "export ~a=\"~a${~a:+:}$~a\""
+       (format #f "export ~a=\"~a${~a:+:}${~a:-}\""
                var (string-join rest ":") var var))
       ((var 'suffix rest)
-       (format #f "export ~a=\"$~a${~a:+:}~a\""
+       (format #f "export ~a=\"${~a:-}${~a:+:}~a\""
                var var var (string-join rest ":")))))
 
   (when (wrapped-program? prog)
diff --git a/guix/search-paths.scm b/guix/search-paths.scm
index fcbe7b7953..13c96ad692 100644
--- a/guix/search-paths.scm
+++ b/guix/search-paths.scm
@@ -225,10 +225,10 @@ (define* (environment-variable-definition variable value
     ('exact
      (format #f "export ~a=\"~a\"" variable value))
     ('prefix
-     (format #f "export ~a=\"~a${~a:+~a}$~a\""
+     (format #f "export ~a=\"~a${~a:+~a}${~a:-}\""
              variable value variable separator variable))
     ('suffix
-     (format #f "export ~a=\"$~a${~a:+~a}~a\""
+     (format #f "export ~a=\"${~a:-}${~a:+~a}~a\""
              variable variable variable separator value))))
 
 (define* (search-path-definition search-path value
diff --git a/tests/guix-environment.sh b/tests/guix-environment.sh
index 1424ea9a88..d0ffcd91f3 100644
--- a/tests/guix-environment.sh
+++ b/tests/guix-environment.sh
@@ -157,6 +157,27 @@ esac
 # in its profile (e.g., for 'gzip'), but we have to accept them.
 guix environment guix --bootstrap -n
 
+# The code emitted by '--search-paths', when adding to either end of a path
+# variable that might not be set to begin with, should not cause errors,
+# whether or not the variable was set, even if the shell is in `set -u` mode.
+# As far as I can tell, there are currently no packages that add to the end
+# of a path variable, so we cannot test that case.
+printf '%s\n' \
+       'set -u' \
+       'set -e' \
+       'unset PKG_CONFIG_PATH' \
+       'export PATH=/nonexistent' \
+       > "$tmpdir/c"
+guix environment guix --bootstrap -n --search-paths >> "$tmpdir/c"
+printf '%s\n' \
+       'echo "PATH=$PATH"' \
+       'echo "PKG_CONFIG_PATH=$PKG_CONFIG_PATH"' \
+       >> "$tmpdir/c"
+bash -x "$tmpdir/c" > "$tmpdir/d"
+grep -E '^PKG_CONFIG_PATH=/gnu/[^:]+$' "$tmpdir/d"
+grep -E '^PATH=/gnu/[^:]+:/nonexistent$' "$tmpdir/d"
+
+
 # Try program transformation options.
 mkdir "$tmpdir/emacs-36.8"
 drv="`guix environment --ad-hoc emacs -n 2>&1 | grep 'emacs.*\.drv'`"
diff --git a/tests/guix-shell.sh b/tests/guix-shell.sh
index ed368515eb..ec30a06288 100644
--- a/tests/guix-shell.sh
+++ b/tests/guix-shell.sh
@@ -41,6 +41,26 @@ guix shell --ad-hoc guile-bootstrap && false
 # Rejecting unsupported packages.
 guix shell -s armhf-linux intelmetool -n && false
 
+# The code emitted by '--search-paths', when adding to either end of a path
+# variable that might not be set to begin with, should not cause errors,
+# whether or not the variable was set, even if the shell is in `set -u` mode.
+# As far as I can tell, there are currently no packages that add to the end
+# of a path variable, so we cannot test that case.
+printf '%s\n' \
+       'set -u' \
+       'set -e' \
+       'unset PKG_CONFIG_PATH' \
+       'export PATH=/nonexistent' \
+       > "$tmpdir/c"
+guix shell -D guix --bootstrap -n --search-paths >> "$tmpdir/c"
+printf '%s\n' \
+       'echo "PATH=$PATH"' \
+       'echo "PKG_CONFIG_PATH=$PKG_CONFIG_PATH"' \
+       >> "$tmpdir/c"
+bash -x "$tmpdir/c" > "$tmpdir/d"
+grep -E '^PKG_CONFIG_PATH=/gnu/[^:]+$' "$tmpdir/d"
+grep -E '^PATH=/gnu/[^:]+:/nonexistent$' "$tmpdir/d"
+
 # Test approximately that the child process does not inherit extra file
 # descriptors.  Ideally we'd check there's nothing more than 0, 1, and 2, but
 # we cannot do that because (1) we might be inheriting additional FDs, for
-- 
2.40.1